diff --git a/.changeset/gentle-badgers-roar.md b/.changeset/gentle-badgers-roar.md new file mode 100644 index 000000000..a20eb5765 --- /dev/null +++ b/.changeset/gentle-badgers-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2048 +--- +**`model_overrides` Claude model IDs now resolve to Agent-tool aliases on the claude runtime** — a full Claude model ID (e.g. `claude-sonnet-5`) in `model_overrides` was returned verbatim and silently dropped by the Claude Agent tool (whose `model` parameter documents only tier aliases), causing the spawned subagent to inherit the parent session model instead of the configured one. It now maps to the tier alias (`sonnet`/`opus`/`haiku`/`fable`), consistent with the `model_policy` path (#1144). Bare aliases, non-Claude values, and non-Claude runtimes are unchanged; a Claude ID with no alias warns once and falls through to tier resolution. (#2041) diff --git a/src/model-resolver.cts b/src/model-resolver.cts index 2fea5d5e1..dc1bc31f1 100644 --- a/src/model-resolver.cts +++ b/src/model-resolver.cts @@ -105,6 +105,66 @@ function _resetModelPolicyWarningCacheForTests(): void { _modelPolicyUnmappableWarned.clear(); } +// Dedupe stderr warnings for unmappable model_overrides Claude IDs (#2041). +const _modelOverrideUnmappableWarned = new Set(); +function warnModelOverrideUnmappable(agentType: string, overrideValue: string): void { + const key = `${agentType}::${overrideValue}`; + if (_modelOverrideUnmappableWarned.has(key)) return; + _modelOverrideUnmappableWarned.add(key); + // Cap emission length so an oversized or secret-shaped value cannot leak in + // full to stderr/logs (#2041 security review). MUST go to stderr — resolve- + // model's JSON result is parsed from stdout. + const safe = overrideValue.length > 64 ? overrideValue.slice(0, 64) + '…' : overrideValue; + process.stderr.write( + `gsd: warning — model_overrides value "${safe}" for ${agentType} ` + + `has no Claude agent alias; falling through to tier resolution.\n`, + ); +} + +// Test-only: reset the model_overrides warn-dedupe cache between cases (#2041). +function _resetModelOverrideWarningCacheForTests(): void { + _modelOverrideUnmappableWarned.clear(); +} + +/** + * #2041 — Map a `model_overrides` value to its Claude Agent-tool alias on the + * claude runtime, mirroring the `model_policy` path (#1144). Claude Code's + * Agent tool `model` parameter documents only tier aliases (opus/sonnet/haiku/ + * fable); a full Claude model ID returned verbatim is silently dropped by the + * spawner. Returns the value to return verbatim, or null to signal "fall + * through to normal tier/dynamic-routing resolution" (used when a Claude full + * ID has no alias — matches model_policy's warn-and-fall-through). Non-Claude + * runtimes and non-Claude values always pass through verbatim. + * + * Hardening (code+security review): a `typeof` guard preserves the pre-fix + * no-crash behavior if a malformed config surfaces a non-string value, and an + * `Object.hasOwn` lookup defeats `__proto__`/`constructor` lookups on the plain + * object literal so those reserved keys cannot return a truthy non-string. + */ +function mapClaudeOverrideForRuntime( + override: string, + configRuntime: string | null | undefined, + agentType: string, +): string | null { + // Defensive: model_overrides is typed Record but a malformed + // config could surface a non-string; pass through verbatim (preserving the + // pre-fix no-crash behaviour) and let the downstream Agent tool reject it. + if (typeof override !== 'string') return override; + const onClaude = !configRuntime || configRuntime === 'claude'; + if (!onClaude) return override; + // Object.hasOwn guards against __proto__/constructor returning a truthy + // non-string from the plain object literal (#2041 security review). + if (Object.hasOwn(CLAUDE_POLICY_ID_TO_ALIAS, override)) { + return CLAUDE_POLICY_ID_TO_ALIAS[override]; + } + if (CLAUDE_AGENT_ALIASES.has(override)) return override; + if (override.startsWith('claude-')) { + warnModelOverrideUnmappable(agentType, override); + return null; + } + return override; +} + /** * #49 — Provider-neutral model policy preset resolution. */ @@ -159,11 +219,15 @@ function resolveModelPolicy(policy: Record | null | undefined, function resolveModelInternal(cwd: string, agentType: string): string { const config = loadConfig(cwd); - // 1. Per-agent override + // 1. Per-agent override (#2041: map Claude full IDs → Agent-tool aliases on + // the claude runtime, mirroring the model_policy path #1144; non-Claude + // runtimes and non-Claude values pass through verbatim). const modelOverrides = config['model_overrides'] as Record | null | undefined; const override = modelOverrides?.[agentType]; if (override) { - return override; + const mapped = mapClaudeOverrideForRuntime(override, config['runtime'] as string | null | undefined, agentType); + if (mapped !== null) return mapped; + // Unmappable Claude ID — fall through to tier resolution (matches model_policy). } // 2. Compute the tier @@ -287,7 +351,11 @@ function resolveModelForTier(cwd: string, agentType: string, attempt?: number): const modelOverrides = config['model_overrides'] as Record | null | undefined; const override = modelOverrides?.[agentType]; - if (override) return override; + if (override) { + const mapped = mapClaudeOverrideForRuntime(override, config['runtime'] as string | null | undefined, agentType); + if (mapped !== null) return mapped; + // Unmappable Claude ID — fall through to dynamic_routing / model_policy resolution. + } if (config['model_policy'] && config['runtime'] && config['runtime'] !== 'claude') { return resolveModelInternal(cwd, agentType); @@ -508,6 +576,7 @@ export = { resolveModelPolicy, resolveModelInternal, _resetModelPolicyWarningCacheForTests, + _resetModelOverrideWarningCacheForTests, VALID_GRANULARITIES, resolveGranularityInternal, assertValidGranularityOverride, diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 3538a26a5..234bc8f99 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -423,6 +423,7 @@ function resetRuntimeWarningCaches() { const modelResolver = require('../gsd-core/bin/lib/model-resolver.cjs'); configLoader._resetRuntimeWarningCacheForTests(); modelResolver._resetModelPolicyWarningCacheForTests(); + modelResolver._resetModelOverrideWarningCacheForTests(); } module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, TOOLS_PATH }; diff --git a/tests/model-resolver.test.cjs b/tests/model-resolver.test.cjs index 9fc7fa30e..3e2d06ddf 100644 --- a/tests/model-resolver.test.cjs +++ b/tests/model-resolver.test.cjs @@ -3770,6 +3770,190 @@ describe('#49 resolveModelPolicy: prototype-pollution guards', () => { }); }); +// ─── #2041: model_overrides Claude full ID → Agent-tool alias on claude runtime ─ +// +// Mirrors the #1133 model_policy alias-mapping tests (above) for the +// model_overrides path. Bug: a full Claude model ID in model_overrides +// (e.g. "claude-sonnet-5") was returned VERBATIM on the claude runtime and +// handed to the Claude Agent tool, whose typed `model` parameter documents only +// tier aliases (opus/sonnet/haiku/fable). The model_policy path already maps +// full IDs → aliases via CLAUDE_POLICY_ID_TO_ALIAS (#1144); model_overrides +// skipped that mapping entirely. The fix mirrors #1144 on the override path. +// Non-Claude runtimes and non-Claude values pass through verbatim (parity). + +describe('#2041 model_overrides: Claude full ID → alias on claude runtime', () => { + let tmpDir; + beforeEach(() => { + tmpDir = makeTmp('2041'); + resetRuntimeWarningCaches(); + }); + afterEach(() => { + rmr(tmpDir); + resetRuntimeWarningCaches(); + }); + + // AC1 + AC2: mappable Claude full IDs resolve to their aliases on claude runtime + test('model_overrides claude-sonnet-5 → "sonnet" on runtime:claude (resolveModelInternal)', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-executor': 'claude-sonnet-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'sonnet'); + }); + + test('model_overrides claude-opus-4-8 → "opus" on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-planner': 'claude-opus-4-8' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-planner'), 'opus'); + }); + + test('model_overrides claude-haiku-4-5 → "haiku" on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-codebase-mapper': 'claude-haiku-4-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-codebase-mapper'), 'haiku'); + }); + + test('model_overrides claude-fable-5 → "fable" on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-planner': 'claude-fable-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-planner'), 'fable'); + }); + + // AC3: bare aliases pass through verbatim + test('model_overrides bare "sonnet" alias passes through verbatim on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-executor': 'sonnet' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'sonnet'); + }); + + test('model_overrides bare "fable" alias passes through verbatim on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-planner': 'fable' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-planner'), 'fable'); + }); + + // AC1 (implicit claude): mapping fires when runtime key is absent (defaults to claude) + test('model_overrides claude-sonnet-5 → "sonnet" with implicit claude runtime (no runtime key)', () => { + writeConfig(tmpDir, { + model_overrides: { 'gsd-executor': 'claude-sonnet-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'sonnet'); + }); + + // AC4: non-claude runtimes keep full IDs verbatim (parity with model_policy path) + test('model_overrides claude-sonnet-5 → verbatim ID on non-claude runtime (opencode)', () => { + writeConfig(tmpDir, { + runtime: 'opencode', + model_overrides: { 'gsd-executor': 'claude-sonnet-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'claude-sonnet-5'); + }); + + // AC5: unmappable Claude full ID warns once + falls through to tier alias + test('model_overrides unmappable claude ID (claude-opus-4-5) falls through to tier alias on claude', () => { + resetRuntimeWarningCaches(); + writeConfig(tmpDir, { + runtime: 'claude', + model_profile: 'balanced', + model_overrides: { 'gsd-planner': 'claude-opus-4-5' }, + }); + // gsd-planner balanced → opus tier; claude-opus-4-5 has no alias → warn + fall through → 'opus' + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-planner'), 'opus'); + }); + + test('model_overrides unmappable claude ID emits a stderr warning exactly once (dedupe)', () => { + resetRuntimeWarningCaches(); + writeConfig(tmpDir, { + runtime: 'claude', + model_profile: 'balanced', + model_overrides: { 'gsd-planner': 'claude-opus-4-5' }, + }); + const writes = []; + const original = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk) => { writes.push(String(chunk)); return true; }; + try { + resolveModelInternal(tmpDir, 'gsd-planner'); + resolveModelInternal(tmpDir, 'gsd-planner'); // second call — dedupe must suppress + } finally { + process.stderr.write = original; + } + const warnings = writes.filter((w) => w.includes('model_overrides') && w.includes('claude-opus-4-5')); + assert.strictEqual(warnings.length, 1, + `expected exactly one override warning, got ${warnings.length}: ${JSON.stringify(writes)}`); + }); + + // AC6: resolveModelForTier (escalation / --attempt path) maps the same way + test('resolveModelForTier maps claude-sonnet-5 → "sonnet" on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-executor': 'claude-sonnet-5' }, + }); + assert.strictEqual(resolveModelForTier(tmpDir, 'gsd-executor', 0), 'sonnet'); + }); + + test('resolveModelForTier keeps full ID verbatim on non-claude runtime', () => { + writeConfig(tmpDir, { + runtime: 'opencode', + model_overrides: { 'gsd-executor': 'claude-sonnet-5' }, + }); + assert.strictEqual(resolveModelForTier(tmpDir, 'gsd-executor', 0), 'claude-sonnet-5'); + }); + + // MEDIUM-1 (review): exercise the unmappable-override fall-through branch in + // resolveModelForTier (closes the mutation-score gap — a future refactor that + // accidentally returned the verbatim override instead of falling through + // would otherwise survive the suite). + test('resolveModelForTier unmappable claude ID falls through to tier alias on claude', () => { + resetRuntimeWarningCaches(); + writeConfig(tmpDir, { + runtime: 'claude', + model_profile: 'balanced', + model_overrides: { 'gsd-planner': 'claude-opus-4-5' }, + }); + // unmappable override → fall through → no dynamic_routing → resolveModelInternal → 'opus' + assert.strictEqual(resolveModelForTier(tmpDir, 'gsd-planner', 0), 'opus'); + }); + + // LOW-2 (review): pin the case-sensitive contract — a case-variant like + // "Claude-Sonnet-5" is NOT mapped (alias keys are case-sensitive, matching + // the model_policy path and the Claude API). + test('model_overrides case-variant "Claude-Sonnet-5" passes through verbatim (case-sensitive contract)', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-executor': 'Claude-Sonnet-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'Claude-Sonnet-5'); + }); + + // Regression guard: non-Claude custom / vendor values still pass through verbatim + // on the claude runtime (the fix must NOT touch values that aren't Claude IDs). + test('model_overrides non-Claude custom model passes through verbatim on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-planner': 'my-custom-model' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-planner'), 'my-custom-model'); + }); + + test('model_overrides non-Claude vendor ID (openai/gpt-5) passes through verbatim on runtime:claude', () => { + writeConfig(tmpDir, { + runtime: 'claude', + model_overrides: { 'gsd-executor': 'openai/gpt-5' }, + }); + assert.strictEqual(resolveModelInternal(tmpDir, 'gsd-executor'), 'openai/gpt-5'); + }); +}); + // ─── resolveModelForTier: model_policy beats dynamic_routing ───────────────── describe('#49 resolveModelForTier: model_policy beats dynamic_routing', () => {