Browse Source

Merge pull request #370 from alvinunreal/fix/369-council-legacy-master-model

fix(council): preserve pre-1.0.0 master.model as fallback for council agent
Alvin 3 months ago
parent
commit
577aa6d55f

+ 85 - 0
src/agents/index.test.ts

@@ -2,6 +2,7 @@ import { describe, expect, test } from 'bun:test';
 import type { PluginConfig } from '../config';
 import {
   AgentOverrideConfigSchema,
+  CouncilConfigSchema,
   DEFAULT_DISABLED_AGENTS,
   DEFAULT_MODELS,
   PluginConfigSchema,
@@ -388,6 +389,90 @@ describe('council agent model resolution', () => {
     const councillor = agents.find((a) => a.name === 'councillor');
     expect(councillor?.config.model).toBe(DEFAULT_MODELS.councillor);
   });
+
+  test('council falls back to legacy master.model when no preset override', () => {
+    // Simulates a pre-1.0.0 config with council.master.model but no council
+    // entry in the agent preset — the exact scenario from issue #369.
+    const config: PluginConfig = {
+      agents: {
+        oracle: { model: 'openai/gpt-5.4' },
+      },
+      council: {
+        presets: {
+          default: {
+            alpha: { model: 'openai/gpt-5.4-mini' },
+          },
+        },
+        _legacyMasterModel: 'anthropic/claude-opus-4-6',
+      },
+    };
+    const agents = createAgents(config);
+    const council = agents.find((a) => a.name === 'council');
+    expect(council?.config.model).toBe('anthropic/claude-opus-4-6');
+  });
+
+  test('council preset override takes precedence over legacy master.model', () => {
+    // If user has explicit council in preset, that wins — legacy is ignored.
+    const config: PluginConfig = {
+      agents: {
+        council: { model: 'google/gemini-3-pro' },
+      },
+      council: {
+        presets: {
+          default: {
+            alpha: { model: 'openai/gpt-5.4-mini' },
+          },
+        },
+        _legacyMasterModel: 'anthropic/claude-opus-4-6',
+      },
+    };
+    const agents = createAgents(config);
+    const council = agents.find((a) => a.name === 'council');
+    expect(council?.config.model).toBe('google/gemini-3-pro');
+  });
+
+  test('council uses default when no legacy master and no preset override', () => {
+    // No legacy master, no preset override → standard default
+    const config: PluginConfig = {
+      council: {
+        presets: {
+          default: {
+            alpha: { model: 'openai/gpt-5.4-mini' },
+          },
+        },
+      },
+    };
+    const agents = createAgents(config);
+    const council = agents.find((a) => a.name === 'council');
+    expect(council?.config.model).toBe(DEFAULT_MODELS.council);
+  });
+
+  test('end-to-end: raw master.model config flows through schema to council agent', () => {
+    // Integration test: start from raw user config with deprecated master.model,
+    // parse through CouncilConfigSchema, then pass to createAgents.
+    // This validates the full seam between schema transform and agent resolution.
+    const rawCouncilConfig = {
+      master: { model: 'anthropic/claude-opus-4-6' },
+      presets: {
+        default: {
+          alpha: { model: 'openai/gpt-5.4-mini' },
+        },
+      },
+    };
+
+    const parsed = CouncilConfigSchema.safeParse(rawCouncilConfig);
+    expect(parsed.success).toBe(true);
+
+    if (parsed.success) {
+      const config: PluginConfig = {
+        council: parsed.data,
+      };
+      const agents = createAgents(config);
+      const council = agents.find((a) => a.name === 'council');
+      // Legacy master.model should flow through schema → agent
+      expect(council?.config.model).toBe('anthropic/claude-opus-4-6');
+    }
+  });
 });
 
 describe('options passthrough', () => {

+ 15 - 0
src/agents/index.ts

@@ -297,6 +297,21 @@ export function createAgents(config?: PluginConfig): AgentDefinition[] {
     return agent;
   });
 
+  // 2b. Backward compat: if council has no preset override and still uses the
+  // hardcoded default model, fall back to the deprecated council.master.model.
+  // See https://github.com/alvinunreal/oh-my-opencode-slim/issues/369
+  const legacyMasterModel = config?.council?._legacyMasterModel;
+  if (legacyMasterModel) {
+    const councilAgent = builtInSubAgents.find((a) => a.name === 'council');
+    if (
+      councilAgent &&
+      !getAgentOverride(config, 'council')?.model &&
+      councilAgent.config.model === DEFAULT_MODELS.council
+    ) {
+      councilAgent.config.model = legacyMasterModel;
+    }
+  }
+
   const customSubAgents = protoCustomAgents.map((agent) => {
     const override = getAgentOverride(config, agent.name);
     if (override) {

+ 41 - 0
src/config/council-schema.test.ts

@@ -45,6 +45,8 @@ describe('CouncillorConfigSchema', () => {
       // Core fields still work normally
       expect(result.data.timeout).toBe(180000);
       expect(Object.keys(result.data.presets.default)).toEqual(['alpha']);
+      // Legacy master.model is extracted for backward-compat fallback
+      expect(result.data._legacyMasterModel).toBe('anthropic/claude-opus-4-6');
     }
   });
 
@@ -62,6 +64,7 @@ describe('CouncillorConfigSchema', () => {
 
     if (result.success) {
       expect(result.data._deprecated).toBeUndefined();
+      expect(result.data._legacyMasterModel).toBeUndefined();
     }
   });
 });
@@ -149,6 +152,44 @@ test('deprecated master with non-standard model ID still parses', () => {
       'master_timeout',
       'master_fallback',
     ]);
+    // Even non-standard model IDs are extracted as-is for backward compat
+    expect(result.data._legacyMasterModel).toBe('claude-opus-4-6');
+  }
+});
+
+test('legacyMasterModel undefined when master.model is not a string', () => {
+  const config = {
+    master: { model: 42 }, // not a string
+    presets: {
+      default: {
+        alpha: { model: 'openai/gpt-5.4-mini' },
+      },
+    },
+  };
+
+  const result = CouncilConfigSchema.safeParse(config);
+  expect(result.success).toBe(true);
+
+  if (result.success) {
+    expect(result.data._legacyMasterModel).toBeUndefined();
+  }
+});
+
+test('legacyMasterModel undefined when master is not an object', () => {
+  const config = {
+    master: 'oops', // not an object
+    presets: {
+      default: {
+        alpha: { model: 'openai/gpt-5.4-mini' },
+      },
+    },
+  };
+
+  const result = CouncilConfigSchema.safeParse(config);
+  expect(result.success).toBe(true);
+
+  if (result.success) {
+    expect(result.data._legacyMasterModel).toBeUndefined();
   }
 });
 

