diff --git a/src/server/__tests__/providers.test.ts b/src/server/__tests__/providers.test.ts index 0427e38c..ab67b3de 100644 --- a/src/server/__tests__/providers.test.ts +++ b/src/server/__tests__/providers.test.ts @@ -478,6 +478,24 @@ describe('ProviderService', () => { expect(updated.apiKey).toBe('sk-test-key-123') }) + test('bumps the revision counter monotonically on every update so runtime overrides can detect stale snapshots', async () => { + const svc = new ProviderService() + const added = await svc.addProvider(sampleInput()) + // New providers don't pre-populate revision (treated as 0 by readers). + expect(added.revision).toBeUndefined() + + const r1 = await svc.updateProvider(added.id, { apiKey: 'rotated-once' }) + expect(r1.revision).toBe(1) + + const r2 = await svc.updateProvider(added.id, { baseUrl: 'https://new.example.com' }) + expect(r2.revision).toBe(2) + + // Edits that touch nothing useful still bump — the contract is + // "user clicked save, force any in-flight session to re-snapshot". + const r3 = await svc.updateProvider(added.id, { notes: 'note' }) + expect(r3.revision).toBe(3) + }) + test('should throw 404 for non-existent provider', async () => { const svc = new ProviderService() diff --git a/src/server/__tests__/runtime-override-match.test.ts b/src/server/__tests__/runtime-override-match.test.ts new file mode 100644 index 00000000..6c8e3988 --- /dev/null +++ b/src/server/__tests__/runtime-override-match.test.ts @@ -0,0 +1,84 @@ +/** + * Unit-locks the short-circuit equality used by `handleSetRuntimeConfig` + * to decide whether a `set_runtime_config` message should respawn the CLI. + * + * The producer-side bug we're guarding against: a user edits provider + * baseUrl / apiKey / apiFormat / model mapping. The desktop fan-out + * re-emits `set_runtime_config` for every idle session on that provider, + * but the (providerId, modelId, effort, thinkingEnabled) tuple is + * identical to the running CLI's. Without `providerRevision` in the + * comparison, the handler short-circuits and the running CLI keeps its + * spawn-time `ANTHROPIC_BASE_URL` / `ANTHROPIC_AUTH_TOKEN` / model env + * forever — resulting in "There's an issue with the selected model (...)" + * after a model rename. + */ +import { describe, expect, it } from 'bun:test' +import { runtimeOverridesMatch, type RuntimeOverride } from '../ws/handler.js' + +const base: RuntimeOverride = { + providerId: 'p1', + modelId: 'mimo-v2.5-pro', + effort: 'medium', + thinkingEnabled: true, + providerRevision: 1, +} + +describe('runtimeOverridesMatch', () => { + it('returns false when prev is undefined (first set_runtime_config)', () => { + expect(runtimeOverridesMatch(undefined, base)).toBe(false) + }) + + it('returns true when every field matches', () => { + expect(runtimeOverridesMatch({ ...base }, { ...base })).toBe(true) + }) + + it('forces a restart when providerRevision differs (PR-A regression)', () => { + // The whole point: same tuple, bumped revision after updateProvider. + expect( + runtimeOverridesMatch( + { ...base, providerRevision: 1 }, + { ...base, providerRevision: 2 }, + ), + ).toBe(false) + }) + + it('treats absent providerRevision as 0 on both sides', () => { + const { providerRevision: _a, ...prev } = base + const { providerRevision: _b, ...next } = base + expect(runtimeOverridesMatch(prev, next)).toBe(true) + }) + + it('treats absent providerRevision (legacy override loaded from disk) and revision=0 as equal', () => { + const { providerRevision: _a, ...prev } = base + expect(runtimeOverridesMatch(prev, { ...base, providerRevision: 0 })).toBe(true) + }) + + it('forces a restart when modelId differs', () => { + expect( + runtimeOverridesMatch(base, { ...base, modelId: 'sonnet-4.5' }), + ).toBe(false) + }) + + it('forces a restart when providerId differs', () => { + expect( + runtimeOverridesMatch(base, { ...base, providerId: 'p2' }), + ).toBe(false) + }) + + it('forces a restart when effort differs', () => { + expect( + runtimeOverridesMatch(base, { ...base, effort: 'high' }), + ).toBe(false) + }) + + it('forces a restart when thinkingEnabled flips', () => { + expect( + runtimeOverridesMatch(base, { ...base, thinkingEnabled: false }), + ).toBe(false) + }) + + it('treats null providerId on both sides as matching (no provider configured)', () => { + const noProvider: RuntimeOverride = { providerId: null, modelId: 'x' } + expect(runtimeOverridesMatch(noProvider, { ...noProvider })).toBe(true) + }) +}) diff --git a/src/server/services/providerService.ts b/src/server/services/providerService.ts index ab80298a..db9649ae 100644 --- a/src/server/services/providerService.ts +++ b/src/server/services/providerService.ts @@ -198,6 +198,12 @@ export class ProviderService { delete updated.thinkingIncompatible delete updated.thinkingIncompatibleReason + // Bump the revision counter so any in-flight `runtimeOverride` snapshot + // for this provider is recognised as stale by handleSetRuntimeConfig + // and forces a CLI restart even when the (providerId, modelId, effort) + // tuple is unchanged. See SavedProviderSchema.revision docstring. + updated.revision = (existing.revision ?? 0) + 1 + index.providers[idx] = updated await this.writeIndex(index) diff --git a/src/server/types/provider.ts b/src/server/types/provider.ts index 6e8dd275..a1ec1fa7 100644 --- a/src/server/types/provider.ts +++ b/src/server/types/provider.ts @@ -75,6 +75,23 @@ export const SavedProviderSchema = z.object({ * providers.json file. */ thinkingIncompatibleReason: z.string().max(500).optional(), + /** + * Monotonically-increasing counter bumped on every {@link updateProvider}. + * The runtime layer captures this value alongside the per-session + * `runtimeOverride` at session start, and the next `set_runtime_config` + * compares revisions to decide whether to force a CLI restart even when + * the (providerId, modelId, effort, thinkingEnabled) tuple is unchanged. + * + * This catches "user changed baseUrl / apiKey / apiFormat / model mapping + * but the override tuple still matches" — without it, the CLI keeps its + * spawn-time `ANTHROPIC_BASE_URL` / `ANTHROPIC_AUTH_TOKEN` env until the + * subprocess is killed for some other reason, which makes provider + * config edits feel "stuck". + * + * Optional for backwards compatibility with providers.json files written + * before this field existed; absent reads as 0. + */ + revision: z.number().int().nonnegative().optional(), }) export const ProvidersIndexSchema = z.object({ diff --git a/src/server/ws/handler.ts b/src/server/ws/handler.ts index 60286d4d..9385b8d2 100644 --- a/src/server/ws/handler.ts +++ b/src/server/ws/handler.ts @@ -81,11 +81,23 @@ const sessionTitleState = new Map() -type RuntimeOverride = { +export type RuntimeOverride = { providerId: string | null modelId: string effort?: string thinkingEnabled?: boolean + /** + * Snapshot of the provider's `revision` at the moment this override was + * captured. Compared on the next `set_runtime_config` to detect that the + * underlying provider config (baseUrl / apiKey / apiFormat / model + * mapping) has changed even when the override tuple is the same — which + * would otherwise silently keep the running CLI on a stale env snapshot. + * + * Absent for runtime overrides loaded from older session JSONL metadata + * that pre-date this field; treated as 0 so any subsequent + * provider.update bumps it past the captured value. + */ + providerRevision?: number } type ActiveUserTurnState = { @@ -894,20 +906,18 @@ async function handleSetRuntimeConfig( const thinkingEnabled = typeof message.thinkingEnabled === 'boolean' ? message.thinkingEnabled : undefined - const nextOverride = { - providerId: message.providerId ?? null, + const providerId = message.providerId ?? null + const providerRevision = await resolveProviderRevision(providerId) + + const nextOverride: RuntimeOverride = { + providerId, modelId, ...(effortLevel ? { effort: effortLevel } : {}), ...(thinkingEnabled !== undefined ? { thinkingEnabled } : {}), + ...(providerRevision > 0 ? { providerRevision } : {}), } const prevOverride = runtimeOverrides.get(sessionId) - if ( - prevOverride && - prevOverride.providerId === nextOverride.providerId && - prevOverride.modelId === nextOverride.modelId && - prevOverride.effort === nextOverride.effort && - prevOverride.thinkingEnabled === nextOverride.thinkingEnabled - ) { + if (runtimeOverridesMatch(prevOverride, nextOverride)) { return } @@ -945,10 +955,7 @@ async function handleSetRuntimeConfig( await pendingStartup.catch(() => undefined) const currentOverride = runtimeOverrides.get(sessionId) if ( - currentOverride?.providerId !== nextOverride.providerId || - currentOverride.modelId !== nextOverride.modelId || - currentOverride.effort !== nextOverride.effort || - currentOverride.thinkingEnabled !== nextOverride.thinkingEnabled || + !runtimeOverridesMatch(currentOverride, nextOverride) || !conversationService.hasSession(sessionId) ) { return @@ -2576,6 +2583,52 @@ function isKnownRuntimeProviderId( ) } +/** + * Look up the current revision of a saved provider for use in + * {@link RuntimeOverride.providerRevision}. Returns 0 for the OpenAI Official + * built-in (it has no provider config to mutate), 0 for unknown / null ids, + * and never throws — a stale providerId is handled by the existing + * stale-providerId guard in {@link getRuntimeSettings}. + */ +async function resolveProviderRevision( + providerId: string | null, +): Promise { + if (!providerId) return 0 + if (isOpenAIOfficialProviderId(providerId)) return 0 + try { + const provider = await providerService.getProvider(providerId) + return provider.revision ?? 0 + } catch { + return 0 + } +} + +/** + * Pure equality check used by `handleSetRuntimeConfig`'s short-circuit. + * Exported for unit testing — kept here (not in a separate module) because + * it depends on the locally-defined RuntimeOverride shape. + * + * Returns true when the two overrides describe the same effective CLI + * runtime — i.e. respawning the CLI would be a no-op. Critically this + * includes `providerRevision`: when the user edits provider config without + * touching modelId/effort, the tuple appears unchanged but the spawn-time + * env (baseUrl / apiKey / model mapping) is stale, so we must consider + * that a difference and force a restart. + */ +export function runtimeOverridesMatch( + prev: RuntimeOverride | undefined, + next: RuntimeOverride, +): boolean { + if (!prev) return false + return ( + prev.providerId === next.providerId && + prev.modelId === next.modelId && + prev.effort === next.effort && + prev.thinkingEnabled === next.thinkingEnabled && + (prev.providerRevision ?? 0) === (next.providerRevision ?? 0) + ) +} + async function getRuntimeSettings(sessionId?: string): Promise { const coordinatorMode = sessionId ? coordinatorModeSessions.has(sessionId) : false const soloPipelineMode = sessionId ? soloPipelineModeSessions.has(sessionId) : false @@ -2606,6 +2659,7 @@ async function getRuntimeSettings(sessionId?: string): Promise ? runtimeOverrides.get(sessionId) ?? persistedRuntimeOverride : undefined if (runtimeOverride) { + let resolvedModelId = runtimeOverride.modelId if (typeof runtimeOverride.providerId === 'string') { const { providers } = await providerService.listProviders() const providerExists = isKnownRuntimeProviderId(runtimeOverride.providerId, providers) @@ -2623,6 +2677,36 @@ async function getRuntimeSettings(sessionId?: string): Promise ...(handoffSystemPrompt ? { handoffSystemPrompt } : {}), } } + + // Stale-modelId guard: when the persisted runtime modelId is no longer + // present in any of the active provider's four model slots + // (main / haiku / sonnet / opus), the upstream will return 404 and we + // surface "There's an issue with the selected model (...)" — which is + // exactly the cycle a user hits when they rename a model in Settings + // and resume an old session. Fall back to the provider's main model + // instead of letting `--model ` reach the wire. + // Skipped for the OpenAI Official built-in (no editable mapping). + if (!isOpenAIOfficialProviderId(runtimeOverride.providerId)) { + const provider = providers.find((p) => p.id === runtimeOverride.providerId) + if (provider) { + const knownModels = new Set( + [ + provider.models.main, + provider.models.haiku, + provider.models.sonnet, + provider.models.opus, + ] + .map((value) => (typeof value === 'string' ? value.trim() : '')) + .filter(Boolean), + ) + if (knownModels.size > 0 && !knownModels.has(runtimeOverride.modelId)) { + console.warn( + `[WS] Persisted runtime modelId '${runtimeOverride.modelId}' is no longer in provider ${provider.id}'s model map; falling back to ${provider.models.main}`, + ) + resolvedModelId = provider.models.main + } + } + } } const userSettings = await settingsService.getUserSettings() @@ -2630,7 +2714,7 @@ async function getRuntimeSettings(sessionId?: string): Promise return { permissionMode: sessionPermissionMode ?? await settingsService.getPermissionMode().catch(() => undefined), - model: runtimeOverride.modelId, + model: resolvedModelId, effort: runtimeOverride.effort, thinking, providerId: runtimeOverride.providerId,