Merge pull request #2048 from open-gsd/fix/2041-model-overrides-claude-alias
fix(#2041): map model_overrides Claude IDs to Agent-tool aliases
This commit is contained in:
Tom Boucher
5
.changeset/gentle-badgers-roar.md
Normal file
5
.changeset/gentle-badgers-roar.md
Normal file
@@ -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)
|
||||
@@ -105,6 +105,66 @@ function _resetModelPolicyWarningCacheForTests(): void {
|
||||
_modelPolicyUnmappableWarned.clear();
|
||||
}
|
||||
|
||||
// Dedupe stderr warnings for unmappable model_overrides Claude IDs (#2041).
|
||||
const _modelOverrideUnmappableWarned = new Set<string>();
|
||||
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<string,string> 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<string, unknown> | 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<string, string> | 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<string, string> | 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,
|
||||
|
||||
@@ -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 };
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
Reference in New Issue
Block a user