diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f36f1..2fc4460 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `--task` オプションでの直接実行時に tasks.yaml へ不要な記録がされる問題を修正 - `--task` でワークツリー作成時は tasks.yaml に記録するよう修正(`takt list` でのブランチ管理に必要) - Provider resolution: removed implicit fallback to `claude` and switched to fail-fast when provider cannot be resolved (#386) +- Provider resolution: unified display and execution provider/model resolution via `movement:start` event providerInfo, ensuring displayed provider always matches execution provider (#390) - E2E テスト config-priority の不安定性を修正 (#388) ### Internal diff --git a/docs/CHANGELOG.ja.md b/docs/CHANGELOG.ja.md index 41847b5..a5b7225 100644 --- a/docs/CHANGELOG.ja.md +++ b/docs/CHANGELOG.ja.md @@ -28,6 +28,7 @@ - `--task` オプションでの直接実行時に tasks.yaml へ不要な記録がされる問題を修正 - `--task` でワークツリー作成時は tasks.yaml に記録するよう修正(`takt list` でのブランチ管理に必要) - プロバイダー解決: 暗黙の `claude` フォールバックを廃止し、プロバイダーを解決できない場合は Fail Fast で終了するよう修正 (#386) +- プロバイダー解決: 表示用と実行用の provider/model 解決を `movement:start` イベントの providerInfo に一元化し、表示されるプロバイダーと実行プロバイダーの一致を構造的に保証 (#390) - E2E テスト config-priority の不安定性を修正 (#388) ### Internal diff --git a/src/__tests__/engine-persona-providers.test.ts b/src/__tests__/engine-persona-providers.test.ts index 97b86e0..118c454 100644 --- a/src/__tests__/engine-persona-providers.test.ts +++ b/src/__tests__/engine-persona-providers.test.ts @@ -306,4 +306,71 @@ describe('PieceEngine persona_providers override', () => { expect(options.stepProvider).toBe('codex'); expect(options.stepModel).toBe('persona-model'); }); + + it('should emit providerInfo in movement:start matching stepProvider/stepModel', async () => { + const movement = makeMovement('implement', { + personaDisplayName: 'coder', + rules: [makeRule('done', 'COMPLETE')], + }); + const config: PieceConfig = { + name: 'provider-info-event-test', + movements: [movement], + initialMovement: 'implement', + maxMovements: 1, + }; + + mockRunAgentSequence([ + makeResponse({ persona: movement.persona, content: 'done' }), + ]); + mockDetectMatchedRuleSequence([{ index: 0, method: 'phase1_tag' }]); + + const engine = new PieceEngine(config, '/tmp/project', 'test task', { + projectCwd: '/tmp/project', + provider: 'claude', + model: 'global-model', + personaProviders: { coder: { provider: 'codex', model: 'o3-mini' } }, + }); + + const startFn = vi.fn(); + engine.on('movement:start', startFn); + + await engine.run(); + + expect(startFn).toHaveBeenCalledTimes(1); + const [, , , providerInfo] = startFn.mock.calls[0]; + expect(providerInfo).toEqual({ provider: 'codex', model: 'o3-mini' }); + }); + + it('should emit engine-level provider in providerInfo when persona has no override', async () => { + const movement = makeMovement('plan', { + personaDisplayName: 'planner', + rules: [makeRule('done', 'COMPLETE')], + }); + const config: PieceConfig = { + name: 'provider-info-no-override', + movements: [movement], + initialMovement: 'plan', + maxMovements: 1, + }; + + mockRunAgentSequence([ + makeResponse({ persona: movement.persona, content: 'done' }), + ]); + mockDetectMatchedRuleSequence([{ index: 0, method: 'phase1_tag' }]); + + const engine = new PieceEngine(config, '/tmp/project', 'test task', { + projectCwd: '/tmp/project', + provider: 'claude', + model: 'sonnet', + }); + + const startFn = vi.fn(); + engine.on('movement:start', startFn); + + await engine.run(); + + expect(startFn).toHaveBeenCalledTimes(1); + const [, , , providerInfo] = startFn.mock.calls[0]; + expect(providerInfo).toEqual({ provider: 'claude', model: 'sonnet' }); + }); }); diff --git a/src/__tests__/options-builder.test.ts b/src/__tests__/options-builder.test.ts index 9acc219..c816590 100644 --- a/src/__tests__/options-builder.test.ts +++ b/src/__tests__/options-builder.test.ts @@ -114,6 +114,86 @@ describe('OptionsBuilder.buildBaseOptions', () => { }); }); +describe('OptionsBuilder.resolveStepProviderModel', () => { + it('should return engine-level provider and model when step has no overrides', () => { + const step = createMovement(); + const builder = createBuilder(step, { provider: 'claude', model: 'sonnet' }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.provider).toBe('claude'); + expect(result.model).toBe('sonnet'); + }); + + it('should prioritize persona providers over engine-level provider', () => { + const step = createMovement({ personaDisplayName: 'coder' }); + const builder = createBuilder(step, { + provider: 'claude', + model: 'sonnet', + personaProviders: { coder: { provider: 'codex', model: 'o3-mini' } }, + }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.provider).toBe('codex'); + expect(result.model).toBe('o3-mini'); + }); + + it('should prioritize step-level provider over engine-level provider', () => { + const step = createMovement({ provider: 'opencode' as 'opencode' }); + const builder = createBuilder(step, { provider: 'claude' }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.provider).toBe('opencode'); + }); + + it('should prioritize persona providers over step-level provider', () => { + const step = createMovement({ personaDisplayName: 'coder', provider: 'claude' as 'claude' }); + const builder = createBuilder(step, { + provider: 'mock', + personaProviders: { coder: { provider: 'codex' } }, + }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.provider).toBe('codex'); + }); + + it('should return undefined model when no model is configured', () => { + const step = createMovement(); + const builder = createBuilder(step, { provider: 'claude', model: undefined }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.model).toBeUndefined(); + }); + + it('should return undefined provider when no provider is configured', () => { + const step = createMovement(); + const builder = createBuilder(step, { provider: undefined }); + + const result = builder.resolveStepProviderModel(step); + + expect(result.provider).toBeUndefined(); + }); + + it('should match buildBaseOptions stepProvider and stepModel', () => { + const step = createMovement({ personaDisplayName: 'coder' }); + const builder = createBuilder(step, { + provider: 'claude', + model: 'sonnet', + personaProviders: { coder: { provider: 'codex', model: 'o3-mini' } }, + }); + + const providerInfo = builder.resolveStepProviderModel(step); + const baseOptions = builder.buildBaseOptions(step); + + expect(providerInfo.provider).toBe(baseOptions.stepProvider); + expect(providerInfo.model).toBe(baseOptions.stepModel); + }); +}); + describe('OptionsBuilder.buildResumeOptions', () => { it('should enforce readonly permission and empty allowedTools for report/status phases', () => { // Given diff --git a/src/__tests__/pieceExecution-ask-user-question.test.ts b/src/__tests__/pieceExecution-ask-user-question.test.ts index 50eae35..81318a6 100644 --- a/src/__tests__/pieceExecution-ask-user-question.test.ts +++ b/src/__tests__/pieceExecution-ask-user-question.test.ts @@ -31,7 +31,7 @@ const { MockPieceEngine } = vi.hoisted(() => { async run(): Promise<{ status: string; iteration: number }> { const firstStep = this.config.movements[0]; if (firstStep) { - this.emit('movement:start', firstStep, 1, firstStep.instructionTemplate); + this.emit('movement:start', firstStep, 1, firstStep.instructionTemplate, { provider: undefined, model: undefined }); } this.emit('piece:complete', { status: 'completed', iteration: 1 }); return { status: 'completed', iteration: 1 }; diff --git a/src/__tests__/pieceExecution-debug-prompts.test.ts b/src/__tests__/pieceExecution-debug-prompts.test.ts index a09c940..123cf68 100644 --- a/src/__tests__/pieceExecution-debug-prompts.test.ts +++ b/src/__tests__/pieceExecution-debug-prompts.test.ts @@ -33,7 +33,8 @@ const { mockIsDebugEnabled, mockWritePromptLog, MockPieceEngine } = vi.hoisted(( const shouldAbort = this.task === 'abort-task'; const shouldRepeatMovement = this.task === 'repeat-movement-task'; - this.emit('movement:start', step, 1, 'movement instruction'); + const providerInfo = { provider: undefined, model: undefined }; + this.emit('movement:start', step, 1, 'movement instruction', providerInfo); this.emit('phase:start', step, 1, 'execute', 'phase prompt'); this.emit('phase:complete', step, 1, 'execute', 'phase response', 'done'); this.emit( @@ -48,7 +49,7 @@ const { mockIsDebugEnabled, mockWritePromptLog, MockPieceEngine } = vi.hoisted(( 'movement instruction' ); if (shouldRepeatMovement) { - this.emit('movement:start', step, 2, 'movement instruction repeat'); + this.emit('movement:start', step, 2, 'movement instruction repeat', providerInfo); this.emit( 'movement:complete', step, diff --git a/src/__tests__/pieceExecution-session-loading.test.ts b/src/__tests__/pieceExecution-session-loading.test.ts index be611ab..bcc952c 100644 --- a/src/__tests__/pieceExecution-session-loading.test.ts +++ b/src/__tests__/pieceExecution-session-loading.test.ts @@ -15,6 +15,19 @@ const { MockPieceEngine, mockLoadPersonaSessions, mockLoadWorktreeSessions } = v const mockLoadPersonaSessions = vi.fn().mockReturnValue({ coder: 'saved-session-id' }); const mockLoadWorktreeSessions = vi.fn().mockReturnValue({ coder: 'worktree-session-id' }); + type PersonaProviderMap = Record; + + function resolveProviderInfo( + step: { personaDisplayName?: string; provider?: string; model?: string }, + opts: Record, + ): { provider: string | undefined; model: string | undefined } { + const personaProviders = opts.personaProviders as PersonaProviderMap | undefined; + const personaEntry = personaProviders?.[step.personaDisplayName ?? '']; + const provider = personaEntry?.provider ?? step.provider ?? opts.provider as string | undefined; + const model = personaEntry?.model ?? step.model ?? opts.model as string | undefined; + return { provider, model }; + } + class MockPieceEngine extends EE { static lastInstance: MockPieceEngine; readonly receivedOptions: Record; @@ -32,7 +45,8 @@ const { MockPieceEngine, mockLoadPersonaSessions, mockLoadWorktreeSessions } = v async run(): Promise<{ status: string; iteration: number }> { const firstStep = this.config.movements[0]; if (firstStep) { - this.emit('movement:start', firstStep, 1, firstStep.instructionTemplate); + const providerInfo = resolveProviderInfo(firstStep, this.receivedOptions); + this.emit('movement:start', firstStep, 1, firstStep.instructionTemplate, providerInfo); } this.emit('piece:complete', { status: 'completed', iteration: 1 }); return { status: 'completed', iteration: 1 }; diff --git a/src/core/piece/engine/OptionsBuilder.ts b/src/core/piece/engine/OptionsBuilder.ts index 8515e19..78231bf 100644 --- a/src/core/piece/engine/OptionsBuilder.ts +++ b/src/core/piece/engine/OptionsBuilder.ts @@ -3,7 +3,7 @@ import type { PieceMovement, PieceState, Language } from '../../models/types.js' import type { MovementProviderOptions } from '../../models/piece-types.js'; import type { RunAgentOptions } from '../../../agents/runner.js'; import type { PhaseRunnerContext } from '../phase-runner.js'; -import type { PieceEngineOptions, PhaseName } from '../types.js'; +import type { PieceEngineOptions, PhaseName, MovementProviderInfo } from '../types.js'; import { buildSessionKey } from '../session-key.js'; import { resolveMovementProviderModel } from '../provider-resolution.js'; import { DEFAULT_PROVIDER_PERMISSION_PROFILES, resolveMovementPermissionMode } from '../permission-profile-resolution.js'; @@ -53,19 +53,26 @@ export class OptionsBuilder { private readonly getPieceDescription: () => string | undefined, ) {} - /** Build common RunAgentOptions shared by all phases */ - buildBaseOptions(step: PieceMovement): RunAgentOptions { - const movements = this.getPieceMovements(); - const currentIndex = movements.findIndex((m) => m.name === step.name); - const currentPosition = currentIndex >= 0 ? `${currentIndex + 1}/${movements.length}` : '?/?'; + /** Resolve effective provider and model for a movement (same logic as buildBaseOptions) */ + resolveStepProviderModel(step: PieceMovement): MovementProviderInfo { const resolved = resolveMovementProviderModel({ step, provider: this.engineOptions.provider, model: this.engineOptions.model, personaProviders: this.engineOptions.personaProviders, }); - const resolvedProvider = resolved.provider ?? this.engineOptions.provider; - const resolvedModel = resolved.model ?? this.engineOptions.model; + return { + provider: resolved.provider ?? this.engineOptions.provider, + model: resolved.model ?? this.engineOptions.model, + }; + } + + /** Build common RunAgentOptions shared by all phases */ + buildBaseOptions(step: PieceMovement): RunAgentOptions { + const movements = this.getPieceMovements(); + const currentIndex = movements.findIndex((m) => m.name === step.name); + const currentPosition = currentIndex >= 0 ? `${currentIndex + 1}/${movements.length}` : '?/?'; + const { provider: resolvedProvider, model: resolvedModel } = this.resolveStepProviderModel(step); return { cwd: this.getCwd(), diff --git a/src/core/piece/engine/PieceEngine.ts b/src/core/piece/engine/PieceEngine.ts index 364fcf3..3a31956 100644 --- a/src/core/piece/engine/PieceEngine.ts +++ b/src/core/piece/engine/PieceEngine.ts @@ -39,6 +39,7 @@ const log = createLogger('engine'); export type { PieceEvents, + MovementProviderInfo, UserInputRequest, IterationLimitRequest, SessionUpdateCallback, @@ -492,7 +493,7 @@ export class PieceEngine extends EventEmitter { judgeMovement, movementIteration, this.state, this.task, this.config.maxMovements, ); - this.emit('movement:start', judgeMovement, this.state.iteration, prebuiltInstruction); + this.emit('movement:start', judgeMovement, this.state.iteration, prebuiltInstruction, this.optionsBuilder.resolveStepProviderModel(judgeMovement)); const { response, instruction } = await this.movementExecutor.runNormalMovement( judgeMovement, @@ -579,7 +580,7 @@ export class PieceEngine extends EventEmitter { movement, movementIteration, this.state, this.task, this.config.maxMovements, ); } - this.emit('movement:start', movement, this.state.iteration, prebuiltInstruction ?? ''); + this.emit('movement:start', movement, this.state.iteration, prebuiltInstruction ?? '', this.optionsBuilder.resolveStepProviderModel(movement)); try { const { response, instruction } = await this.runMovement(movement, prebuiltInstruction); diff --git a/src/core/piece/index.ts b/src/core/piece/index.ts index d93e7d8..f52d653 100644 --- a/src/core/piece/index.ts +++ b/src/core/piece/index.ts @@ -18,6 +18,7 @@ export { AskUserQuestionDeniedError, createDenyAskUserQuestionHandler } from './ export type { PieceEvents, PhaseName, + MovementProviderInfo, UserInputRequest, IterationLimitRequest, SessionUpdateCallback, diff --git a/src/core/piece/types.ts b/src/core/piece/types.ts index 1cd2639..86c49c5 100644 --- a/src/core/piece/types.ts +++ b/src/core/piece/types.ts @@ -110,9 +110,15 @@ export type AiJudgeCaller = ( export type PhaseName = 'execute' | 'report' | 'judge'; +/** Provider and model info resolved for a movement */ +export interface MovementProviderInfo { + provider: ProviderType | undefined; + model: string | undefined; +} + /** Events emitted by piece engine */ export interface PieceEvents { - 'movement:start': (step: PieceMovement, iteration: number, instruction: string) => void; + 'movement:start': (step: PieceMovement, iteration: number, instruction: string, providerInfo: MovementProviderInfo) => void; 'movement:complete': (step: PieceMovement, response: AgentResponse, instruction: string) => void; 'movement:report': (step: PieceMovement, filePath: string, fileName: string) => void; 'movement:blocked': (step: PieceMovement, response: AgentResponse) => void; diff --git a/src/features/tasks/execute/pieceExecution.ts b/src/features/tasks/execute/pieceExecution.ts index 0e8e888..b7a33d6 100644 --- a/src/features/tasks/execute/pieceExecution.ts +++ b/src/features/tasks/execute/pieceExecution.ts @@ -71,7 +71,6 @@ import { getLabel } from '../../../shared/i18n/index.js'; import { EXIT_SIGINT } from '../../../shared/exitCodes.js'; import { ShutdownManager } from './shutdownManager.js'; import { buildRunPaths } from '../../../core/piece/run/run-paths.js'; -import { resolveMovementProviderModel } from '../../../core/piece/provider-resolution.js'; import { resolveRuntimeConfig } from '../../../core/runtime/runtime-environment.js'; import { writeFileAtomic, ensureDir } from '../../../infra/config/index.js'; import { getGlobalConfigDir } from '../../../infra/config/paths.js'; @@ -540,7 +539,7 @@ export async function executePiece( } }); - engine.on('movement:start', (step, iteration, instruction) => { + engine.on('movement:start', (step, iteration, instruction, providerInfo) => { log.debug('Movement starting', { step: step.name, persona: step.personaDisplayName, iteration }); currentIteration = iteration; const movementIteration = (movementIterations.get(step.name) ?? 0) + 1; @@ -552,15 +551,8 @@ export async function executePiece( movementIteration, }); out.info(`[${iteration}/${pieceConfig.maxMovements}] ${step.name} (${step.personaDisplayName})`); - const resolved = resolveMovementProviderModel({ - step, - provider: options.provider, - model: options.model, - personaProviders: options.personaProviders, - }); - const movementProvider = resolved.provider ?? options.provider ?? currentProvider; - const resolvedModel = resolved.model; - const movementModel = resolvedModel ?? '(default)'; + const movementProvider = providerInfo.provider ?? currentProvider; + const movementModel = providerInfo.model ?? '(default)'; currentMovementProvider = movementProvider; currentMovementModel = movementModel; providerEventLogger.setMovement(step.name);