-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add resilience to provider API response handling #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
95d4499
4181c35
537a64a
ef5930c
9337314
b8c2f35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,11 @@ export function ConfigProvider(props: ParentProps) { | |
| let refreshSeq = 0 | ||
| let lastUpdateAt = 0 | ||
|
|
||
| // Validate that a response has the expected Config shape (must be a non-array object) | ||
| function isValidConfig(data: unknown): data is Config { | ||
| return !!data && typeof data === "object" && !Array.isArray(data) | ||
| } | ||
|
|
||
| async function refresh() { | ||
| const seq = ++refreshSeq | ||
| setLoading(true) | ||
|
|
@@ -45,7 +50,11 @@ export function ConfigProvider(props: ParentProps) { | |
| try { | ||
| const projRes = await sdk.client.config.get() | ||
| if (seq !== refreshSeq) return // superseded by newer refresh | ||
| setProject(reconcile((projRes?.data as Config) ?? {})) | ||
| const projData = projRes?.data | ||
| if (projData && !isValidConfig(projData)) { | ||
| console.error("[Config] Unexpected project config response shape:", projData) | ||
| } | ||
| setProject(reconcile(isValidConfig(projData) ? projData : {})) | ||
| } catch (e) { | ||
| console.error("[Config] Failed to fetch project config:", e) | ||
| if (seq !== refreshSeq) return | ||
|
|
@@ -58,7 +67,11 @@ export function ConfigProvider(props: ParentProps) { | |
| try { | ||
| const globalRes = await sdk.client.global.config.get() | ||
| if (seq !== refreshSeq) return | ||
| setGlobal(reconcile((globalRes?.data as Config) ?? {})) | ||
| const globalData = globalRes?.data | ||
| if (globalData && !isValidConfig(globalData)) { | ||
| console.error("[Config] Unexpected global config response shape:", globalData) | ||
| } | ||
| setGlobal(reconcile(isValidConfig(globalData) ? globalData : {})) | ||
| } catch (e) { | ||
| console.error("[Config] Failed to fetch global config:", e) | ||
| if (seq !== refreshSeq) return | ||
|
|
@@ -76,8 +89,12 @@ export function ConfigProvider(props: ParentProps) { | |
| setError(null) | ||
| try { | ||
| const res = await sdk.client.config.update({ config: patch }) | ||
| const data = res.data as Config | undefined | ||
| if (data) { | ||
| const data = res.data | ||
| if (data && !isValidConfig(data)) { | ||
| console.error("[Config] Unexpected project update response shape:", data) | ||
| return null | ||
| } | ||
| if (isValidConfig(data)) { | ||
| lastUpdateAt = Date.now() | ||
| setProject(reconcile(data)) | ||
| return data | ||
|
|
@@ -101,8 +118,12 @@ export function ConfigProvider(props: ParentProps) { | |
| ? { ...patch, disabled_providers: global.disabled_providers } | ||
| : patch | ||
| const res = await sdk.client.global.config.update({ config: safePatch }) | ||
| const data = res.data as Config | undefined | ||
| if (data) { | ||
| const data = res.data | ||
| if (data && !isValidConfig(data)) { | ||
| console.error("[Config] Unexpected global update response shape:", data) | ||
| return null | ||
| } | ||
|
Comment on lines
121
to
+127
|
||
| if (isValidConfig(data)) { | ||
| lastUpdateAt = Date.now() | ||
| setGlobal(reconcile(data)) | ||
| return data | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,7 @@ interface ProviderContextValue { | |||||||||||||||||||||
| providers: Provider[] | ||||||||||||||||||||||
| connected: string[] | ||||||||||||||||||||||
| defaults: Record<string, string> | ||||||||||||||||||||||
| providerError: string | null | ||||||||||||||||||||||
| authMethods: Record<string, ProviderAuthMethod[]> | ||||||||||||||||||||||
| agents: Agent[] | ||||||||||||||||||||||
| loading: boolean | ||||||||||||||||||||||
|
|
@@ -89,7 +90,7 @@ interface ProviderContextValue { | |||||||||||||||||||||
| setSelectedAgent: (agent: string) => void | ||||||||||||||||||||||
| getSessionModel: (sessionID: string) => ModelKey | null | ||||||||||||||||||||||
| setSessionModel: (sessionID: string, model: ModelKey | null) => void | ||||||||||||||||||||||
| refetch: () => void | ||||||||||||||||||||||
| refetch: () => Promise<void> | ||||||||||||||||||||||
| connectProvider: (providerID: string, apiKey: string) => Promise<boolean> | ||||||||||||||||||||||
| startOAuth: (providerID: string, methodIndex: number) => Promise<OAuthAuthorization | undefined> | ||||||||||||||||||||||
| completeOAuth: (providerID: string, methodIndex: number, code?: string) => Promise<boolean> | ||||||||||||||||||||||
|
|
@@ -159,23 +160,106 @@ export function ProviderProvider(props: ParentProps) { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function isRecord(value: unknown): value is Record<string, unknown> { | ||||||||||||||||||||||
| return !!value && typeof value === "object" && !Array.isArray(value) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function normalizeModel(providerID: string, modelID: string, model: unknown): Model { | ||||||||||||||||||||||
| if (!isRecord(model)) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Provider "${providerID}" model "${modelID}" must be an object`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| const name = typeof model.name === "string" && model.name ? model.name : modelID | ||||||||||||||||||||||
| const rawLimit = model.limit | ||||||||||||||||||||||
| const limit = isRecord(rawLimit) ? rawLimit : {} | ||||||||||||||||||||||
| return { | ||||||||||||||||||||||
| ...(model as Omit<Model, "id" | "name" | "limit" | "providerID">), | ||||||||||||||||||||||
| id: typeof model.id === "string" && model.id ? model.id : modelID, | ||||||||||||||||||||||
| name, | ||||||||||||||||||||||
| providerID, | ||||||||||||||||||||||
| limit: { | ||||||||||||||||||||||
| context: typeof limit.context === "number" ? limit.context : 0, | ||||||||||||||||||||||
| input: typeof limit.input === "number" ? limit.input : undefined, | ||||||||||||||||||||||
| output: typeof limit.output === "number" ? limit.output : 0, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function normalizeProviderListData(data: unknown): ProviderListData { | ||||||||||||||||||||||
| if (!isRecord(data)) { | ||||||||||||||||||||||
| throw new Error("[Providers] Provider list response must be an object") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (!Array.isArray(data.all)) { | ||||||||||||||||||||||
| throw new Error("[Providers] Provider list field \"all\" must be an array") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (!Array.isArray(data.connected)) { | ||||||||||||||||||||||
| throw new Error("[Providers] Provider list field \"connected\" must be an array") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (data.default !== undefined && !isRecord(data.default)) { | ||||||||||||||||||||||
| throw new Error("[Providers] Provider list field \"default\" must be an object") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const all = data.all.map((entry, i) => { | ||||||||||||||||||||||
| if (!isRecord(entry)) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Provider entry at index ${i} must be an object`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (typeof entry.id !== "string" || !entry.id) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Provider entry at index ${i} is missing a string \"id\"`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (typeof entry.name !== "string" || !entry.name) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Provider \"${entry.id}\" is missing a string \"name\"`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const rawModels = entry.models | ||||||||||||||||||||||
| if (rawModels === undefined || rawModels === null) { | ||||||||||||||||||||||
| return { ...(entry as Provider), id: entry.id, name: entry.name, models: {} } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (!isRecord(rawModels)) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Provider \"${entry.id}\" field \"models\" must be an object`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| const models = Object.fromEntries( | ||||||||||||||||||||||
| Object.entries(rawModels).map(([modelID, model]) => [modelID, normalizeModel(entry.id, modelID, model)]) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| return { ...(entry as Provider), id: entry.id, name: entry.name, models } | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const connected = data.connected.map((id, i) => { | ||||||||||||||||||||||
| if (typeof id !== "string" || !id) { | ||||||||||||||||||||||
| throw new Error(`[Providers] Connected provider at index ${i} must be a string`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return id | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const defaults = Object.fromEntries( | ||||||||||||||||||||||
| Object.entries(data.default ?? {}).map(([k, v]) => { | ||||||||||||||||||||||
| if (typeof v !== "string") { | ||||||||||||||||||||||
| throw new Error(`[Providers] Default model for provider \"${k}\" must be a string`) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return [k, v] | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return { all, connected, default: defaults } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async function refetchProvidersWithRetry(providerID: string): Promise<boolean> { | ||||||||||||||||||||||
| const delays = [0, 150, 300, 600, 1200] | ||||||||||||||||||||||
| for (const delay of delays) { | ||||||||||||||||||||||
| if (delay > 0) await new Promise((resolve) => setTimeout(resolve, delay)) | ||||||||||||||||||||||
| const data = await refetchProviders() | ||||||||||||||||||||||
| if (data?.connected.includes(providerID)) return true | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return false | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Fetch providers | ||||||||||||||||||||||
| const [providerData, { refetch: refetchProviders }] = createResource(async () => { | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| const res = await client.provider.list() | ||||||||||||||||||||||
| const data = res.data as ProviderListData | undefined | ||||||||||||||||||||||
| if (!data) return undefined | ||||||||||||||||||||||
| // Inject providerID into each model since the SDK response doesn't include it | ||||||||||||||||||||||
| const all = data.all.map((provider) => ({ | ||||||||||||||||||||||
| ...provider, | ||||||||||||||||||||||
| models: Object.fromEntries( | ||||||||||||||||||||||
| Object.entries(provider.models).map(([k, m]) => [k, { ...m, providerID: provider.id }]) | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| })) | ||||||||||||||||||||||
| return { ...data, all } | ||||||||||||||||||||||
| return normalizeProviderListData(res.data) | ||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||
| console.error("Failed to fetch providers:", e) | ||||||||||||||||||||||
| return undefined | ||||||||||||||||||||||
| const msg = e instanceof Error ? e.message : "Failed to fetch providers" | ||||||||||||||||||||||
| throw new Error(msg) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -236,7 +320,12 @@ export function ProviderProvider(props: ParentProps) { | |||||||||||||||||||||
| const [authData] = createResource(async () => { | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| const res = await client.provider.auth() | ||||||||||||||||||||||
| return (res.data as Record<string, ProviderAuthMethod[]>) ?? {} | ||||||||||||||||||||||
| const data = res.data | ||||||||||||||||||||||
| if (!data || typeof data !== "object" || Array.isArray(data)) { | ||||||||||||||||||||||
| console.error("[Providers] Invalid auth methods response shape:", data) | ||||||||||||||||||||||
| return {} | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return data as Record<string, ProviderAuthMethod[]> | ||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||
| console.error("Failed to fetch auth methods:", e) | ||||||||||||||||||||||
| return {} | ||||||||||||||||||||||
|
|
@@ -304,10 +393,10 @@ export function ProviderProvider(props: ParentProps) { | |||||||||||||||||||||
| providerID, | ||||||||||||||||||||||
| auth: { type: "api", key: apiKey }, | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| // Dispose instance to reload provider state, then refresh | ||||||||||||||||||||||
| // Dispose instance to reload provider state, then wait for the | ||||||||||||||||||||||
| // server to reinitialize and provider state to be observable. | ||||||||||||||||||||||
| await client.instance.dispose() | ||||||||||||||||||||||
| await refetchProviders() | ||||||||||||||||||||||
| return true | ||||||||||||||||||||||
| return refetchProvidersWithRetry(providerID) | ||||||||||||||||||||||
|
||||||||||||||||||||||
| return refetchProvidersWithRetry(providerID) | |
| return await refetchProvidersWithRetry(providerID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9337314: connectProvider now uses return await refetchProvidersWithRetry(providerID) so rejections are handled by the surrounding try/catch and the function reliably returns a boolean.
Copilot
AI
Apr 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as connectProvider: completeOAuth returns the refetchProvidersWithRetry() promise without awaiting it, so any rejection from the retry/refetch path will not be caught by this try/catch and can surface as an unhandled rejection. Await the retry call (or handle errors inside the retry loop) to ensure completeOAuth returns false on failure as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9337314 as well: completeOAuth now awaits refetchProvidersWithRetry(providerID) so failures are caught and the function returns false on error.
Copilot
AI
Apr 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The providers/connected getters throw when providerData.error is set. Several pages (e.g. Session/Layout) access providers.providers and providers.connected without any surrounding ErrorBoundary, so a transient or shape-mismatch error from client.provider.list() will crash the whole route (returning to a blank page) instead of degrading gracefully. Prefer returning safe defaults (e.g. []) and exposing the error via providerError, or add an app-wide ErrorBoundary that covers all consumers of this context (not just Settings).
| if (providerData.error) throw providerData.error | |
| const disabled = cfg.global.disabled_providers ?? [] | |
| return (providerData()?.all ?? []).filter((p) => !disabled.includes(p.id)) | |
| }, | |
| get connected() { | |
| if (providerData.error) throw providerData.error | |
| const disabled = cfg.global.disabled_providers ?? [] | |
| return (providerData()?.all ?? []).filter((p) => !disabled.includes(p.id)) | |
| }, | |
| get connected() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ef5930c: removed throws from providers/connected getters to avoid route crashes in consumers without ErrorBoundary; safe defaults are preserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
config.updateProject/config.updateGlobalreceive a non-null response that failsisValidConfig, they currently justconsole.error(...)and returnnullwhile leavingerror()cleared. Call sites (e.g. Settings) only show UI feedback viaconfig.error()/saveError(), so this becomes a silent save failure. Consider settingsetError(...)(or throwing) on invalid response shapes so the user gets a visible error state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ef5930c: invalid non-object project update responses now set a user-visible config error before returning
null.