From 23bd238f05a0bcbf0a98c97c8b6df37b375743b5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 23 Oct 2026 10:21:19 -0700 Subject: [PATCH] fix(costs): stop OAuth refreshes from duplicating unverified subscriptions (#16512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Costs page shows one card for each AI subscription account. Each card comes from an `ai_subscriptions` row. > - Paperclip cannot verify an xAI/Grok account with the provider. So each Grok row used a key made from a hash of the raw credential. > - The Grok CLI refreshes its OAuth tokens during runs. Paperclip writes the refreshed `auth.json` back to the vault and keeps the secret's `aiSessionEpoch`. > - The next run sees a new credential hash. `subscriptionService.register()` finds no match and adds a new subscription row. The old row stays on the Costs page. One Grok connection becomes many duplicate cards. > - Claude and OpenAI rows usually avoid this problem, because a provider profile check merges them. Grok rows never get verified, so nothing merges them. > - This pull request keys unverified rows on the credential session (secret id + `aiSessionEpoch`), not on the token bytes. > - The benefit is one subscription card per Grok connection through token refreshes. A real reconnect still gets its own card. ## Linked Issues or Issue Description No public issue exists. Bug description: **What happened?** On a hosted staging instance, the Costs → Subscriptions list showed many duplicate Grok subscriptions for one Grok connection. Each Grok OAuth token refresh added one more card. **Expected behavior** One Grok connection shows one subscription card. A token refresh does not change it. **Steps to reproduce** 1. Connect an xAI/Grok subscription account. 2. Run an agent until the Grok CLI refreshes its OAuth token. Paperclip writes the refreshed credential back to the vault. 3. Run the agent again. 4. Open Costs → Subscriptions. A second Grok card is there. Each refresh adds another card. **Agent adapter(s) involved** Not adapter-specific (core bug). The trigger is the Grok OAuth refresh write-back in `server/src/services/ai-connection-runtime.ts`. **Paperclip version or commit** `master` at 8aaef59. ## What Changed - `server/src/services/subscriptions.ts`: For an unverified identity, `register()` now reads the grant's `ai.credential` secret ref and that secret's `aiSessionEpoch`. The account key becomes `grant + session + secretId + epoch`. Before, it was `grant + credential hash`. - Refresh write-backs use `preserveAiSessionEpoch: true`, so a refreshed token keeps the same row. - A reconnect advances the epoch, so it still gets a separate row. A reconnect can sign in to a different account. - A grant with no vaulted secret ref falls back to the old credential-scoped key. - Unverified rows stay grant-scoped. Only a verified provider observation can match the billing identity of a different grant. - Stale sessions are rejected. `SubscriptionConnection` now carries the `session` (secret id + epoch) captured together with the credential. If that session does not match the grant's current secret and epoch, `register()` returns `409` and does not use the live epoch. Before this, a run could read account A's credential, pause, and register after a reconnect to account B. It then got B's subscription id, so A's usage was saved against B. - `server/src/services/ai-connection-runtime.ts`: The run passes the secret id and epoch from its existing freshness check (read together with the credential). - `server/src/services/subscription-refresh.ts`: Discovery reads the session before it reads the credential. Then a reconnect at any time after that read makes `register()` reject the stale session. - `server/src/__tests__/subscriptions.test.ts`: Added regression tests. - Two refreshes keep one row and its saved price. A reconnect (epoch bump) makes a second row, and a refresh after that stays on the second row. - A registration with an older epoch, or with no session, is rejected with `409`. The grant binding stays on the current account. - `server/src/__tests__/subscription-refresh.test.ts`: The module mock now exports `subscriptionSession`. - This pull request adds no code comments. The explanation is in this description. ## Verification Run on `master` + this branch (head `11900e4`): - `cd server && npx vitest run src/__tests__/subscriptions.test.ts src/__tests__/subscription-refresh.test.ts src/__tests__/subscription-routes.test.ts src/__tests__/ai-connections.test.ts src/__tests__/ai-connection-session.test.ts src/__tests__/costs-service.test.ts` → 6 files, 191 tests pass. - The refresh test fails on `master` without the service change. The refreshed token gets a new subscription id (`expected '6683…' to be '942b…'`). - The stale-session test fails when the guard is removed. The stale registration resolves to the reconnected account's id instead of rejecting. - `pnpm -r typecheck` passes for every package except `server` and `cli`. The runner passes `typecheck:typescript`. - I could not complete some checks in my sandbox. CI must confirm them: - The `server` and `cli` typechecks hit the 4 GB sandbox memory limit (OOM kill). A narrow `tsc --noEmit` of `subscriptions.ts` and its imports passes with 0 errors. A narrow check of `ai-connection-runtime.ts` and `subscription-refresh.ts` also hits the memory limit, because they import most of the server. - The full `pnpm test:run` hit the same memory limit. - `pnpm build` and the runner `typecheck:rust` need `cargo`, and the sandbox does not have it. This change does not touch Rust or the runner. Manual check after deploy: Connect Grok and run an agent through a token refresh. Then make sure that Costs → Subscriptions shows one Grok card. ## Risks - Low risk. There is no schema change and no migration. - Existing unverified rows keep their old credential-scoped keys. On the next refresh after deploy, each existing Grok connection can get one more row in the new key format. After that, it stays stable. - This change does not remove duplicate rows that exist now. You can merge them with the **Link** control on each Costs card. A separate data repair is planned. - `register()` does one more indexed read of `company_secrets` (by id) when a run registers an unverified identity. Discovery does one more read of that row for each connection. The cached-binding fast path does not change. - A run that loses the race with a reconnect now fails its preparation with `409` ("The AI credential changed. Retry this execution."). This is the same retry behavior as the existing credential-freshness guard. Discovery logs the conflict and checks again on the next pass. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (Anthropic), model ID `claude-opus-5-5`, run through Claude Code as a Paperclip agent with tool use (shell, file edit, test execution). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass (targeted suites; the full suite is not complete, see Verification) - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (no documentation change is necessary) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending CI) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Amy (Opus) Co-authored-by: Claude Opus 5.5 Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> --- .../__tests__/subscription-refresh.test.ts | 2 +- server/src/__tests__/subscriptions.test.ts | 35 ++++++++++++++++++- server/src/services/ai-connection-runtime.ts | 1 + server/src/services/subscription-refresh.ts | 5 +-- server/src/services/subscriptions.ts | 24 ++++++++++--- 5 files changed, 58 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/subscription-refresh.test.ts b/server/src/__tests__/subscription-refresh.test.ts index cb2f4bddc49..dc59f6a2fce 100644 --- a/server/src/__tests__/subscription-refresh.test.ts +++ b/server/src/__tests__/subscription-refresh.test.ts @@ -6,7 +6,7 @@ import { idleWorkSnapshot } from "../services/task-admission.js"; const mocks = vi.hoisted(() => ({ accounts: vi.fn(), credential: vi.fn(), register: vi.fn(), probe: vi.fn() })); vi.mock("../services/ai-connections.js", () => ({ aiConnectionService: () => ({ subscriptionAccounts: mocks.accounts, credential: mocks.credential }) })); -vi.mock("../services/subscriptions.js", () => ({ subscriptionService: () => ({ register: mocks.register }) })); +vi.mock("../services/subscriptions.js", () => ({ subscriptionService: () => ({ register: mocks.register }), subscriptionSession: async () => null })); vi.mock("../services/subscription-identity.js", () => ({ probeSubscriptionIdentity: mocks.probe })); function deferred() { diff --git a/server/src/__tests__/subscriptions.test.ts b/server/src/__tests__/subscriptions.test.ts index 1b8f391dbc9..99d1ba61517 100644 --- a/server/src/__tests__/subscriptions.test.ts +++ b/server/src/__tests__/subscriptions.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { randomUUID } from "node:crypto"; import { readFile } from "node:fs/promises"; import { and, eq, sql } from "drizzle-orm"; -import { createDb, companies, agents, heartbeatRuns, costEvents, activityLog, aiSubscriptions, aiSubscriptionPrices, aiSubscriptionConnections, toolApplications, toolConnections, connectionGrants, connectionGrantMembers } from "@paperclipai/db"; +import { createDb, companies, agents, heartbeatRuns, costEvents, activityLog, aiSubscriptions, aiSubscriptionPrices, aiSubscriptionConnections, companySecrets, toolApplications, toolConnections, connectionGrants, connectionGrantMembers } from "@paperclipai/db"; import { startEmbeddedPostgresTestDatabase } from "@paperclipai/db/test-embedded-postgres"; import { monthlySubscriptionCents, subscriptionPlan, subscriptionPriceSchema } from "@paperclipai/shared"; import { subscriptionService, type SubscriptionConnection } from "../services/subscriptions.js"; @@ -475,6 +475,39 @@ describe("durable subscription reporting", () => { await refreshSubscriptionConnection(db, input, id, fixtureRequest(usage("pro"))); expect((await subscriptionCostReport(db, c, owner)).accounts[0].price.amountCents).toBe("1750.0000000"); }); + it("keeps one unverified account through OAuth refreshes and separates a reconnect", async () => { + const c = await company(), connected = await connection(c, "grok-first", "xai"), service = subscriptionService(db); + const secretId = randomUUID(); + await db.insert(companySecrets).values({ id: secretId, companyId: c, key: `ai-${connected.grantId}`, name: `ai-${connected.grantId}` }); + await db.update(connectionGrants).set({ credentialSecretRefs: [{ secretId, configPath: "ai.credential" }] }).where(eq(connectionGrants.id, connected.grantId)); + const input = { ...connected, session: { secretId, epoch: 0 } }; + const id = await service.register(input); + await price(c, id, "3000"); + for (const credential of ["grok-refreshed", "grok-refreshed-again"]) expect(await service.register({ ...input, credential })).toBe(id); + await refreshSubscriptionConnection(db, { ...input, credential: "grok-refreshed-again" }, id, fixtureRequest(usage())); + let report = await subscriptionCostReport(db, c, owner); + expect(report.accounts.map(account => account.id)).toEqual([id]); + expect(report.accounts[0]).toMatchObject({ identityVerified: false, price: { amountCents: "3000.0000000" } }); + await db.update(companySecrets).set({ aiSessionEpoch: 1 }).where(eq(companySecrets.id, secretId)); + const reconnected = await service.register({ ...input, credential: "grok-reconnected", session: { secretId, epoch: 1 } }); + expect(reconnected).not.toBe(id); + expect(await service.register({ ...input, credential: "grok-reconnected-refreshed", session: { secretId, epoch: 1 } })).toBe(reconnected); + report = await subscriptionCostReport(db, c, owner); + expect(report.accounts.map(account => account.id).sort()).toEqual([id, reconnected].sort()); + }); + it("rejects an unverified registration from a session older than the latest reconnect", async () => { + const c = await company(), connected = await connection(c, "grok-old", "xai"), service = subscriptionService(db); + const secretId = randomUUID(); + await db.insert(companySecrets).values({ id: secretId, companyId: c, key: `ai-${connected.grantId}`, name: `ai-${connected.grantId}` }); + await db.update(connectionGrants).set({ credentialSecretRefs: [{ secretId, configPath: "ai.credential" }] }).where(eq(connectionGrants.id, connected.grantId)); + await db.update(companySecrets).set({ aiSessionEpoch: 1 }).where(eq(companySecrets.id, secretId)); + const current = await service.register({ ...connected, credential: "grok-new", session: { secretId, epoch: 1 } }); + await expect(service.register({ ...connected, credential: "grok-old-refreshed", session: { secretId, epoch: 0 } })).rejects.toMatchObject({ status: 409 }); + await expect(service.register({ ...connected, credential: "grok-old-refreshed" })).rejects.toMatchObject({ status: 409 }); + const [binding] = await db.select().from(aiSubscriptionConnections).where(eq(aiSubscriptionConnections.grantId, connected.grantId)); + expect(binding.subscriptionId).toBe(current); + expect((await subscriptionCostReport(db, c, owner)).accounts.map(account => account.id)).toEqual([current]); + }); it("ignores a delayed provider observation after reconnecting another account", async () => { const c = await company(), input = await connection(c), service = subscriptionService(db), id = await service.register(input); let finish!: (response: Response) => void; diff --git a/server/src/services/ai-connection-runtime.ts b/server/src/services/ai-connection-runtime.ts index 2620e8cbb53..54fe1ded35b 100644 --- a/server/src/services/ai-connection-runtime.ts +++ b/server/src/services/ai-connection-runtime.ts @@ -384,6 +384,7 @@ export async function prepareManagedAiRuntime( companyId: input.companyId, connectionId: selection.connection.id, grantId: selection.grant.id, provider: selection.attribution.provider, name: selection.connection.name, ownerUserId: selection.grant.subjectUserId, credential: value, + session: credentialRef && freshness ? { secretId: credentialRef.secretId, epoch: freshness.epoch } : null, }; attribution.subscriptionId = await subscriptionService(db).register(subscription); void refreshSubscriptionConnection(db, subscription, attribution.subscriptionId); diff --git a/server/src/services/subscription-refresh.ts b/server/src/services/subscription-refresh.ts index 24322d099ee..6cec8ee524f 100644 --- a/server/src/services/subscription-refresh.ts +++ b/server/src/services/subscription-refresh.ts @@ -1,7 +1,7 @@ import { and, eq, lt, or, isNull } from "drizzle-orm"; import { aiSubscriptions, aiSubscriptionConnections, type Db } from "@paperclipai/db"; import { aiConnectionService } from "./ai-connections.js"; -import { subscriptionService, type SubscriptionConnection } from "./subscriptions.js"; +import { subscriptionService, subscriptionSession, type SubscriptionConnection } from "./subscriptions.js"; import { probeSubscriptionIdentity } from "./subscription-identity.js"; import { logger } from "../middleware/logger.js"; import { tooManyRequests } from "../errors.js"; @@ -58,9 +58,10 @@ export function refreshCompanySubscriptions(db: Db, companyId: string, userId: s for (const row of await service.subscriptionAccounts(companyId, userId)) { if (row.summary.status !== "connected") continue; try { + const session = await subscriptionSession(db, companyId, row.grant.credentialSecretRefs); const credential = await service.credential(row); const input: SubscriptionConnection = { companyId, connectionId: row.connection.id, grantId: row.grant.id, - provider: row.summary.provider, name: row.connection.name, ownerUserId: row.grant.subjectUserId, credential }; + provider: row.summary.provider, name: row.connection.name, ownerUserId: row.grant.subjectUserId, credential, session }; const id = await subscriptionService(db).register(input); // Accepted discovery must not silently skip an account. At most twenty // discovery tasks can wait here; never create an unbounded provider queue. diff --git a/server/src/services/subscriptions.ts b/server/src/services/subscriptions.ts index e1d80430e02..45a3fc3b71a 100644 --- a/server/src/services/subscriptions.ts +++ b/server/src/services/subscriptions.ts @@ -1,5 +1,5 @@ import { and, eq, inArray, sql } from "drizzle-orm"; -import { aiSubscriptions, aiSubscriptionPrices, aiSubscriptionConnections, connectionGrants, toolConnections, type Db } from "@paperclipai/db"; +import { aiSubscriptions, aiSubscriptionPrices, aiSubscriptionConnections, companySecrets, connectionGrants, toolConnections, type Db } from "@paperclipai/db"; import { aiConnectionMetadataSchema, normalizeCents, subscriptionPlan, type AiProvider, type UpdateSubscriptionPrice } from "@paperclipai/shared"; import { conflict, forbidden, notFound } from "../errors.js"; import { logActivity, type ActivityPublication } from "./activity-log.js"; @@ -12,6 +12,16 @@ export interface SubscriptionActor { userId: string; canManage: boolean; readOnl export interface SubscriptionConnection { companyId: string; connectionId: string; grantId: string; provider: AiProvider; name: string; ownerUserId: string | null; credential: string; + session?: SubscriptionSession | null; +} +export interface SubscriptionSession { secretId: string; epoch: number } + +export async function subscriptionSession(db: Db, companyId: string, refs: { secretId: string; configPath: string }[]): Promise { + const ref = refs.find(r => r.configPath === "ai.credential"); + if (!ref) return null; + const [secret] = await db.select({ epoch: companySecrets.aiSessionEpoch }).from(companySecrets) + .where(and(eq(companySecrets.companyId, companyId), eq(companySecrets.id, ref.secretId))); + return secret ? { secretId: ref.secretId, epoch: secret.epoch } : null; } export function canonicalSubscriptionId(id: string, accounts: Pick[] | Map): string { @@ -88,10 +98,6 @@ async function mergeInTransaction(db: Db, companyId: string, source: Account, ta export function subscriptionService(db: Db) { async function register(input: SubscriptionConnection, observed?: SubscriptionIdentity) { const identity = { ...(observed ?? subscriptionCredentialIdentity(input.companyId, input.provider, input.credential)) }; - // Only a successful provider observation may match another grant's billing - // identity or add its editors. Provisional rows are grant-scoped, even when - // two callers supply the same unverified credential or forged ID claims. - if (!observed?.verified) identity.accountKey = subscriptionIdentityKey(input.companyId, input.provider, ["grant", input.grantId, identity.credentialKey]); if (!observed) { const [cached] = await db.select().from(aiSubscriptionConnections).where(and(eq(aiSubscriptionConnections.companyId, input.companyId), eq(aiSubscriptionConnections.grantId, input.grantId))); if (cached?.credentialKey === identity.credentialKey && cached.connectionId === input.connectionId) return cached.subscriptionId; @@ -107,6 +113,14 @@ export function subscriptionService(db: Db) { || !metadata.success || metadata.data.method !== "subscription" || metadata.data.provider !== input.provider) { throw notFound("Subscription connection not found"); } + if (!observed?.verified) { + const session = await subscriptionSession(tx, input.companyId, grant.grant.credentialSecretRefs); + if (session && (input.session?.secretId !== session.secretId || input.session.epoch !== session.epoch)) { + throw conflict("The AI credential changed. Retry this execution."); + } + identity.accountKey = subscriptionIdentityKey(input.companyId, input.provider, ["grant", input.grantId, + ...(session ? ["session", session.secretId, String(session.epoch)] : [identity.credentialKey])]); + } const [binding] = await tx.select().from(aiSubscriptionConnections).where(and(eq(aiSubscriptionConnections.companyId, input.companyId), eq(aiSubscriptionConnections.grantId, input.grantId))); const accounts = await tx.select().from(aiSubscriptions).where(eq(aiSubscriptions.companyId, input.companyId)); if (observed && binding && binding.credentialKey !== identity.credentialKey) return canonicalSubscriptionId(binding.subscriptionId, accounts);