takt: github-issue-390-to-no-provide (#393)
This commit is contained in:
parent
798e89605d
commit
551299dbf8
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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' });
|
||||
});
|
||||
});
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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 };
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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<string, { provider?: string; model?: string }>;
|
||||
|
||||
function resolveProviderInfo(
|
||||
step: { personaDisplayName?: string; provider?: string; model?: string },
|
||||
opts: Record<string, unknown>,
|
||||
): { 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<string, unknown>;
|
||||
@ -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 };
|
||||
|
||||
@ -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(),
|
||||
|
||||
@ -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);
|
||||
|
||||
@ -18,6 +18,7 @@ export { AskUserQuestionDeniedError, createDenyAskUserQuestionHandler } from './
|
||||
export type {
|
||||
PieceEvents,
|
||||
PhaseName,
|
||||
MovementProviderInfo,
|
||||
UserInputRequest,
|
||||
IterationLimitRequest,
|
||||
SessionUpdateCallback,
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user