update stances
This commit is contained in:
nrslib
parent
e23cfa9a3b
commit
3b23493213
@ -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',
|
||||
|
||||
@ -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<Response>
|
||||
interface Channel {
|
||||
send(message: Message): Promise<Result>
|
||||
}
|
||||
```
|
||||
|
||||
@ -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)
|
||||
```
|
||||
|
||||
## 構造
|
||||
|
||||
@ -40,6 +40,7 @@
|
||||
- 3箇所以上の重複コード（DRY違反）
|
||||
- 同じことをするメソッドの増殖（構成の違いで吸収すべき）
|
||||
- 特定実装の汎用層への漏洩（汎用層に特定実装のインポート・分岐がある）
|
||||
- 関連フィールドのクロスバリデーション欠如（意味的に結合した設定値の不変条件が未検証）
|
||||
|
||||
### Warning（警告）
|
||||
|
||||
@ -112,9 +113,9 @@
|
||||
|
||||
## 堂々巡りの検出
|
||||
|
||||
レビュー回数が渡される場合（例: 「レビュー回数: 3回目」）、回数に応じて判断を変える。
|
||||
同じ種類の指摘が繰り返されている場合、修正指示の繰り返しではなくアプローチ自体を見直す。
|
||||
|
||||
### 3回目以降のレビューでは
|
||||
### 同じ問題が繰り返されたら
|
||||
|
||||
1. 同じ種類の問題が繰り返されていないか確認
|
||||
2. 繰り返されている場合、細かい修正指示ではなくアプローチ自体の代替案を提示
|
||||
|
||||
@ -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 })
|
||||
```
|
||||
|
||||
@ -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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@ -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;
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user