mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-02-02 08:33:36 +00:00
fix: centralize model prefix handling to prevent provider errors
Moves prefix stripping from individual providers to AgentService/IdeationService and adds validation to ensure providers receive bare model IDs. This prevents bugs like the Codex CLI receiving "codex-gpt-5.1-codex-max" instead of the expected "gpt-5.1-codex-max". - Add validateBareModelId() helper with fail-fast validation - Add originalModel field to ExecuteOptions for logging - Update all providers to validate model has no prefix - Centralize prefix stripping in service layer - Remove redundant prefix stripping from individual providers Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -10,7 +10,7 @@ import { BaseProvider } from './base-provider.js';
|
|||||||
import { classifyError, getUserFriendlyErrorMessage, createLogger } from '@automaker/utils';
|
import { classifyError, getUserFriendlyErrorMessage, createLogger } from '@automaker/utils';
|
||||||
|
|
||||||
const logger = createLogger('ClaudeProvider');
|
const logger = createLogger('ClaudeProvider');
|
||||||
import { getThinkingTokenBudget } from '@automaker/types';
|
import { getThinkingTokenBudget, validateBareModelId } from '@automaker/types';
|
||||||
import type {
|
import type {
|
||||||
ExecuteOptions,
|
ExecuteOptions,
|
||||||
ProviderMessage,
|
ProviderMessage,
|
||||||
@@ -53,6 +53,10 @@ export class ClaudeProvider extends BaseProvider {
|
|||||||
* Execute a query using Claude Agent SDK
|
* Execute a query using Claude Agent SDK
|
||||||
*/
|
*/
|
||||||
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
||||||
|
// Validate that model doesn't have a provider prefix
|
||||||
|
// AgentService should strip prefixes before passing to providers
|
||||||
|
validateBareModelId(options.model, 'ClaudeProvider');
|
||||||
|
|
||||||
const {
|
const {
|
||||||
prompt,
|
prompt,
|
||||||
model,
|
model,
|
||||||
|
|||||||
@@ -31,6 +31,7 @@ import type {
|
|||||||
import {
|
import {
|
||||||
CODEX_MODEL_MAP,
|
CODEX_MODEL_MAP,
|
||||||
supportsReasoningEffort,
|
supportsReasoningEffort,
|
||||||
|
validateBareModelId,
|
||||||
type CodexApprovalPolicy,
|
type CodexApprovalPolicy,
|
||||||
type CodexSandboxMode,
|
type CodexSandboxMode,
|
||||||
type CodexAuthStatus,
|
type CodexAuthStatus,
|
||||||
@@ -663,6 +664,10 @@ export class CodexProvider extends BaseProvider {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
||||||
|
// Validate that model doesn't have a provider prefix
|
||||||
|
// AgentService should strip prefixes before passing to providers
|
||||||
|
validateBareModelId(options.model, 'CodexProvider');
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const mcpServers = options.mcpServers ?? {};
|
const mcpServers = options.mcpServers ?? {};
|
||||||
const hasMcpServers = Object.keys(mcpServers).length > 0;
|
const hasMcpServers = Object.keys(mcpServers).length > 0;
|
||||||
@@ -760,6 +765,7 @@ export class CodexProvider extends BaseProvider {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Model is already bare (no prefix) - validated by executeQuery
|
||||||
const args = [
|
const args = [
|
||||||
CODEX_EXEC_SUBCOMMAND,
|
CODEX_EXEC_SUBCOMMAND,
|
||||||
CODEX_YOLO_FLAG,
|
CODEX_YOLO_FLAG,
|
||||||
|
|||||||
@@ -28,7 +28,7 @@ import type {
|
|||||||
ModelDefinition,
|
ModelDefinition,
|
||||||
ContentBlock,
|
ContentBlock,
|
||||||
} from './types.js';
|
} from './types.js';
|
||||||
import { stripProviderPrefix } from '@automaker/types';
|
import { validateBareModelId } from '@automaker/types';
|
||||||
import { validateApiKey } from '../lib/auth-utils.js';
|
import { validateApiKey } from '../lib/auth-utils.js';
|
||||||
import { getEffectivePermissions } from '../services/cursor-config-service.js';
|
import { getEffectivePermissions } from '../services/cursor-config-service.js';
|
||||||
import {
|
import {
|
||||||
@@ -317,8 +317,8 @@ export class CursorProvider extends CliProvider {
|
|||||||
}
|
}
|
||||||
|
|
||||||
buildCliArgs(options: ExecuteOptions): string[] {
|
buildCliArgs(options: ExecuteOptions): string[] {
|
||||||
// Extract model (strip 'cursor-' prefix if present)
|
// Model is already bare (no prefix) - validated by executeQuery
|
||||||
const model = stripProviderPrefix(options.model || 'auto');
|
const model = options.model || 'auto';
|
||||||
|
|
||||||
// Build CLI arguments for cursor-agent
|
// Build CLI arguments for cursor-agent
|
||||||
// NOTE: Prompt is NOT included here - it's passed via stdin to avoid
|
// NOTE: Prompt is NOT included here - it's passed via stdin to avoid
|
||||||
@@ -649,6 +649,10 @@ export class CursorProvider extends CliProvider {
|
|||||||
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> {
|
||||||
this.ensureCliDetected();
|
this.ensureCliDetected();
|
||||||
|
|
||||||
|
// Validate that model doesn't have a provider prefix
|
||||||
|
// AgentService should strip prefixes before passing to providers
|
||||||
|
validateBareModelId(options.model, 'CursorProvider');
|
||||||
|
|
||||||
if (!this.cliPath) {
|
if (!this.cliPath) {
|
||||||
throw this.createError(
|
throw this.createError(
|
||||||
CursorErrorCode.NOT_INSTALLED,
|
CursorErrorCode.NOT_INSTALLED,
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import path from 'path';
|
|||||||
import * as secureFs from '../lib/secure-fs.js';
|
import * as secureFs from '../lib/secure-fs.js';
|
||||||
import type { EventEmitter } from '../lib/events.js';
|
import type { EventEmitter } from '../lib/events.js';
|
||||||
import type { ExecuteOptions, ThinkingLevel, ReasoningEffort } from '@automaker/types';
|
import type { ExecuteOptions, ThinkingLevel, ReasoningEffort } from '@automaker/types';
|
||||||
|
import { stripProviderPrefix } from '@automaker/types';
|
||||||
import {
|
import {
|
||||||
readImageAsBase64,
|
readImageAsBase64,
|
||||||
buildPromptWithImages,
|
buildPromptWithImages,
|
||||||
@@ -290,13 +291,17 @@ export class AgentService {
|
|||||||
const maxTurns = sdkOptions.maxTurns;
|
const maxTurns = sdkOptions.maxTurns;
|
||||||
const allowedTools = sdkOptions.allowedTools as string[] | undefined;
|
const allowedTools = sdkOptions.allowedTools as string[] | undefined;
|
||||||
|
|
||||||
// Get provider for this model
|
// Get provider for this model (with prefix)
|
||||||
const provider = ProviderFactory.getProviderForModel(effectiveModel);
|
const provider = ProviderFactory.getProviderForModel(effectiveModel);
|
||||||
|
|
||||||
|
// Strip provider prefix - providers should receive bare model IDs
|
||||||
|
const bareModel = stripProviderPrefix(effectiveModel);
|
||||||
|
|
||||||
// Build options for provider
|
// Build options for provider
|
||||||
const options: ExecuteOptions = {
|
const options: ExecuteOptions = {
|
||||||
prompt: '', // Will be set below based on images
|
prompt: '', // Will be set below based on images
|
||||||
model: effectiveModel,
|
model: bareModel, // Bare model ID (e.g., "gpt-5.1-codex-max", "composer-1")
|
||||||
|
originalModel: effectiveModel, // Original with prefix for logging (e.g., "codex-gpt-5.1-codex-max")
|
||||||
cwd: effectiveWorkDir,
|
cwd: effectiveWorkDir,
|
||||||
systemPrompt: sdkOptions.systemPrompt,
|
systemPrompt: sdkOptions.systemPrompt,
|
||||||
maxTurns: maxTurns,
|
maxTurns: maxTurns,
|
||||||
|
|||||||
@@ -40,6 +40,7 @@ import type { SettingsService } from './settings-service.js';
|
|||||||
import type { FeatureLoader } from './feature-loader.js';
|
import type { FeatureLoader } from './feature-loader.js';
|
||||||
import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js';
|
import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js';
|
||||||
import { resolveModelString } from '@automaker/model-resolver';
|
import { resolveModelString } from '@automaker/model-resolver';
|
||||||
|
import { stripProviderPrefix } from '@automaker/types';
|
||||||
|
|
||||||
const logger = createLogger('IdeationService');
|
const logger = createLogger('IdeationService');
|
||||||
|
|
||||||
@@ -201,7 +202,7 @@ export class IdeationService {
|
|||||||
existingWorkContext
|
existingWorkContext
|
||||||
);
|
);
|
||||||
|
|
||||||
// Resolve model alias to canonical identifier
|
// Resolve model alias to canonical identifier (with prefix)
|
||||||
const modelId = resolveModelString(options?.model ?? 'sonnet');
|
const modelId = resolveModelString(options?.model ?? 'sonnet');
|
||||||
|
|
||||||
// Create SDK options
|
// Create SDK options
|
||||||
@@ -214,9 +215,13 @@ export class IdeationService {
|
|||||||
|
|
||||||
const provider = ProviderFactory.getProviderForModel(modelId);
|
const provider = ProviderFactory.getProviderForModel(modelId);
|
||||||
|
|
||||||
|
// Strip provider prefix - providers need bare model IDs
|
||||||
|
const bareModel = stripProviderPrefix(modelId);
|
||||||
|
|
||||||
const executeOptions: ExecuteOptions = {
|
const executeOptions: ExecuteOptions = {
|
||||||
prompt: message,
|
prompt: message,
|
||||||
model: modelId,
|
model: bareModel,
|
||||||
|
originalModel: modelId,
|
||||||
cwd: projectPath,
|
cwd: projectPath,
|
||||||
systemPrompt: sdkOptions.systemPrompt,
|
systemPrompt: sdkOptions.systemPrompt,
|
||||||
maxTurns: 1, // Single turn for ideation
|
maxTurns: 1, // Single turn for ideation
|
||||||
@@ -648,7 +653,7 @@ export class IdeationService {
|
|||||||
existingWorkContext
|
existingWorkContext
|
||||||
);
|
);
|
||||||
|
|
||||||
// Resolve model alias to canonical identifier
|
// Resolve model alias to canonical identifier (with prefix)
|
||||||
const modelId = resolveModelString('sonnet');
|
const modelId = resolveModelString('sonnet');
|
||||||
|
|
||||||
// Create SDK options
|
// Create SDK options
|
||||||
@@ -661,9 +666,13 @@ export class IdeationService {
|
|||||||
|
|
||||||
const provider = ProviderFactory.getProviderForModel(modelId);
|
const provider = ProviderFactory.getProviderForModel(modelId);
|
||||||
|
|
||||||
|
// Strip provider prefix - providers need bare model IDs
|
||||||
|
const bareModel = stripProviderPrefix(modelId);
|
||||||
|
|
||||||
const executeOptions: ExecuteOptions = {
|
const executeOptions: ExecuteOptions = {
|
||||||
prompt: prompt.prompt,
|
prompt: prompt.prompt,
|
||||||
model: modelId,
|
model: bareModel,
|
||||||
|
originalModel: modelId,
|
||||||
cwd: projectPath,
|
cwd: projectPath,
|
||||||
systemPrompt: sdkOptions.systemPrompt,
|
systemPrompt: sdkOptions.systemPrompt,
|
||||||
maxTurns: 1,
|
maxTurns: 1,
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Hello',
|
prompt: 'Hello',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -88,6 +89,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Test',
|
prompt: 'Test',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -112,6 +114,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Test',
|
prompt: 'Test',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
abortController,
|
abortController,
|
||||||
});
|
});
|
||||||
@@ -140,6 +143,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Current message',
|
prompt: 'Current message',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
conversationHistory,
|
conversationHistory,
|
||||||
sdkSessionId: 'test-session-id',
|
sdkSessionId: 'test-session-id',
|
||||||
@@ -170,6 +174,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: arrayPrompt as any,
|
prompt: arrayPrompt as any,
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -189,6 +194,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Test',
|
prompt: 'Test',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -214,6 +220,7 @@ describe('claude-provider.ts', () => {
|
|||||||
|
|
||||||
const generator = provider.executeQuery({
|
const generator = provider.executeQuery({
|
||||||
prompt: 'Test',
|
prompt: 'Test',
|
||||||
|
model: 'claude-opus-4-5-20251101',
|
||||||
cwd: '/test',
|
cwd: '/test',
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -185,6 +185,7 @@ export {
|
|||||||
addProviderPrefix,
|
addProviderPrefix,
|
||||||
getBareModelId,
|
getBareModelId,
|
||||||
normalizeModelString,
|
normalizeModelString,
|
||||||
|
validateBareModelId,
|
||||||
} from './provider-utils.js';
|
} from './provider-utils.js';
|
||||||
|
|
||||||
// Pipeline types
|
// Pipeline types
|
||||||
|
|||||||
@@ -196,3 +196,37 @@ export function normalizeModelString(model: string | undefined | null): string {
|
|||||||
|
|
||||||
return model;
|
return model;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validate that a model ID does not contain a provider prefix
|
||||||
|
*
|
||||||
|
* Providers should receive bare model IDs (e.g., "gpt-5.1-codex-max", "composer-1")
|
||||||
|
* without provider prefixes (e.g., NOT "codex-gpt-5.1-codex-max", NOT "cursor-composer-1").
|
||||||
|
*
|
||||||
|
* This validation ensures the ProviderFactory properly stripped prefixes before
|
||||||
|
* passing models to providers.
|
||||||
|
*
|
||||||
|
* @param model - Model ID to validate
|
||||||
|
* @param providerName - Name of the provider for error messages
|
||||||
|
* @throws Error if model contains a provider prefix
|
||||||
|
*
|
||||||
|
* @example
|
||||||
|
* validateBareModelId("gpt-5.1-codex-max", "CodexProvider"); // ✅ OK
|
||||||
|
* validateBareModelId("codex-gpt-5.1-codex-max", "CodexProvider"); // ❌ Throws error
|
||||||
|
*/
|
||||||
|
export function validateBareModelId(model: string, providerName: string): void {
|
||||||
|
if (!model || typeof model !== 'string') {
|
||||||
|
throw new Error(`[${providerName}] Invalid model ID: expected string, got ${typeof model}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
for (const [provider, prefix] of Object.entries(PROVIDER_PREFIXES)) {
|
||||||
|
if (model.startsWith(prefix)) {
|
||||||
|
throw new Error(
|
||||||
|
`[${providerName}] Model ID should not contain provider prefix '${prefix}'. ` +
|
||||||
|
`Got: '${model}'. ` +
|
||||||
|
`This is likely a bug in ProviderFactory - it should strip the '${provider}' prefix ` +
|
||||||
|
`before passing the model to the provider.`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -81,7 +81,10 @@ export interface McpHttpServerConfig {
|
|||||||
*/
|
*/
|
||||||
export interface ExecuteOptions {
|
export interface ExecuteOptions {
|
||||||
prompt: string | Array<{ type: string; text?: string; source?: object }>;
|
prompt: string | Array<{ type: string; text?: string; source?: object }>;
|
||||||
|
/** Bare model ID without provider prefix (e.g., "gpt-5.1-codex-max", "composer-1") */
|
||||||
model: string;
|
model: string;
|
||||||
|
/** Original model ID with provider prefix for logging (e.g., "codex-gpt-5.1-codex-max") */
|
||||||
|
originalModel?: string;
|
||||||
cwd: string;
|
cwd: string;
|
||||||
systemPrompt?: string | SystemPromptPreset;
|
systemPrompt?: string | SystemPromptPreset;
|
||||||
maxTurns?: number;
|
maxTurns?: number;
|
||||||
|
|||||||
Reference in New Issue
Block a user