+ 12 - 0
src/config/council-schema.ts

@@ -163,6 +163,17 @@ export const CouncilConfigSchema = z
     if (data.master_timeout !== undefined) deprecated.push('master_timeout');
     if (data.master_fallback !== undefined) deprecated.push('master_fallback');
 
+    // Backward compat: extract master.model so the council agent can use it
+    // as a fallback when no explicit council entry exists in the active preset.
+    // See https://github.com/alvinunreal/oh-my-opencode-slim/issues/369
+    const legacyMasterModel: string | undefined =
+      typeof data.master === 'object' &&
+      data.master !== null &&
+      'model' in data.master &&
+      typeof (data.master as { model: unknown }).model === 'string'
+        ? (data.master as { model: string }).model
+        : undefined;
+
     return {
       presets: data.presets,
       timeout: data.timeout,
@@ -170,6 +181,7 @@ export const CouncilConfigSchema = z
       councillor_execution_mode: data.councillor_execution_mode,
       councillor_retries: data.councillor_retries,
       _deprecated: deprecated.length > 0 ? deprecated : undefined,
+      _legacyMasterModel: legacyMasterModel,
     };
   });
 

+ 7 - 0
src/council/council-manager.ts

@@ -39,6 +39,7 @@ export class CouncilManager {
   private depthTracker?: SubagentDepthTracker;
   private tmuxEnabled: boolean;
   private deprecatedFields?: string[];
+  private legacyMasterModel?: string;
 
   constructor(
     ctx: PluginInput,
@@ -50,6 +51,7 @@ export class CouncilManager {
     this.directory = ctx.directory;
     this.config = config;
     this.deprecatedFields = config?.council?._deprecated;
+    this.legacyMasterModel = config?.council?._legacyMasterModel;
     this.depthTracker = depthTracker;
     this.tmuxEnabled = tmuxEnabled;
   }
@@ -59,6 +61,11 @@ export class CouncilManager {
     return this.deprecatedFields;
   }
 
+  /** Return the legacy master.model if it was used as fallback. */
+  getLegacyMasterModel(): string | undefined {
+    return this.legacyMasterModel;
+  }
+
   /**
    * Run a full council session.
    *

+ 34 - 0
src/tools/council.test.ts

@@ -508,6 +508,7 @@ describe('council_session tool', () => {
           ],
         })),
         getDeprecatedFields: mock(() => ['master', 'master_timeout']),
+        getLegacyMasterModel: mock(() => undefined),
       } as unknown as CouncilManager;
       const tools = createCouncilTool(ctx, councilManager);
 
@@ -518,6 +519,39 @@ describe('council_session tool', () => {
       expect(result).toContain('Config warning');
       expect(result).toContain('`council.master`');
       expect(result).toContain('`council.master_timeout`');
+      // master with no legacy model → both treated as ignored
+      expect(result).toContain('deprecated and ignored');
+    });
+
+    test('includes fallback warning when legacy master.model is used', async () => {
+      const ctx = createMockPluginContext();
+      const councilManager = {
+        runCouncil: mock(async () => ({
+          success: true,
+          result: 'Synthesized response',
+          councillorResults: [
+            {
+              name: 'alpha',
+              model: 'test/model',
+              status: 'completed',
+              result: 'Response',
+            },
+          ],
+        })),
+        getDeprecatedFields: mock(() => ['master', 'master_timeout']),
+        getLegacyMasterModel: mock(() => 'anthropic/claude-opus-4-6'),
+      } as unknown as CouncilManager;
+      const tools = createCouncilTool(ctx, councilManager);
+
+      const result = await tools.council_session.execute({ prompt: 'Test' }, {
+        sessionID: 'test',
+      } as any);
+
+      expect(result).toContain('Config warning');
+      expect(result).toContain('`council.master`');
+      // master with legacy model → fallback warning
+      expect(result).toContain('fallback for the council agent');
+      // master_timeout is still "ignored"
       expect(result).toContain('deprecated and ignored');
     });
   });

+ 18 - 1
src/tools/council.ts

@@ -96,7 +96,24 @@ Returns the councillor responses with a summary footer.`,
       // Warn about deprecated config fields if detected
       const deprecated = councilManager.getDeprecatedFields();
       if (deprecated && deprecated.length > 0) {
-        output += `\n⚠ Config warning: ${deprecated.map((f) => `\`council.${f}\``).join(', ')} ${deprecated.length === 1 ? 'is' : 'are'} deprecated and ignored. The council agent synthesizes directly — remove ${deprecated.length === 1 ? 'it' : 'them'} from your config.`;
+        const legacyMasterModel = councilManager.getLegacyMasterModel();
+        const hasMaster = deprecated.includes('master');
+        const trulyIgnored =
+          hasMaster && !legacyMasterModel
+            ? deprecated // master has no model → treat as ignored too
+            : deprecated.filter((f) => f !== 'master');
+        const parts: string[] = [];
+        if (hasMaster && legacyMasterModel) {
+          parts.push(
+            `\`council.master\` is deprecated and will be removed in a future version. Its \`model\` is currently used as a fallback for the council agent — add a \`council\` entry to your preset to make this explicit.`,
+          );
+        }
+        if (trulyIgnored.length > 0) {
+          parts.push(
+            `${trulyIgnored.map((f) => `\`council.${f}\``).join(', ')} ${trulyIgnored.length === 1 ? 'is' : 'are'} deprecated and ignored — remove ${trulyIgnored.length === 1 ? 'it' : 'them'} from your config.`,
+          );
+        }
+        output += `\n⚠ Config warning: ${parts.join(' ')}`;
       }
 
       return output;