refactor common settings logic for skills,agents

packages/cli/src/utils/agentSettings.test.ts

@@ -0,0 +1,147 @@
+/**
+ * @license
+ * Copyright 2026 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import {
+  SettingScope,
+  type LoadedSettings,
+  type LoadableSettingScope,
+} from '../config/settings.js';
+import { enableAgent, disableAgent } from './agentSettings.js';
+
+function createMockLoadedSettings(opts: {
+  userSettings?: Record<string, unknown>;
+  workspaceSettings?: Record<string, unknown>;
+  userPath?: string;
+  workspacePath?: string;
+}): LoadedSettings {
+  const scopes: Record<
+    string,
+    {
+      settings: Record<string, unknown>;
+      originalSettings: Record<string, unknown>;
+      path: string;
+    }
+  > = {
+    [SettingScope.User]: {
+      settings: opts.userSettings ?? {},
+      originalSettings: opts.userSettings ?? {},
+      path: opts.userPath ?? '/home/user/.gemini/settings.json',
+    },
+    [SettingScope.Workspace]: {
+      settings: opts.workspaceSettings ?? {},
+      originalSettings: opts.workspaceSettings ?? {},
+      path: opts.workspacePath ?? '/project/.gemini/settings.json',
+    },
+  };
+
+  return {
+    forScope: vi.fn((scope: LoadableSettingScope) => scopes[scope]),
+    setValue: vi.fn(),
+  } as unknown as LoadedSettings;
+}
+
+describe('agentSettings', () => {
+  describe('agentStrategy (via enableAgent / disableAgent)', () => {
+    describe('enableAgent', () => {
+      it('should return no-op when the agent is already enabled in both scopes', () => {
+        const settings = createMockLoadedSettings({
+          userSettings: {
+            agents: { overrides: { 'my-agent': { enabled: true } } },
+          },
+          workspaceSettings: {
+            agents: { overrides: { 'my-agent': { enabled: true } } },
+          },
+        });
+
+        const result = enableAgent(settings, 'my-agent');
+
+        expect(result.status).toBe('no-op');
+        expect(result.action).toBe('enable');
+        expect(result.agentName).toBe('my-agent');
+        expect(result.modifiedScopes).toHaveLength(0);
+        expect(settings.setValue).not.toHaveBeenCalled();
+      });
+
+      it('should enable the agent when not present in any scope', () => {
+        const settings = createMockLoadedSettings({
+          userSettings: {},
+          workspaceSettings: {},
+        });
+
+        const result = enableAgent(settings, 'my-agent');
+
+        expect(result.status).toBe('success');
+        expect(result.action).toBe('enable');
+        expect(result.agentName).toBe('my-agent');
+        expect(result.modifiedScopes).toHaveLength(2);
+        expect(settings.setValue).toHaveBeenCalledTimes(2);
+      });
+
+      it('should enable the agent only in the scope where it is not enabled', () => {
+        const settings = createMockLoadedSettings({
+          userSettings: {
+            agents: { overrides: { 'my-agent': { enabled: true } } },
+          },
+          workspaceSettings: {
+            agents: { overrides: { 'my-agent': { enabled: false } } },
+          },
+        });
+
+        const result = enableAgent(settings, 'my-agent');
+
+        expect(result.status).toBe('success');
+        expect(result.modifiedScopes).toHaveLength(1);
+        expect(result.modifiedScopes[0].scope).toBe(SettingScope.Workspace);
+        expect(result.alreadyInStateScopes).toHaveLength(1);
+        expect(result.alreadyInStateScopes[0].scope).toBe(SettingScope.User);
+        expect(settings.setValue).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    describe('disableAgent', () => {
+      it('should return no-op when agent is already explicitly disabled', () => {
+        const settings = createMockLoadedSettings({
+          userSettings: {
+            agents: { overrides: { 'my-agent': { enabled: false } } },
+          },
+        });
+
+        const result = disableAgent(settings, 'my-agent', SettingScope.User);
+
+        expect(result.status).toBe('no-op');
+        expect(result.action).toBe('disable');
+        expect(result.agentName).toBe('my-agent');
+        expect(settings.setValue).not.toHaveBeenCalled();
+      });
+
+      it('should disable the agent when it is currently enabled', () => {
+        const settings = createMockLoadedSettings({
+          userSettings: {
+            agents: { overrides: { 'my-agent': { enabled: true } } },
+          },
+        });
+
+        const result = disableAgent(settings, 'my-agent', SettingScope.User);
+
+        expect(result.status).toBe('success');
+        expect(result.action).toBe('disable');
+        expect(result.modifiedScopes).toHaveLength(1);
+        expect(result.modifiedScopes[0].scope).toBe(SettingScope.User);
+        expect(settings.setValue).toHaveBeenCalledTimes(1);
+      });
+
+      it('should return error for an invalid scope', () => {
+        const settings = createMockLoadedSettings({});
+
+        const result = disableAgent(settings, 'my-agent', SettingScope.Session);
+
+        expect(result.status).toBe('error');
+        expect(result.error).toContain('Invalid settings scope');
+      });
+    });
+  });
+});

packages/cli/src/utils/agentSettings.ts

@@ -4,30 +4,41 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import type { SettingScope, LoadedSettings } from '../config/settings.js';
 import {
-  SettingScope,
-  isLoadableSettingScope,
-  type LoadedSettings,
-} from '../config/settings.js';
-import type { ModifiedScope } from './skillSettings.js';
+  type FeatureActionResult,
+  type FeatureToggleStrategy,
+  enableFeature,
+  disableFeature,
+} from './featureToggleUtils.js';
 
 export type AgentActionStatus = 'success' | 'no-op' | 'error';
 
 /**
  * Metadata representing the result of an agent settings operation.
  */
