From 5f14a1f5ee9a8e9fcce83e5d55d46a68b3469a3a Mon Sep 17 00:00:00 2001 From: AgentSeal Date: Fri, 10 Jul 2026 00:00:22 +0200 Subject: [PATCH] fix(providers): isolate a throwing provider so one bad file can't blank every scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both provider-discovery loops (discoverAllSessions, and the menubar provider enumeration in usage-aggregator) called discoverSessions with no try/catch, so any provider that threw — a crafted or corrupt file reaching a string op — took down usage discovery for all 32 providers at once, blanking the whole dashboard, CLI, and menubar. Add safeDiscoverSessions: wrap discovery in try/catch, warn once per provider per run to stderr (matching the per-file parse-failure isolation already in parser.ts and the whole-data-loss notices in fs-utils), and skip the provider instead of aborting. Both loop sites use it. discoverAllSessions takes an injectable provider list so the isolation loop itself is tested; mutation-verified that reverting either call site fails the new loop-level test. Follow-up (separate): surface a structured 'incomplete discovery' signal in the JSON/MCP payload so resident consumers can distinguish a real $0 from a dropped provider, and scope the warn-dedup per request rather than per process lifetime. --- src/providers/index.ts | 32 +++++++++++++++++-- src/usage-aggregator.ts | 4 +-- tests/provider-registry.test.ts | 55 +++++++++++++++++++++++++++++++-- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/providers/index.ts b/src/providers/index.ts index 501777d7..b61121a0 100644 --- a/src/providers/index.ts +++ b/src/providers/index.ts @@ -226,14 +226,40 @@ export async function getAllProviders(): Promise { export const providers = coreProviders -export async function discoverAllSessions(providerFilter?: string): Promise { - const allProviders = await getAllProviders() +// Isolate one provider's discovery. A provider that throws (a crafted/corrupt +// file reaching a string op, an unexpected on-disk shape) must never take down +// the whole scan and blank every other provider's usage. Warn once per +// provider per run, then skip it. Mirrors the parse-failure isolation already +// used per-file in parser.ts. +const warnedDiscoveryFailures = new Set() +export async function safeDiscoverSessions(provider: Provider): Promise { + try { + return await provider.discoverSessions() + } catch (err) { + if (!warnedDiscoveryFailures.has(provider.name)) { + warnedDiscoveryFailures.add(provider.name) + const msg = err instanceof Error ? err.message : String(err) + process.stderr.write( + `codeburn: skipped ${provider.name} discovery after an error: ${msg}\n` + ) + } + return [] + } +} + +export async function discoverAllSessions( + providerFilter?: string, + // Injectable for tests so the isolation loop itself is exercised, not just + // the helper. Defaults to the real registry. + providerList?: Provider[], +): Promise { + const allProviders = providerList ?? await getAllProviders() const filtered = providerFilter && providerFilter !== 'all' ? allProviders.filter(p => p.name === providerFilter) : allProviders const all: SessionSource[] = [] for (const provider of filtered) { - const sessions = await provider.discoverSessions() + const sessions = await safeDiscoverSessions(provider) all.push(...sessions) } return all diff --git a/src/usage-aggregator.ts b/src/usage-aggregator.ts index 6fceb7af..38ed0732 100644 --- a/src/usage-aggregator.ts +++ b/src/usage-aggregator.ts @@ -3,7 +3,7 @@ import { CATEGORY_LABELS, type ProjectSummary, type TaskCategory, type DateRange import { type PeriodData, type ProviderCost, type BreakdownArrays, type MenubarPayload, buildMenubarPayload } from './menubar-json.js' import { parseAllSessions, filterProjectsByName, filterProjectsByDays } from './parser.js' import { findUnpricedModels, getLocalModelSavingsConfigHash, getPriceOverridesConfigHash, getShortModelName } from './models.js' -import { getAllProviders } from './providers/index.js' +import { getAllProviders, safeDiscoverSessions } from './providers/index.js' import { aggregateProjectsIntoDays, buildPeriodDataFromDays } from './day-aggregator.js' import { aggregateModelEfficiency } from './model-efficiency.js' import { aggregateModels } from './models-report.js' @@ -200,7 +200,7 @@ export async function buildMenubarPayloadForRange(periodInfo: PeriodInfo, opts: } for (const p of allProviders) { if (providers.some(pc => pc.name === p.displayName)) continue - const sources = await p.discoverSessions() + const sources = await safeDiscoverSessions(p) if (sources.length > 0) providers.push({ name: p.displayName, cost: 0 }) } } else { diff --git a/tests/provider-registry.test.ts b/tests/provider-registry.test.ts index 004dc02c..1c346dd8 100644 --- a/tests/provider-registry.test.ts +++ b/tests/provider-registry.test.ts @@ -1,5 +1,16 @@ -import { describe, it, expect } from 'vitest' -import { providers, getAllProviders, getProvider } from '../src/providers/index.js' +import { describe, it, expect, vi } from 'vitest' +import { providers, getAllProviders, getProvider, safeDiscoverSessions, discoverAllSessions } from '../src/providers/index.js' +import type { Provider } from '../src/providers/types.js' + +function fakeProvider(name: string, discover: Provider['discoverSessions']): Provider { + return { + name, + displayName: name, + modelDisplayName: (m: string) => m, + toolDisplayName: (t: string) => t, + discoverSessions: discover, + } as unknown as Provider +} describe('provider registry', () => { it('has core providers registered synchronously', () => { @@ -117,4 +128,44 @@ describe('provider registry', () => { expect(cursor.modelDisplayName('grok-code-fast-1')).toBe('Grok Code Fast') expect(cursor.modelDisplayName('unknown-model')).toBe('unknown-model') }) + + describe('provider-discovery isolation', () => { + it('safeDiscoverSessions returns [] and warns once instead of propagating', async () => { + const warn = vi.spyOn(process.stderr, 'write').mockReturnValue(true) + const boom = fakeProvider('boom-helper', async () => { throw new Error('crafted file blew up') }) + try { + await expect(safeDiscoverSessions(boom)).resolves.toEqual([]) + expect(warn.mock.calls.length).toBeGreaterThanOrEqual(1) + expect(String(warn.mock.calls[0]![0])).toContain('boom-helper') + // Deduped on repeat within the same run: no additional warning. + const afterFirst = warn.mock.calls.length + await safeDiscoverSessions(boom) + expect(warn.mock.calls.length).toBe(afterFirst) + } finally { + warn.mockRestore() + } + }) + + it('discoverAllSessions drops a throwing provider but keeps the healthy ones', async () => { + const warn = vi.spyOn(process.stderr, 'write').mockReturnValue(true) + const boom = fakeProvider('boom-loop', async () => { throw new Error('kaboom') }) + const ok1 = fakeProvider('ok1', async () => [{ path: '/a.jsonl', project: 'p1', provider: 'ok1' }]) + const ok2 = fakeProvider('ok2', async () => [{ path: '/b.jsonl', project: 'p2', provider: 'ok2' }]) + try { + // A throwing provider in the middle must not abort the loop. + const sources = await discoverAllSessions('all', [ok1, boom, ok2]) + expect(sources.map(s => s.path)).toEqual(['/a.jsonl', '/b.jsonl']) + expect(warn.mock.calls.some(c => String(c[0]).includes('boom-loop'))).toBe(true) + } finally { + warn.mockRestore() + } + }) + + it('discoverAllSessions honors the provider filter', async () => { + const ok1 = fakeProvider('keep', async () => [{ path: '/keep.jsonl', project: 'k', provider: 'keep' }]) + const ok2 = fakeProvider('drop', async () => [{ path: '/drop.jsonl', project: 'd', provider: 'drop' }]) + const sources = await discoverAllSessions('keep', [ok1, ok2]) + expect(sources.map(s => s.path)).toEqual(['/keep.jsonl']) + }) + }) })