diff --git a/apps/desktop/src/main/__tests__/provider-add-submission.test.ts b/apps/desktop/src/main/__tests__/provider-add-submission.test.ts new file mode 100644 index 0000000000..b40cc2b07e --- /dev/null +++ b/apps/desktop/src/main/__tests__/provider-add-submission.test.ts @@ -0,0 +1,205 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { + createProviderWithDiscovery, + validateAddProviderDraft, + type AddProviderDraft, + type AddProviderField, +} from '../../renderer/settings/provider-add-submission.js'; +import { + PROVIDER_DEFAULTS, + providerSupportsModelDiscovery, + type CreateConnectionInput, + type LlmConnection, + type ProviderType, +} from '@maka/core/llm-connections'; + +// A compile-time half of the same promise: the gate's field union has no +// model rule to report, so one cannot be added without this line failing. +type NoModelRule = 'defaultModel' extends AddProviderField ? never : true; +const _fieldGateHasNoModelRule: NoModelRule = true; +void _fieldGateHasNoModelRule; + +const RELAY_TYPES: readonly ProviderType[] = ['openai-compatible', 'openai-responses-compatible']; + +function draft(over: Partial = {}): AddProviderDraft { + return { + providerType: 'openai-compatible', + slug: 'house-relay', + existingSlugs: [], + apiKey: 'sk-test', + cloudflareAccountId: '', + baseUrl: 'https://relay.example.com/v1', + ...over, + }; +} + +function connection(slug: string): LlmConnection { + return { + slug, + name: slug, + providerType: 'openai-compatible', + defaultModel: '', + enabled: true, + createdAt: 0, + updatedAt: 0, + } as LlmConnection; +} + +function bridge(over: { + create?: (input: CreateConnectionInput) => Promise; + fetchModels?: (slug: string) => Promise; +}) { + return { + create: over.create ?? (async (input) => connection(input.slug)), + fetchModels: over.fetchModels ?? (async () => ({ models: [], source: 'fetched' })), + }; +} + +// The first of the two behaviours this module exists to protect. A custom +// relay used to be the only provider class that refused to be created without +// a hand-typed model id — before the app had asked the relay what it serves. +test('a custom relay is created without a hand-typed model id', () => { + for (const providerType of RELAY_TYPES) { + assert.equal(validateAddProviderDraft(draft({ providerType })), null, providerType); + } +}); + +test('no provider type demands a model id at creation', () => { + // Stated across the catalog rather than for the two relays alone: the rule + // that came back would be a per-provider `if`, and asserting only where it + // used to live would let it reappear next door. + for (const providerType of Object.keys(PROVIDER_DEFAULTS) as ProviderType[]) { + const defaults = PROVIDER_DEFAULTS[providerType]; + if (defaults.status === 'phase3-experimental') continue; + const issue = validateAddProviderDraft( + draft({ + providerType, + slug: 'probe-connection', + apiKey: 'sk-test', + baseUrl: 'https://example.com/v1', + cloudflareAccountId: 'account-id', + }), + ); + assert.equal(issue, null, `${providerType} refused a draft with no model id`); + } +}); + +// The second. Discovery failures were reported for every provider except the +// custom relays, which are the endpoints most likely to be misconfigured. +test('a discovery failure reaches the caller for a custom relay', async () => { + for (const providerType of RELAY_TYPES) { + const failure = new Error('relay refused /v1/models'); + const created = await createProviderWithDiscovery( + bridge({ + fetchModels: async () => { + throw failure; + }, + }), + { slug: 'house-relay', name: 'House', providerType } as CreateConnectionInput, + ); + assert.equal(created.connection.slug, 'house-relay'); + assert.equal(created.modelDiscoveryError, failure, providerType); + } +}); + +test('a discovery failure reaches the caller for a built-in provider too', async () => { + const failure = new Error('401'); + const created = await createProviderWithDiscovery( + bridge({ + fetchModels: async () => { + throw failure; + }, + }), + { slug: 'openai-main', name: 'OpenAI', providerType: 'openai' } as CreateConnectionInput, + ); + assert.equal(created.modelDiscoveryError, failure); +}); + +test('a failed catalog fetch still yields the created connection', async () => { + // Discovery is a convenience on top of a successful create, never a + // condition of it: reporting the failure must not read as "nothing was + // created", or the user is sent to make a duplicate. + const created = await createProviderWithDiscovery( + bridge({ + fetchModels: async () => { + throw new Error('ECONNREFUSED'); + }, + }), + { slug: 'house-relay', name: 'House', providerType: 'openai-compatible' } as CreateConnectionInput, + ); + assert.equal(created.connection.slug, 'house-relay'); +}); + +test('a successful catalog fetch reports no error', async () => { + const created = await createProviderWithDiscovery( + bridge({}), + { slug: 'house-relay', name: 'House', providerType: 'openai-compatible' } as CreateConnectionInput, + ); + assert.equal(created.modelDiscoveryError, undefined); +}); + +test('a provider without discovery is not asked, and reports no error', async () => { + const withoutDiscovery = (Object.keys(PROVIDER_DEFAULTS) as ProviderType[]).find( + (providerType) => !providerSupportsModelDiscovery(providerType), + ); + assert.ok(withoutDiscovery, 'expected at least one provider with no discovery endpoint'); + let asked = false; + const created = await createProviderWithDiscovery( + bridge({ + fetchModels: async () => { + asked = true; + return {}; + }, + }), + { slug: 'static-catalog', name: 'Static', providerType: withoutDiscovery } as CreateConnectionInput, + ); + assert.equal(asked, false); + assert.equal(created.modelDiscoveryError, undefined); +}); + +test('a create failure propagates instead of being reported as a discovery problem', async () => { + const failure = new Error('slug already exists'); + await assert.rejects( + createProviderWithDiscovery( + bridge({ + create: async () => { + throw failure; + }, + }), + { slug: 'house-relay', name: 'House', providerType: 'openai-compatible' } as CreateConnectionInput, + ), + failure, + ); +}); + +test('the field gate still reports the rules that survived', () => { + assert.deepEqual(validateAddProviderDraft(draft({ slug: 'Not A Slug' }))?.field, 'slug'); + assert.deepEqual(validateAddProviderDraft(draft({ existingSlugs: ['house-relay'] })), { + field: 'slug', + reason: 'duplicate', + }); + assert.deepEqual(validateAddProviderDraft(draft({ providerType: 'openai', apiKey: ' ' })), { + field: 'apiKey', + reason: 'required', + }); + assert.deepEqual( + validateAddProviderDraft( + draft({ providerType: 'cloudflare-workers-ai', cloudflareAccountId: ' ' }), + ), + { field: 'accountId', reason: 'required' }, + ); + assert.deepEqual(validateAddProviderDraft(draft({ baseUrl: ' ' })), { + field: 'baseUrl', + reason: 'required', + }); +}); + +test('a duplicate slug outranks a missing key, so one fix is asked for at a time', () => { + assert.deepEqual( + validateAddProviderDraft( + draft({ providerType: 'openai', slug: 'taken', existingSlugs: ['taken'], apiKey: '' }), + ), + { field: 'slug', reason: 'duplicate' }, + ); +}); diff --git a/apps/desktop/src/renderer/locales/settings-provider-copy.ts b/apps/desktop/src/renderer/locales/settings-provider-copy.ts index 57fb39e33e..019aba8d80 100644 --- a/apps/desktop/src/renderer/locales/settings-provider-copy.ts +++ b/apps/desktop/src/renderer/locales/settings-provider-copy.ts @@ -162,7 +162,7 @@ const zhCopy = { accountIdPlaceholder: '填写账户 ID', saving: '保存中…', save: '保存供应商', keyRequired: (name: string) => `请填写 ${name} API Key`, apiKeyLabel: 'API Key', accountIdLabel: 'Cloudflare Account ID', endpointLabel: '服务地址', - defaultModel: '默认模型', defaultModelPlaceholder: '填写你的中转站模型 ID,例如 gpt-4o、claude-sonnet-4-5 或自定义模型名', defaultModelHelp: '用于首次连接测试和模型选择器兜底;保存后仍会自动拉取模型目录。', defaultModelRequired: '请填写默认模型 ID。保存后仍会自动拉取模型目录。', + defaultModel: '默认模型', defaultModelPlaceholder: '留空即可,保存后自动拉取', defaultModelHelp: '保存后 Maka 会向该端点拉取模型目录。只有当端点不提供目录时,才需要在这里手填一个模型 ID。', ...zhCapabilitiesCopy, }, oauthFlow: { @@ -307,7 +307,7 @@ const enCopy: ProviderSettingsCopy = { accountIdPlaceholder: 'Enter account ID', saving: 'Saving…', save: 'Save provider', keyRequired: (name: string) => `Enter the ${name} API key`, apiKeyLabel: 'API key', accountIdLabel: 'Cloudflare Account ID', endpointLabel: 'Service URL', - defaultModel: 'Default model', defaultModelPlaceholder: 'Enter your relay model id, e.g. gpt-4o, claude-sonnet-4-5, or a custom model name', defaultModelHelp: 'Used as the first connection-test and picker fallback; Maka still fetches the model catalog after saving.', defaultModelRequired: 'Enter a default model id. Maka still fetches the model catalog after saving.', + defaultModel: 'Default model', defaultModelPlaceholder: 'Leave empty — fetched after saving', defaultModelHelp: 'Maka fetches the model catalog from this endpoint after saving. Type a model id here only if the endpoint serves no catalog.', ...enCapabilitiesCopy, }, oauthFlow: { diff --git a/apps/desktop/src/renderer/settings/provider-add-form.tsx b/apps/desktop/src/renderer/settings/provider-add-form.tsx index a3f77c95cf..db712c6305 100644 --- a/apps/desktop/src/renderer/settings/provider-add-form.tsx +++ b/apps/desktop/src/renderer/settings/provider-add-form.tsx @@ -3,15 +3,10 @@ import { OPENCODE_FREE_DEFAULT_ENABLED_MODELS, type ProviderType, } from '@maka/core/llm-connections'; -import { - PROVIDER_DEFAULTS, - deriveConnectionSlug, - validateSlug, -} from '@maka/core/llm-connections'; +import { PROVIDER_DEFAULTS, deriveConnectionSlug } from '@maka/core/llm-connections'; import { providerAuthRequiresSecret, providerAuthSupportsApiKey, - providerSupportsModelDiscovery, } from '@maka/core/llm-connections'; import { Banner, HStack, VStack } from '@astryxdesign/core'; import { Collapsible } from '@astryxdesign/core/Collapsible'; @@ -39,15 +34,17 @@ import { RequestCustomizationEditor, type RequestHeaderDraft, } from './request-customization-editor'; +import { + createProviderWithDiscovery, + validateAddProviderDraft, + type AddProviderIssue, +} from './provider-add-submission'; -type ProviderFormField = - | 'slug' - | 'apiKey' - | 'accountId' - | 'baseUrl' - | 'defaultModel' - | 'advancedRequest' - | 'form'; +/* No `defaultModel`: the creation gate has no rule that can fail on the model + id, so an error could never be reported against that field. The union is + kept aligned with `AddProviderIssue` plus the two form-local fields the + gate does not own. */ +type ProviderFormField = 'slug' | 'apiKey' | 'accountId' | 'baseUrl' | 'advancedRequest' | 'form'; type ProviderFormError = { field: ProviderFormField; @@ -85,9 +82,7 @@ export function AddProviderForm(props: { const isCloudflareWorkersAi = props.providerType === 'cloudflare-workers-ai'; const requiresBaseUrl = !defaults.baseUrl && !isCloudflareWorkersAi; const showsDefaultModel = recommendedDefaultModel.trim() === ''; - const isCustomRelay = defaults.category === 'custom'; const isExperimental = defaults.status === 'phase3-experimental'; - const supportsRemoteDiscovery = providerSupportsModelDiscovery(props.providerType); const supportsApiKey = providerAuthSupportsApiKey(props.providerType); const requiresApiKey = providerAuthRequiresSecret(props.providerType) && supportsApiKey; const usesApiKeyDialog = usesQuickApiKeyDialog(props.providerType); @@ -98,52 +93,38 @@ export function AddProviderForm(props: { ); } + // The localized sentence for one field gate. The gate itself is in + // provider-add-submission, so the order and the rules are testable without + // a locale in the assertion. + function issueMessage(issue: AddProviderIssue): string { + if (issue.field === 'slug') { + return issue.reason === 'duplicate' + ? copy.duplicateSlug + : locale === 'zh' + ? issue.detail + : copy.invalidSlug; + } + if (issue.field === 'apiKey') return copy.keyRequired(display.name); + if (issue.field === 'accountId') return copy.cloudflareAccount; + if (issue.field === 'baseUrl') return copy.endpointRequired; + return copy.accountLogin; + } + async function submit() { if (submitGuard.current !== null) return; setError(null); - const slugError = validateSlug(slug); - if (slugError) { - return setError({ - field: 'slug', - message: locale === 'zh' ? slugError : copy.invalidSlug, - }); - } - if (props.existingSlugs.includes(slug)) { - return setError({ field: 'slug', message: copy.duplicateSlug }); - } + const issue = validateAddProviderDraft({ + providerType: props.providerType, + slug, + existingSlugs: props.existingSlugs, + apiKey, + cloudflareAccountId, + baseUrl, + }); + if (issue) return setError({ field: issue.field, message: issueMessage(issue) }); const normalizedApiKey = apiKey.trim(); - if (requiresApiKey && !normalizedApiKey) { - return setError({ - field: 'apiKey', - message: copy.keyRequired(display.name), - }); - } const normalizedCloudflareAccountId = cloudflareAccountId.trim(); - if (isCloudflareWorkersAi && !normalizedCloudflareAccountId) { - return setError({ - field: 'accountId', - message: copy.cloudflareAccount, - }); - } - if (requiresBaseUrl && !baseUrl.trim()) { - return setError({ - field: 'baseUrl', - message: copy.endpointRequired, - }); - } const normalizedDefaultModel = defaultModel.trim(); - if (isCustomRelay && !normalizedDefaultModel) { - return setError({ - field: 'defaultModel', - message: copy.defaultModelRequired, - }); - } - if (isExperimental) { - return setError({ - field: 'form', - message: copy.accountLogin, - }); - } let normalizedRequestHeaders: Readonly>; let requestBodyOverlay: ReturnType; try { @@ -163,7 +144,7 @@ export function AddProviderForm(props: { ) : baseUrl || undefined; const createdDefaultModel = normalizedDefaultModel || recommendedDefaultModel; - const connection = await props.bridge.create({ + const created = await createProviderWithDiscovery(props.bridge, { slug, name: name || display.name, providerType: props.providerType, @@ -179,16 +160,7 @@ export function AddProviderForm(props: { ...(requestBodyOverlay === undefined ? {} : { requestBodyOverlay }), }); if (!addProviderMountedRef.current) return; - let modelDiscoveryError: unknown; - if (supportsRemoteDiscovery) { - try { - await props.bridge.fetchModels(connection.slug); - } catch (error) { - if (!isCustomRelay) modelDiscoveryError = error; - } - } - if (!addProviderMountedRef.current) return; - await props.onCreated(connection.slug, modelDiscoveryError); + await props.onCreated(created.connection.slug, created.modelDiscoveryError); } catch (err) { if (addProviderMountedRef.current) { setError({ @@ -366,20 +338,11 @@ export function AddProviderForm(props: { {showsDefaultModel && ( { - setDefaultModel(value); - clearFieldError('defaultModel'); - }} + onChange={setDefaultModel} placeholder={copy.defaultModelPlaceholder} isDisabled={isExperimental || busy} label={copy.defaultModel} description={copy.defaultModelHelp} - isRequired={isCustomRelay} - status={ - error?.field === 'defaultModel' - ? { type: 'error', message: error.message } - : undefined - } /> )} {advancedRequestEditor} diff --git a/apps/desktop/src/renderer/settings/provider-add-submission.ts b/apps/desktop/src/renderer/settings/provider-add-submission.ts new file mode 100644 index 0000000000..d05fd6a49d --- /dev/null +++ b/apps/desktop/src/renderer/settings/provider-add-submission.ts @@ -0,0 +1,112 @@ +import { + PROVIDER_DEFAULTS, + providerAuthRequiresSecret, + providerAuthSupportsApiKey, + providerSupportsModelDiscovery, + validateSlug, + type ProviderType, +} from '@maka/core/llm-connections'; +import type { CreateConnectionInput, LlmConnection } from '@maka/core/llm-connections'; + +/** + * The two decisions 添加连接 makes that are not layout: which fields a provider + * type actually demands, and what the caller learns when the catalog fetch + * that follows creation fails. + * + * They live outside the component because both used to carry a relay-only + * special case, and neither was observable from a test: the form required a + * hand-typed model id from custom relays alone, and then discarded exactly + * those relays' discovery failures. Reverting either of those would have left + * every suite green. + */ + +export type AddProviderField = 'slug' | 'apiKey' | 'accountId' | 'baseUrl' | 'form'; + +export type AddProviderIssue = + | { readonly field: 'slug'; readonly reason: 'invalid'; readonly detail: string } + | { readonly field: 'slug'; readonly reason: 'duplicate' } + | { readonly field: 'apiKey'; readonly reason: 'required' } + | { readonly field: 'accountId'; readonly reason: 'required' } + | { readonly field: 'baseUrl'; readonly reason: 'required' } + | { readonly field: 'form'; readonly reason: 'experimental' }; + +export interface AddProviderDraft { + readonly providerType: ProviderType; + readonly slug: string; + readonly existingSlugs: readonly string[]; + readonly apiKey: string; + readonly cloudflareAccountId: string; + readonly baseUrl: string; +} + +/** + * The field gate, in the order the form reports it — first issue wins, so a + * user fixes one thing at a time rather than being handed a wall. + * + * Reason codes, not sentences: the component owns the localized copy, and a + * test asserting on `{field, reason}` keeps saying the same thing when the + * wording changes. + * + * There is deliberately no rule for the model id. A provider that ships a + * recommended default does not ask, and one that does not ship a default can + * discover its catalog after creation — so requiring a typed id ahead of + * either would demand a guess about a catalog the app is about to fetch. + */ +export function validateAddProviderDraft(draft: AddProviderDraft): AddProviderIssue | null { + const defaults = PROVIDER_DEFAULTS[draft.providerType]; + const slugIssue = validateSlug(draft.slug); + if (slugIssue) return { field: 'slug', reason: 'invalid', detail: slugIssue }; + if (draft.existingSlugs.includes(draft.slug)) return { field: 'slug', reason: 'duplicate' }; + const requiresApiKey = + providerAuthRequiresSecret(draft.providerType) && + providerAuthSupportsApiKey(draft.providerType); + if (requiresApiKey && !draft.apiKey.trim()) return { field: 'apiKey', reason: 'required' }; + const isCloudflareWorkersAi = draft.providerType === 'cloudflare-workers-ai'; + if (isCloudflareWorkersAi && !draft.cloudflareAccountId.trim()) { + return { field: 'accountId', reason: 'required' }; + } + // Cloudflare builds its endpoint from the account id above, so it is not + // missing one — it just has not composed it yet. + const requiresBaseUrl = !defaults.baseUrl && !isCloudflareWorkersAi; + if (requiresBaseUrl && !draft.baseUrl.trim()) return { field: 'baseUrl', reason: 'required' }; + if (defaults.status === 'phase3-experimental') return { field: 'form', reason: 'experimental' }; + return null; +} + +export interface CreatedProvider { + readonly connection: LlmConnection; + /** + * Present when the catalog fetch that follows creation threw. The connection + * exists either way — discovery is a convenience on top of a successful + * create, never a condition of it — so this is something to report, not a + * failure to roll back. + */ + readonly modelDiscoveryError?: unknown; +} + +export interface ProviderCreationBridge { + create(input: CreateConnectionInput): Promise; + fetchModels(slug: string): Promise; +} + +/** + * Create the connection, then let its catalog answer for itself. + * + * Every provider that supports discovery reports its failure. The custom + * relays used to be the exception — swallowed on the reasoning that a relay + * may not implement the endpoint — which left the one provider class most + * likely to be misconfigured with an empty model picker and nothing said. + */ +export async function createProviderWithDiscovery( + bridge: ProviderCreationBridge, + input: CreateConnectionInput, +): Promise { + const connection = await bridge.create(input); + if (!providerSupportsModelDiscovery(input.providerType)) return { connection }; + try { + await bridge.fetchModels(connection.slug); + } catch (modelDiscoveryError) { + return { connection, modelDiscoveryError }; + } + return { connection }; +}