diff --git a/packages/browseros-agent/apps/app/lib/llm-providers/types.ts b/packages/browseros-agent/apps/app/lib/llm-providers/types.ts index ba7226463..71088669d 100644 --- a/packages/browseros-agent/apps/app/lib/llm-providers/types.ts +++ b/packages/browseros-agent/apps/app/lib/llm-providers/types.ts @@ -62,6 +62,15 @@ export interface LlmProviderConfig { // so it is stored as a free string validated against the model at selection time. reasoningEffort?: string reasoningSummary?: 'auto' | 'concise' | 'detailed' + + // Whether a credential is stored on the server, for providers read back from + // it. The values themselves never leave the server, so a form editing an + // existing provider sees these rather than the secret, and leaving a field + // blank keeps what is stored. + hasApiKey?: boolean + hasAccessKeyId?: boolean + hasSecretAccessKey?: boolean + hasSessionToken?: boolean } /** diff --git a/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.test.ts b/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.test.ts index 3be6dba6b..a1e3989c2 100644 --- a/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.test.ts +++ b/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.test.ts @@ -245,3 +245,51 @@ function createSelectionStore( watch: () => () => {}, } } + +// Each extension surface holds its own cache of a list that lives on the +// server, so one can be a refetch behind another. Repairing against a list +// that has not caught up destroys a choice the user just made, which is what +// made selecting a new provider appear to revert to BrowserOS. +describe('resolveRepairedSelection with an incomplete list', () => { + const browserosTarget = { + kind: 'llm' as const, + id: 'browseros', + name: 'BrowserOS', + type: 'browseros' as const, + provider: {} as never, + } + + it('leaves a selection this surface has not seen yet alone', () => { + expect( + resolveRepairedSelection({ + selection: { kind: 'llm', id: 'just-created' }, + resolvedTarget: browserosTarget, + ready: true, + knownIds: new Set(['browseros']), + }).repair, + ).toBe(false) + }) + + // The case repair exists for: the provider is gone from a list that does + // know about it, so the selection genuinely dangles. + it('still repairs a selection the list can account for', () => { + expect( + resolveRepairedSelection({ + selection: { kind: 'llm', id: 'deleted-but-known' }, + resolvedTarget: browserosTarget, + ready: true, + knownIds: new Set(['browseros', 'deleted-but-known']), + }), + ).toEqual({ repair: true, selection: { kind: 'llm', id: 'browseros' } }) + }) + + it('repairs as before when no list is given', () => { + expect( + resolveRepairedSelection({ + selection: { kind: 'llm', id: 'gone' }, + resolvedTarget: browserosTarget, + ready: true, + }).repair, + ).toBe(true) + }) +}) diff --git a/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.ts b/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.ts index 7334d9759..5519e8b4c 100644 --- a/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.ts +++ b/packages/browseros-agent/apps/app/modules/chat/sidepanel-chat-targets.ts @@ -121,24 +121,39 @@ export type RepairSelectionDecision = | { repair: true; selection: SidepanelChatTargetSelection | null } /** - * Decides whether a persisted sidebar selection needs repair. It never repairs - * an ACP selection: the agents list is fetch-backed and can be stale (a - * persisted react-query cache, or a different extension context that has not - * refetched a newly-created agent), so repairing here would wipe a valid ACP - * default and silently downgrade it to the LLM fallback. Stale ACP selections - * are cleaned by `clearSidepanelChatTargetSelectionForAgent` on delete, and - * `resolveSidepanelChatTarget` already falls back non-destructively at render. - * Only LLM selections are repaired, since providers load reliably from local - * storage, and only once loads are settled. + * Decides whether a persisted sidebar selection needs repair. + * + * Repair exists for one case: the selection names something that has been + * deleted, so the sidebar would otherwise point at nothing. Absence from the + * list in hand is not evidence of that. Each extension surface holds its own + * query cache of a list that lives on the server, so a provider added in + * another surface is missing here until this one refetches, and rewriting the + * selection then destroys a choice the user just made. That is what made + * picking a new provider appear to revert to BrowserOS while picking an agent + * worked, since ACP selections were already exempt. + * + * So a selection is only repaired when the list is known to be complete, and a + * selection this list cannot confirm is left alone for the next render, when + * the revision signal will have brought the list up to date. + * `resolveSidepanelChatTarget` already falls back non-destructively for + * display, so nothing is broken while that happens. Stale entries are still + * cleaned on delete by `clearSidepanelChatTargetSelectionForAgent`. */ export function resolveRepairedSelection({ selection, resolvedTarget, ready, + knownIds, }: { selection: SidepanelChatTargetSelection | null resolvedTarget: SidepanelChatTarget | undefined ready: boolean + /** + * Every id this surface currently knows about, of either kind. A selection + * naming something absent from it is treated as not yet loaded rather than + * as deleted. + */ + knownIds?: ReadonlySet }): RepairSelectionDecision { if (!ready || !selection) return { repair: false } if (selection.kind === 'acp') return { repair: false } @@ -149,6 +164,8 @@ export function resolveRepairedSelection({ ) { return { repair: false } } + // Inconclusive rather than deleted: this surface has not seen it yet. + if (knownIds && !knownIds.has(selection.id)) return { repair: false } return { repair: true, selection: resolvedTarget diff --git a/packages/browseros-agent/apps/app/modules/chat/use-chat-target-selection.ts b/packages/browseros-agent/apps/app/modules/chat/use-chat-target-selection.ts index 6904ba838..c15556de7 100644 --- a/packages/browseros-agent/apps/app/modules/chat/use-chat-target-selection.ts +++ b/packages/browseros-agent/apps/app/modules/chat/use-chat-target-selection.ts @@ -97,11 +97,18 @@ export function useChatTargetSelection() { selection: targetSelection, resolvedTarget: selectedChatTarget, ready, + knownIds: new Set(chatTargets.map((target) => target.id)), }) if (!decision.repair) return setTargetSelection(decision.selection) void persistSidepanelChatTargetSelection(selectedChatTarget) - }, [agentsSettled, isLoadingProviders, selectedChatTarget, targetSelection]) + }, [ + agentsSettled, + chatTargets, + isLoadingProviders, + selectedChatTarget, + targetSelection, + ]) const selectedLlmProviderRef = useRef( selectedLlmProvider, diff --git a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.api.ts b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.api.ts index cc3f840a5..3aa0c517a 100644 --- a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.api.ts +++ b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.api.ts @@ -3,11 +3,8 @@ import { hc } from 'hono/client' import { createDefaultBrowserOSProvider } from '@/lib/llm-providers/storage' import type { LlmProviderConfig } from '@/lib/llm-providers/types' import { resolveAgentServerUrlWithRetry } from '@/modules/browseros/agent-server-url.helpers' -import { - type ProviderRow, - toProviderConfigs, - toProviderPayload, -} from './llm-providers.helpers' +import { toProviderConfigs, toProviderPayload } from './llm-providers.helpers' +import { bumpProviderRevision } from './llm-providers.revision' async function providersClient() { const baseUrl = await resolveAgentServerUrlWithRetry() @@ -23,6 +20,7 @@ export async function putProvider(config: LlmProviderConfig): Promise { if (!response.ok) { throw new Error(`Failed to save provider (${response.status})`) } + await bumpProviderRevision() } export async function deleteProvider(providerId: string): Promise { @@ -33,6 +31,7 @@ export async function deleteProvider(providerId: string): Promise { if (!response.ok && response.status !== 404) { throw new Error(`Failed to delete provider (${response.status})`) } + await bumpProviderRevision() } /** @@ -58,6 +57,7 @@ export async function putDefaultProvider(providerId: string): Promise { if (!response.ok) { throw new Error(`Failed to set the default provider (${response.status})`) } + await bumpProviderRevision() } export async function listProviders(): Promise { @@ -67,7 +67,7 @@ export async function listProviders(): Promise { throw new Error(`Failed to load providers (${response.status})`) } const { providers } = await response.json() - return toProviderConfigs(providers as ProviderRow[]) + return toProviderConfigs(providers) } /** diff --git a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.test.ts b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.test.ts index f854625a6..6c221daef 100644 --- a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.test.ts +++ b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.test.ts @@ -13,15 +13,16 @@ function row(overrides: Partial = {}): ProviderRow { id: 'provider-1', type: 'openai', name: 'My OpenAI', + kind: 'llm', baseUrl: null, modelId: 'gpt-5.5', supportsImages: true, contextWindow: 200000, temperature: 0.2, - apiKey: null, - accessKeyId: null, - secretAccessKey: null, - sessionToken: null, + hasApiKey: false, + hasAccessKeyId: false, + hasSecretAccessKey: false, + hasSessionToken: false, resourceName: null, region: null, reasoningEffort: null, @@ -54,20 +55,26 @@ describe('toProviderConfig', () => { const converted = toProviderConfig(row()) expect(converted?.baseUrl).toBeUndefined() - expect(converted?.apiKey).toBeUndefined() expect(converted?.reasoningSummary).toBeUndefined() }) - it('carries the credentials across', () => { + // Credentials never leave the server now, so the row reports only whether + // one is set and the config carries that instead. + it('carries the credential flags, not the credentials', () => { const converted = toProviderConfig( - row({ apiKey: 'sk-test', accessKeyId: 'AKIA', region: 'us-east-1' }), + row({ hasApiKey: true, region: 'us-east-1' }), ) - expect(converted).toMatchObject({ - apiKey: 'sk-test', - accessKeyId: 'AKIA', - region: 'us-east-1', - }) + expect(converted).toMatchObject({ hasApiKey: true, region: 'us-east-1' }) + expect(converted?.apiKey).toBeUndefined() + }) + + // Coding agents share this table and this endpoint. Filtering on kind is + // what excludes them; the unknown-type guard below used to do it by accident. + it('rejects a coding agent', () => { + expect( + toProviderConfig(row({ kind: 'acp', type: 'claude', modelId: null })), + ).toBeNull() }) // The row survives in the database and comes back on upgrade. Showing it diff --git a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.ts b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.ts index eeeed8b73..b5539fcfa 100644 --- a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.ts +++ b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.helpers.ts @@ -1,20 +1,28 @@ import { isProviderType } from '@/lib/llm-providers/providerTemplates' import type { LlmProviderConfig } from '@/lib/llm-providers/types' -/** A provider row as the server returns it: absent values are null, not undefined. */ +/** + * A provider row as the server returns it. + * + * Absent values are null rather than undefined, and credentials are not here + * at all: the server reports only whether each is set, so a key cannot reach + * a surface that has no use for it. + */ export interface ProviderRow { id: string + kind: 'llm' | 'acp' type: string name: string baseUrl: string | null - modelId: string + // Nullable since the table holds coding agents too, and those carry neither. + modelId: string | null supportsImages: boolean - contextWindow: number + contextWindow: number | null temperature: number - apiKey: string | null - accessKeyId: string | null - secretAccessKey: string | null - sessionToken: string | null + hasApiKey: boolean + hasAccessKeyId: boolean + hasSecretAccessKey: boolean + hasSessionToken: boolean resourceName: string | null region: string | null reasoningEffort: string | null @@ -45,7 +53,13 @@ function toReasoningSummary( * map, the template lookup and the default base URLs, all keyed by the union. */ export function toProviderConfig(row: ProviderRow): LlmProviderConfig | null { + // Coding agents share this table and this endpoint, and are served to the + // surfaces that want them through their own hook. Filtering on kind says + // that; leaning on the unknown-type guard below to drop them happened to + // work and said something else entirely. + if (row.kind !== 'llm') return null if (!isProviderType(row.type)) return null + if (row.modelId === null || row.contextWindow === null) return null return { id: row.id, @@ -56,10 +70,10 @@ export function toProviderConfig(row: ProviderRow): LlmProviderConfig | null { supportsImages: row.supportsImages, contextWindow: row.contextWindow, temperature: row.temperature, - apiKey: orUndefined(row.apiKey), - accessKeyId: orUndefined(row.accessKeyId), - secretAccessKey: orUndefined(row.secretAccessKey), - sessionToken: orUndefined(row.sessionToken), + hasApiKey: row.hasApiKey, + hasAccessKeyId: row.hasAccessKeyId, + hasSecretAccessKey: row.hasSecretAccessKey, + hasSessionToken: row.hasSessionToken, resourceName: orUndefined(row.resourceName), region: orUndefined(row.region), reasoningEffort: orUndefined(row.reasoningEffort), diff --git a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.hooks.ts b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.hooks.ts index dfff1bd6e..fe1ec9a43 100644 --- a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.hooks.ts +++ b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.hooks.ts @@ -1,4 +1,5 @@ import { useMutation, useQueryClient } from '@tanstack/react-query' +import { useEffect } from 'react' import { createQuery } from 'react-query-kit' import { resolveDefaultProviderId, @@ -14,6 +15,7 @@ import { putProvider, } from './llm-providers.api' import { planProviderSave } from './llm-providers.helpers' +import { watchProviderRevision } from './llm-providers.revision' export interface UseLlmProvidersReturn { providers: LlmProviderConfig[] @@ -60,11 +62,36 @@ export async function persistDefaultProviderId( await putDefaultProvider(providerId) } +/** + * Keeps this surface current with provider writes made in another one. + * + * Each extension surface has its own query cache, and the rows live on the + * server where nothing can watch them. Writers bump a revision in extension + * storage instead, which does broadcast to every context, and each mounted + * view refetches. This is what `providersStorage.watch` used to do before the + * list moved off extension storage. + */ +function useProviderRevision(): void { + const queryClient = useQueryClient() + + useEffect( + () => + watchProviderRevision(() => { + queryClient.invalidateQueries({ queryKey: useProvidersQuery.getKey() }) + queryClient.invalidateQueries({ + queryKey: useDefaultProviderIdQuery.getKey(), + }) + }), + [queryClient], + ) +} + /** Hook for managing LLM provider configurations. */ export function useLlmProviders(): UseLlmProvidersReturn { const queryClient = useQueryClient() const providersQuery = useProvidersQuery() const defaultQuery = useDefaultProviderIdQuery() + useProviderRevision() const storedDefaultId = defaultQuery.data ?? DEFAULT_PROVIDER_ID const providers = providersQuery.data ?? [] diff --git a/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.revision.ts b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.revision.ts new file mode 100644 index 000000000..c346f3dc2 --- /dev/null +++ b/packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.revision.ts @@ -0,0 +1,28 @@ +import { storage } from '@wxt-dev/storage' + +/** + * A change signal, not data. + * + * The provider list and the selected provider live on the server, and every + * extension surface holds its own query cache of them. Extension storage used + * to carry the data itself, so `watch` kept the side panel, the new tab, the + * settings page and the scheduled tasks page in step for free. Moving the data + * to the server kept the cache per surface and dropped the broadcast, so a + * provider added in one place stayed invisible to the others until they + * happened to refetch. This keeps the broadcast half. + * + * The value is a timestamp rather than a counter so two surfaces writing at + * once cannot lose a bump to a read-modify-write race. + */ +export const providerRevisionStorage = storage.defineItem( + 'local:provider-revision', + { fallback: 0 }, +) + +export async function bumpProviderRevision(): Promise { + await providerRevisionStorage.setValue(Date.now()) +} + +export function watchProviderRevision(onChange: () => void): () => void { + return providerRevisionStorage.watch(() => onChange()) +} diff --git a/packages/browseros-agent/apps/app/screens/ai-settings/NewProviderDialog.tsx b/packages/browseros-agent/apps/app/screens/ai-settings/NewProviderDialog.tsx index 6d26ebbb1..d74cc3a31 100644 --- a/packages/browseros-agent/apps/app/screens/ai-settings/NewProviderDialog.tsx +++ b/packages/browseros-agent/apps/app/screens/ai-settings/NewProviderDialog.tsx @@ -153,9 +153,29 @@ function isProviderTestable(input: { accessKeyId?: string secretAccessKey?: string region?: string + /** + * Credentials already held by the server for this provider. Reads do not + * return the values, so editing one leaves the fields blank; a stored + * credential satisfies the requirement exactly as a typed one does, and + * leaving it blank keeps what is stored. + */ + stored?: { + hasApiKey?: boolean + hasAccessKeyId?: boolean + hasSecretAccessKey?: boolean + hasSessionToken?: boolean + } }): boolean { if (!input.modelId) return false + const hasApiKey = Boolean(input.apiKey || input.stored?.hasApiKey) + const hasAccessKeyId = Boolean( + input.accessKeyId || input.stored?.hasAccessKeyId, + ) + const hasSecretAccessKey = Boolean( + input.secretAccessKey || input.stored?.hasSecretAccessKey, + ) + if ( input.type === 'chatgpt-pro' || input.type === 'github-copilot' || @@ -165,13 +185,13 @@ function isProviderTestable(input: { } if (input.type === 'azure') { - return Boolean((input.resourceName || input.baseUrl) && input.apiKey) + return Boolean((input.resourceName || input.baseUrl) && hasApiKey) } if (input.type === 'bedrock') { - return Boolean(input.accessKeyId && input.secretAccessKey && input.region) + return Boolean(hasAccessKeyId && hasSecretAccessKey && input.region) } if (!input.baseUrl) return false - if (!['ollama', 'lmstudio'].includes(input.type) && !input.apiKey) { + if (!['ollama', 'lmstudio'].includes(input.type) && !hasApiKey) { return false } return true @@ -437,6 +457,7 @@ export const NewProviderDialog: FC = ({ accessKeyId: watchedAccessKeyId, secretAccessKey: watchedSecretAccessKey, region: watchedRegion, + stored: initialValues, }) const handleTest = async () => { diff --git a/packages/browseros-agent/apps/app/screens/scheduled-tasks/NewScheduledTaskDialog.tsx b/packages/browseros-agent/apps/app/screens/scheduled-tasks/NewScheduledTaskDialog.tsx index efb23f98b..cc6da02c4 100644 --- a/packages/browseros-agent/apps/app/screens/scheduled-tasks/NewScheduledTaskDialog.tsx +++ b/packages/browseros-agent/apps/app/screens/scheduled-tasks/NewScheduledTaskDialog.tsx @@ -36,14 +36,14 @@ import { } from '@/components/ui/select' import { Textarea } from '@/components/ui/textarea' import { SCHEDULED_TASK_PROMPT_REFINED_EVENT } from '@/lib/constants/analyticsEvents' -import { - findChatProviderById, - resolveChatProvider, -} from '@/lib/llm-providers/provider-runtime' +import { resolveChatProvider } from '@/lib/llm-providers/provider-runtime' import { BrowserOSIcon, ProviderIcon } from '@/lib/llm-providers/providerIcons' import type { ProviderType } from '@/lib/llm-providers/types' import { track } from '@/lib/metrics/track' import { refinePrompt } from '@/lib/schedules/refine-prompt' +import { useAcpAgents } from '@/modules/agents/agents.hooks' +import { toProviderOption } from '@/modules/chat/chat-session-request' +import { buildSidepanelChatTargets } from '@/modules/chat/sidepanel-chat-targets' import { useLlmProviders } from '@/modules/llm-providers/llm-providers.hooks' import type { ScheduledJob } from './types' @@ -97,6 +97,12 @@ export const NewScheduledTaskDialog: FC = ({ }) => { const isEditing = !!initialValues const { providers, defaultProviderId } = useLlmProviders() + const { agents } = useAcpAgents() + // A scheduled job can target a coding agent as readily as an llm provider + // now that both are rows of one table, so the picker offers both. While they + // were separate tables the job's provider reference could only ever name an + // llm one, which is why this dialog listed those alone. + const chatTargets = buildSidepanelChatTargets({ providers, agents }) const form = useForm({ resolver: zodResolver(formSchema), @@ -149,9 +155,12 @@ export const NewScheduledTaskDialog: FC = ({ }, [open, initialValues, form]) const resolvedProvider: Provider | null = (() => { - const found = - findChatProviderById(providers, selectedProviderId) ?? - resolveChatProvider(providers, defaultProviderId) + const chosen = chatTargets.find( + (target) => target.id === selectedProviderId, + ) + if (chosen) return toProviderOption(chosen) + + const found = resolveChatProvider(providers, defaultProviderId) if (found) { return { kind: 'llm' as const, @@ -171,12 +180,7 @@ export const NewScheduledTaskDialog: FC = ({ return null })() - const providerOptions: Provider[] = providers.map((provider) => ({ - kind: 'llm', - id: provider.id, - name: provider.name, - type: provider.type, - })) + const providerOptions: Provider[] = chatTargets.map(toProviderOption) // Replace textarea content via execCommand so the browser's native undo // stack (Cmd+Z / Ctrl+Z) records the change. Falls back to form.setValue @@ -233,7 +237,11 @@ export const NewScheduledTaskDialog: FC = ({ } const onSubmit = (values: FormValues) => { - const provider = findChatProviderById(providers, values.providerId) + // Either kind, so an id that names an agent is stored as readily as one + // that names a provider. + const provider = chatTargets.find( + (target) => target.id === values.providerId, + ) onSave({ name: values.name.trim(), query: values.query.trim(), diff --git a/packages/browseros-agent/apps/server/src/api/routes/chat.ts b/packages/browseros-agent/apps/server/src/api/routes/chat.ts index ccf5d3a93..96f559fb4 100644 --- a/packages/browseros-agent/apps/server/src/api/routes/chat.ts +++ b/packages/browseros-agent/apps/server/src/api/routes/chat.ts @@ -45,6 +45,18 @@ interface ChatRouteDeps { providerStore?: ChatProviderLookup } +/** + * The one place a route reads provider credentials. + * + * The store's ordinary reads return a projection without them, so building an + * outbound model request has to ask for them by name. Anything else that + * reaches for this lookup is doing something it should not. + */ +const credentialedProviderLookup: ChatProviderLookup = { + get: (id) => dbProviderStore.getWithCredentials(id), + getDefault: () => dbProviderStore.getDefaultWithCredentials(), +} + // /chat deliberately exposes a plain Hono type. Its AI SDK stream payloads are // not an RPC contract, and carrying every inferred route through the root app // exceeds TypeScript's instantiation depth. @@ -81,7 +93,7 @@ export function createChatRoutes(deps: ChatRouteDeps): Hono { if (parsedBrowserRequest) { const hydrated = await hydrateChatProvider( parsedBrowserRequest, - deps.providerStore ?? dbProviderStore, + deps.providerStore ?? credentialedProviderLookup, ) if (!hydrated.ok) return c.json({ error: hydrated.error }, 400) // A browseros request is otherwise allowed without the app-origin check, diff --git a/packages/browseros-agent/apps/server/src/lib/providers/provider-store.ts b/packages/browseros-agent/apps/server/src/lib/providers/provider-store.ts index 3cbd0752f..1a283c8ca 100644 --- a/packages/browseros-agent/apps/server/src/lib/providers/provider-store.ts +++ b/packages/browseros-agent/apps/server/src/lib/providers/provider-store.ts @@ -4,10 +4,59 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { and, eq, ne } from 'drizzle-orm' +import { and, eq, ne, sql } from 'drizzle-orm' +import type { AnySQLiteColumn } from 'drizzle-orm/sqlite-core' import { getDb } from '../db' import { type NewProviderRow, type ProviderRow, providers } from '../db/schema' +/** + * Every column except the four that hold secrets, plus flags saying whether + * each is set so the UI can still show that a key exists. + * + * Drizzle has no view, but naming the columns gives the same guarantee: a + * caller of the public reads cannot receive a credential even by accident, + * where a `select()` would hand them out on every list, get and default. + */ +function isSet(column: AnySQLiteColumn) { + return sql`${column} IS NOT NULL AND ${column} <> ''`.mapWith( + Boolean, + ) +} + +const publicColumns = { + id: providers.id, + profileId: providers.profileId, + kind: providers.kind, + type: providers.type, + name: providers.name, + modelId: providers.modelId, + reasoningEffort: providers.reasoningEffort, + isDefault: providers.isDefault, + createdAt: providers.createdAt, + updatedAt: providers.updatedAt, + baseUrl: providers.baseUrl, + supportsImages: providers.supportsImages, + contextWindow: providers.contextWindow, + temperature: providers.temperature, + resourceName: providers.resourceName, + region: providers.region, + reasoningSummary: providers.reasoningSummary, + workingDirectory: providers.workingDirectory, + customConfig: providers.customConfig, + // Empty counts as unset, matching what the upsert treats as not supplied. + // Otherwise a blank field would read back as a stored credential. + hasApiKey: isSet(providers.apiKey), + hasAccessKeyId: isSet(providers.accessKeyId), + hasSecretAccessKey: isSet(providers.secretAccessKey), + hasSessionToken: isSet(providers.sessionToken), +} + +export type PublicProviderRow = { + [K in keyof typeof publicColumns]: K extends `has${string}` + ? boolean + : ProviderRow[Extract] +} + /** * The store stamps `updatedAt` and defaults `createdAt`, so callers supply * neither. `createdAt` stays optional so an import can preserve the original @@ -21,11 +70,16 @@ export type ProviderUpsert = Omit< } export interface ProviderStore { - /** Every provider, whatever its kind. */ - list(): Promise - /** Only the LLM providers, for the surfaces that still separate them. */ - listLlm(): Promise - get(id: string): Promise + /** Every provider, whatever its kind, without credentials. */ + list(): Promise + /** Only the LLM providers, without credentials. */ + listLlm(): Promise + get(id: string): Promise + /** + * The full row, credentials included. Only for callers inside the server + * that have to build an outbound request, never for a route response. + */ + getWithCredentials(id: string): Promise /** Insert or replace by id. This is the app's ordinary write path. */ upsert(row: ProviderUpsert): Promise /** @@ -39,7 +93,9 @@ export interface ProviderStore { insertIfAbsent(row: ProviderUpsert): Promise remove(id: string): Promise /** The one selected provider, of any kind, or null when none is set. */ - getDefault(): Promise + getDefault(): Promise + /** The selected provider with its credentials, for the same callers as above. */ + getDefaultWithCredentials(): Promise /** * Points the default at one provider of any kind. Returns false when the id * is unknown, so a stale pointer cannot be stored. @@ -47,15 +103,28 @@ export interface ProviderStore { setDefault(id: string): Promise } -async function list(): Promise { - return getDb().select().from(providers).all() +async function list(): Promise { + return getDb().select(publicColumns).from(providers).all() } -async function listLlm(): Promise { - return getDb().select().from(providers).where(eq(providers.kind, 'llm')).all() +async function listLlm(): Promise { + return getDb() + .select(publicColumns) + .from(providers) + .where(eq(providers.kind, 'llm')) + .all() +} + +async function get(id: string): Promise { + const [row] = await getDb() + .select(publicColumns) + .from(providers) + .where(eq(providers.id, id)) + .limit(1) + return row ?? null } -async function get(id: string): Promise { +async function getWithCredentials(id: string): Promise { const [row] = await getDb() .select() .from(providers) @@ -64,6 +133,36 @@ async function get(id: string): Promise { return row ?? null } +const CREDENTIAL_FIELDS = [ + 'apiKey', + 'accessKeyId', + 'secretAccessKey', + 'sessionToken', +] as const + +/** + * Drops credential fields the caller did not supply, so they keep their stored + * value. + * + * Reads no longer return credentials, so a client editing a provider cannot + * send back what it never received. A plain whole-row upsert would then write + * over a working key on every rename. + * + * An empty string counts as not supplied, not as an instruction to clear. A + * form field that was never filled in submits as `''` rather than undefined, + * so treating the two differently would wipe the key on exactly the edit this + * exists to protect. Clearing is deliberate and explicit: send null. + */ +function withoutAbsentCredentials>( + row: T, +): Partial { + const next: Record = { ...row } + for (const field of CREDENTIAL_FIELDS) { + if (next[field] === undefined || next[field] === '') delete next[field] + } + return next as Partial +} + async function upsert(row: ProviderUpsert): Promise { const now = Date.now() const values = { ...row, kind: 'llm' as const } @@ -76,7 +175,7 @@ async function upsert(row: ProviderUpsert): Promise { // rewrite when the user originally created it. isDefault likewise, so a // save does not silently move the selection. set: { - ...values, + ...withoutAbsentCredentials(values), createdAt: undefined, isDefault: undefined, updatedAt: now, @@ -113,7 +212,16 @@ async function remove(id: string): Promise { return deleted.length > 0 } -async function getDefault(): Promise { +async function getDefault(): Promise { + const [row] = await getDb() + .select(publicColumns) + .from(providers) + .where(eq(providers.isDefault, true)) + .limit(1) + return row ?? null +} + +async function getDefaultWithCredentials(): Promise { const [row] = await getDb() .select() .from(providers) @@ -146,9 +254,11 @@ export const dbProviderStore: ProviderStore = { list, listLlm, get, + getWithCredentials, upsert, insertIfAbsent, remove, getDefault, + getDefaultWithCredentials, setDefault, } diff --git a/packages/browseros-agent/apps/server/tests/lib/providers/provider-store.test.ts b/packages/browseros-agent/apps/server/tests/lib/providers/provider-store.test.ts index aa14ac13b..1d6a20694 100644 --- a/packages/browseros-agent/apps/server/tests/lib/providers/provider-store.test.ts +++ b/packages/browseros-agent/apps/server/tests/lib/providers/provider-store.test.ts @@ -62,7 +62,11 @@ describe('dbProviderStore', () => { expect(saved).toBeNull() const existing = await dbProviderStore.get(PROVIDER_ID) expect(existing?.name).toBe('Edited since') - expect(existing?.apiKey).toBe('sk-test') + // The credential is checked through the credentialed read: the ordinary + // one no longer returns it. + expect( + (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, + ).toBe('sk-test') }) // Integer would floor this to 0 and silently make every model deterministic. @@ -176,4 +180,146 @@ describe('dbProviderStore', () => { ]) expect(await dbProviderStore.list()).toHaveLength(2) }) + + describe('credentials', () => { + // Every provider read used to hand back the api key and the aws secret, + // on the list, the get and the default alike. + test('the ordinary reads return no credentials', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + await dbProviderStore.setDefault(PROVIDER_ID) + + for (const row of [ + await dbProviderStore.get(PROVIDER_ID), + (await dbProviderStore.list())[0], + (await dbProviderStore.listLlm())[0], + await dbProviderStore.getDefault(), + ]) { + for (const field of [ + 'apiKey', + 'accessKeyId', + 'secretAccessKey', + 'sessionToken', + ]) { + expect(row && field in row).toBe(false) + } + } + }) + + // The UI still has to show that a key is set, without being given it. + test('the ordinary reads report whether a credential is set', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + await dbProviderStore.upsert({ + ...baseProvider(), + id: 'no-key', + apiKey: undefined, + }) + + expect((await dbProviderStore.get(PROVIDER_ID))?.hasApiKey).toBe(true) + expect((await dbProviderStore.get('no-key'))?.hasApiKey).toBe(false) + }) + + test('the credentialed read still returns them', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + + expect( + (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, + ).toBe('sk-test') + }) + + // Reads no longer return the key, so an edit cannot send it back. Writing + // undefined over a working credential on every rename is the failure this + // prevents. + test('an absent credential keeps its stored value', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + + await dbProviderStore.upsert({ + id: PROVIDER_ID, + type: 'openai', + name: 'Renamed', + modelId: 'gpt-5.5', + contextWindow: 200000, + }) + + const saved = await dbProviderStore.getWithCredentials(PROVIDER_ID) + expect(saved?.name).toBe('Renamed') + expect(saved?.apiKey).toBe('sk-test') + }) + + // A form field the user never filled in submits as an empty string, not + // as undefined. Treating that as an instruction to clear would wipe the key + // on exactly the edit this protects. + test('an empty credential is treated as not supplied', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + + await dbProviderStore.upsert({ ...baseProvider(), apiKey: '' }) + + expect( + (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, + ).toBe('sk-test') + }) + + // And an empty value must not read back as a stored credential either. + // Every flag, not just the api key: they are one definition and a + // divergence would only show on the credential nobody tested. + test('an empty credential does not report as set', async () => { + useTempDb() + await dbProviderStore.upsert({ + ...baseProvider(), + apiKey: '', + accessKeyId: '', + secretAccessKey: '', + sessionToken: '', + }) + + const row = await dbProviderStore.get(PROVIDER_ID) + expect(row?.hasApiKey).toBe(false) + expect(row?.hasAccessKeyId).toBe(false) + expect(row?.hasSecretAccessKey).toBe(false) + expect(row?.hasSessionToken).toBe(false) + }) + + test('every credential survives a blank edit, not just the api key', async () => { + useTempDb() + await dbProviderStore.upsert({ + ...baseProvider(), + accessKeyId: 'AKIA', + secretAccessKey: 'aws-secret', + sessionToken: 'token', + }) + + await dbProviderStore.upsert({ + ...baseProvider(), + apiKey: '', + accessKeyId: '', + secretAccessKey: '', + sessionToken: '', + }) + + expect( + await dbProviderStore.getWithCredentials(PROVIDER_ID), + ).toMatchObject({ + apiKey: 'sk-test', + accessKeyId: 'AKIA', + secretAccessKey: 'aws-secret', + sessionToken: 'token', + }) + }) + + test('an explicitly null credential clears it', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + + await dbProviderStore.upsert({ ...baseProvider(), apiKey: null }) + + expect( + (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, + ).toBeNull() + expect((await dbProviderStore.get(PROVIDER_ID))?.hasApiKey).toBe(false) + }) + }) })