From cc7ac553e326d7db84ef6ed02211a5afb58e4875 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Sun, 7 Jun 2026 16:37:59 -0400 Subject: [PATCH] fix(usage): correct per-account cost attribution Drop the detail.source fallback and dead numeric-key lookup in the usage transformer so unmapped auth_index rows go to the 'unknown' bucket instead of mis-keying cost; null out today_cost when an email cost-key is shared by more than one account (duplicate-email providers) instead of double-displaying the combined spend. --- src/web-server/routes/bar-routes.ts | 28 ++++- .../usage/cliproxy-usage-transformer.ts | 8 +- tests/unit/web-server/bar-routes.test.ts | 112 ++++++++++++++++++ .../usage/account-attribution.test.ts | 38 ++++-- 4 files changed, 170 insertions(+), 16 deletions(-) diff --git a/src/web-server/routes/bar-routes.ts b/src/web-server/routes/bar-routes.ts index d23e32ae..e80b81ae 100644 --- a/src/web-server/routes/bar-routes.ts +++ b/src/web-server/routes/bar-routes.ts @@ -214,11 +214,18 @@ function buildRow( account: AccountInfo, fetchResult: AccountFetchResult, costByAccount: Record, - overallHealth: 'ok' | 'warning' | 'error' + overallHealth: 'ok' | 'warning' | 'error', + /** Set of cost-keys that are shared by more than one account. Cost is unknowable for these. */ + sharedCostKeys: ReadonlySet ): BarSummaryRow { const { quota, cached, fetchedAt } = fetchResult; const costKey = resolveCostKey(account); + // Fix #11: when multiple accounts share the same cost-key (e.g. two codex accounts with + // the same email), we cannot attribute the combined cost to either individual account. + // Report null (unknowable) rather than the inflated shared total. + const todayCost = sharedCostKeys.has(costKey) ? null : (costByAccount[costKey] ?? 0); + if (!quota || !quota.success) { // Degraded row: preserve identity fields, null out quota data return { @@ -229,7 +236,7 @@ function buildRow( paused: account.paused ?? false, quota_percentage: null, next_reset: null, - today_cost: costByAccount[costKey] ?? 0, + today_cost: todayCost, health: quota?.needsReauth ? 'error' : overallHealth, cached, fetchedAt, @@ -245,7 +252,7 @@ function buildRow( paused: account.paused ?? false, quota_percentage: extractQuotaPercentage(quota), next_reset: extractNextReset(quota), - today_cost: costByAccount[costKey] ?? 0, + today_cost: todayCost, health: overallHealth, cached, fetchedAt, @@ -315,6 +322,19 @@ export function createBarRouter(deps: BarRouterDeps): Router { const summary = deps.getAllAccountsSummary(); const allAccounts: AccountInfo[] = Object.values(summary).flat(); + // Fix #11: compute which cost-keys are shared by >1 account so buildRow can + // report null (unknowable) rather than the combined total for those rows. + const costKeyCount = new Map(); + for (const account of allAccounts) { + const key = resolveCostKey(account); + costKeyCount.set(key, (costKeyCount.get(key) ?? 0) + 1); + } + const sharedCostKeys = new Set( + Array.from(costKeyCount.entries()) + .filter(([, count]) => count > 1) + .map(([key]) => key) + ); + // Fetch quota in parallel with per-account error isolation. // Concurrency is capped to avoid fan-out across large account lists. // Paused accounts are handled inside fetchAccountData (cache/degrade, no live fetch). @@ -325,7 +345,7 @@ export function createBarRouter(deps: BarRouterDeps): Router { const batchRows = await Promise.all( batch.map(async (account): Promise => { const fetchResult = await fetchAccountData(account, doForceRefresh, deps); - return buildRow(account, fetchResult, costByAccount, overallHealth); + return buildRow(account, fetchResult, costByAccount, overallHealth, sharedCostKeys); }) ); rows.push(...batchRows); diff --git a/src/web-server/usage/cliproxy-usage-transformer.ts b/src/web-server/usage/cliproxy-usage-transformer.ts index d1982501..4e229a58 100644 --- a/src/web-server/usage/cliproxy-usage-transformer.ts +++ b/src/web-server/usage/cliproxy-usage-transformer.ts @@ -69,11 +69,15 @@ function createHistoryDetail( const outputTokens = detail.tokens?.output_tokens ?? 0; const cacheReadTokens = detail.tokens?.cached_tokens ?? 0; - // Resolve accountId from auth_index → account map, falling back to detail.source + // Resolve accountId from auth_index → account map. + // buildAuthIndexToAccountMap stores only String(auth_index) keys, so the numeric-key + // lookup is dead code and the detail.source fallback mis-attributes cost to a CLIProxy + // source label rather than an email. Leave accountId undefined when the index is absent + // so getTodayCostByAccount buckets it under 'unknown' and the bar excludes it. let accountId: string | undefined; if (accountMap !== undefined) { const key = String(detail.auth_index); - accountId = accountMap.get(key) ?? accountMap.get(detail.auth_index) ?? detail.source; + accountId = accountMap.get(key); } return { diff --git a/tests/unit/web-server/bar-routes.test.ts b/tests/unit/web-server/bar-routes.test.ts index 09556c88..f84fa0de 100644 --- a/tests/unit/web-server/bar-routes.test.ts +++ b/tests/unit/web-server/bar-routes.test.ts @@ -736,3 +736,115 @@ describe('force-fresh: paused accounts and concurrency cap (finding #7)', () => expect(pausedRow?.paused).toBe(true); }); }); + +// ============================================================================ +// Finding #11: codex duplicate-email cost double-count +// ============================================================================ + +describe('today_cost: duplicate-email accounts get null (finding #11)', () => { + // When two codex accounts share the same email, both rows must report today_cost: null + // instead of the combined total that would otherwise be double-counted. + + async function buildRouter(accounts: object[], costMap: Record) { + const { createBarRouter } = await import('../../../src/web-server/routes/bar-routes'); + const { resetForceFreshDebounce: resetDebounce } = await import('../../../src/web-server/routes/bar-routes'); + + const app = express(); + app.use(express.json()); + + const router = createBarRouter({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getAllAccountsSummary: () => ({ codex: accounts }) as any, + getCachedQuota: () => null, + setCachedQuota: () => {}, + invalidateQuotaCache: () => {}, + fetchAccountQuota: async () => makeQuotaResult(), + getTodayCostByAccount: () => costMap, + loadCliproxyDetails: async () => [], + runHealthChecks: async () => makeHealthReport(), + }); + + app.use('/api/bar', router); + + const srv = await new Promise((resolve, reject) => { + const instance = app.listen(0, '127.0.0.1'); + instance.once('error', reject); + instance.once('listening', () => resolve(instance)); + }); + + const addr = srv.address(); + if (!addr || typeof addr === 'string') throw new Error('No server address'); + resetDebounce(); + + return { srv, url: `http://127.0.0.1:${(addr as { port: number }).port}` }; + } + + it('two codex accounts sharing an email both get today_cost: null (not the doubled total)', async () => { + const sharedEmail = 'shared@example.com'; + const accounts = [ + { + id: `${sharedEmail}#free`, + email: sharedEmail, + provider: 'codex', + nickname: 'free-codex', + tier: 'free', + paused: false, + isDefault: false, + tokenFile: 'codex-shared-free.json', + createdAt: '2026-01-01T00:00:00.000Z', + }, + { + id: `${sharedEmail}#pro`, + email: sharedEmail, + provider: 'codex', + nickname: 'pro-codex', + tier: 'pro', + paused: false, + isDefault: true, + tokenFile: 'codex-shared-pro.json', + createdAt: '2026-01-01T00:00:00.000Z', + }, + ]; + // The cost map only has a combined total for the shared email + const costMap = { [sharedEmail]: 5.00 }; + + const { srv, url } = await buildRouter(accounts, costMap); + + const { body } = await getJson(url, '/api/bar/summary'); + await new Promise((resolve) => srv.close(() => resolve())); + + expect(body.length).toBe(2); + // Both rows must be null — the cost is unknowable per-account when email is shared + expect(body[0].today_cost).toBeNull(); + expect(body[1].today_cost).toBeNull(); + // Neither should show the combined total + expect(body[0].today_cost).not.toBe(5.00); + expect(body[1].today_cost).not.toBe(5.00); + }); + + it('unique-email account still shows its individual cost', async () => { + const accounts = [ + { + id: 'unique@example.com', + email: 'unique@example.com', + provider: 'codex', + nickname: 'unique-codex', + tier: 'pro', + paused: false, + isDefault: true, + tokenFile: 'codex-unique.json', + createdAt: '2026-01-01T00:00:00.000Z', + }, + ]; + const costMap = { 'unique@example.com': 3.75 }; + + const { srv, url } = await buildRouter(accounts, costMap); + + const { body } = await getJson(url, '/api/bar/summary'); + await new Promise((resolve) => srv.close(() => resolve())); + + expect(body.length).toBe(1); + // Unique email — cost is attributable + expect(body[0].today_cost).toBeCloseTo(3.75); + }); +}); diff --git a/tests/unit/web-server/usage/account-attribution.test.ts b/tests/unit/web-server/usage/account-attribution.test.ts index f82f27a0..d149d8e3 100644 --- a/tests/unit/web-server/usage/account-attribution.test.ts +++ b/tests/unit/web-server/usage/account-attribution.test.ts @@ -87,9 +87,11 @@ const twoAccountResponse = makeResponse([ }, ]); +// Fix #7/#13/#15: buildAuthIndexToAccountMap stores String(auth_index) keys only. +// The map must use string keys so accountMap.get(String(detail.auth_index)) resolves correctly. const authFileMap: Map = new Map([ - [0, 'alice@example.com'], - [1, 'bob@example.com'], + ['0', 'alice@example.com'], + ['1', 'bob@example.com'], ]); // ============================================================================ @@ -109,14 +111,25 @@ describe('extractCliproxyUsageHistoryDetails with accountMap', () => { expect(bobDetails).toHaveLength(1); // auth_index 1 appears once }); - it('falls back to detail.source when auth_index not in accountMap', async () => { + it('leaves accountId undefined when auth_index is not in accountMap (no source fallback)', async () => { + // Fix #7/#13/#15: detail.source is a CLIProxy source label, not an email. + // Using it as a cost key caused mis-attribution. When auth_index is absent from the + // map, accountId must be undefined so getTodayCostByAccount buckets under 'unknown'. const { extractCliproxyUsageHistoryDetails } = await import('../../../../src/web-server/usage/cliproxy-usage-transformer'); - const partialMap: Map = new Map([[0, 'alice@example.com']]); + // Use string key matching buildAuthIndexToAccountMap's String(auth_index) output + const partialMap: Map = new Map([['0', 'alice@example.com']]); const details = extractCliproxyUsageHistoryDetails(twoAccountResponse, partialMap); - const unknownAccount = details.find((d) => d.accountId === 'old-source-b'); - expect(unknownAccount).toBeDefined(); + // auth_index 1 (bob) is not in the partial map — must be undefined, not 'old-source-b' + const bobDetail = details.find((d) => d.accountId === undefined && !d.accountId); + // There should be exactly one detail with no accountId (bob's request) + const unmappedDetails = details.filter((d) => d.accountId === undefined); + expect(unmappedDetails).toHaveLength(1); + // Confirm it is NOT keyed under the source string + const sourceFallback = details.find((d) => d.accountId === 'old-source-b'); + expect(sourceFallback).toBeUndefined(); + void bobDetail; // suppress lint }); it('does not include accountId when no accountMap is provided (backward compat)', async () => { @@ -255,7 +268,9 @@ describe('getTodayCostByAccount', () => { expect(result['alice@example.com']).toBeCloseTo(aliceCostFromDetails, 10); }); - it('details without accountId are grouped under fallback source key', async () => { + it('details without accountId are grouped under the "unknown" key', async () => { + // Fix #7/#13/#15: when no accountMap is provided, accountId is undefined on all details. + // getTodayCostByAccount buckets these under 'unknown' — not under detail.source. const { getTodayCostByAccount } = await import('../../../../src/web-server/usage/data-aggregator'); const { extractCliproxyUsageHistoryDetails } = await import('../../../../src/web-server/usage/cliproxy-usage-transformer'); @@ -263,9 +278,12 @@ describe('getTodayCostByAccount', () => { const details = extractCliproxyUsageHistoryDetails(twoAccountResponse); const result = getTodayCostByAccount(details, TODAY); - // With no accountId, details with cost > 0 should still contribute - // grouped under some non-empty key derived from the detail - expect(Object.values(result).some((v) => v > 0)).toBe(true); + // All costs should be accumulated under the literal key 'unknown' + expect(typeof result['unknown']).toBe('number'); + expect(result['unknown']).toBeGreaterThan(0); + // No source-string keys should appear in the result + expect(result['old-source-a']).toBeUndefined(); + expect(result['old-source-b']).toBeUndefined(); }); });