-export interface AgentActionResult {
-  status: AgentActionStatus;
+export interface AgentActionResult
+  extends Omit<FeatureActionResult, 'featureName'> {
   agentName: string;
-  action: 'enable' | 'disable';
-  /** Scopes where the agent's state was actually changed. */
-  modifiedScopes: ModifiedScope[];
-  /** Scopes where the agent was already in the desired state. */
-  alreadyInStateScopes: ModifiedScope[];
-  /** Error message if status is 'error'. */
-  error?: string;
 }
 
+const agentStrategy: FeatureToggleStrategy = {
+  needsEnabling: (settings, scope, agentName) => {
+    const agentOverrides = settings.forScope(scope).settings.agents?.overrides;
+    return agentOverrides?.[agentName]?.enabled !== true;
+  },
+  enable: (settings, scope, agentName) => {
+    settings.setValue(scope, `agents.overrides.${agentName}.enabled`, true);
+  },
+  isExplicitlyDisabled: (settings, scope, agentName) => {
+    const agentOverrides = settings.forScope(scope).settings.agents?.overrides;
+    return agentOverrides?.[agentName]?.enabled === false;
+  },
+  disable: (settings, scope, agentName) => {
+    settings.setValue(scope, `agents.overrides.${agentName}.enabled`, false);
+  },
+};
+
 /**
  * Enables an agent by ensuring it is enabled in any writable scope (User and Workspace).
  * It sets `agents.overrides.<agentName>.enabled` to `true`.
@@ -36,50 +47,14 @@ export function enableAgent(
   settings: LoadedSettings,
   agentName: string,
 ): AgentActionResult {
-  const writableScopes = [SettingScope.Workspace, SettingScope.User];
-  const foundInDisabledScopes: ModifiedScope[] = [];
-  const alreadyEnabledScopes: ModifiedScope[] = [];
-
-  for (const scope of writableScopes) {
-    if (isLoadableSettingScope(scope)) {
-      const scopePath = settings.forScope(scope).path;
-      const agentOverrides =
-        settings.forScope(scope).settings.agents?.overrides;
-      const isEnabled = agentOverrides?.[agentName]?.enabled === true;
-
-      if (!isEnabled) {
-        foundInDisabledScopes.push({ scope, path: scopePath });
-      } else {
-        alreadyEnabledScopes.push({ scope, path: scopePath });
-      }
-    }
-  }
-
-  if (foundInDisabledScopes.length === 0) {
-    return {
-      status: 'no-op',
-      agentName,
-      action: 'enable',
-      modifiedScopes: [],
-      alreadyInStateScopes: alreadyEnabledScopes,
-    };
-  }
-
-  const modifiedScopes: ModifiedScope[] = [];
-  for (const { scope, path } of foundInDisabledScopes) {
-    if (isLoadableSettingScope(scope)) {
-      // Explicitly enable it.
-      settings.setValue(scope, `agents.overrides.${agentName}.enabled`, true);
-      modifiedScopes.push({ scope, path });
-    }
-  }
-
-  return {
-    status: 'success',
+  const { featureName, ...rest } = enableFeature(
+    settings,
     agentName,
-    action: 'enable',
-    modifiedScopes,
-    alreadyInStateScopes: alreadyEnabledScopes,
+    agentStrategy,
+  );
+  return {
+    ...rest,
+    agentName: featureName,
   };
 }
 
@@ -91,56 +66,14 @@ export function disableAgent(
   agentName: string,
   scope: SettingScope,
 ): AgentActionResult {
-  if (!isLoadableSettingScope(scope)) {
-    return {
-      status: 'error',
-      agentName,
-      action: 'disable',
-      modifiedScopes: [],
-      alreadyInStateScopes: [],
-      error: `Invalid settings scope: ${scope}`,
-    };
-  }
-
-  const scopePath = settings.forScope(scope).path;
-  const agentOverrides = settings.forScope(scope).settings.agents?.overrides;
-  const isEnabled = agentOverrides?.[agentName]?.enabled !== false;
-
-  if (!isEnabled) {
-    return {
-      status: 'no-op',
-      agentName,
-      action: 'disable',
-      modifiedScopes: [],
-      alreadyInStateScopes: [{ scope, path: scopePath }],
-    };
-  }
-
-  // Check if it's already disabled in the other writable scope
-  const otherScope =
-    scope === SettingScope.Workspace
-      ? SettingScope.User
-      : SettingScope.Workspace;
-  const alreadyDisabledInOther: ModifiedScope[] = [];
-
-  if (isLoadableSettingScope(otherScope)) {
-    const otherOverrides =
-      settings.forScope(otherScope).settings.agents?.overrides;
-    if (otherOverrides?.[agentName]?.enabled === false) {
-      alreadyDisabledInOther.push({
-        scope: otherScope,
-        path: settings.forScope(otherScope).path,
-      });
-    }
-  }
-
-  settings.setValue(scope, `agents.overrides.${agentName}.enabled`, false);
-
-  return {
-    status: 'success',
+  const { featureName, ...rest } = disableFeature(
+    settings,
     agentName,
-    action: 'disable',
-    modifiedScopes: [{ scope, path: scopePath }],
-    alreadyInStateScopes: alreadyDisabledInOther,
+    scope,
+    agentStrategy,
+  );
+  return {
+    ...rest,
+    agentName: featureName,
   };
 }