diff --git a/packages/ui/src/components/model-picker/ModelPickerList.tsx b/packages/ui/src/components/model-picker/ModelPickerList.tsx index f06dc190..99080f9c 100644 --- a/packages/ui/src/components/model-picker/ModelPickerList.tsx +++ b/packages/ui/src/components/model-picker/ModelPickerList.tsx @@ -436,7 +436,10 @@ export const ModelPickerList: React.FC = ({ ); const allowedProviderSet = React.useMemo(() => { - if (!allowedProviderIds || allowedProviderIds.length === 0) return null; + // undefined = no restriction; [] = allow none. Treating empty like + // "unrestricted" would resurface providers without a login in pickers that + // intentionally pass the authenticated-only list. + if (!allowedProviderIds) return null; return new Set(allowedProviderIds); }, [allowedProviderIds]); diff --git a/packages/ui/src/components/views/walkthrough/WalkthroughBlocker.tsx b/packages/ui/src/components/views/walkthrough/WalkthroughBlocker.tsx index 2f0af021..a9a620ac 100644 --- a/packages/ui/src/components/views/walkthrough/WalkthroughBlocker.tsx +++ b/packages/ui/src/components/views/walkthrough/WalkthroughBlocker.tsx @@ -150,7 +150,7 @@ export const WalkthroughBlocker = ({ onChange={(providerId, modelId) => { void handleModelChange(providerId, modelId); }} - allowedProviderIds={providers} + allowedProviderIds={providers ?? []} isModelAllowed={isStructuredOutputCapable} /> diff --git a/packages/ui/src/components/views/walkthrough/WalkthroughView.tsx b/packages/ui/src/components/views/walkthrough/WalkthroughView.tsx index c0403b75..246a818c 100644 --- a/packages/ui/src/components/views/walkthrough/WalkthroughView.tsx +++ b/packages/ui/src/components/views/walkthrough/WalkthroughView.tsx @@ -323,15 +323,36 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { // Explicit pick first, then the model that actually produced what is on // screen, then whatever settings resolve to. The middle step is what makes // reopening a review show the model behind it rather than the default. - const activeModel = selectedModel - ?? (entry.result?.model ? `${entry.result.model.providerID}/${entry.result.model.modelID}` : undefined) - ?? (entry.readiness?.model ? `${entry.readiness.model.providerID}/${entry.readiness.model.modelID}` : undefined); - const [activeProviderId, ...activeModelParts] = (activeModel ?? '').split('/'); - const activeModelId = activeModelParts.join('/'); - + // Never present a provider without a usable login as the current selection — + // the picker already hides them from the menu; showing one as selected was + // the whole "why say so?" failure mode. const modelsMetadata = useConfigStore((state) => state.modelsMetadata); const [modelProviders, setModelProviders] = useState(undefined); + const providerIsAuthenticated = (providerId: string | undefined) => { + if (!providerId) return false; + // Until the auth list loads, do not present a candidate as selected — + // otherwise an unauthenticated config model flashes in the picker. + if (modelProviders === undefined) return false; + return modelProviders.includes(providerId); + }; + const readinessModelRef = entry.readiness?.model + && entry.readiness.model.hasLogin !== false + && providerIsAuthenticated(entry.readiness.model.providerID) + ? `${entry.readiness.model.providerID}/${entry.readiness.model.modelID}` + : undefined; + const resultModelRef = entry.result?.model + && providerIsAuthenticated(entry.result.model.providerID) + ? `${entry.result.model.providerID}/${entry.result.model.modelID}` + : undefined; + const selectedModelUsable = selectedModel + && providerIsAuthenticated(selectedModel.split('/')[0]) + ? selectedModel + : undefined; + const activeModel = selectedModelUsable ?? resultModelRef ?? readinessModelRef; + const [activeProviderId, ...activeModelParts] = (activeModel ?? '').split('/'); + const activeModelId = activeModelParts.join('/'); + useEffect(() => { if (modelProviders !== undefined) return; let cancelled = false; @@ -403,6 +424,8 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { const showStages = startedFromEmptyRef.current && (entry.status === 'generating' || stageProgress.holding); + // Auth/login gaps are not a full-panel blocker: hide the unusable model and + // disable Generate instead of explaining a raw provider error. const blockedReason = entry.error?.code === 'context-too-small' || entry.error?.code === 'structured-output-unsupported' || entry.error?.code === 'no-model' @@ -411,6 +434,7 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { || entry.error?.code === 'output-exhausted' ? entry.error.code : entry.readiness && !entry.readiness.ready && !view + && entry.readiness.reason !== 'no-provider-login' ? entry.readiness.reason : undefined; @@ -420,11 +444,16 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { const blockedRequiredChars = entry.error?.requiredChars ?? entry.readiness?.requiredChars; const blockedAvailableChars = entry.error?.availableChars ?? entry.readiness?.availableChars; + // Not ready, or no usable selected model, means Generate must not look + // actionable — including when the resolved model has no login. + const generateDisabled = !activeModel || Boolean(entry.readiness && !entry.readiness.ready); + const handleGenerate = useCallback( (force: boolean) => { + if (generateDisabled) return; void generate(directory, source, { force, language: activeLanguage }); }, - [activeLanguage, directory, generate, source] + [activeLanguage, directory, generate, generateDisabled, source] ); return ( @@ -544,7 +573,8 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { onChange={(providerId, modelId) => { selectModel(directory, source, providerId && modelId ? `${providerId}/${modelId}` : null); }} - allowedProviderIds={modelProviders} + // While the auth list is loading, allow none — not every provider. + allowedProviderIds={modelProviders ?? []} isModelAllowed={isStructuredOutputCapable} tooltipsEnabled={false} dropdownPortalToBody @@ -592,7 +622,10 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { type="button" variant="outline" size="sm" - className={WALKTHROUGH_ACTION_CLASS} + className={generateDisabled + ? 'border-border text-muted-foreground' + : WALKTHROUGH_ACTION_CLASS} + disabled={generateDisabled} aria-label={compactHeader ? (view ? t('walkthrough.action.regenerate') : t('walkthrough.action.generate')) : undefined} @@ -646,6 +679,7 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { variant="ghost" size="xs" className="ml-auto" + disabled={generateDisabled} // Not forced: if an entry for this exact request existed the banner // would not be here, and a forced run would refuse the cache it may // find on the way. @@ -677,7 +711,7 @@ export const WalkthroughView = ({ directory }: WalkthroughViewProps) => { )} - {entry.error && !blockedReason && ( + {entry.error && !blockedReason && entry.error.code !== 'no-provider-login' && (
{/* Provider errors arrive as raw JSON bodies. Show a readable amount diff --git a/packages/ui/src/lib/walkthrough/types.ts b/packages/ui/src/lib/walkthrough/types.ts index 8c6b5a23..5f8ef80a 100644 --- a/packages/ui/src/lib/walkthrough/types.ts +++ b/packages/ui/src/lib/walkthrough/types.ts @@ -91,6 +91,7 @@ export type WalkthroughStage = 'collecting' | 'asking' | 'retrying' | 'assemblin export type WalkthroughBlockedReason = | 'no-model' + | 'no-provider-login' | 'empty-diff' | 'only-generated' | 'context-too-small' @@ -104,6 +105,8 @@ export interface WalkthroughReadiness { inputCharBudget?: number; contextTokens?: number; structuredOutput?: boolean | null; + /** False when the resolved provider has no usable OpenCode login. */ + hasLogin?: boolean; }; requiredChars?: number; availableChars?: number; diff --git a/packages/web/server/lib/small-model/DOCUMENTATION.md b/packages/web/server/lib/small-model/DOCUMENTATION.md index a9d60a99..1776f1e1 100644 --- a/packages/web/server/lib/small-model/DOCUMENTATION.md +++ b/packages/web/server/lib/small-model/DOCUMENTATION.md @@ -69,10 +69,17 @@ other runtime API. - `timeoutMs` overrides the 60s default per call; `signal` lets a caller abort a request that is no longer wanted. Both apply to every wire format. - `describeSmallModel()` additionally reports `inputCharBudget`, - `contextTokens`, `contextKnown`, and `structuredOutput`. The last is - tri-state: `true`/`false` from the catalog, `null` when the catalog omits the - field — which it does for roughly half of all models, aggregators and proxies - especially. Callers must treat `null` as "try it", not "unsupported". + `contextTokens`, `contextKnown`, `structuredOutput`, and `hasLogin`. The last + is whether the resolved provider has a usable credential (`auth.json` or + config `provider..options.apiKey`) — settings/config overrides can name a + provider with none, and callers such as the walkthrough refuse before the + request. `structuredOutput` is tri-state: `true`/`false` from the catalog, + `null` when the catalog omits the field — which it does for roughly half of + all models, aggregators and proxies especially. Callers must treat `null` as + "try it", not "unsupported". +- Missing credentials throw with `statusCode: 401` and + `code: 'no-provider-login'` rather than a bare `Error`, so UI callers can show + a blocker instead of a raw 500 message. - `call.js` — wire formats and per-provider auth, replicating OpenCode's plugin auth loaders: - **GitHub Copilot**: fetches the requested model's authenticated `/models` diff --git a/packages/web/server/lib/small-model/call.js b/packages/web/server/lib/small-model/call.js index 17c8949c..c3a090a1 100644 --- a/packages/web/server/lib/small-model/call.js +++ b/packages/web/server/lib/small-model/call.js @@ -566,15 +566,31 @@ const readProviderConfig = (workingDirectory, providerID) => { // Dispatch // --------------------------------------------------------------------------- +/** + * Same credential resolution the request path uses: config + * `provider..options.apiKey` wins, then the auth.json entry. + * Callers that need to refuse before spending a request (walkthrough readiness) + * must use this rather than inventing a second rule. + */ +export function resolveProviderLogin({ auth, workingDirectory, providerID }) { + const providerConfig = readProviderConfig(workingDirectory, providerID); + return providerConfig?.auth || getAuthEntryForProvider(auth, providerID) || null; +} + export async function callSmallModel({ auth, catalog, workingDirectory, providerID, modelID, prompt, system, maxOutputTokens, responseSchema, timeoutMs, signal }) { const tokens = Number(maxOutputTokens) > 0 ? Number(maxOutputTokens) : DEFAULT_MAX_OUTPUT_TOKENS; const providerConfig = readProviderConfig(workingDirectory, providerID); // Match OpenCode's resolveSDK precedence: - // config provider..options.apiKey (providerConfig.auth) wins; the - // auth.json entry is only a fallback. + // config provider..options.apiKey wins; the auth.json entry is only a fallback. const entry = providerConfig?.auth || getAuthEntryForProvider(auth, providerID); if (!entry) { - throw new Error(`No OpenCode login found for provider "${providerID}"`); + // Structured so the walkthrough (and any other caller) can show a blocker + // instead of a raw 500 banner with this developer-oriented sentence. + throw Object.assign(new Error(`No OpenCode login found for provider "${providerID}"`), { + statusCode: 401, + code: 'no-provider-login', + providerID, + }); } if (providerID === 'github-copilot') { diff --git a/packages/web/server/lib/small-model/call.test.js b/packages/web/server/lib/small-model/call.test.js index 78ef733f..154fd986 100644 --- a/packages/web/server/lib/small-model/call.test.js +++ b/packages/web/server/lib/small-model/call.test.js @@ -171,14 +171,21 @@ describe('callSmallModel — custom provider config', () => { provider: { custom: { options: { baseURL: 'https://proxy.example.test/v1' } } }, }); - await expect(callSmallModel({ + const error = await callSmallModel({ auth: {}, catalog: {}, workingDirectory: '/proj', providerID: 'custom', modelID: 'gpt-4o-mini', prompt: 'hi', - })).rejects.toThrow('No OpenCode login found for provider "custom"'); + }).then(() => null, (e) => e); + + expect(error).toMatchObject({ + message: 'No OpenCode login found for provider "custom"', + code: 'no-provider-login', + statusCode: 401, + providerID: 'custom', + }); // The credential gate fires before any network call. expect(fetchMock).not.toHaveBeenCalled(); diff --git a/packages/web/server/lib/small-model/index.js b/packages/web/server/lib/small-model/index.js index 40e5e7f5..955797e6 100644 --- a/packages/web/server/lib/small-model/index.js +++ b/packages/web/server/lib/small-model/index.js @@ -5,7 +5,7 @@ import { readAuthFile } from '../opencode/auth.js'; import { readConfigLayers } from '../opencode/shared.js'; import { getModelCatalog } from './catalog.js'; import { resolveSmallModel, parseModelRef, isUsableAuthEntry, getAuthEntryForProvider } from './resolve.js'; -import { callSmallModel } from './call.js'; +import { callSmallModel, resolveProviderLogin } from './call.js'; const OPENCHAMBER_SETTINGS_FILE = path.join( process.env.OPENCHAMBER_DATA_DIR @@ -252,8 +252,17 @@ export async function describeSmallModel({ directory, preferredProviderID, prefe outputReserveTokens: reserveTokens, }); + // Settings/config/request overrides can name a provider with no usable login. + // Report that here so readiness can refuse before the user pays for a 401. + const hasLogin = Boolean(resolveProviderLogin({ + auth, + workingDirectory: directory, + providerID: resolved.providerID, + })); + return { ...resolved, + hasLogin, inputCharBudget: maxChars, contextTokens, contextKnown, diff --git a/packages/web/server/lib/small-model/index.test.js b/packages/web/server/lib/small-model/index.test.js index 14e5f015..fc5d741c 100644 --- a/packages/web/server/lib/small-model/index.test.js +++ b/packages/web/server/lib/small-model/index.test.js @@ -18,7 +18,13 @@ vi.mock('./catalog.js', () => ({ getModelCatalog: vi.fn(), getCatalogProvider: vi.fn(), })); -vi.mock('./call.js', () => ({ callSmallModel: vi.fn() })); +vi.mock('./call.js', () => ({ + callSmallModel: vi.fn(), + resolveProviderLogin: vi.fn(({ auth, providerID }) => { + const entry = auth?.[providerID]; + return entry && typeof entry === 'object' ? entry : null; + }), +})); const { generateSmallModelText, describeSmallModel } = await import('./index.js'); const { readAuthFile } = await import('../opencode/auth.js'); @@ -126,6 +132,19 @@ describe('describeSmallModel — capability reporting', () => { contextTokens: 8_000, contextKnown: true, structuredOutput: true, + hasLogin: true, + }); + }); + + it('reports hasLogin false when the resolved provider has no usable credential', async () => { + readAuthFile.mockReturnValue({}); + + const described = await describeSmallModel({ directory: '/proj' }); + + expect(described).toMatchObject({ + providerID: 'anthropic', + modelID: 'claude-haiku-4-5', + hasLogin: false, }); }); diff --git a/packages/web/server/lib/walkthrough/DOCUMENTATION.md b/packages/web/server/lib/walkthrough/DOCUMENTATION.md index 43d59d16..0f23fac5 100644 --- a/packages/web/server/lib/walkthrough/DOCUMENTATION.md +++ b/packages/web/server/lib/walkthrough/DOCUMENTATION.md @@ -118,6 +118,14 @@ model picker, only shows providers with a usable login. The in-panel picker on a blocked walkthrough writes this setting too, so recovering from a refusal never silently changes the model behind commit messages. +A settings or `opencode.json` `small_model` override can still name a provider +with no usable login (neither `auth.json` nor `provider..options.apiKey`). +`describeSmallModel` reports that as `hasLogin: false`, readiness refuses with +`reason: 'no-provider-login'` and omits the unusable model so the panel cannot +present it as selected, and generation maps the same code to HTTP 401. The UI +disables Generate and keeps the picker on authenticated providers only — it does +not surface a raw auth error or a special login blocker for this case. + ## Output language A walkthrough its reader cannot read is worth nothing, so the prose language is diff --git a/packages/web/server/lib/walkthrough/index.js b/packages/web/server/lib/walkthrough/index.js index 600b8af4..f65d3e2f 100644 --- a/packages/web/server/lib/walkthrough/index.js +++ b/packages/web/server/lib/walkthrough/index.js @@ -333,6 +333,13 @@ function computeReadiness({ model, digest, files, fileCount, hunkCount, generate return { ready: false, reason, model, generatedFileCount }; } + // A resolved override/config model can still have no usable login. Refuse up + // front and omit the model — offering an unauthenticated selection in the + // picker is what made the old raw auth error feel like a product bug. + if (model.hasLogin === false) { + return { ready: false, reason: 'no-provider-login' }; + } + // Built with the same language the generation would use: the instruction is // part of the prompt, so a readiness answer computed without it would be // measuring a request nobody is going to send. @@ -392,6 +399,13 @@ async function runGeneration({ directory, source, repoRoot, key, force, explicit if (!model) { throw fail('No model is available — sign in to a provider first', 404, { code: 'no-model' }); } + if (model.hasLogin === false) { + throw fail( + `No OpenCode login found for provider "${model.providerID}" — sign in or choose a different model`, + 401, + { code: 'no-provider-login', model }, + ); + } const { digest, files, idByAlias, fileCount, hunkCount, generatedFileCount } = await loadCurrentDiff(directory, source, deps); setStage(repoRoot, key, 'asking'); @@ -494,6 +508,9 @@ async function runGeneration({ directory, source, repoRoot, key, force, explicit if (error?.code === 'output-exhausted') { return fail(error.message, 409, { code: 'output-exhausted', model }); } + if (error?.code === 'no-provider-login') { + return fail(error.message, 401, { code: 'no-provider-login', model }); + } return null; }; diff --git a/packages/web/server/lib/walkthrough/reproduce-2607.test.js b/packages/web/server/lib/walkthrough/reproduce-2607.test.js new file mode 100644 index 00000000..8cca6e55 --- /dev/null +++ b/packages/web/server/lib/walkthrough/reproduce-2607.test.js @@ -0,0 +1,148 @@ +import { execFileSync } from 'child_process'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import express from 'express'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; + +// --------------------------------------------------------------------------- +// Regression for https://github.com/openchamber/openchamber/issues/2607 +// "[Bug] Why say so?" (walkthrough panel) +// +// Before the fix, a walkthrough small model whose provider had no usable login +// reported readiness ready:true, then generation returned HTTP 500 with the raw +// message `No OpenCode login found for provider "deepseek"` — shown in the +// error banner above the "No walkthrough yet" empty state. +// +// After the fix: readiness refuses with `no-provider-login`, and generation +// answers 401 with the same structured code so the UI can show a blocker. +// --------------------------------------------------------------------------- + +const TEMP_HOME = fs.mkdtempSync(path.join(os.tmpdir(), 'oc-home-2607-')); +process.env.HOME = TEMP_HOME; +process.env.OPENCHAMBER_DATA_DIR = path.join(TEMP_HOME, '.config', 'openchamber'); + +const CATALOG = { + deepseek: { + id: 'deepseek', + name: 'DeepSeek', + api: 'https://api.deepseek.com', + models: { + 'deepseek-v4-flash': { + id: 'deepseek-v4-flash', + name: 'DeepSeek V4 Flash', + family: 'deepseek-flash', + limit: { context: 128_000 }, + }, + }, + }, +}; + +vi.mock('../../opencode/models-metadata.js', () => ({ + getModelsMetadata: vi.fn(async () => ({ metadata: CATALOG, fromCache: false })), +})); + +const SOURCE = { kind: 'working-tree', scope: 'all' }; +const REPO_DIR = fs.mkdtempSync(path.join(os.tmpdir(), 'oc-repo-2607-')); + +const setupGitRepo = () => { + const run = (args) => { + try { + return execFileSync('git', args, { cwd: REPO_DIR, encoding: 'utf8' }); + } catch (error) { + throw new Error(`git ${args.join(' ')} failed: ${error.stderr?.toString() ?? error.message}`); + } + }; + + run(['init', '-b', 'main']); + run(['config', 'user.email', 'test@example.com']); + run(['config', 'user.name', 'Test']); + fs.mkdirSync(path.join(REPO_DIR, 'src'), { recursive: true }); + fs.writeFileSync(path.join(REPO_DIR, 'src', 'a.ts'), 'export const a = 1;\n', 'utf8'); + run(['add', 'src/a.ts']); + run(['commit', '-m', 'init']); + fs.writeFileSync(path.join(REPO_DIR, 'src', 'a.ts'), 'export const a = 1;\nexport const b = 2;\n', 'utf8'); +}; + +let walkthrough; +let callSmallModel; + +describe('issue 2607 — walkthrough blocks unauthenticated providers', () => { + beforeAll(async () => { + setupGitRepo(); + fs.writeFileSync( + path.join(REPO_DIR, 'opencode.json'), + JSON.stringify({ small_model: 'deepseek/deepseek-v4-flash' }, null, 2), + 'utf8', + ); + + walkthrough = await import('./index.js'); + callSmallModel = await import('../small-model/call.js'); + }); + + afterAll(() => { + fs.rmSync(TEMP_HOME, { recursive: true, force: true }); + fs.rmSync(REPO_DIR, { recursive: true, force: true }); + }); + + it('resolves the deepseek model but reports not ready without a login', async () => { + const result = await walkthrough.getWalkthrough({ directory: REPO_DIR, source: SOURCE }); + + expect(result.readiness.ready).toBe(false); + expect(result.readiness.reason).toBe('no-provider-login'); + // Unusable models must not be offered as the current selection. + expect(result.readiness.model).toBeUndefined(); + }); + + it('callSmallModel throws a structured no-provider-login error', async () => { + const error = await callSmallModel.callSmallModel({ + auth: {}, + catalog: CATALOG, + workingDirectory: REPO_DIR, + providerID: 'deepseek', + modelID: 'deepseek-v4-flash', + prompt: 'x', + }).then(() => null, (e) => e); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toBe('No OpenCode login found for provider "deepseek"'); + expect(error.code).toBe('no-provider-login'); + expect(error.statusCode).toBe(401); + }); + + it('generateWalkthrough rejects with structured no-provider-login', async () => { + const error = await walkthrough.generateWalkthrough({ directory: REPO_DIR, source: SOURCE }) + .then(() => null, (e) => e); + + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe('no-provider-login'); + expect(error.statusCode).toBe(401); + expect(error.model).toMatchObject({ providerID: 'deepseek', modelID: 'deepseek-v4-flash' }); + }); + + it('answers the generate route with HTTP 401 and code no-provider-login', async () => { + const service = { ...walkthrough, getPullRequestDiff: async () => { throw new Error('not used'); } }; + const app = express(); + app.use(express.json()); + const { registerWalkthroughRoutes } = await import('./routes.js'); + registerWalkthroughRoutes(app, { getWalkthroughService: async () => service }); + + const server = app.listen(0); + await new Promise((resolve) => server.once('listening', resolve)); + const base = `http://127.0.0.1:${server.address().port}`; + try { + const response = await fetch(`${base}/api/walkthrough/generate`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ directory: REPO_DIR, source: SOURCE }), + }); + const body = await response.json(); + + expect(response.status).toBe(401); + expect(body.code).toBe('no-provider-login'); + expect(body.model).toMatchObject({ providerID: 'deepseek', modelID: 'deepseek-v4-flash' }); + } finally { + await new Promise((resolve) => server.close(resolve)); + } + }); +});