Fix extensions enablement bugs (#8797)

docs/extension.md

@@ -36,11 +36,13 @@ gemini extensions uninstall gemini-cli-security
 
 Extensions are, by default, enabled across all workspaces. You can disable an extension entirely or for specific workspace.
 
-For example, `gemini extensions disable extension-name` will disable the extension at the user level, so it will be disabled everywhere. `gemini extensions disable extension-name --scope=Workspace` will only disable the extension in the current workspace.
+For example, `gemini extensions disable extension-name` will disable the extension at the user level, so it will be disabled everywhere. `gemini extensions disable extension-name --scope=workspace` will only disable the extension in the current workspace.
 
 ### Enabling an extension
 
-You can re-enable extensions using `gemini extensions enable extension-name`. Note that if an extension is disabled at the user-level, enabling it at the workspace level will not do anything.
+You can enable extensions using `gemini extensions enable extension-name`. You can also enable an extension for a specific workspace using `gemini extensions enable extension-name --scope=workspace` from within that workspace.
+
+This is useful if you have an extension disabled at the top-level and only enabled in specific places.
 
 ### Updating an extension
 

packages/cli/src/commands/extensions/disable.ts

@@ -11,12 +11,16 @@ import { getErrorMessage } from '../../utils/errors.js';
 
 interface DisableArgs {
   name: string;
-  scope: SettingScope;
+  scope?: string;
 }
 
 export function handleDisable(args: DisableArgs) {
   try {
-    disableExtension(args.name, args.scope);
+    if (args.scope?.toLowerCase() === 'workspace') {
+      disableExtension(args.name, SettingScope.Workspace);
+    } else {
+      disableExtension(args.name, SettingScope.User);
+    }
     console.log(
       `Extension "${args.name}" successfully disabled for scope "${args.scope}".`,
     );
@@ -39,13 +43,28 @@ export const disableCommand: CommandModule = {
         describe: 'The scope to disable the extenison in.',
         type: 'string',
         default: SettingScope.User,
-        choices: [SettingScope.User, SettingScope.Workspace],
       })
-      .check((_argv) => true),
+      .check((argv) => {
+        if (
+          argv.scope &&
+          !Object.values(SettingScope)
+            .map((s) => s.toLowerCase())
+            .includes((argv.scope as string).toLowerCase())
+        ) {
+          throw new Error(
+            `Invalid scope: ${argv.scope}. Please use one of ${Object.values(
+              SettingScope,
+            )
+              .map((s) => s.toLowerCase())
+              .join(', ')}.`,
+          );
+        }
+        return true;
+      }),
   handler: (argv) => {
     handleDisable({
       name: argv['name'] as string,
-      scope: argv['scope'] as SettingScope,
+      scope: argv['scope'] as string,
     });
   },
 };

packages/cli/src/commands/extensions/enable.ts

@@ -11,13 +11,16 @@ import { SettingScope } from '../../config/settings.js';
 
 interface EnableArgs {
   name: string;
-  scope?: SettingScope;
+  scope?: string;
 }
 
 export function handleEnable(args: EnableArgs) {
   try {
-    const scope = args.scope ? args.scope : SettingScope.User;
-    enableExtension(args.name, scope);
+    if (args.scope?.toLowerCase() === 'workspace') {
+      enableExtension(args.name, SettingScope.Workspace);
+    } else {
+      enableExtension(args.name, SettingScope.User);
+    }
     if (args.scope) {
       console.log(
         `Extension "${args.name}" successfully enabled for scope "${args.scope}".`,
@@ -45,13 +48,28 @@ export const enableCommand: CommandModule = {
         describe:
           'The scope to enable the extenison in. If not set, will be enabled in all scopes.',
         type: 'string',
-        choices: [SettingScope.User, SettingScope.Workspace],
       })
-      .check((_argv) => true),
+      .check((argv) => {
+        if (
+          argv.scope &&
+          !Object.values(SettingScope)
+            .map((s) => s.toLowerCase())
+            .includes((argv.scope as string).toLowerCase())
+        ) {
+          throw new Error(
+            `Invalid scope: ${argv.scope}. Please use one of ${Object.values(
+              SettingScope,
+            )
+              .map((s) => s.toLowerCase())
+              .join(', ')}.`,
+          );
+        }
+        return true;
+      }),
   handler: (argv) => {
     handleEnable({
       name: argv['name'] as string,
-      scope: argv['scope'] as SettingScope,
+      scope: argv['scope'] as string,
     });
   },
 };

packages/cli/src/config/extensions/extensionEnablement.test.ts

@@ -4,11 +4,11 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import * as path from 'node:path';
 import fs from 'node:fs';
 import os from 'node:os';
-import path from 'node:path';
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
-import { ExtensionEnablementManager } from './extensionEnablement.js';
+import { ExtensionEnablementManager, Override } from './extensionEnablement.js';
 
 // Helper to create a temporary directory for testing
 function createTestDir() {
@@ -48,7 +48,7 @@ describe('ExtensionEnablementManager', () => {
     });
 
     it('should enable a path based on an override rule', () => {
-      manager.disable('ext-test', true, '*'); // Disable globally
+      manager.disable('ext-test', true, '/');
       manager.enable('ext-test', true, '/home/user/projects/');
       expect(manager.isEnabled('ext-test', '/home/user/projects/my-app')).toBe(
         true,
@@ -56,7 +56,7 @@ describe('ExtensionEnablementManager', () => {
     });
 
     it('should disable a path based on a disable override rule', () => {
-      manager.enable('ext-test', true, '*'); // Enable globally
+      manager.enable('ext-test', true, '/');
       manager.disable('ext-test', true, '/home/user/projects/');
       expect(manager.isEnabled('ext-test', '/home/user/projects/my-app')).toBe(
         false,
@@ -78,65 +78,253 @@ describe('ExtensionEnablementManager', () => {
         false,
       );
     });
+
+    it('should handle', () => {
+      manager.enable('ext-test', true, '/home/user/projects');
+      manager.disable('ext-test', false, '/home/user/projects/my-app');
+      expect(manager.isEnabled('ext-test', '/home/user/projects/my-app')).toBe(
+        false,
+      );
+      expect(
+        manager.isEnabled('ext-test', '/home/user/projects/something-else'),
+      ).toBe(true);
+    });
   });
 
   describe('includeSubdirs', () => {
     it('should add a glob when enabling with includeSubdirs', () => {
       manager.enable('ext-test', true, '/path/to/dir');
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('/path/to/dir*');
+      expect(config['ext-test'].overrides).toContain('/path/to/dir/*');
     });
 
     it('should not add a glob when enabling without includeSubdirs', () => {
       manager.enable('ext-test', false, '/path/to/dir');
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('/path/to/dir');
-      expect(config['ext-test'].overrides).not.toContain('/path/to/dir*');
+      expect(config['ext-test'].overrides).toContain('/path/to/dir/');
+      expect(config['ext-test'].overrides).not.toContain('/path/to/dir/*');
     });
 
     it('should add a glob when disabling with includeSubdirs', () => {
       manager.disable('ext-test', true, '/path/to/dir');
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('!/path/to/dir*');
+      expect(config['ext-test'].overrides).toContain('!/path/to/dir/*');
     });
 
     it('should remove conflicting glob rule when enabling without subdirs', () => {
       manager.enable('ext-test', true, '/path/to/dir'); // Adds /path/to/dir*
       manager.enable('ext-test', false, '/path/to/dir'); // Should remove the glob
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('/path/to/dir');
-      expect(config['ext-test'].overrides).not.toContain('/path/to/dir*');
+      expect(config['ext-test'].overrides).toContain('/path/to/dir/');
+      expect(config['ext-test'].overrides).not.toContain('/path/to/dir/*');
     });
 
     it('should remove conflicting non-glob rule when enabling with subdirs', () => {
       manager.enable('ext-test', false, '/path/to/dir'); // Adds /path/to/dir
       manager.enable('ext-test', true, '/path/to/dir'); // Should remove the non-glob
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('/path/to/dir*');
-      expect(config['ext-test'].overrides).not.toContain('/path/to/dir');
+      expect(config['ext-test'].overrides).toContain('/path/to/dir/*');
+      expect(config['ext-test'].overrides).not.toContain('/path/to/dir/');
     });
 
     it('should remove conflicting rules when disabling', () => {
       manager.enable('ext-test', true, '/path/to/dir'); // enabled with glob
       manager.disable('ext-test', false, '/path/to/dir'); // disabled without
       const config = manager.readConfig();
-      expect(config['ext-test'].overrides).toContain('!/path/to/dir');
-      expect(config['ext-test'].overrides).not.toContain('/path/to/dir*');
+      expect(config['ext-test'].overrides).toContain('!/path/to/dir/');
+      expect(config['ext-test'].overrides).not.toContain('/path/to/dir/*');
     });
 
     it('should correctly evaluate isEnabled with subdirs', () => {
-      manager.disable('ext-test', true, '*');
+      manager.disable('ext-test', true, '/');
       manager.enable('ext-test', true, '/path/to/dir');
-      expect(manager.isEnabled('ext-test', '/path/to/dir')).toBe(true);
-      expect(manager.isEnabled('ext-test', '/path/to/dir/sub')).toBe(true);
-      expect(manager.isEnabled('ext-test', '/path/to/another')).toBe(false);
+      expect(manager.isEnabled('ext-test', '/path/to/dir/')).toBe(true);
+      expect(manager.isEnabled('ext-test', '/path/to/dir/sub/')).toBe(true);
+      expect(manager.isEnabled('ext-test', '/path/to/another/')).toBe(false);
     });
 
     it('should correctly evaluate isEnabled without subdirs', () => {
-      manager.disable('ext-test', true, '*');
+      manager.disable('ext-test', true, '/*');
       manager.enable('ext-test', false, '/path/to/dir');
       expect(manager.isEnabled('ext-test', '/path/to/dir')).toBe(true);
       expect(manager.isEnabled('ext-test', '/path/to/dir/sub')).toBe(false);
     });
   });
+
+  describe('pruning child rules', () => {
+    it('should remove child rules when enabling a parent with subdirs', () => {
+      // Pre-existing rules for children
+      manager.enable('ext-test', false, '/path/to/dir/subdir1');
+      manager.disable('ext-test', true, '/path/to/dir/subdir2');
+      manager.enable('ext-test', false, '/path/to/another/dir');
+
+      // Enable the parent directory
+      manager.enable('ext-test', true, '/path/to/dir');
+
+      const config = manager.readConfig();
+      const overrides = config['ext-test'].overrides;
+
+      // The new parent rule should be present
+      expect(overrides).toContain(`/path/to/dir/*`);
+
+      // Child rules should be removed
+      expect(overrides).not.toContain('/path/to/dir/subdir1/');
+      expect(overrides).not.toContain(`!/path/to/dir/subdir2/*`);
+
+      // Unrelated rules should remain
+      expect(overrides).toContain('/path/to/another/dir/');
+    });
+
+    it('should remove child rules when disabling a parent with subdirs', () => {
+      // Pre-existing rules for children
+      manager.enable('ext-test', false, '/path/to/dir/subdir1');
+      manager.disable('ext-test', true, '/path/to/dir/subdir2');
+      manager.enable('ext-test', false, '/path/to/another/dir');
+
+      // Disable the parent directory
+      manager.disable('ext-test', true, '/path/to/dir');
+
+      const config = manager.readConfig();
+      const overrides = config['ext-test'].overrides;
+
+      // The new parent rule should be present
+      expect(overrides).toContain(`!/path/to/dir/*`);
+
+      // Child rules should be removed
+      expect(overrides).not.toContain('/path/to/dir/subdir1/');
+      expect(overrides).not.toContain(`!/path/to/dir/subdir2/*`);
+
+      // Unrelated rules should remain
+      expect(overrides).toContain('/path/to/another/dir/');
+    });
+
+    it('should not remove child rules if includeSubdirs is false', () => {
+      manager.enable('ext-test', false, '/path/to/dir/subdir1');
+      manager.enable('ext-test', false, '/path/to/dir'); // Not including subdirs
+
+      const config = manager.readConfig();
+      const overrides = config['ext-test'].overrides;
+
+      expect(overrides).toContain('/path/to/dir/subdir1/');
+      expect(overrides).toContain('/path/to/dir/');
+    });
+  });
+
+  it('should enable a path based on an enable override', () => {
+    manager.disable('ext-test', true, '/Users/chrstn');
+    manager.enable('ext-test', true, '/Users/chrstn/gemini-cli');
+
+    expect(manager.isEnabled('ext-test', '/Users/chrstn/gemini-cli')).toBe(
+      true,
+    );
+  });
+
+  it('should ignore subdirs', () => {
+    manager.disable('ext-test', false, '/Users/chrstn');
+    expect(manager.isEnabled('ext-test', '/Users/chrstn/gemini-cli')).toBe(
+      true,
+    );
+  });
+});
+
+describe('Override', () => {
+  it('should create an override from input', () => {
+    const override = Override.fromInput('/path/to/dir', true);
+    expect(override.baseRule).toBe(`/path/to/dir/`);
+    expect(override.isDisable).toBe(false);
+    expect(override.includeSubdirs).toBe(true);
+  });
+
+  it('should create a disable override from input', () => {
+    const override = Override.fromInput('!/path/to/dir', false);
+    expect(override.baseRule).toBe(`/path/to/dir/`);
+    expect(override.isDisable).toBe(true);
+    expect(override.includeSubdirs).toBe(false);
+  });
+
+  it('should create an override from a file rule', () => {
+    const override = Override.fromFileRule('/path/to/dir');
+    expect(override.baseRule).toBe('/path/to/dir');
+    expect(override.isDisable).toBe(false);
+    expect(override.includeSubdirs).toBe(false);
+  });
+
+  it('should create a disable override from a file rule', () => {
+    const override = Override.fromFileRule('!/path/to/dir/');
+    expect(override.isDisable).toBe(true);
+    expect(override.baseRule).toBe('/path/to/dir/');
+    expect(override.includeSubdirs).toBe(false);
+  });
+
+  it('should create an override with subdirs from a file rule', () => {
+    const override = Override.fromFileRule('/path/to/dir/*');
+    expect(override.baseRule).toBe('/path/to/dir/');
+    expect(override.isDisable).toBe(false);
+    expect(override.includeSubdirs).toBe(true);
+  });
+
+  it('should correctly identify conflicting overrides', () => {
+    const override1 = Override.fromInput('/path/to/dir', true);
+    const override2 = Override.fromInput('/path/to/dir', false);
+    expect(override1.conflictsWith(override2)).toBe(true);
+  });
+
+  it('should correctly identify non-conflicting overrides', () => {
+    const override1 = Override.fromInput('/path/to/dir', true);
+    const override2 = Override.fromInput('/path/to/another/dir', true);
+    expect(override1.conflictsWith(override2)).toBe(false);
+  });
+
+  it('should correctly identify equal overrides', () => {
+    const override1 = Override.fromInput('/path/to/dir', true);
+    const override2 = Override.fromInput('/path/to/dir', true);
+    expect(override1.isEqualTo(override2)).toBe(true);
+  });
+
+  it('should correctly identify unequal overrides', () => {
+    const override1 = Override.fromInput('/path/to/dir', true);
+    const override2 = Override.fromInput('!/path/to/dir', true);
+    expect(override1.isEqualTo(override2)).toBe(false);
+  });
+
+  it('should generate the correct regex', () => {
+    const override = Override.fromInput('/path/to/dir', true);
+    const regex = override.asRegex();
+    expect(regex.test('/path/to/dir/')).toBe(true);
+    expect(regex.test('/path/to/dir/subdir')).toBe(true);
+    expect(regex.test('/path/to/another/dir')).toBe(false);
+  });
+
+  it('should correctly identify child overrides', () => {
+    const parent = Override.fromInput('/path/to/dir', true);
+    const child = Override.fromInput('/path/to/dir/subdir', false);
+    expect(child.isChildOf(parent)).toBe(true);
+  });
+
+  it('should correctly identify child overrides with glob', () => {
+    const parent = Override.fromInput('/path/to/dir/*', true);
+    const child = Override.fromInput('/path/to/dir/subdir', false);
+    expect(child.isChildOf(parent)).toBe(true);
+  });
+
+  it('should correctly identify non-child overrides', () => {
+    const parent = Override.fromInput('/path/to/dir', true);
+    const other = Override.fromInput('/path/to/another/dir', false);
+    expect(other.isChildOf(parent)).toBe(false);
+  });
+
+  it('should generate the correct output string', () => {
+    const override = Override.fromInput('/path/to/dir', true);
+    expect(override.output()).toBe(`/path/to/dir/*`);
+  });
+
+  it('should generate the correct output string for a disable override', () => {
+    const override = Override.fromInput('!/path/to/dir', false);
+    expect(override.output()).toBe(`!/path/to/dir/`);
+  });
+
+  it('should disable a path based on a disable override rule', () => {
+    const override = Override.fromInput('!/path/to/dir', false);
+    expect(override.output()).toBe(`!/path/to/dir/`);
+  });
 });

packages/cli/src/config/extensions/extensionEnablement.ts

@@ -15,6 +15,80 @@ export interface AllExtensionsEnablementConfig {
   [extensionName: string]: ExtensionEnablementConfig;
 }
 
+export class Override {
+  constructor(
+    public baseRule: string,
+    public isDisable: boolean,
+    public includeSubdirs: boolean,
+  ) {}
+
+  static fromInput(inputRule: string, includeSubdirs: boolean): Override {
+    const isDisable = inputRule.startsWith('!');
+    let baseRule = isDisable ? inputRule.substring(1) : inputRule;
+    baseRule = ensureLeadingAndTrailingSlash(baseRule);
+    return new Override(baseRule, isDisable, includeSubdirs);
+  }
+
+  static fromFileRule(fileRule: string): Override {
+    const isDisable = fileRule.startsWith('!');
+    let baseRule = isDisable ? fileRule.substring(1) : fileRule;
+    const includeSubdirs = baseRule.endsWith('*');
+    baseRule = includeSubdirs
+      ? baseRule.substring(0, baseRule.length - 1)
+      : baseRule;
+    return new Override(baseRule, isDisable, includeSubdirs);
+  }
+
+  conflictsWith(other: Override): boolean {
+    if (this.baseRule === other.baseRule) {
+      return (
+        this.includeSubdirs !== other.includeSubdirs ||
+        this.isDisable !== other.isDisable
+      );
+    }
+    return false;
+  }
+
+  isEqualTo(other: Override): boolean {
+    return (
+      this.baseRule === other.baseRule &&
+      this.includeSubdirs === other.includeSubdirs &&
+      this.isDisable === other.isDisable
+    );
+  }
+
+  asRegex(): RegExp {
+    return globToRegex(`${this.baseRule}${this.includeSubdirs ? '*' : ''}`);
+  }
+
+  isChildOf(parent: Override) {
+    if (!parent.includeSubdirs) {
+      return false;
+    }
+    return parent.asRegex().test(this.baseRule);
+  }
+
+  output(): string {
+    return `${this.isDisable ? '!' : ''}${this.baseRule}${this.includeSubdirs ? '*' : ''}`;
+  }
+
+  matchesPath(path: string) {
+    return this.asRegex().test(path);
+  }
+}
+
+const ensureLeadingAndTrailingSlash = function (dirPath: string): string {
+  // Normalize separators to forward slashes for consistent matching across platforms.
+  let result = dirPath.replace(/\\/g, '/');
+  if (result.charAt(0) !== '/') {
+    result = '/' + result;
+  }
+  if (result.charAt(result.length - 1) !== '/') {
+    result = result + '/';
+  }
+  return result;
+};
+
 /**
  * Converts a glob pattern to a RegExp object.
  * This is a simplified implementation that supports `*`.
@@ -25,7 +99,7 @@ export interface AllExtensionsEnablementConfig {
 function globToRegex(glob: string): RegExp {
   const regexString = glob
     .replace(/[.+?^${}()|[\]\\]/g, '\\$&') // Escape special regex characters
-    .replace(/\*/g, '.*'); // Convert * to .*
+    .replace(/(\/?)\*/g, '($1.*)?'); // Convert * to optional group
 
   return new RegExp(`^${regexString}$`);
 }
@@ -52,16 +126,13 @@ export class ExtensionEnablementManager {
     const extensionConfig = config[extensionName];
     // Extensions are enabled by default.
     let enabled = true;
-
-    for (const rule of extensionConfig?.overrides ?? []) {
-      const isDisableRule = rule.startsWith('!');
-      const globPattern = isDisableRule ? rule.substring(1) : rule;
-      const regex = globToRegex(globPattern);
-      if (regex.test(currentPath)) {
-        enabled = !isDisableRule;
+    const allOverrides = extensionConfig?.overrides ?? [];
+    for (const rule of allOverrides) {
+      const override = Override.fromFileRule(rule);
+      if (override.matchesPath(ensureLeadingAndTrailingSlash(currentPath))) {
+        enabled = !override.isDisable;
       }
     }
-
     return enabled;
   }
 
@@ -96,24 +167,19 @@ export class ExtensionEnablementManager {
     if (!config[extensionName]) {
       config[extensionName] = { overrides: [] };
     }
-
-    const pathWithGlob = `${scopePath}*`;
-    const pathWithoutGlob = scopePath;
-
-    const newPath = includeSubdirs ? pathWithGlob : pathWithoutGlob;
-    const conflictingPath = includeSubdirs ? pathWithoutGlob : pathWithGlob;
-
-    config[extensionName].overrides = config[extensionName].overrides.filter(
-      (rule) =>
-        rule !== conflictingPath &&
-        rule !== `!${conflictingPath}` &&
-        rule !== `!${newPath}`,
-    );
-
-    if (!config[extensionName].overrides.includes(newPath)) {
-      config[extensionName].overrides.push(newPath);
-    }
-
+    const override = Override.fromInput(scopePath, includeSubdirs);
+    const overrides = config[extensionName].overrides.filter((rule) => {
+      const fileOverride = Override.fromFileRule(rule);
+      if (
+        fileOverride.conflictsWith(override) ||
+        fileOverride.isEqualTo(override)
+      ) {
+        return false; // Remove conflicts and equivalent values.
+      }
+      return !fileOverride.isChildOf(override);
+    });
+    overrides.push(override.output());
+    config[extensionName].overrides = overrides;
     this.writeConfig(config);
   }
 
@@ -122,30 +188,7 @@ export class ExtensionEnablementManager {
     includeSubdirs: boolean,
     scopePath: string,
   ): void {
-    const config = this.readConfig();
-    if (!config[extensionName]) {
-      config[extensionName] = { overrides: [] };
-    }
-
-    const pathWithGlob = `${scopePath}*`;
-    const pathWithoutGlob = scopePath;
-
-    const targetPath = includeSubdirs ? pathWithGlob : pathWithoutGlob;
-    const newRule = `!${targetPath}`;
-    const conflictingPath = includeSubdirs ? pathWithoutGlob : pathWithGlob;
-
-    config[extensionName].overrides = config[extensionName].overrides.filter(
-      (rule) =>
-        rule !== conflictingPath &&
-        rule !== `!${conflictingPath}` &&
-        rule !== targetPath,
-    );
-
-    if (!config[extensionName].overrides.includes(newRule)) {
-      config[extensionName].overrides.push(newRule);
-    }
-
-    this.writeConfig(config);
+    this.enable(extensionName, includeSubdirs, `!${scopePath}`);
   }
 
   remove(extensionName: string): void {