From d14e67cad8c260e6aa3cb54ffe8378c6d8a44ec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=82CHES?= Date: Sat, 21 Mar 2026 09:46:25 -0600 Subject: [PATCH] fix: hook model field uses model-router resolution instead of Claude-only registry (#1720) (#1781) dispatchHookUnit() previously used a simplistic lookup that only matched exact model IDs or "provider/id" against ctx.modelRegistry.getAvailable(). Non-Claude models (e.g. openrouter/openai/gpt-5.4-codex) silently fell back to the session model with no warning. - Replace simplistic lookup in dispatchHookUnit with resolveModelId() which handles provider/model, bare-id, and OpenRouter org/model formats - Add warning notification when a hook model can't be resolved instead of silent fallback - Wire sidecar hook model override through runUnitPhase so post-unit hook model fields are applied after standard model selection - Consume pre-dispatch hook model field (hookModelOverride) in IterationData so it reaches the dispatch phase - Export resolveModelId from auto-model-selection.ts for reuse - Add resolveModelId to LoopDeps interface for testability Closes #1720 Co-authored-by: Claude Opus 4.6 (1M context) --- .../extensions/gsd/auto-model-selection.ts | 2 +- src/resources/extensions/gsd/auto.ts | 13 ++- .../extensions/gsd/auto/loop-deps.ts | 5 + src/resources/extensions/gsd/auto/phases.ts | 25 +++++ src/resources/extensions/gsd/auto/types.ts | 2 + .../gsd/tests/hook-model-resolution.test.ts | 98 +++++++++++++++++++ 6 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/hook-model-resolution.test.ts diff --git a/src/resources/extensions/gsd/auto-model-selection.ts b/src/resources/extensions/gsd/auto-model-selection.ts index 70b2fa7e1..5523854d3 100644 --- a/src/resources/extensions/gsd/auto-model-selection.ts +++ b/src/resources/extensions/gsd/auto-model-selection.ts @@ -164,7 +164,7 @@ export async function selectAndApplyModel( * Resolve a model ID string to a model object from the available models list. * Handles formats: "provider/model", "bare-id", "org/model-name" (OpenRouter). */ -function resolveModelId( +export function resolveModelId( modelId: string, availableModels: T[], currentProvider: string | undefined, diff --git a/src/resources/extensions/gsd/auto.ts b/src/resources/extensions/gsd/auto.ts index 8ffc0097a..dd60be458 100644 --- a/src/resources/extensions/gsd/auto.ts +++ b/src/resources/extensions/gsd/auto.ts @@ -86,7 +86,7 @@ import { import { closeoutUnit } from "./auto-unit-closeout.js"; import { recoverTimedOutUnit } from "./auto-timeout-recovery.js"; import { selfHealRuntimeRecords } from "./auto-recovery.js"; -import { selectAndApplyModel } from "./auto-model-selection.js"; +import { selectAndApplyModel, resolveModelId } from "./auto-model-selection.js"; import { syncProjectRootToWorktree, syncStateToProjectRoot, @@ -912,6 +912,7 @@ function buildLoopDeps(): LoopDeps { // Model selection + supervision selectAndApplyModel, + resolveModelId, startUnitSupervision, // Prompt helpers @@ -1307,15 +1308,19 @@ export async function dispatchHookUnit( if (hookModel) { const availableModels = ctx.modelRegistry.getAvailable(); - const match = availableModels.find( - (m) => m.id === hookModel || `${m.provider}/${m.id}` === hookModel, - ); + const match = resolveModelId(hookModel, availableModels, ctx.model?.provider); if (match) { try { await pi.setModel(match); } catch { /* non-fatal */ } + } else { + ctx.ui.notify( + `Hook model "${hookModel}" not found in available models. Falling back to current session model. ` + + `Ensure the model is defined in models.json and has auth configured.`, + "warning", + ); } } diff --git a/src/resources/extensions/gsd/auto/loop-deps.ts b/src/resources/extensions/gsd/auto/loop-deps.ts index 17d8083d6..c016fa852 100644 --- a/src/resources/extensions/gsd/auto/loop-deps.ts +++ b/src/resources/extensions/gsd/auto/loop-deps.ts @@ -236,6 +236,11 @@ export interface LoopDeps { startModel: { provider: string; id: string } | null, retryContext?: { isRetry: boolean; previousTier?: string }, ) => Promise<{ routing: { tier: string; modelDowngraded: boolean } | null }>; + resolveModelId: ( + modelId: string, + availableModels: T[], + currentProvider: string | undefined, + ) => T | undefined; startUnitSupervision: (sctx: { s: AutoSession; ctx: ExtensionContext; diff --git a/src/resources/extensions/gsd/auto/phases.ts b/src/resources/extensions/gsd/auto/phases.ts index 0d02ad777..6ede2fa67 100644 --- a/src/resources/extensions/gsd/auto/phases.ts +++ b/src/resources/extensions/gsd/auto/phases.ts @@ -642,6 +642,7 @@ export async function runDispatch( pauseAfterUatDispatch, observabilityIssues, state, mid, midTitle, isRetry: false, previousTier: undefined, + hookModelOverride: preDispatchResult.model, }, }; } @@ -928,6 +929,30 @@ export async function runUnitPhase( s.currentUnitRouting = modelResult.routing as AutoSession["currentUnitRouting"]; + // Apply sidecar/pre-dispatch hook model override (takes priority over standard model selection) + const hookModelOverride = sidecarItem?.model ?? iterData.hookModelOverride; + if (hookModelOverride) { + const availableModels = ctx.modelRegistry.getAvailable(); + const match = deps.resolveModelId(hookModelOverride, availableModels, ctx.model?.provider); + if (match) { + const ok = await pi.setModel(match, { persist: false }); + if (ok) { + ctx.ui.notify(`Hook model override: ${match.provider}/${match.id}`, "info"); + } else { + ctx.ui.notify( + `Hook model "${hookModelOverride}" found but setModel failed. Using default.`, + "warning", + ); + } + } else { + ctx.ui.notify( + `Hook model "${hookModelOverride}" not found in available models. Falling back to current session model. ` + + `Ensure the model is defined in models.json and has auth configured.`, + "warning", + ); + } + } + // Start unit supervision deps.clearUnitTimeout(); deps.startUnitSupervision({ diff --git a/src/resources/extensions/gsd/auto/types.ts b/src/resources/extensions/gsd/auto/types.ts index 06605c5b8..0fadf7119 100644 --- a/src/resources/extensions/gsd/auto/types.ts +++ b/src/resources/extensions/gsd/auto/types.ts @@ -94,6 +94,8 @@ export interface IterationData { midTitle: string | undefined; isRetry: boolean; previousTier: string | undefined; + /** Model override from pre-dispatch hooks (applied after standard model selection). */ + hookModelOverride?: string; } export type WindowEntry = { key: string; error?: string }; diff --git a/src/resources/extensions/gsd/tests/hook-model-resolution.test.ts b/src/resources/extensions/gsd/tests/hook-model-resolution.test.ts new file mode 100644 index 000000000..fb571fcdf --- /dev/null +++ b/src/resources/extensions/gsd/tests/hook-model-resolution.test.ts @@ -0,0 +1,98 @@ +/** + * Tests for hook model resolution (#1720). + * + * Verifies that resolveModelId handles all model ID formats correctly, + * including OpenRouter-style "org/model" IDs, provider-prefixed IDs, + * and bare IDs. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { resolveModelId } from "../auto-model-selection.js"; + +// ─── Test Models ───────────────────────────────────────────────────────────── + +type TestModel = { id: string; provider: string }; + +const AVAILABLE_MODELS: TestModel[] = [ + { id: "claude-sonnet-4-6", provider: "anthropic" }, + { id: "claude-opus-4-6", provider: "anthropic" }, + { id: "claude-haiku-4-5", provider: "anthropic" }, + { id: "openai/gpt-5.4-codex", provider: "openrouter" }, + { id: "google/gemini-2.5-pro", provider: "openrouter" }, + { id: "gpt-4o", provider: "openai" }, + { id: "gpt-4o", provider: "azure" }, +]; + +// ─── Bare model ID ─────────────────────────────────────────────────────────── + +test("resolveModelId: bare ID resolves to current provider first", () => { + const match = resolveModelId("gpt-4o", AVAILABLE_MODELS, "openai"); + assert.ok(match); + assert.equal(match.provider, "openai"); + assert.equal(match.id, "gpt-4o"); +}); + +test("resolveModelId: bare ID falls back to first available when no current provider match", () => { + const match = resolveModelId("claude-sonnet-4-6", AVAILABLE_MODELS, "openai"); + assert.ok(match); + assert.equal(match.provider, "anthropic"); + assert.equal(match.id, "claude-sonnet-4-6"); +}); + +// ─── Provider-prefixed ID ──────────────────────────────────────────────────── + +test("resolveModelId: provider/model resolves correctly", () => { + const match = resolveModelId("anthropic/claude-opus-4-6", AVAILABLE_MODELS, undefined); + assert.ok(match); + assert.equal(match.provider, "anthropic"); + assert.equal(match.id, "claude-opus-4-6"); +}); + +test("resolveModelId: provider/model case-insensitive", () => { + const match = resolveModelId("Anthropic/Claude-Sonnet-4-6", AVAILABLE_MODELS, undefined); + assert.ok(match); + assert.equal(match.provider, "anthropic"); +}); + +// ─── OpenRouter-style model IDs (org/model as the ID) ─────────────────────── + +test("resolveModelId: openrouter/org/model resolves full string as ID", () => { + const match = resolveModelId("openrouter/openai/gpt-5.4-codex", AVAILABLE_MODELS, undefined); + assert.ok(match, "should find the OpenRouter model with org/model ID"); + assert.equal(match.provider, "openrouter"); + assert.equal(match.id, "openai/gpt-5.4-codex"); +}); + +test("resolveModelId: openrouter org/model resolves when used as bare ID", () => { + // When the user specifies "openai/gpt-5.4-codex" without provider prefix, + // and "openai" is not a known provider, it should try matching the full + // string as a model ID. + const modelsWithoutOpenai = AVAILABLE_MODELS.filter(m => m.provider !== "openai" && m.provider !== "azure"); + const match = resolveModelId("openai/gpt-5.4-codex", modelsWithoutOpenai, undefined); + assert.ok(match, "should find the model when openai is not a known provider"); + assert.equal(match.provider, "openrouter"); + assert.equal(match.id, "openai/gpt-5.4-codex"); +}); + +// ─── Disambiguation with multiple providers ────────────────────────────────── + +test("resolveModelId: azure/gpt-4o resolves to azure provider", () => { + const match = resolveModelId("azure/gpt-4o", AVAILABLE_MODELS, undefined); + assert.ok(match); + assert.equal(match.provider, "azure"); + assert.equal(match.id, "gpt-4o"); +}); + +// ─── Missing model ─────────────────────────────────────────────────────────── + +test("resolveModelId: returns undefined for unknown model", () => { + const match = resolveModelId("nonexistent-model", AVAILABLE_MODELS, "anthropic"); + assert.equal(match, undefined); +}); + +test("resolveModelId: returns undefined for unknown provider/model combo", () => { + const match = resolveModelId("fakeprovider/fake-model", AVAILABLE_MODELS, undefined); + assert.equal(match, undefined); +});