diff --git a/e2e/helpers/isolated-env.ts b/e2e/helpers/isolated-env.ts index 0baf538..5f08be4 100644 --- a/e2e/helpers/isolated-env.ts +++ b/e2e/helpers/isolated-env.ts @@ -24,11 +24,13 @@ export function createIsolatedEnv(): IsolatedEnv { const gitConfigPath = join(baseDir, '.gitconfig'); // Create TAKT config directory and config.yaml + // Use TAKT_E2E_PROVIDER to match config provider with the actual provider being tested + const configProvider = process.env.TAKT_E2E_PROVIDER ?? 'claude'; mkdirSync(taktDir, { recursive: true }); writeFileSync( join(taktDir, 'config.yaml'), [ - 'provider: claude', + `provider: ${configProvider}`, 'language: en', 'log_level: info', 'default_piece: default', diff --git a/resources/global/ja/stances/coding.md b/resources/global/ja/stances/coding.md index 15574e4..6e586c4 100644 --- a/resources/global/ja/stances/coding.md +++ b/resources/global/ja/stances/coding.md @@ -26,6 +26,7 @@ | デフォルト引数の濫用 | `function f(x = 'default')` で全呼び出し元が省略 | 値がどこから来るか分からない | | null合体で渡す口がない | `options?.cwd ?? process.cwd()` で上位から渡す経路なし | 常にフォールバックになる（意味がない） | | try-catch で空値返却 | `catch { return ''; }` | エラーを握りつぶす | +| 不整合な値のサイレントスキップ | `if (a !== expected) return undefined` | 設定ミスが実行時に黙って無視される | ### 正しい実装 @@ -73,6 +74,7 @@ function createEngine(config, cwd: string) { 1. **必須データか？** → フォールバックせず、エラーにする 2. **全呼び出し元が省略しているか？** → デフォルト引数を削除し、必須にする 3. **上位から値を渡す経路があるか？** → なければ引数・フィールドを追加 +4. **関連する値に不変条件があるか？** → ロード・セットアップ時にクロスバリデーションする ## 抽象化 @@ -131,19 +133,19 @@ function processOrder(order) { ```typescript // ❌ メソッド増殖 — 構成の違いを呼び出し側に押し付けている -interface Provider { - call(name, prompt, options) - callCustom(name, prompt, systemPrompt, options) - callAgent(name, prompt, options) - callSkill(name, prompt, options) +interface NotificationService { + sendEmail(to, subject, body) + sendSMS(to, message) + sendPush(to, title, body) + sendSlack(channel, message) } // ✅ 構成と実行の分離 -interface Provider { - setup(config: Setup): Agent +interface NotificationService { + setup(config: ChannelConfig): Channel } -interface Agent { - call(prompt, options): Promise +interface Channel { + send(message: Message): Promise } ``` @@ -153,14 +155,14 @@ interface Agent { ```typescript // ❌ 汎用層に特定実装のインポートと分岐 -import { callSpecificImpl } from '../specific/index.js' -if (config.specificFlag) { - return callSpecificImpl(config.name, task, options) +import { uploadToS3 } from '../aws/s3.js' +if (config.storage === 's3') { + return uploadToS3(config.bucket, file, options) } -// ✅ 汎用層はインターフェースのみ。非対応は setup 時にエラー -const agent = provider.setup({ specificFlag: config.specificFlag }) -return agent.call(task, options) +// ✅ 汎用層はインターフェースのみ。非対応は生成時にエラー +const storage = createStorage(config) +return storage.upload(file, options) ``` ## 構造 diff --git a/resources/global/ja/stances/review.md b/resources/global/ja/stances/review.md index 94daa81..fd5477f 100644 --- a/resources/global/ja/stances/review.md +++ b/resources/global/ja/stances/review.md @@ -40,6 +40,7 @@ - 3箇所以上の重複コード（DRY違反） - 同じことをするメソッドの増殖（構成の違いで吸収すべき） - 特定実装の汎用層への漏洩（汎用層に特定実装のインポート・分岐がある） +- 関連フィールドのクロスバリデーション欠如（意味的に結合した設定値の不変条件が未検証） ### Warning（警告） @@ -112,9 +113,9 @@ ## 堂々巡りの検出 -レビュー回数が渡される場合（例: 「レビュー回数: 3回目」）、回数に応じて判断を変える。 +同じ種類の指摘が繰り返されている場合、修正指示の繰り返しではなくアプローチ自体を見直す。 -### 3回目以降のレビューでは +### 同じ問題が繰り返されたら 1. 同じ種類の問題が繰り返されていないか確認 2. 繰り返されている場合、細かい修正指示ではなくアプローチ自体の代替案を提示 diff --git a/resources/global/ja/stances/testing.md b/resources/global/ja/stances/testing.md index e323e0b..0f633d7 100644 --- a/resources/global/ja/stances/testing.md +++ b/resources/global/ja/stances/testing.md @@ -67,3 +67,22 @@ test('ユーザーが存在しない場合、NotFoundエラーを返す', async - ロジックにはユニットテスト、境界にはインテグレーションテストを優先 - ユニットテストでカバーできるものにE2Eテストを使いすぎない - 新しいロジックにE2Eテストしかない場合、ユニットテストの追加を提案する + +## テスト環境の分離 + +テストインフラの設定はテストシナリオのパラメータに連動させる。ハードコードされた前提は別シナリオで壊れる。 + +| 原則 | 基準 | +|------|------| +| パラメータ連動 | テストの入力パラメータに応じてフィクスチャ・設定を生成する | +| 暗黙の前提排除 | 特定の環境（ユーザーの個人設定等）に依存しない | +| 整合性 | テスト設定内の関連する値は互いに矛盾しない | + +```typescript +// ❌ ハードコードされた前提 — 別のバックエンドでテストすると不整合になる +writeConfig({ backend: 'postgres', connectionPool: 10 }) + +// ✅ パラメータに連動 +const backend = process.env.TEST_BACKEND ?? 'postgres' +writeConfig({ backend, connectionPool: backend === 'sqlite' ? 1 : 10 }) +``` diff --git a/src/__tests__/globalConfig-defaults.test.ts b/src/__tests__/globalConfig-defaults.test.ts index 9718d1e..2a6b159 100644 --- a/src/__tests__/globalConfig-defaults.test.ts +++ b/src/__tests__/globalConfig-defaults.test.ts @@ -240,4 +240,78 @@ describe('loadGlobalConfig', () => { const config = loadGlobalConfig(); expect(config.preventSleep).toBeUndefined(); }); + + describe('provider/model compatibility validation', () => { + it('should throw when provider is codex but model is a Claude alias (opus)', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: codex\nmodel: opus\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).toThrow(/model 'opus' is a Claude model alias but provider is 'codex'/); + }); + + it('should throw when provider is codex but model is sonnet', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: codex\nmodel: sonnet\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).toThrow(/model 'sonnet' is a Claude model alias but provider is 'codex'/); + }); + + it('should throw when provider is codex but model is haiku', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: codex\nmodel: haiku\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).toThrow(/model 'haiku' is a Claude model alias but provider is 'codex'/); + }); + + it('should not throw when provider is codex with a compatible model', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: codex\nmodel: gpt-4o\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).not.toThrow(); + }); + + it('should not throw when provider is claude with Claude models', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: claude\nmodel: opus\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).not.toThrow(); + }); + + it('should not throw when provider is codex without a model', () => { + const taktDir = join(testHomeDir, '.takt'); + mkdirSync(taktDir, { recursive: true }); + writeFileSync( + getGlobalConfigPath(), + 'provider: codex\n', + 'utf-8', + ); + + expect(() => loadGlobalConfig()).not.toThrow(); + }); + }); }); diff --git a/src/infra/config/global/globalConfig.ts b/src/infra/config/global/globalConfig.ts index d6d4cdc..9d503a4 100644 --- a/src/infra/config/global/globalConfig.ts +++ b/src/infra/config/global/globalConfig.ts @@ -12,6 +12,21 @@ import type { GlobalConfig, DebugConfig, Language } from '../../../core/models/i import { getGlobalConfigPath, getProjectConfigPath } from '../paths.js'; import { DEFAULT_LANGUAGE } from '../../../shared/constants.js'; +/** Claude-specific model aliases that are not valid for other providers */ +const CLAUDE_MODEL_ALIASES = new Set(['opus', 'sonnet', 'haiku']); + +/** Validate that provider and model are compatible */ +function validateProviderModelCompatibility(provider: string | undefined, model: string | undefined): void { + if (!provider || !model) return; + + if (provider === 'codex' && CLAUDE_MODEL_ALIASES.has(model)) { + throw new Error( + `Configuration error: model '${model}' is a Claude model alias but provider is '${provider}'. ` + + `Either change the provider to 'claude' or specify a Codex-compatible model.` + ); + } +} + /** Create default global configuration (fresh instance each call) */ function createDefaultGlobalConfig(): GlobalConfig { return { @@ -91,6 +106,7 @@ export class GlobalConfigManager { branchNameStrategy: parsed.branch_name_strategy, preventSleep: parsed.prevent_sleep, }; + validateProviderModelCompatibility(config.provider, config.model); this.cachedConfig = config; return config; }