From 30eb4c8dd0ab2212b224686a84f09dc4e6d441ef Mon Sep 17 00:00:00 2001 From: Dani Akash Date: Thu, 3 Sep 2026 16:30:46 +0530 Subject: [PATCH 1/2] fix(app): restore cross-surface provider sync and stop hiding credentials Three issues from testing on a real profile, two of which share a cause. Extension surfaces are separate contexts with separate query caches, and the provider list broadcast to all of them through extension storage until it moved to the server. Nothing replaced that, so a provider added in one surface stayed invisible to the others. The scheduled tasks dialog showed only the built-in provider for exactly that reason. Writers now bump a revision in extension storage, which does reach every context, mirroring what the scheduled runs already do. The same staleness turned destructive in the sidebar. Selecting a new provider wrote the choice and the server accepted it, then the repair effect ran against a list this surface had not refreshed, failed to find it, and wrote the built-in provider back over the selection. Agents were unaffected only because the repair skips them, which is why it looked like a provider problem. Absence from a list in hand is no longer read as deletion. A scheduled job can target a coding agent since the two provider tables merged, but the dialog only ever offered llm providers, so the capability was unreachable. It now offers both. Every provider read returned the api key and the aws secret. The store gained a projection that omits them and reports only whether each is set, and the one caller that needs them, building an outbound model request, asks by name. Since a client can no longer read a credential back, an upsert that omits one keeps what is stored, or a rename would wipe the key on save. Also filters the provider list on kind rather than leaning on the unknown-type guard, which dropped coding agents by accident and said something else in its comment. --- .../apps/app/lib/llm-providers/types.ts | 9 ++ .../chat/sidepanel-chat-targets.test.ts | 48 +++++++ .../modules/chat/sidepanel-chat-targets.ts | 35 +++-- .../modules/chat/use-chat-target-selection.ts | 9 +- .../llm-providers/llm-providers.api.ts | 12 +- .../llm-providers.helpers.test.ts | 31 +++-- .../llm-providers/llm-providers.helpers.ts | 36 +++-- .../llm-providers/llm-providers.hooks.ts | 27 ++++ .../llm-providers/llm-providers.revision.ts | 28 ++++ .../screens/ai-settings/NewProviderDialog.tsx | 27 +++- .../NewScheduledTaskDialog.tsx | 36 +++-- .../apps/server/src/api/routes/chat.ts | 14 +- .../src/lib/providers/provider-store.ts | 129 ++++++++++++++++-- .../lib/providers/provider-store.test.ts | 86 +++++++++++- 14 files changed, 455 insertions(+), 72 deletions(-) create mode 100644 packages/browseros-agent/apps/app/modules/llm-providers/llm-providers.revision.ts 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..aa939e4b4 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,55 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { and, eq, ne } from 'drizzle-orm' +import { and, eq, ne, sql } from 'drizzle-orm' 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. + */ +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, + hasApiKey: sql`${providers.apiKey} IS NOT NULL`.mapWith(Boolean), + hasAccessKeyId: sql`${providers.accessKeyId} IS NOT NULL`.mapWith( + Boolean, + ), + hasSecretAccessKey: + sql`${providers.secretAccessKey} IS NOT NULL`.mapWith(Boolean), + hasSessionToken: sql`${providers.sessionToken} IS NOT NULL`.mapWith( + Boolean, + ), +} + +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 +66,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 +89,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 +99,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(publicColumns) + .from(providers) + .where(eq(providers.kind, 'llm')) + .all() } -async function listLlm(): Promise { - return getDb().select().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 +129,31 @@ async function get(id: string): Promise { return row ?? null } +const CREDENTIAL_FIELDS = [ + 'apiKey', + 'accessKeyId', + 'secretAccessKey', + 'sessionToken', +] as const + +/** + * Drops credential fields the caller left out, 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 + * undefined over a working key on every rename. Clearing one is still possible + * by sending it explicitly as an empty string or null. + */ +function withoutAbsentCredentials>( + row: T, +): Partial { + const next: Record = { ...row } + for (const field of CREDENTIAL_FIELDS) { + if (next[field] === undefined) 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 +166,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 +203,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 +245,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..e3e268966 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,84 @@ 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') + }) + + test('an explicitly empty credential clears it', async () => { + useTempDb() + await dbProviderStore.upsert(baseProvider()) + + await dbProviderStore.upsert({ ...baseProvider(), apiKey: null }) + + expect( + (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, + ).toBeNull() + }) + }) }) From 9b68e0a0f03d0521108f3a8807c44e6e2f7cef42 Mon Sep 17 00:00:00 2001 From: Dani Akash Date: Thu, 3 Sep 2026 16:36:57 +0530 Subject: [PATCH 2/2] fix(server): treat a blank credential as not supplied, not as a clear The keep-existing guard only skipped undefined, but a form field the user never filled in submits as an empty string, and the schema accepts it. So the very edit the guard exists to protect, renaming a provider without retyping a key that is no longer readable, wrote an empty string over the key. The flags then reported it as stored, because an empty string is not null, so nothing looked wrong until the next request failed to authenticate. Blank now counts as not supplied on the way in, and as unset on the way out. Clearing stays deliberate: send null. The four flags share one definition rather than four copies, since a divergence would only ever show on whichever credential nobody tested. --- .../src/lib/providers/provider-store.ts | 35 ++++++---- .../lib/providers/provider-store.test.ts | 64 ++++++++++++++++++- 2 files changed, 85 insertions(+), 14 deletions(-) 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 aa939e4b4..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 @@ -5,6 +5,7 @@ */ 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' @@ -16,6 +17,12 @@ import { type NewProviderRow, type ProviderRow, providers } from '../db/schema' * 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, @@ -36,15 +43,12 @@ const publicColumns = { reasoningSummary: providers.reasoningSummary, workingDirectory: providers.workingDirectory, customConfig: providers.customConfig, - hasApiKey: sql`${providers.apiKey} IS NOT NULL`.mapWith(Boolean), - hasAccessKeyId: sql`${providers.accessKeyId} IS NOT NULL`.mapWith( - Boolean, - ), - hasSecretAccessKey: - sql`${providers.secretAccessKey} IS NOT NULL`.mapWith(Boolean), - hasSessionToken: sql`${providers.sessionToken} IS NOT NULL`.mapWith( - Boolean, - ), + // 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 = { @@ -137,19 +141,24 @@ const CREDENTIAL_FIELDS = [ ] as const /** - * Drops credential fields the caller left out, so they keep their stored value. + * 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 - * undefined over a working key on every rename. Clearing one is still possible - * by sending it explicitly as an empty string or null. + * 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) delete next[field] + if (next[field] === undefined || next[field] === '') delete next[field] } return next as Partial } 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 e3e268966..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 @@ -249,7 +249,68 @@ describe('dbProviderStore', () => { expect(saved?.apiKey).toBe('sk-test') }) - test('an explicitly empty credential clears it', async () => { + // 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()) @@ -258,6 +319,7 @@ describe('dbProviderStore', () => { expect( (await dbProviderStore.getWithCredentials(PROVIDER_ID))?.apiKey, ).toBeNull() + expect((await dbProviderStore.get(PROVIDER_ID))?.hasApiKey).toBe(false) }) }) })