diff --git a/server/services/agentProviderResolution.js b/server/services/agentProviderResolution.js index 53472e8965..deff86d377 100644 --- a/server/services/agentProviderResolution.js +++ b/server/services/agentProviderResolution.js @@ -63,12 +63,30 @@ async function resolvePublicReviewAgentProvider(task, posture) { providerId: provider.id, }); } - // A model pin only survives when it was chosen FOR this provider; otherwise - // fall back to the provider's own default rather than handing one vendor's - // model id to another (the failure mode documented in the ordinary path). + // A model pin only survives when it was chosen FOR this provider AND that + // provider still offers it; otherwise fall back to the provider's own default + // rather than handing one vendor's model id to another (the failure mode + // documented in the ordinary path). Matching the provider alone is not + // enough: a stage pin outlives edits to that provider's own `models` list, + // and a retired id reaches the CLI as a model it cannot serve, so the stage + // spawns, produces no output, and is retried. The ordinary path guards this + // at its list check; `cliProviderRun.js#resolveCliProviderAndModel` does too. const modelSelection = await selectModelForTask(task, provider); const pinnedModel = task.metadata?.model; - const selectedModel = pinnedModel && task.metadata?.provider === provider.id + const pinnedForThisProvider = Boolean(pinnedModel) && task.metadata?.provider === provider.id; + // A provider that enumerates NO models is a pass-through (any id is its + // caller's to choose), so only a non-empty list can invalidate a pin. + const offeredModels = Array.isArray(provider.models) ? provider.models : []; + const pinIsOffered = offeredModels.length === 0 || offeredModels.includes(pinnedModel); + if (pinnedForThisProvider && !pinIsOffered) { + emitLog('warn', `Public-review stage model "${pinnedModel}" is not offered by provider "${provider.id}" — using its default instead`, { + taskId: task.id, + requestedModel: pinnedModel, + providerId: provider.id, + validModels: offeredModels, + }); + } + const selectedModel = pinnedForThisProvider && pinIsOffered ? pinnedModel : (modelSelection.model || provider.defaultModel || null); emitLog('info', `Public-review stage (${posture}) resolved to provider ${provider.id}${selectedModel ? ` model ${selectedModel}` : ''}`, { diff --git a/server/services/agentProviderResolution.test.js b/server/services/agentProviderResolution.test.js index 6f02e9797e..67de32bf0c 100644 --- a/server/services/agentProviderResolution.test.js +++ b/server/services/agentProviderResolution.test.js @@ -384,6 +384,30 @@ describe('resolveAgentProviderAndModel — public-review stages', () => { .resolves.toMatchObject({ provider: { id: 'codex-cli' }, selectedModel: 'm-default' }); }); + // A stage pin outlives edits to the provider's own model list: the live + // pr-reviewer gate sat pinned to an id its provider no longer offered, so + // every run spawned a CLI that could not serve the model, produced no + // output, and was retried — matching the provider is not enough on its own. + it('drops a model pin the matching provider no longer offers', async () => { + const CURATED = { id: 'grok-cli', type: 'cli', command: 'grok', models: ['grok-4'], defaultModel: 'grok-4' }; + getAllProviders.mockResolvedValue({ providers: [CURATED], activeProvider: null }); + + // Still listed → honored. + await expect(resolveAgentProviderAndModel(gateTask({ provider: 'grok-cli', model: 'grok-4' }))) + .resolves.toMatchObject({ provider: { id: 'grok-cli' }, selectedModel: 'grok-4' }); + + // Retired from the list → the provider's own selection wins instead. + await expect(resolveAgentProviderAndModel(gateTask({ provider: 'grok-cli', model: 'grok-3-retired' }))) + .resolves.toMatchObject({ provider: { id: 'grok-cli' }, selectedModel: 'm-default' }); + }); + + // A provider that enumerates no models is a pass-through, so the pin stands. + it('honors a model pin on a provider that enumerates no models', async () => { + getAllProviders.mockResolvedValue({ providers: [GROK], activeProvider: null }); + await expect(resolveAgentProviderAndModel(gateTask({ provider: 'grok-cli', model: 'anything-goes' }))) + .resolves.toMatchObject({ provider: { id: 'grok-cli' }, selectedModel: 'anything-goes' }); + }); + it('blocks PERMANENTLY when no enabled provider can enforce the posture', async () => { getAllProviders.mockResolvedValue({ providers: [OPENCODE], activeProvider: { id: 'opencode' } }); const r = await resolveAgentProviderAndModel(gateTask());