From f99277e0bcd6dff79f1f67e0f933b8d2dfbd8597 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 11:40:47 -0500 Subject: [PATCH 01/12] feat(measurement-jobs): free-tier card gate (setup mode) + instant backfill drain (#671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two chat#1796 refinements on the historical (Songstats) path: 1. Free-tier card-on-file link. The gate was issuing the paid subscription checkout ($99/mo after a 30-day trial). New createCardOnFileSession uses Stripe Checkout `mode: "setup"` — collects a card for $0, no subscription, no Stripe product. The account then pays only for metered usage via credits. 2. Instant drain. After enqueuing a historical job, fire-and-forget start(songstatsBackfillWorkflow) so the backfill begins immediately instead of waiting up to 24h for the cron. Safe by reuse: the workflow's budget gate (limit − reserve − rolling-30d ledger) caps it to the Songstats quota and SKIP LOCKED prevents double-claiming with the daily cron, which stays as the backstop. Only kicks when something was actually enqueued. 26 new/updated unit tests; research+stripe+workflows suite 453 green; tsc/lint/format clean. --- .../enqueueHistoricalBackfill.test.ts | 19 +++++++++ .../ensureSongstatsPaymentMethod.test.ts | 12 +++--- .../enqueueHistoricalBackfill.ts | 15 +++++++ .../ensureSongstatsPaymentMethod.ts | 7 +++- .../__tests__/createCardOnFileSession.test.ts | 39 +++++++++++++++++++ lib/stripe/createCardOnFileSession.ts | 31 +++++++++++++++ 6 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 lib/stripe/__tests__/createCardOnFileSession.test.ts create mode 100644 lib/stripe/createCardOnFileSession.ts diff --git a/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts b/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts index 20cde74a7..03010fa41 100644 --- a/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts +++ b/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts @@ -3,6 +3,7 @@ import { enqueueHistoricalBackfill } from "../enqueueHistoricalBackfill"; import { resolveScopeSongs } from "../resolveScopeSongs"; import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; import { upsertSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue"; +import { start } from "workflow/api"; vi.mock("../resolveScopeSongs", () => ({ resolveScopeSongs: vi.fn() })); vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ @@ -11,6 +12,10 @@ vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ vi.mock("@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue", () => ({ upsertSongstatsBackfillQueue: vi.fn(), })); +vi.mock("workflow/api", () => ({ start: vi.fn() })); +vi.mock("@/app/workflows/songstatsBackfillWorkflow", () => ({ + songstatsBackfillWorkflow: vi.fn(), +})); describe("enqueueHistoricalBackfill", () => { beforeEach(() => { @@ -47,6 +52,8 @@ describe("enqueueHistoricalBackfill", () => { expect(r).toEqual({ data: { status: "success", source: "historical", id: null, enqueued: 2, skipped: 1 }, }); + // kick the drain immediately so the user doesn't wait for the daily cron + expect(start).toHaveBeenCalledTimes(1); }); it("ranks a song with no prior measurement at 0", async () => { @@ -57,4 +64,16 @@ describe("enqueueHistoricalBackfill", () => { expect(upsertSongstatsBackfillQueue).toHaveBeenCalledWith({ song: "I9", rank_score: 0 }); }); + + it("does NOT kick the drain when everything was already backfilled (nothing enqueued)", async () => { + vi.mocked(resolveScopeSongs).mockResolvedValue(["I1"]); + vi.mocked(selectSongMeasurements).mockResolvedValue([ + { song: "I1", value: 500, data_source: "songstats" }, + ] as never); + + const r = await enqueueHistoricalBackfill({ isrcs: ["I1"] }); + + expect((r as { data: { enqueued: number } }).data.enqueued).toBe(0); + expect(start).not.toHaveBeenCalled(); + }); }); diff --git a/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts b/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts index 1e2c1f072..d75629a53 100644 --- a/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts +++ b/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { ensureSongstatsPaymentMethod } from "../ensureSongstatsPaymentMethod"; import { findStripeCustomerForAccount } from "@/lib/stripe/findStripeCustomerForAccount"; import { findDefaultPaymentMethodForCustomer } from "@/lib/stripe/findDefaultPaymentMethodForCustomer"; -import { createStripeSession } from "@/lib/stripe/createStripeSession"; +import { createCardOnFileSession } from "@/lib/stripe/createCardOnFileSession"; vi.mock("@/lib/networking/getCorsHeaders", () => ({ getCorsHeaders: vi.fn(() => ({})) })); vi.mock("@/lib/stripe/findStripeCustomerForAccount", () => ({ @@ -11,7 +11,7 @@ vi.mock("@/lib/stripe/findStripeCustomerForAccount", () => ({ vi.mock("@/lib/stripe/findDefaultPaymentMethodForCustomer", () => ({ findDefaultPaymentMethodForCustomer: vi.fn(), })); -vi.mock("@/lib/stripe/createStripeSession", () => ({ createStripeSession: vi.fn() })); +vi.mock("@/lib/stripe/createCardOnFileSession", () => ({ createCardOnFileSession: vi.fn() })); describe("ensureSongstatsPaymentMethod", () => { beforeEach(() => vi.clearAllMocks()); @@ -23,17 +23,17 @@ describe("ensureSongstatsPaymentMethod", () => { const r = await ensureSongstatsPaymentMethod("acc_1"); expect(r).toBeNull(); - expect(createStripeSession).not.toHaveBeenCalled(); + expect(createCardOnFileSession).not.toHaveBeenCalled(); }); it("402s with a free-tier checkout link when there is no Stripe customer", async () => { vi.mocked(findStripeCustomerForAccount).mockResolvedValue(null); - vi.mocked(createStripeSession).mockResolvedValue({ url: "https://checkout/free" } as never); + vi.mocked(createCardOnFileSession).mockResolvedValue({ url: "https://checkout/free" } as never); const r = await ensureSongstatsPaymentMethod("acc_1"); expect(findDefaultPaymentMethodForCustomer).not.toHaveBeenCalled(); - expect(createStripeSession).toHaveBeenCalledWith("acc_1", expect.any(String)); + expect(createCardOnFileSession).toHaveBeenCalledWith("acc_1", expect.any(String)); expect((r as Response).status).toBe(402); expect(await (r as Response).json()).toMatchObject({ status: "error", @@ -44,7 +44,7 @@ describe("ensureSongstatsPaymentMethod", () => { it("402s with a checkout link when the customer exists but has no card", async () => { vi.mocked(findStripeCustomerForAccount).mockResolvedValue("cus_1"); vi.mocked(findDefaultPaymentMethodForCustomer).mockResolvedValue(null); - vi.mocked(createStripeSession).mockResolvedValue({ url: "https://checkout/free" } as never); + vi.mocked(createCardOnFileSession).mockResolvedValue({ url: "https://checkout/free" } as never); const r = await ensureSongstatsPaymentMethod("acc_1"); diff --git a/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts b/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts index 3c0cc7af5..ffd6f3f20 100644 --- a/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts +++ b/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts @@ -1,6 +1,8 @@ +import { start } from "workflow/api"; import { resolveScopeSongs } from "./resolveScopeSongs"; import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; import { upsertSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue"; +import { songstatsBackfillWorkflow } from "@/app/workflows/songstatsBackfillWorkflow"; import type { CreateMeasurementJobBody } from "./validateCreateMeasurementJobRequest"; const METRIC = "platform_displayed_play_count"; @@ -61,5 +63,18 @@ export async function enqueueHistoricalBackfill( enqueued += batch.length; } + // Kick the drain now instead of waiting for the daily cron. The workflow's own + // budget gate (limit − reserve − rolling-30d ledger) means it only drains what + // the Songstats quota allows and then stops; SKIP LOCKED claims keep it from + // double-processing alongside the cron, which stays as the backstop. Fire-and- + // forget — a scheduling hiccup must not fail the (already-enqueued) job. + if (enqueued > 0) { + try { + await start(songstatsBackfillWorkflow); + } catch (error) { + console.error("[measurement-jobs] failed to kick backfill drain:", error); + } + } + return { data: { status: "success", source: "historical", id: null, enqueued, skipped } }; } diff --git a/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts b/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts index 71e3dd37c..8654dd19e 100644 --- a/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts +++ b/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts @@ -2,7 +2,7 @@ import { NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { findStripeCustomerForAccount } from "@/lib/stripe/findStripeCustomerForAccount"; import { findDefaultPaymentMethodForCustomer } from "@/lib/stripe/findDefaultPaymentMethodForCustomer"; -import { createStripeSession } from "@/lib/stripe/createStripeSession"; +import { createCardOnFileSession } from "@/lib/stripe/createCardOnFileSession"; import { CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL } from "@/lib/credits/const"; /** @@ -22,7 +22,10 @@ export async function ensureSongstatsPaymentMethod( const paymentMethod = customerId ? await findDefaultPaymentMethodForCustomer(customerId) : null; if (paymentMethod) return null; - const session = await createStripeSession(accountId, CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL); + const session = await createCardOnFileSession( + accountId, + CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL, + ); return NextResponse.json( { status: "error", diff --git a/lib/stripe/__tests__/createCardOnFileSession.test.ts b/lib/stripe/__tests__/createCardOnFileSession.test.ts new file mode 100644 index 000000000..14d1e0016 --- /dev/null +++ b/lib/stripe/__tests__/createCardOnFileSession.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { checkoutSessionsCreate, resolveStripeCustomerForAccountMock } = vi.hoisted(() => ({ + checkoutSessionsCreate: vi.fn(), + resolveStripeCustomerForAccountMock: vi.fn(), +})); + +vi.mock("@/lib/stripe/client", () => ({ + default: { checkout: { sessions: { create: checkoutSessionsCreate } } }, +})); +vi.mock("@/lib/stripe/resolveStripeCustomerForAccount", () => ({ + resolveStripeCustomerForAccount: resolveStripeCustomerForAccountMock, +})); + +const { createCardOnFileSession } = await import("@/lib/stripe/createCardOnFileSession"); + +describe("createCardOnFileSession", () => { + beforeEach(() => { + vi.clearAllMocks(); + checkoutSessionsCreate.mockResolvedValue({ id: "cs_x", url: "https://checkout/setup" }); + resolveStripeCustomerForAccountMock.mockResolvedValue("cus_acc1"); + }); + + it("creates a setup-mode session (collect card only, no subscription/price)", async () => { + await createCardOnFileSession("acc-1", "https://example.com/success"); + + expect(resolveStripeCustomerForAccountMock).toHaveBeenCalledWith("acc-1"); + const params = checkoutSessionsCreate.mock.calls[0][0]; + expect(params).toMatchObject({ + customer: "cus_acc1", + mode: "setup", + client_reference_id: "acc-1", + success_url: "https://example.com/success", + }); + // free tier: no subscription, no line_items/price + expect(params.mode).not.toBe("subscription"); + expect(params.line_items).toBeUndefined(); + }); +}); diff --git a/lib/stripe/createCardOnFileSession.ts b/lib/stripe/createCardOnFileSession.ts new file mode 100644 index 000000000..f94959542 --- /dev/null +++ b/lib/stripe/createCardOnFileSession.ts @@ -0,0 +1,31 @@ +import type Stripe from "stripe"; +import stripeClient from "@/lib/stripe/client"; +import { resolveStripeCustomerForAccount } from "@/lib/stripe/resolveStripeCustomerForAccount"; + +/** + * A Stripe Checkout session that **only collects a card on file** — `mode: + * "setup"`, $0, no subscription or price. This is the "free tier": the account + * saves a payment method (so metered Songstats usage can be charged later via + * the credits system) without committing to any recurring plan. Needs no Stripe + * product. Contrast with {@link createStripeSession}, which is the paid + * subscription flow. + * + * @param accountId - The account to attach the card to. + * @param successUrl - Where Stripe redirects after the card is saved. + */ +export async function createCardOnFileSession( + accountId: string, + successUrl: string, +): Promise { + const metadata = { accountId }; + const customer = await resolveStripeCustomerForAccount(accountId); + + return stripeClient.checkout.sessions.create({ + customer, + mode: "setup", + currency: "usd", + client_reference_id: accountId, + metadata, + success_url: successUrl, + }); +} From 5fa5e3ac7c580ba52c5c53db7a9bdc2f7216035b Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 16:41:22 -0500 Subject: [PATCH 02/12] fix(songstats-backfill): backoff on 429 + defer instead of churn (chat#1797) (#673) Pacing/backoff + per-step logging for the Songstats backfill drain (chat#1797 bullets 1 & 3). Bounded exponential backoff (fetchSongstatsWithBackoff, 502/503/504/408/429), defer-to-pending past the bound with claimed-batch release, per-step + per-batch logging. --- .../__tests__/backfillTrackStep.test.ts | 92 +++++++++---------- .../songstatsBackfillWorkflow.test.ts | 61 ++++++++++++ app/workflows/backfillTrackStep.ts | 59 +++++++----- app/workflows/releaseClaimedRowsStep.ts | 15 +++ app/workflows/songstatsBackfillWorkflow.ts | 38 ++++++-- .../fetchSongstatsWithBackoff.test.ts | 91 ++++++++++++++++++ .../__tests__/isRetryableStatus.test.ts | 26 ++++++ lib/songstats/fetchSongstatsWithBackoff.ts | 61 ++++++++++++ lib/songstats/isRetryableStatus.ts | 14 +++ .../updateSongstatsBackfillQueue.test.ts | 32 +++++-- .../updateSongstatsBackfillQueue.ts | 15 ++- 11 files changed, 406 insertions(+), 98 deletions(-) create mode 100644 app/workflows/__tests__/songstatsBackfillWorkflow.test.ts create mode 100644 app/workflows/releaseClaimedRowsStep.ts create mode 100644 lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts create mode 100644 lib/songstats/__tests__/isRetryableStatus.test.ts create mode 100644 lib/songstats/fetchSongstatsWithBackoff.ts create mode 100644 lib/songstats/isRetryableStatus.ts diff --git a/app/workflows/__tests__/backfillTrackStep.test.ts b/app/workflows/__tests__/backfillTrackStep.test.ts index ebe773704..6c211ed61 100644 --- a/app/workflows/__tests__/backfillTrackStep.test.ts +++ b/app/workflows/__tests__/backfillTrackStep.test.ts @@ -1,12 +1,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { backfillTrackStep } from "../backfillTrackStep"; -import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; -vi.mock("@/lib/songstats/fetchSongstats", () => ({ fetchSongstats: vi.fn() })); +vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ + fetchSongstatsWithBackoff: vi.fn(), +})); vi.mock("@/lib/supabase/song_measurements/upsertSongMeasurements", () => ({ upsertSongMeasurements: vi.fn(), })); @@ -22,22 +24,20 @@ const ROW = { id: "q1", song: "USA2P2015959" } as never; describe("backfillTrackStep", () => { beforeEach(() => { vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => {}); vi.mocked(upsertSongMeasurements).mockResolvedValue([] as never); }); - it("writes the historic series as songstats measurements, records spend, marks done", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ + it("writes the historic series, records the spend, marks done on 200", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 200, + attempts: 1, + retriesExhausted: false, data: { stats: [ { source: "spotify", - data: { - history: [ - { date: "2025-01-01", streams_total: 1008736324 }, - { date: "2026-01-01", streams_total: 1330251464 }, - ], - }, + data: { history: [{ date: "2025-01-01", streams_total: 1008736324 }] }, }, ], }, @@ -45,10 +45,11 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); - expect(fetchSongstats).toHaveBeenCalledWith("tracks/historic_stats", { + expect(fetchSongstatsWithBackoff).toHaveBeenCalledWith("tracks/historic_stats", { isrc: "USA2P2015959", source: "spotify", }); + // exact transformation: each history point → a permanent songstats measurement expect(upsertSongMeasurements).toHaveBeenCalledWith([ { song: "USA2P2015959", @@ -59,74 +60,65 @@ describe("backfillTrackStep", () => { data_source: "songstats", raw_ref: "songstats-backfill", }, - { - song: "USA2P2015959", - platform: "spotify", - metric: "platform_displayed_play_count", - value: 1330251464, - captured_at: "2026-01-01T00:00:00.000Z", - data_source: "songstats", - raw_ref: "songstats-backfill", - }, ]); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, purpose: "backfill USA2P2015959", }); - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(result).toEqual({ ok: true, hitsSpent: 1 }); }); - it("marks a transient upstream error (429) as failed (reclaimable) and records the spend", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); - - const result = await backfillTrackStep(ROW); - - // transient -> 'failed' so the daily reclaim sweep returns it to 'pending' - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "failed" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (failed 429)", + it("DEFERS (pending, no quota hit, signals stop) when backoff is exhausted on 429", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 429, + attempts: 6, + retriesExhausted: true, + data: {}, }); - expect(upsertSongMeasurements).not.toHaveBeenCalled(); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); - }); - - it("marks a transient 5xx as failed (reclaimable)", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 504, data: {} }); const result = await backfillTrackStep(ROW); - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "failed" }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + // left pending for the next drain; NO ledger hit (Songstats consumed nothing) + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "pending" }); + expect(insertSongstatsQuotaLedger).not.toHaveBeenCalled(); + expect(upsertSongMeasurements).not.toHaveBeenCalled(); + expect(result).toEqual({ ok: false, hitsSpent: 0, deferred: true }); }); - it("marks a permanent client error (403) as done so reclaim never recycles it", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 403, data: {} }); + it("marks a definitive 404 (no history) as done and records the spend", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 404, + attempts: 1, + retriesExhausted: false, + data: {}, + }); const result = await backfillTrackStep(ROW); - // non-retryable 4xx (not 408/429) is terminal -> 'done', not 'failed' - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, - purpose: "backfill USA2P2015959 (terminal 403)", + purpose: "backfill USA2P2015959 (no data 404)", }); expect(result).toEqual({ ok: false, hitsSpent: 1 }); }); - it("marks a definitive 404 (no history exists) as done so it is never retried", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 404, data: {} }); + it("marks a permanent 4xx (403) as done (terminal) and records the spend", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 403, + attempts: 1, + retriesExhausted: false, + data: {}, + }); const result = await backfillTrackStep(ROW); - // terminal no-data -> 'done', not 'failed' — the reclaim sweep must not resurrect it - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, - purpose: "backfill USA2P2015959 (no data 404)", + purpose: "backfill USA2P2015959 (terminal 403)", }); - expect(upsertSongMeasurements).not.toHaveBeenCalled(); expect(result).toEqual({ ok: false, hitsSpent: 1 }); }); }); diff --git a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts new file mode 100644 index 000000000..cc435558a --- /dev/null +++ b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { songstatsBackfillWorkflow } from "../songstatsBackfillWorkflow"; + +import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; +import { claimBackfillRowsStep } from "../claimBackfillRowsStep"; +import { backfillTrackStep } from "../backfillTrackStep"; +import { releaseClaimedRowsStep } from "../releaseClaimedRowsStep"; + +vi.mock("../getBackfillBudgetStep", () => ({ getBackfillBudgetStep: vi.fn() })); +vi.mock("../claimBackfillRowsStep", () => ({ claimBackfillRowsStep: vi.fn() })); +vi.mock("../backfillTrackStep", () => ({ backfillTrackStep: vi.fn() })); +vi.mock("../releaseClaimedRowsStep", () => ({ releaseClaimedRowsStep: vi.fn() })); + +const row = (id: string) => ({ id, song: id }) as never; + +describe("songstatsBackfillWorkflow", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.mocked(releaseClaimedRowsStep).mockResolvedValue(undefined); + }); + + it("releases the rest of the claimed batch to pending when a track defers", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); + vi.mocked(claimBackfillRowsStep).mockResolvedValue([row("r1"), row("r2"), row("r3")]); + vi.mocked(backfillTrackStep) + .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) // r1 + .mockResolvedValueOnce({ ok: false, hitsSpent: 0, deferred: true }); // r2 defers + + const result = await songstatsBackfillWorkflow(); + + // r2 is set pending by the step itself; the unprocessed remainder (r3) is released here + expect(releaseClaimedRowsStep).toHaveBeenCalledWith(["r3"]); + expect(backfillTrackStep).toHaveBeenCalledTimes(2); // stopped at the defer, never reached r3 + expect(result).toEqual({ backfilled: 1, failed: 0, deferred: true }); + }); + + it("drains until the queue is empty and never releases when nothing defers", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); + vi.mocked(claimBackfillRowsStep) + .mockResolvedValueOnce([row("a"), row("b")]) + .mockResolvedValueOnce([]); // queue drained + vi.mocked(backfillTrackStep) + .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) + .mockResolvedValueOnce({ ok: false, hitsSpent: 1 }); // terminal (e.g. 404) + + const result = await songstatsBackfillWorkflow(); + + expect(releaseClaimedRowsStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 1, failed: 1, deferred: false }); + }); + + it("does not drain when there is no budget", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(0); + + const result = await songstatsBackfillWorkflow(); + + expect(claimBackfillRowsStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 0, failed: 0, deferred: false }); + }); +}); diff --git a/app/workflows/backfillTrackStep.ts b/app/workflows/backfillTrackStep.ts index 3247ff4cf..fd3947432 100644 --- a/app/workflows/backfillTrackStep.ts +++ b/app/workflows/backfillTrackStep.ts @@ -1,4 +1,4 @@ -import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; @@ -8,40 +8,48 @@ import { Tables } from "@/types/database.types"; const METRIC = "platform_displayed_play_count"; /** - * Backfill one claimed queue row: fetch the track's Songstats historic series - * (one quota hit — recorded win or lose), write each point as a permanent - * `songstats`-labeled measurement, and close the row. Failures mark the row - * failed without failing the run — the next row may still succeed. + * Backfill one claimed queue row, with bounded exponential backoff on Songstats' + * rate limit (Songstats is the rate authority — see chat#1797): + * - **200** → write each history point as a permanent `songstats` measurement, + * record the spend, mark `done`. + * - **404 / other 4xx** → a real request with a definitive answer; terminal, so + * mark `done` (404 = no history) and record the spend. + * - **backoff exhausted** (still 429/5xx after retries) → **defer**: leave the row + * `pending` for the next drain, consume no quota, and signal the workflow to + * stop (`deferred`) — Songstats is saturated right now. * * @param row - The claimed queue row (already in_progress) - * @returns ok + hits spent (always 1; the hit is consumed even on failure) + * @returns ok + hitsSpent (0 when deferred) + `deferred` when Songstats is saturated */ export async function backfillTrackStep( row: Tables<"songstats_backfill_queue">, -): Promise<{ ok: boolean; hitsSpent: number }> { +): Promise<{ ok: boolean; hitsSpent: number; deferred?: boolean }> { "use step"; - const result = await fetchSongstats("tracks/historic_stats", { + const result = await fetchSongstatsWithBackoff("tracks/historic_stats", { isrc: row.song, source: "spotify", }); - if (result.status !== 200) { - const status = result.status; - const isNoData = status === 404; - // Only transient errors are retryable: 408 (timeout), 429 (quota), any 5xx. - const isRetryable = status === 408 || status === 429 || status >= 500; - - // `failed` is reclaimable (the daily sweep returns it to `pending`, bounded - // by the rolling-window budget). 404 no-data and other permanent 4xx are - // terminal → `done`, so reclaim never recycles a track that can't succeed. - const nextStatus = isRetryable ? "failed" : "done"; - - let outcome = `terminal ${status}`; - if (isNoData) outcome = "no data 404"; - else if (isRetryable) outcome = `failed ${status}`; + if (result.retriesExhausted) { + // Still retryable (429 throttle / 408 / gateway 5xx) past the backoff bound — + // leave it for the next run, spend nothing. + console.log( + `[backfill] ${row.song} deferred (retryable ${result.status} after ${result.attempts} tries)`, + ); + await updateSongstatsBackfillQueue([row.id], { status: "pending" }); + return { ok: false, hitsSpent: 0, deferred: true }; + } - await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song} (${outcome})` }); - await updateSongstatsBackfillQueue(row.id, { status: nextStatus }); + if (result.status !== 200) { + const noData = result.status === 404; + console.log( + `[backfill] ${row.song} done (${noData ? "no data 404" : `terminal ${result.status}`})`, + ); + await insertSongstatsQuotaLedger({ + hits: 1, + purpose: `backfill ${row.song} (${noData ? "no data 404" : `terminal ${result.status}`})`, + }); + await updateSongstatsBackfillQueue([row.id], { status: "done" }); return { ok: false, hitsSpent: 1 }; } @@ -67,7 +75,8 @@ export async function backfillTrackStep( }); await upsertSongMeasurements(rows); + console.log(`[backfill] ${row.song} done (${rows.length} points written)`); await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song}` }); - await updateSongstatsBackfillQueue(row.id, { status: "done" }); + await updateSongstatsBackfillQueue([row.id], { status: "done" }); return { ok: true, hitsSpent: 1 }; } diff --git a/app/workflows/releaseClaimedRowsStep.ts b/app/workflows/releaseClaimedRowsStep.ts new file mode 100644 index 000000000..788d752a0 --- /dev/null +++ b/app/workflows/releaseClaimedRowsStep.ts @@ -0,0 +1,15 @@ +import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; + +/** + * Durable step: return unprocessed claimed rows to `pending` when the drain + * stops early on a defer, so the next run retries them immediately instead of + * waiting on the stale-reclaim sweep. + * + * @param ids - Queue row ids still `in_progress` from the aborted batch. + */ +export async function releaseClaimedRowsStep(ids: string[]): Promise { + "use step"; + if (ids.length === 0) return; + await updateSongstatsBackfillQueue(ids, { status: "pending" }); + console.log(`[songstats-backfill] released ${ids.length} claimed rows back to pending`); +} diff --git a/app/workflows/songstatsBackfillWorkflow.ts b/app/workflows/songstatsBackfillWorkflow.ts index 0a1960bf9..f94c0223d 100644 --- a/app/workflows/songstatsBackfillWorkflow.ts +++ b/app/workflows/songstatsBackfillWorkflow.ts @@ -1,15 +1,20 @@ import { getBackfillBudgetStep } from "@/app/workflows/getBackfillBudgetStep"; import { claimBackfillRowsStep } from "@/app/workflows/claimBackfillRowsStep"; import { backfillTrackStep } from "@/app/workflows/backfillTrackStep"; +import { releaseClaimedRowsStep } from "@/app/workflows/releaseClaimedRowsStep"; const BATCH_SIZE = 25; /** - * Durable Songstats backfill drain (recoupable/chat#1791 write path): check - * the rolling-window budget, claim value-ranked rows via the SKIP LOCKED RPC, - * backfill each track's historic series into the measurement store, and stop - * when the queue or the budget is dry. Every quota hit converts into - * permanent owned data (fetch-once: captured history is never refetched). + * Durable Songstats backfill drain (recoupable/chat#1791 write path): claim + * value-ranked rows via the SKIP LOCKED RPC and backfill each track's historic + * series, with per-track exponential backoff handling Songstats' rate limit + * (chat#1797). **Stops as soon as a track defers** — Songstats still + * rate-limiting it past the backoff bound — releasing the rest of the claimed + * batch back to `pending` (so the next drain retries them immediately rather + * than waiting on stale-reclaim) instead of hammering a saturated API. Every + * successful hit converts into permanent owned data (fetch-once: captured + * history is never refetched). */ export async function songstatsBackfillWorkflow() { "use workflow"; @@ -17,13 +22,23 @@ export async function songstatsBackfillWorkflow() { let budget = await getBackfillBudgetStep(); let backfilled = 0; let failed = 0; + let deferred = false; - while (budget > 0) { + drain: while (budget > 0) { const rows = await claimBackfillRowsStep(Math.min(budget, BATCH_SIZE)); if (rows.length === 0) break; + console.log(`[songstats-backfill] claimed ${rows.length} rows`); - for (const row of rows) { - const result = await backfillTrackStep(row); + for (let i = 0; i < rows.length; i += 1) { + const result = await backfillTrackStep(rows[i]); + if (result.deferred) { + // Songstats is saturated — stop now. The deferred row is already back to + // `pending`; release the rest of this claimed batch too so they don't sit + // `in_progress` until stale-reclaim. + deferred = true; + await releaseClaimedRowsStep(rows.slice(i + 1).map(r => r.id)); + break drain; + } budget -= result.hitsSpent; if (result.ok) backfilled += 1; else failed += 1; @@ -31,6 +46,9 @@ export async function songstatsBackfillWorkflow() { } } - console.log(`[songstats-backfill] done: ${backfilled} backfilled, ${failed} failed`); - return { backfilled, failed }; + console.log( + `[songstats-backfill] done: ${backfilled} backfilled, ${failed} terminal` + + (deferred ? ", deferred (rate-limited)" : ""), + ); + return { backfilled, failed, deferred }; } diff --git a/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts b/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts new file mode 100644 index 000000000..21015ef14 --- /dev/null +++ b/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { fetchSongstatsWithBackoff } from "../fetchSongstatsWithBackoff"; +import { fetchSongstats } from "../fetchSongstats"; + +vi.mock("../fetchSongstats", () => ({ fetchSongstats: vi.fn() })); + +const noSleep = vi.fn().mockResolvedValue(undefined); + +describe("fetchSongstatsWithBackoff", () => { + beforeEach(() => vi.clearAllMocks()); + + it("returns immediately on 200 with no retries or sleeps", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 200, data: { ok: true } }); + + const r = await fetchSongstatsWithBackoff( + "tracks/historic_stats", + { isrc: "I" }, + { sleep: noSleep }, + ); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(noSleep).not.toHaveBeenCalled(); + expect(r).toMatchObject({ status: 200, attempts: 1, retriesExhausted: false }); + }); + + it("does NOT retry a non-retryable status (404)", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 404, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep }); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(r).toMatchObject({ status: 404, retriesExhausted: false }); + }); + + it("backs off and retries on 429, succeeding on a later attempt", async () => { + vi.mocked(fetchSongstats) + .mockResolvedValueOnce({ status: 429, data: {} }) + .mockResolvedValueOnce({ status: 429, data: {} }) + .mockResolvedValueOnce({ status: 200, data: { ok: true } }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, baseMs: 100 }); + + expect(fetchSongstats).toHaveBeenCalledTimes(3); + expect(noSleep).toHaveBeenCalledTimes(2); + // exponential: 100 then 200 + expect(noSleep).toHaveBeenNthCalledWith(1, 100); + expect(noSleep).toHaveBeenNthCalledWith(2, 200); + expect(r).toMatchObject({ status: 200, attempts: 3, retriesExhausted: false }); + }); + + it("gives up after maxRetries on persistent 429 and flags retriesExhausted", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, maxRetries: 3 }); + + expect(fetchSongstats).toHaveBeenCalledTimes(4); // 1 initial + 3 retries + expect(noSleep).toHaveBeenCalledTimes(3); + expect(r).toMatchObject({ status: 429, retriesExhausted: true }); + }); + + it("treats transient gateway 5xx (503) and 408 as retryable", async () => { + vi.mocked(fetchSongstats) + .mockResolvedValueOnce({ status: 503, data: {} }) + .mockResolvedValueOnce({ status: 200, data: {} }); + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, maxRetries: 2 }); + expect(fetchSongstats).toHaveBeenCalledTimes(2); + expect(r).toMatchObject({ status: 200, retriesExhausted: false }); + }); + + it("does NOT retry a 500 (fetchSongstats maps missing key / fetch failure to 500)", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 500, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep }); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(noSleep).not.toHaveBeenCalled(); + expect(r).toMatchObject({ status: 500, retriesExhausted: false }); + }); + + it("caps the backoff at maxMs", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); + await fetchSongstatsWithBackoff("p", undefined, { + sleep: noSleep, + baseMs: 1000, + maxMs: 1500, + maxRetries: 3, + }); + // 1000, 2000->capped 1500, 4000->capped 1500 + expect(noSleep.mock.calls.map(c => c[0])).toEqual([1000, 1500, 1500]); + }); +}); diff --git a/lib/songstats/__tests__/isRetryableStatus.test.ts b/lib/songstats/__tests__/isRetryableStatus.test.ts new file mode 100644 index 000000000..a1b707f09 --- /dev/null +++ b/lib/songstats/__tests__/isRetryableStatus.test.ts @@ -0,0 +1,26 @@ +import { describe, it, expect } from "vitest"; +import { isRetryableStatus } from "../isRetryableStatus"; + +describe("isRetryableStatus", () => { + it("retries transient throttling/timeouts: 408, 429", () => { + expect(isRetryableStatus(408)).toBe(true); + expect(isRetryableStatus(429)).toBe(true); + }); + + it("retries transient gateway 5xx: 502, 503, 504", () => { + expect(isRetryableStatus(502)).toBe(true); + expect(isRetryableStatus(503)).toBe(true); + expect(isRetryableStatus(504)).toBe(true); + }); + + it("does NOT retry 500/501 (fetchSongstats maps missing key + fetch failures to 500)", () => { + expect(isRetryableStatus(500)).toBe(false); + expect(isRetryableStatus(501)).toBe(false); + }); + + it("does NOT retry definitive responses: 200, 404, 403", () => { + expect(isRetryableStatus(200)).toBe(false); + expect(isRetryableStatus(404)).toBe(false); + expect(isRetryableStatus(403)).toBe(false); + }); +}); diff --git a/lib/songstats/fetchSongstatsWithBackoff.ts b/lib/songstats/fetchSongstatsWithBackoff.ts new file mode 100644 index 000000000..8b3a4b097 --- /dev/null +++ b/lib/songstats/fetchSongstatsWithBackoff.ts @@ -0,0 +1,61 @@ +import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { isRetryableStatus } from "@/lib/songstats/isRetryableStatus"; +import { delay } from "@/lib/time/delay"; +import type { ProxyResult } from "@/lib/research/ProxyResult"; + +// Short, in-step backoff for Songstats' per-second rate limit. The default +// budget is 1+2+4+8 = 15s of waits (base 1s, doubling, capped at 8s, 4 retries), +// well within a workflow step's duration. Persistent rejection defers the row to +// the next drain run rather than sleeping for minutes inside one invocation. +const DEFAULT_MAX_RETRIES = 4; +const DEFAULT_BASE_MS = 1000; +const DEFAULT_MAX_MS = 8_000; + +export type FetchSongstatsBackoffOptions = { + maxRetries?: number; + baseMs?: number; + maxMs?: number; + /** Injectable for tests; defaults to a real timer. */ + sleep?: (ms: number) => Promise; +}; + +export type BackoffResult = ProxyResult & { + /** Total attempts made (1 + retries). */ + attempts: number; + /** True when the final result is still retryable after exhausting retries. */ + retriesExhausted: boolean; +}; + +/** + * Call Songstats with bounded exponential backoff on transient rejections + * (429 rate limit, 408, 5xx). Retries the **same** request up to `maxRetries` + * with `min(maxMs, baseMs * 2^attempt)` waits; a 200 or any non-retryable status + * (e.g. 404) returns immediately. When backoff is exhausted and the call is + * still being rejected, `retriesExhausted` is true so the caller can defer the + * work (leave it `pending`) rather than burn it — Songstats is the rate + * authority, not a local quota ledger. + * + * @param path - Songstats enterprise path (e.g. `tracks/historic_stats`). + * @param queryParams - Query params forwarded to Songstats. + * @param options - Backoff tuning + an injectable `sleep`. + */ +export async function fetchSongstatsWithBackoff( + path: string, + queryParams?: Record, + options: FetchSongstatsBackoffOptions = {}, +): Promise { + const maxRetries = options.maxRetries ?? DEFAULT_MAX_RETRIES; + const baseMs = options.baseMs ?? DEFAULT_BASE_MS; + const maxMs = options.maxMs ?? DEFAULT_MAX_MS; + const sleep = options.sleep ?? delay; + + let result = await fetchSongstats(path, queryParams); + let retries = 0; + while (isRetryableStatus(result.status) && retries < maxRetries) { + await sleep(Math.min(maxMs, baseMs * 2 ** retries)); + result = await fetchSongstats(path, queryParams); + retries += 1; + } + + return { ...result, attempts: retries + 1, retriesExhausted: isRetryableStatus(result.status) }; +} diff --git a/lib/songstats/isRetryableStatus.ts b/lib/songstats/isRetryableStatus.ts new file mode 100644 index 000000000..eb0004ee3 --- /dev/null +++ b/lib/songstats/isRetryableStatus.ts @@ -0,0 +1,14 @@ +/** + * Whether a Songstats response status is worth retrying with backoff. + * + * Transient: 408 (request timeout), 429 (rate limit), and the gateway 5xx + * 502/503/504 (bad gateway / unavailable / gateway timeout). Deliberately + * **excludes** 500/501 — `fetchSongstats` maps a missing API key and any + * non-HTTP fetch failure to 500, which are permanent for that request, so + * retrying them just burns the backoff budget before the row defers. + * + * @param status - HTTP status from `fetchSongstats`. + */ +export function isRetryableStatus(status: number): boolean { + return status === 408 || status === 429 || status === 502 || status === 503 || status === 504; +} diff --git a/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts b/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts index eb8d02300..73be73472 100644 --- a/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts +++ b/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts @@ -14,24 +14,40 @@ vi.mock("../../serverClient", () => { describe("updateSongstatsBackfillQueue", () => { beforeEach(() => vi.clearAllMocks()); - it("updates a queue row's status by id", async () => { - const eq = vi.fn().mockResolvedValue({ error: null }); - const update = vi.fn().mockReturnValue({ eq }); + it("updates a single row by id (one-element array)", async () => { + const inFn = vi.fn().mockResolvedValue({ error: null }); + const update = vi.fn().mockReturnValue({ in: inFn }); vi.mocked(supabase.from).mockReturnValue({ update } as never); - await updateSongstatsBackfillQueue("q1", { status: "done" }); + await updateSongstatsBackfillQueue(["q1"], { status: "done" }); expect(supabase.from).toHaveBeenCalledWith("songstats_backfill_queue"); expect(update).toHaveBeenCalledWith({ status: "done" }); - expect(eq).toHaveBeenCalledWith("id", "q1"); + expect(inFn).toHaveBeenCalledWith("id", ["q1"]); + }); + + it("bulk-updates many rows in one call (e.g. releasing a claimed batch)", async () => { + const inFn = vi.fn().mockResolvedValue({ error: null }); + const update = vi.fn().mockReturnValue({ in: inFn }); + vi.mocked(supabase.from).mockReturnValue({ update } as never); + + await updateSongstatsBackfillQueue(["q1", "q2"], { status: "pending" }); + + expect(update).toHaveBeenCalledWith({ status: "pending" }); + expect(inFn).toHaveBeenCalledWith("id", ["q1", "q2"]); + }); + + it("is a no-op on an empty id list (no DB call)", async () => { + await updateSongstatsBackfillQueue([], { status: "pending" }); + expect(supabase.from).not.toHaveBeenCalled(); }); it("throws on update error", async () => { - const eq = vi.fn().mockResolvedValue({ error: { message: "boom" } }); - const update = vi.fn().mockReturnValue({ eq }); + const inFn = vi.fn().mockResolvedValue({ error: { message: "boom" } }); + const update = vi.fn().mockReturnValue({ in: inFn }); vi.mocked(supabase.from).mockReturnValue({ update } as never); - await expect(updateSongstatsBackfillQueue("q1", { status: "failed" })).rejects.toThrow( + await expect(updateSongstatsBackfillQueue(["q1"], { status: "failed" })).rejects.toThrow( "Failed to update songstats backfill queue: boom", ); }); diff --git a/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts b/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts index b462381ab..d2469412d 100644 --- a/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts +++ b/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts @@ -2,17 +2,22 @@ import supabase from "../serverClient"; import { TablesUpdate } from "@/types/database.types"; /** - * Update a backfill queue row (mark done/failed after a claim). + * Update one or more backfill queue rows by id in a single round trip. Handles + * both the per-row status flip after a claim (`[row.id]`) and the bulk release + * of a claimed batch back to `pending` when the drain stops early (`.in` works + * for one id or many). No-op on an empty id list. * - * @param id - The queue row id - * @param fields - Fields to update + * @param ids - Queue row ids to update + * @param fields - Fields to set (e.g. `{ status: "done" }`) * @throws Error if the update fails */ export async function updateSongstatsBackfillQueue( - id: string, + ids: string[], fields: TablesUpdate<"songstats_backfill_queue">, ): Promise { - const { error } = await supabase.from("songstats_backfill_queue").update(fields).eq("id", id); + if (ids.length === 0) return; + + const { error } = await supabase.from("songstats_backfill_queue").update(fields).in("id", ids); if (error) { throw new Error(`Failed to update songstats backfill queue: ${error.message}`); From 5f459463996e1139a40743eaf5fd83c54f4ed04a Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 17:14:31 -0500 Subject: [PATCH 03/12] refactor(songstats): remove local quota ledger + budget gate (chat#1797) (#674) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bullet 2 of chat#1797 (code half). Songstats is the rate authority — removes getBackfillBudgetStep, the budget gate, and insertSongstatsQuotaLedger/selectSongstatsQuotaSpent. The drain now claims+processes regardless of the ledger (un-stalls the backfill); the songstats_quota_ledger table is dropped in recoupable/database#35 (apply AFTER this deploys). --- .../__tests__/backfillTrackStep.test.ts | 34 +++++------------- .../__tests__/getBackfillBudgetStep.test.ts | 32 ----------------- .../songstatsBackfillWorkflow.test.ts | 24 ++++++------- app/workflows/backfillTrackStep.ts | 29 +++++++-------- app/workflows/getBackfillBudgetStep.ts | 21 ----------- app/workflows/songstatsBackfillWorkflow.ts | 29 +++++++-------- .../insertSongstatsQuotaLedger.test.ts | 32 ----------------- .../selectSongstatsQuotaSpent.test.ts | 35 ------------------- .../insertSongstatsQuotaLedger.ts | 19 ---------- .../selectSongstatsQuotaSpent.ts | 23 ------------ 10 files changed, 42 insertions(+), 236 deletions(-) delete mode 100644 app/workflows/__tests__/getBackfillBudgetStep.test.ts delete mode 100644 app/workflows/getBackfillBudgetStep.ts delete mode 100644 lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts delete mode 100644 lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts delete mode 100644 lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts delete mode 100644 lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts diff --git a/app/workflows/__tests__/backfillTrackStep.test.ts b/app/workflows/__tests__/backfillTrackStep.test.ts index 6c211ed61..c923cb030 100644 --- a/app/workflows/__tests__/backfillTrackStep.test.ts +++ b/app/workflows/__tests__/backfillTrackStep.test.ts @@ -3,7 +3,6 @@ import { backfillTrackStep } from "../backfillTrackStep"; import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; -import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ @@ -12,9 +11,6 @@ vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ vi.mock("@/lib/supabase/song_measurements/upsertSongMeasurements", () => ({ upsertSongMeasurements: vi.fn(), })); -vi.mock("@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger", () => ({ - insertSongstatsQuotaLedger: vi.fn(), -})); vi.mock("@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue", () => ({ updateSongstatsBackfillQueue: vi.fn(), })); @@ -28,7 +24,7 @@ describe("backfillTrackStep", () => { vi.mocked(upsertSongMeasurements).mockResolvedValue([] as never); }); - it("writes the historic series, records the spend, marks done on 200", async () => { + it("writes the historic series and marks done on 200", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 200, attempts: 1, @@ -61,15 +57,11 @@ describe("backfillTrackStep", () => { raw_ref: "songstats-backfill", }, ]); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959", - }); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(result).toEqual({ ok: true, hitsSpent: 1 }); + expect(result).toEqual({ ok: true }); }); - it("DEFERS (pending, no quota hit, signals stop) when backoff is exhausted on 429", async () => { + it("DEFERS (pending, signals stop) when backoff is exhausted on 429", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 429, attempts: 6, @@ -79,14 +71,12 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); - // left pending for the next drain; NO ledger hit (Songstats consumed nothing) expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "pending" }); - expect(insertSongstatsQuotaLedger).not.toHaveBeenCalled(); expect(upsertSongMeasurements).not.toHaveBeenCalled(); - expect(result).toEqual({ ok: false, hitsSpent: 0, deferred: true }); + expect(result).toEqual({ ok: false, deferred: true }); }); - it("marks a definitive 404 (no history) as done and records the spend", async () => { + it("marks a definitive 404 (no history) as done", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 404, attempts: 1, @@ -97,14 +87,10 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (no data 404)", - }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + expect(result).toEqual({ ok: false }); }); - it("marks a permanent 4xx (403) as done (terminal) and records the spend", async () => { + it("marks a permanent 4xx (403) as done (terminal)", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 403, attempts: 1, @@ -115,10 +101,6 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (terminal 403)", - }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + expect(result).toEqual({ ok: false }); }); }); diff --git a/app/workflows/__tests__/getBackfillBudgetStep.test.ts b/app/workflows/__tests__/getBackfillBudgetStep.test.ts deleted file mode 100644 index 6c3fb8159..000000000 --- a/app/workflows/__tests__/getBackfillBudgetStep.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; - -import { selectSongstatsQuotaSpent } from "@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent"; - -vi.mock("@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent", () => ({ - selectSongstatsQuotaSpent: vi.fn(), -})); - -describe("getBackfillBudgetStep", () => { - beforeEach(() => { - vi.clearAllMocks(); - delete process.env.SONGSTATS_QUOTA_LIMIT; - delete process.env.SONGSTATS_QUOTA_RESERVE; - }); - - it("computes limit - reserve - spent over the rolling 30d window", async () => { - vi.mocked(selectSongstatsQuotaSpent).mockResolvedValue(300); - - const budget = await getBackfillBudgetStep(); - - expect(budget).toBe(1000 - 100 - 300); - const since = vi.mocked(selectSongstatsQuotaSpent).mock.calls[0][0]; - const ageDays = (Date.now() - new Date(since).getTime()) / 86400000; - expect(Math.round(ageDays)).toBe(30); - }); - - it("never returns negative", async () => { - vi.mocked(selectSongstatsQuotaSpent).mockResolvedValue(5000); - expect(await getBackfillBudgetStep()).toBe(0); - }); -}); diff --git a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts index cc435558a..b18c5aeb7 100644 --- a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts +++ b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts @@ -1,12 +1,10 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { songstatsBackfillWorkflow } from "../songstatsBackfillWorkflow"; -import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; import { claimBackfillRowsStep } from "../claimBackfillRowsStep"; import { backfillTrackStep } from "../backfillTrackStep"; import { releaseClaimedRowsStep } from "../releaseClaimedRowsStep"; -vi.mock("../getBackfillBudgetStep", () => ({ getBackfillBudgetStep: vi.fn() })); vi.mock("../claimBackfillRowsStep", () => ({ claimBackfillRowsStep: vi.fn() })); vi.mock("../backfillTrackStep", () => ({ backfillTrackStep: vi.fn() })); vi.mock("../releaseClaimedRowsStep", () => ({ releaseClaimedRowsStep: vi.fn() })); @@ -21,41 +19,39 @@ describe("songstatsBackfillWorkflow", () => { }); it("releases the rest of the claimed batch to pending when a track defers", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); vi.mocked(claimBackfillRowsStep).mockResolvedValue([row("r1"), row("r2"), row("r3")]); vi.mocked(backfillTrackStep) - .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) // r1 - .mockResolvedValueOnce({ ok: false, hitsSpent: 0, deferred: true }); // r2 defers + .mockResolvedValueOnce({ ok: true }) // r1 + .mockResolvedValueOnce({ ok: false, deferred: true }); // r2 defers const result = await songstatsBackfillWorkflow(); // r2 is set pending by the step itself; the unprocessed remainder (r3) is released here expect(releaseClaimedRowsStep).toHaveBeenCalledWith(["r3"]); expect(backfillTrackStep).toHaveBeenCalledTimes(2); // stopped at the defer, never reached r3 - expect(result).toEqual({ backfilled: 1, failed: 0, deferred: true }); + expect(result).toEqual({ backfilled: 1, terminal: 0, deferred: true }); }); it("drains until the queue is empty and never releases when nothing defers", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); vi.mocked(claimBackfillRowsStep) .mockResolvedValueOnce([row("a"), row("b")]) .mockResolvedValueOnce([]); // queue drained vi.mocked(backfillTrackStep) - .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) - .mockResolvedValueOnce({ ok: false, hitsSpent: 1 }); // terminal (e.g. 404) + .mockResolvedValueOnce({ ok: true }) + .mockResolvedValueOnce({ ok: false }); // terminal (e.g. 404) const result = await songstatsBackfillWorkflow(); expect(releaseClaimedRowsStep).not.toHaveBeenCalled(); - expect(result).toEqual({ backfilled: 1, failed: 1, deferred: false }); + expect(result).toEqual({ backfilled: 1, terminal: 1, deferred: false }); }); - it("does not drain when there is no budget", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(0); + it("stops immediately when the first claim is empty", async () => { + vi.mocked(claimBackfillRowsStep).mockResolvedValue([]); const result = await songstatsBackfillWorkflow(); - expect(claimBackfillRowsStep).not.toHaveBeenCalled(); - expect(result).toEqual({ backfilled: 0, failed: 0, deferred: false }); + expect(backfillTrackStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 0, terminal: 0, deferred: false }); }); }); diff --git a/app/workflows/backfillTrackStep.ts b/app/workflows/backfillTrackStep.ts index fd3947432..4a627ee88 100644 --- a/app/workflows/backfillTrackStep.ts +++ b/app/workflows/backfillTrackStep.ts @@ -1,6 +1,5 @@ import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; -import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; import { historicStatsPayloadSchema } from "@/lib/research/playcounts/historicStatsPayloadSchema"; import { Tables } from "@/types/database.types"; @@ -9,21 +8,22 @@ const METRIC = "platform_displayed_play_count"; /** * Backfill one claimed queue row, with bounded exponential backoff on Songstats' - * rate limit (Songstats is the rate authority — see chat#1797): + * rate limit (Songstats is the rate authority — see chat#1797; there is no local + * quota ledger): * - **200** → write each history point as a permanent `songstats` measurement, - * record the spend, mark `done`. + * mark `done`. * - **404 / other 4xx** → a real request with a definitive answer; terminal, so - * mark `done` (404 = no history) and record the spend. + * mark `done` (404 = no history). * - **backoff exhausted** (still 429/5xx after retries) → **defer**: leave the row - * `pending` for the next drain, consume no quota, and signal the workflow to - * stop (`deferred`) — Songstats is saturated right now. + * `pending` for the next drain and signal the workflow to stop (`deferred`) — + * Songstats is saturated right now. * * @param row - The claimed queue row (already in_progress) - * @returns ok + hitsSpent (0 when deferred) + `deferred` when Songstats is saturated + * @returns `ok` (true on a written backfill) + `deferred` when Songstats is saturated */ export async function backfillTrackStep( row: Tables<"songstats_backfill_queue">, -): Promise<{ ok: boolean; hitsSpent: number; deferred?: boolean }> { +): Promise<{ ok: boolean; deferred?: boolean }> { "use step"; const result = await fetchSongstatsWithBackoff("tracks/historic_stats", { isrc: row.song, @@ -32,12 +32,12 @@ export async function backfillTrackStep( if (result.retriesExhausted) { // Still retryable (429 throttle / 408 / gateway 5xx) past the backoff bound — - // leave it for the next run, spend nothing. + // leave it for the next run. console.log( `[backfill] ${row.song} deferred (retryable ${result.status} after ${result.attempts} tries)`, ); await updateSongstatsBackfillQueue([row.id], { status: "pending" }); - return { ok: false, hitsSpent: 0, deferred: true }; + return { ok: false, deferred: true }; } if (result.status !== 200) { @@ -45,12 +45,8 @@ export async function backfillTrackStep( console.log( `[backfill] ${row.song} done (${noData ? "no data 404" : `terminal ${result.status}`})`, ); - await insertSongstatsQuotaLedger({ - hits: 1, - purpose: `backfill ${row.song} (${noData ? "no data 404" : `terminal ${result.status}`})`, - }); await updateSongstatsBackfillQueue([row.id], { status: "done" }); - return { ok: false, hitsSpent: 1 }; + return { ok: false }; } const parsed = historicStatsPayloadSchema.safeParse(result.data); @@ -76,7 +72,6 @@ export async function backfillTrackStep( await upsertSongMeasurements(rows); console.log(`[backfill] ${row.song} done (${rows.length} points written)`); - await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song}` }); await updateSongstatsBackfillQueue([row.id], { status: "done" }); - return { ok: true, hitsSpent: 1 }; + return { ok: true }; } diff --git a/app/workflows/getBackfillBudgetStep.ts b/app/workflows/getBackfillBudgetStep.ts deleted file mode 100644 index 866ee7605..000000000 --- a/app/workflows/getBackfillBudgetStep.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { selectSongstatsQuotaSpent } from "@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent"; - -const DEFAULT_QUOTA_LIMIT = 1000; -const DEFAULT_QUOTA_RESERVE = 100; -const WINDOW_DAYS = 30; - -/** - * Remaining Songstats budget for this run: plan limit minus a reserve (kept - * for request-path fallbacks and manual research) minus hits spent in the - * rolling 30-day window. Never negative. - * - * @returns Hits the backfill worker may spend now - */ -export async function getBackfillBudgetStep(): Promise { - "use step"; - const limit = Number(process.env.SONGSTATS_QUOTA_LIMIT) || DEFAULT_QUOTA_LIMIT; - const reserve = Number(process.env.SONGSTATS_QUOTA_RESERVE) || DEFAULT_QUOTA_RESERVE; - const since = new Date(Date.now() - WINDOW_DAYS * 24 * 60 * 60 * 1000).toISOString(); - const spent = await selectSongstatsQuotaSpent(since); - return Math.max(0, limit - reserve - spent); -} diff --git a/app/workflows/songstatsBackfillWorkflow.ts b/app/workflows/songstatsBackfillWorkflow.ts index f94c0223d..459d18870 100644 --- a/app/workflows/songstatsBackfillWorkflow.ts +++ b/app/workflows/songstatsBackfillWorkflow.ts @@ -1,4 +1,3 @@ -import { getBackfillBudgetStep } from "@/app/workflows/getBackfillBudgetStep"; import { claimBackfillRowsStep } from "@/app/workflows/claimBackfillRowsStep"; import { backfillTrackStep } from "@/app/workflows/backfillTrackStep"; import { releaseClaimedRowsStep } from "@/app/workflows/releaseClaimedRowsStep"; @@ -8,24 +7,22 @@ const BATCH_SIZE = 25; /** * Durable Songstats backfill drain (recoupable/chat#1791 write path): claim * value-ranked rows via the SKIP LOCKED RPC and backfill each track's historic - * series, with per-track exponential backoff handling Songstats' rate limit - * (chat#1797). **Stops as soon as a track defers** — Songstats still - * rate-limiting it past the backoff bound — releasing the rest of the claimed - * batch back to `pending` (so the next drain retries them immediately rather - * than waiting on stale-reclaim) instead of hammering a saturated API. Every - * successful hit converts into permanent owned data (fetch-once: captured - * history is never refetched). + * series. Per-track exponential backoff absorbs Songstats' rate limit; the run + * **stops as soon as a track defers** (still retryable past the backoff bound), + * releasing the rest of the claimed batch back to `pending` so the next drain + * retries them immediately rather than waiting on stale-reclaim. Otherwise it + * drains until the queue has no claimable `pending` rows. Every backfill + * converts into permanent owned data (fetch-once). */ export async function songstatsBackfillWorkflow() { "use workflow"; - let budget = await getBackfillBudgetStep(); let backfilled = 0; - let failed = 0; + let terminal = 0; let deferred = false; - drain: while (budget > 0) { - const rows = await claimBackfillRowsStep(Math.min(budget, BATCH_SIZE)); + drain: while (true) { + const rows = await claimBackfillRowsStep(BATCH_SIZE); if (rows.length === 0) break; console.log(`[songstats-backfill] claimed ${rows.length} rows`); @@ -39,16 +36,14 @@ export async function songstatsBackfillWorkflow() { await releaseClaimedRowsStep(rows.slice(i + 1).map(r => r.id)); break drain; } - budget -= result.hitsSpent; if (result.ok) backfilled += 1; - else failed += 1; - if (budget <= 0) break; + else terminal += 1; } } console.log( - `[songstats-backfill] done: ${backfilled} backfilled, ${failed} terminal` + + `[songstats-backfill] done: ${backfilled} backfilled, ${terminal} terminal` + (deferred ? ", deferred (rate-limited)" : ""), ); - return { backfilled, failed, deferred }; + return { backfilled, terminal, deferred }; } diff --git a/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts b/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts deleted file mode 100644 index 4f7d5c2a8..000000000 --- a/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { insertSongstatsQuotaLedger } from "../insertSongstatsQuotaLedger"; -import supabase from "../../serverClient"; - -vi.mock("../../serverClient", () => { - const mockFrom = vi.fn(); - const mockRpc = vi.fn(); - return { default: { from: mockFrom, rpc: mockRpc } }; -}); - -describe("insertSongstatsQuotaLedger", () => { - beforeEach(() => vi.clearAllMocks()); - - it("records spent hits", async () => { - const insert = vi.fn().mockResolvedValue({ error: null }); - vi.mocked(supabase.from).mockReturnValue({ insert } as never); - - await insertSongstatsQuotaLedger({ hits: 1, purpose: "backfill USA2P2015959" }); - - expect(supabase.from).toHaveBeenCalledWith("songstats_quota_ledger"); - expect(insert).toHaveBeenCalledWith([{ hits: 1, purpose: "backfill USA2P2015959" }]); - }); - - it("throws on insert error (spend must be recorded)", async () => { - const insert = vi.fn().mockResolvedValue({ error: { message: "boom" } }); - vi.mocked(supabase.from).mockReturnValue({ insert } as never); - - await expect(insertSongstatsQuotaLedger({ hits: 1 })).rejects.toThrow( - "Failed to record songstats quota spend: boom", - ); - }); -}); diff --git a/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts b/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts deleted file mode 100644 index 6d59c6267..000000000 --- a/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { selectSongstatsQuotaSpent } from "../selectSongstatsQuotaSpent"; -import supabase from "../../serverClient"; - -vi.mock("../../serverClient", () => { - const mockFrom = vi.fn(); - const mockRpc = vi.fn(); - return { default: { from: mockFrom, rpc: mockRpc } }; -}); - -describe("selectSongstatsQuotaSpent", () => { - beforeEach(() => vi.clearAllMocks()); - - it("sums hits spent since the window start", async () => { - const gte = vi.fn().mockResolvedValue({ data: [{ hits: 3 }, { hits: 7 }], error: null }); - const select = vi.fn().mockReturnValue({ gte }); - vi.mocked(supabase.from).mockReturnValue({ select } as never); - - const result = await selectSongstatsQuotaSpent("2026-05-12T00:00:00Z"); - - expect(supabase.from).toHaveBeenCalledWith("songstats_quota_ledger"); - expect(gte).toHaveBeenCalledWith("spent_at", "2026-05-12T00:00:00Z"); - expect(result).toBe(10); - }); - - it("treats errors as full quota spent (fail safe — do not overspend)", async () => { - const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); - const gte = vi.fn().mockResolvedValue({ data: null, error: { message: "boom" } }); - const select = vi.fn().mockReturnValue({ gte }); - vi.mocked(supabase.from).mockReturnValue({ select } as never); - - expect(await selectSongstatsQuotaSpent("2026-05-12T00:00:00Z")).toBe(Number.MAX_SAFE_INTEGER); - consoleError.mockRestore(); - }); -}); diff --git a/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts b/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts deleted file mode 100644 index 777c42a79..000000000 --- a/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts +++ /dev/null @@ -1,19 +0,0 @@ -import supabase from "../serverClient"; -import { TablesInsert } from "@/types/database.types"; - -/** - * Record Songstats quota spend. Throws on failure — unrecorded spend would - * let the worker silently blow the rolling-window budget. - * - * @param entry - hits spent, optional purpose/account attribution - * @throws Error if the insert fails - */ -export async function insertSongstatsQuotaLedger( - entry: TablesInsert<"songstats_quota_ledger">, -): Promise { - const { error } = await supabase.from("songstats_quota_ledger").insert([entry]); - - if (error) { - throw new Error(`Failed to record songstats quota spend: ${error.message}`); - } -} diff --git a/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts b/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts deleted file mode 100644 index 0610a4b27..000000000 --- a/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts +++ /dev/null @@ -1,23 +0,0 @@ -import supabase from "../serverClient"; - -/** - * Sum Songstats hits spent since a window start (the rolling 30-day quota - * check). Errors are treated as the full quota being spent — failing safe: - * the worker must never overspend because the ledger was unreadable. - * - * @param since - Inclusive ISO lower bound of the window - * @returns Total hits spent in the window - */ -export async function selectSongstatsQuotaSpent(since: string): Promise { - const { data, error } = await supabase - .from("songstats_quota_ledger") - .select("hits") - .gte("spent_at", since); - - if (error) { - console.error("Error reading songstats quota ledger:", error); - return Number.MAX_SAFE_INTEGER; - } - - return (data || []).reduce((sum, row) => sum + row.hits, 0); -} From f24d4c7ecaf4469c234378916dafffddc1425416 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 15:06:39 -0500 Subject: [PATCH 04/12] feat: POST /api/catalogs (create + materialize from valuation snapshot) (#677) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: POST /api/catalogs create + materialize from valuation snapshot Creates a catalog owned by the authenticated account (account derived from credentials via validateAuthContext, never the body). With from.snapshot_id, materializes the catalog from a completed valuation snapshot: creates the catalogs row, links account_catalogs, adds the snapshot's measured ISRCs as catalog_songs, and records the catalog on the snapshot. Re-claiming the same snapshot is idempotent. TDD: validateCreateCatalogBody (6 tests) + createCatalogHandler (8 tests), red->green. New supabase wrappers: insertCatalog, selectCatalogById, insertAccountCatalog, updateSnapshotCatalog. Implements recoupable/chat#1801 Phase 2. Matches docs contract recoupable/docs#243. Co-Authored-By: Claude Opus 4.8 (1M context) * refactor: re-anchor POST /api/catalogs to merged contract + review fixes - Flatten request to the merged docs#243 contract: from:{snapshot_id} -> a root snapshot field (validator + handler + tests). Error copy follows. - DRY/SRP: drop the inline success() helper; use the shared successResponse(). - KISS rename: materializeSnapshotCatalog.ts -> createSnapshotCatalog.ts. - DRY: delete the redundant updateSnapshotCatalog helper; reuse the existing updatePlaycountSnapshot(id, fields). Validator change done red->green. lib/catalog: 24 tests pass; tsc + eslint clean. Addresses review on PR #677. * fix: materialize catalog songs from song_measurements, not snapshot.isrcs Testing the full materialize path surfaced a real bug: a valuation snapshot is album_ids-scoped, so its own isrcs column is null — createSnapshotCatalog read snapshot.isrcs and would link an EMPTY catalog. The measured ISRCs live in song_measurements (snapshot lineage), so source them there. New selectSnapshotIsrcs(snapshotId) helper (distinct song_measurements.song for the snapshot). createSnapshotCatalog now uses it. TDD: new createSnapshotCatalog.test.ts (3 tests) red->green; lib/catalog 27 pass. Addresses PR #677 verification. * refactor: reuse selectSongMeasurements (snapshot filter) instead of a new helper KISS/DRY per review: drop selectSnapshotIsrcs; add an optional snapshot filter to the existing selectSongMeasurements, and derive distinct ISRCs in createSnapshotCatalog. lib/catalog + song_measurements: 36 tests pass. Addresses review on PR #677. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- app/api/catalogs/route.ts | 26 +++ .../__tests__/createCatalogHandler.test.ts | 160 ++++++++++++++++++ .../__tests__/createSnapshotCatalog.test.ts | 92 ++++++++++ .../validateCreateCatalogBody.test.ts | 50 ++++++ lib/catalog/createCatalogHandler.ts | 74 ++++++++ lib/catalog/createSnapshotCatalog.ts | 45 +++++ lib/catalog/validateCreateCatalogBody.ts | 47 +++++ .../account_catalogs/insertAccountCatalog.ts | 21 +++ lib/supabase/catalogs/insertCatalog.ts | 19 +++ lib/supabase/catalogs/selectCatalogById.ts | 19 +++ .../selectSongMeasurements.ts | 14 +- 11 files changed, 562 insertions(+), 5 deletions(-) create mode 100644 app/api/catalogs/route.ts create mode 100644 lib/catalog/__tests__/createCatalogHandler.test.ts create mode 100644 lib/catalog/__tests__/createSnapshotCatalog.test.ts create mode 100644 lib/catalog/__tests__/validateCreateCatalogBody.test.ts create mode 100644 lib/catalog/createCatalogHandler.ts create mode 100644 lib/catalog/createSnapshotCatalog.ts create mode 100644 lib/catalog/validateCreateCatalogBody.ts create mode 100644 lib/supabase/account_catalogs/insertAccountCatalog.ts create mode 100644 lib/supabase/catalogs/insertCatalog.ts create mode 100644 lib/supabase/catalogs/selectCatalogById.ts diff --git a/app/api/catalogs/route.ts b/app/api/catalogs/route.ts new file mode 100644 index 000000000..5ab6c6692 --- /dev/null +++ b/app/api/catalogs/route.ts @@ -0,0 +1,26 @@ +import { NextRequest, NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { createCatalogHandler } from "@/lib/catalog/createCatalogHandler"; + +/** + * OPTIONS handler for CORS preflight requests. + * + * @returns A NextResponse with CORS headers. + */ +export async function OPTIONS() { + return new NextResponse(null, { + status: 200, + headers: getCorsHeaders(), + }); +} + +/** + * POST handler for creating a catalog (optionally materialized from a + * valuation snapshot). + * + * @param request - The request object containing the catalog body. + * @returns A NextResponse with the created catalog. + */ +export async function POST(request: NextRequest) { + return createCatalogHandler(request); +} diff --git a/lib/catalog/__tests__/createCatalogHandler.test.ts b/lib/catalog/__tests__/createCatalogHandler.test.ts new file mode 100644 index 000000000..5e2b1ebf0 --- /dev/null +++ b/lib/catalog/__tests__/createCatalogHandler.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; + +import { createCatalogHandler } from "../createCatalogHandler"; +import { validateCreateCatalogBody } from "../validateCreateCatalogBody"; +import { createSnapshotCatalog } from "../createSnapshotCatalog"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { selectPlaycountSnapshots } from "@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots"; +import { selectCatalogById } from "@/lib/supabase/catalogs/selectCatalogById"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; + +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: vi.fn(() => ({ "Access-Control-Allow-Origin": "*" })), +})); +vi.mock("../validateCreateCatalogBody", () => ({ validateCreateCatalogBody: vi.fn() })); +vi.mock("../createSnapshotCatalog", () => ({ createSnapshotCatalog: vi.fn() })); +vi.mock("@/lib/auth/validateAuthContext", () => ({ validateAuthContext: vi.fn() })); +vi.mock("@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots", () => ({ + selectPlaycountSnapshots: vi.fn(), +})); +vi.mock("@/lib/supabase/catalogs/selectCatalogById", () => ({ selectCatalogById: vi.fn() })); +vi.mock("@/lib/supabase/catalogs/insertCatalog", () => ({ insertCatalog: vi.fn() })); +vi.mock("@/lib/supabase/account_catalogs/insertAccountCatalog", () => ({ + insertAccountCatalog: vi.fn(), +})); + +const accountId = "550e8400-e29b-41d4-a716-446655440000"; +const otherAccountId = "550e8400-e29b-41d4-a716-446655440999"; +const snapshotId = "11111111-2222-3333-4444-555555555555"; +const catalogId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + +const catalog = { + id: catalogId, + name: "Bad Bunny — Catalog", + created_at: "2026-06-18T00:00:00Z", + updated_at: "2026-06-18T00:00:00Z", +}; + +const makeRequest = () => new NextRequest("http://localhost/api/catalogs", { method: "POST" }); + +const okAuth = () => + vi.mocked(validateAuthContext).mockResolvedValue({ accountId, orgId: null, authToken: "t" }); + +describe("createCatalogHandler", () => { + beforeEach(() => vi.clearAllMocks()); + + it("short-circuits with the validator error and never authenticates", async () => { + const err = NextResponse.json({ status: "error" }, { status: 400 }); + vi.mocked(validateCreateCatalogBody).mockReturnValue(err); + + const res = await createCatalogHandler(makeRequest()); + + expect(res).toBe(err); + expect(validateAuthContext).not.toHaveBeenCalled(); + }); + + it("short-circuits with the auth error when unauthenticated", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "X" }); + const authErr = NextResponse.json({ status: "error" }, { status: 401 }); + vi.mocked(validateAuthContext).mockResolvedValue(authErr); + + const res = await createCatalogHandler(makeRequest()); + + expect(res).toBe(authErr); + expect(insertCatalog).not.toHaveBeenCalled(); + }); + + it("creates an empty catalog from a name-only body and links the account", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "Bad Bunny — Catalog" }); + okAuth(); + vi.mocked(insertCatalog).mockResolvedValue(catalog); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(insertCatalog).toHaveBeenCalledWith("Bad Bunny — Catalog"); + expect(insertAccountCatalog).toHaveBeenCalledWith({ account: accountId, catalog: catalogId }); + expect(body).toEqual({ status: "success", catalog, songs_added: 0 }); + expect(selectPlaycountSnapshots).not.toHaveBeenCalled(); + }); + + it("materializes a catalog from an owned, not-yet-claimed snapshot", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: accountId, catalog: null, isrcs: ["A", "B"] } as never, + ]); + vi.mocked(createSnapshotCatalog).mockResolvedValue({ catalog, songsAdded: 2 }); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(createSnapshotCatalog).toHaveBeenCalledWith({ + accountId, + snapshot: expect.objectContaining({ id: snapshotId }), + name: "Bad Bunny — Catalog", + }); + expect(body).toEqual({ status: "success", catalog, songs_added: 2 }); + }); + + it("returns 404 when the snapshot does not exist", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([]); + + const res = await createCatalogHandler(makeRequest()); + + expect(res.status).toBe(404); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + }); + + it("returns 403 when the snapshot belongs to a different account", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: otherAccountId, catalog: null, isrcs: ["A"] } as never, + ]); + + const res = await createCatalogHandler(makeRequest()); + + expect(res.status).toBe(403); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + }); + + it("is idempotent: returns the existing catalog when the snapshot is already claimed", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: accountId, catalog: catalogId, isrcs: ["A", "B"] } as never, + ]); + vi.mocked(selectCatalogById).mockResolvedValue(catalog); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(selectCatalogById).toHaveBeenCalledWith(catalogId); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + expect(body).toEqual({ status: "success", catalog, songs_added: 0 }); + }); + + it("returns a generic 500 without leaking the underlying error", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "X" }); + okAuth(); + vi.mocked(insertCatalog).mockRejectedValue(new Error("db down at 10.0.0.1:5432")); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(500); + expect(body.status).toBe("error"); + expect(JSON.stringify(body)).not.toContain("10.0.0.1"); + }); +}); diff --git a/lib/catalog/__tests__/createSnapshotCatalog.test.ts b/lib/catalog/__tests__/createSnapshotCatalog.test.ts new file mode 100644 index 000000000..9ee7a3ede --- /dev/null +++ b/lib/catalog/__tests__/createSnapshotCatalog.test.ts @@ -0,0 +1,92 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { createSnapshotCatalog } from "../createSnapshotCatalog"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; +import { insertCatalogSongs } from "@/lib/supabase/catalog_songs/insertCatalogSongs"; +import { updatePlaycountSnapshot } from "@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot"; +import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; + +vi.mock("@/lib/supabase/catalogs/insertCatalog", () => ({ insertCatalog: vi.fn() })); +vi.mock("@/lib/supabase/account_catalogs/insertAccountCatalog", () => ({ + insertAccountCatalog: vi.fn(), +})); +vi.mock("@/lib/supabase/catalog_songs/insertCatalogSongs", () => ({ insertCatalogSongs: vi.fn() })); +vi.mock("@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot", () => ({ + updatePlaycountSnapshot: vi.fn(), +})); +vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ + selectSongMeasurements: vi.fn(), +})); + +const accountId = "550e8400-e29b-41d4-a716-446655440000"; +const snapshotId = "11111111-2222-3333-4444-555555555555"; +const catalogId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; +const catalog = { id: catalogId, name: "Bad Bunny Catalog", created_at: "t", updated_at: "t" }; +// A real valuation snapshot is scoped by album_ids, so its own `isrcs` column is null — +// the measured ISRCs live in song_measurements (sourced via selectSongMeasurements). +const snapshot = { id: snapshotId, account: accountId, catalog: null, isrcs: null } as never; +const measurement = (song: string) => ({ song }) as never; + +describe("createSnapshotCatalog", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(insertCatalog).mockResolvedValue(catalog); + }); + + it("sources measured ISRCs from song_measurements (by snapshot) and adds them as catalog songs", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([ + measurement("ISRC_A"), + measurement("ISRC_B"), + measurement("ISRC_C"), + ]); + + const result = await createSnapshotCatalog({ accountId, snapshot, name: "Bad Bunny Catalog" }); + + expect(insertCatalog).toHaveBeenCalledWith("Bad Bunny Catalog"); + expect(insertAccountCatalog).toHaveBeenCalledWith({ account: accountId, catalog: catalogId }); + // ISRCs come from measurements by snapshot, NOT snapshot.isrcs (null here) + expect(selectSongMeasurements).toHaveBeenCalledWith({ snapshot: snapshotId }); + expect(insertCatalogSongs).toHaveBeenCalledWith([ + { catalog: catalogId, song: "ISRC_A" }, + { catalog: catalogId, song: "ISRC_B" }, + { catalog: catalogId, song: "ISRC_C" }, + ]); + expect(updatePlaycountSnapshot).toHaveBeenCalledWith(snapshotId, { catalog: catalogId }); + expect(result).toEqual({ catalog, songsAdded: 3 }); + }); + + it("dedupes ISRCs across multiple measurement rows per track", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([ + measurement("ISRC_A"), + measurement("ISRC_A"), + measurement("ISRC_B"), + ]); + + const result = await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalogSongs).toHaveBeenCalledWith([ + { catalog: catalogId, song: "ISRC_A" }, + { catalog: catalogId, song: "ISRC_B" }, + ]); + expect(result).toEqual({ catalog, songsAdded: 2 }); + }); + + it("adds no songs when the snapshot has no measurements", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([]); + + const result = await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalogSongs).not.toHaveBeenCalled(); + expect(updatePlaycountSnapshot).toHaveBeenCalledWith(snapshotId, { catalog: catalogId }); + expect(result).toEqual({ catalog, songsAdded: 0 }); + }); + + it("falls back to a default name when none is supplied", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([]); + + await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalog).toHaveBeenCalledWith("Valuation Catalog"); + }); +}); diff --git a/lib/catalog/__tests__/validateCreateCatalogBody.test.ts b/lib/catalog/__tests__/validateCreateCatalogBody.test.ts new file mode 100644 index 000000000..670895f6a --- /dev/null +++ b/lib/catalog/__tests__/validateCreateCatalogBody.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from "vitest"; +import { NextResponse } from "next/server"; + +import { validateCreateCatalogBody } from "../validateCreateCatalogBody"; + +const snapshotId = "550e8400-e29b-41d4-a716-446655440000"; + +describe("validateCreateCatalogBody", () => { + it("accepts a name-only body", () => { + const result = validateCreateCatalogBody({ name: "My Catalog" }); + expect(result).toEqual({ name: "My Catalog" }); + }); + + it("accepts a snapshot-only body", () => { + const result = validateCreateCatalogBody({ snapshot: snapshotId }); + expect(result).toEqual({ snapshot: snapshotId }); + }); + + it("accepts name and snapshot together", () => { + const result = validateCreateCatalogBody({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + expect(result).toEqual({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + }); + + it("rejects an empty body (neither name nor snapshot) with 400", async () => { + const result = validateCreateCatalogBody({}); + expect(result).toBeInstanceOf(NextResponse); + const res = result as NextResponse; + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.status).toBe("error"); + }); + + it("rejects a non-uuid snapshot with 400", async () => { + const result = validateCreateCatalogBody({ snapshot: "not-a-uuid" }); + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + }); + + it("rejects a null/non-object body with 400", async () => { + const result = validateCreateCatalogBody(null); + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + }); +}); diff --git a/lib/catalog/createCatalogHandler.ts b/lib/catalog/createCatalogHandler.ts new file mode 100644 index 000000000..583746971 --- /dev/null +++ b/lib/catalog/createCatalogHandler.ts @@ -0,0 +1,74 @@ +import { NextRequest, NextResponse } from "next/server"; +import { errorResponse } from "@/lib/networking/errorResponse"; +import { successResponse } from "@/lib/networking/successResponse"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { validateCreateCatalogBody } from "./validateCreateCatalogBody"; +import { createSnapshotCatalog } from "./createSnapshotCatalog"; +import { selectPlaycountSnapshots } from "@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots"; +import { selectCatalogById } from "@/lib/supabase/catalogs/selectCatalogById"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; + +const DEFAULT_CATALOG_NAME = "Valuation Catalog"; + +/** + * POST /api/catalogs + * + * Creates a catalog owned by the authenticated account. The owning account is + * resolved from credentials (Privy bearer or x-api-key), never from the body. + * + * With `snapshot`, materializes the catalog from a completed valuation snapshot: + * the snapshot must be owned by the caller, and re-claiming the same snapshot is + * idempotent (returns the catalog already created for that run). + * + * @param request - The request object + * @returns A NextResponse with `{ status, catalog, songs_added }` + */ +export async function createCatalogHandler(request: NextRequest): Promise { + try { + const body = await request.json().catch(() => null); + + const validated = validateCreateCatalogBody(body); + if (validated instanceof NextResponse) { + return validated; + } + + const authResult = await validateAuthContext(request); + if (authResult instanceof NextResponse) { + return authResult; + } + const { accountId } = authResult; + + if (!validated.snapshot) { + const catalog = await insertCatalog(validated.name ?? DEFAULT_CATALOG_NAME); + await insertAccountCatalog({ account: accountId, catalog: catalog.id }); + return successResponse({ catalog, songs_added: 0 }); + } + + const [snapshot] = await selectPlaycountSnapshots({ id: validated.snapshot }); + if (!snapshot) { + return errorResponse("Snapshot not found", 404); + } + if (snapshot.account !== accountId) { + return errorResponse("Snapshot belongs to a different account", 403); + } + + // Idempotent re-claim: the run already produced a catalog. + if (snapshot.catalog) { + const existing = await selectCatalogById(snapshot.catalog); + if (existing) { + return successResponse({ catalog: existing, songs_added: 0 }); + } + } + + const { catalog, songsAdded } = await createSnapshotCatalog({ + accountId, + snapshot, + name: validated.name, + }); + return successResponse({ catalog, songs_added: songsAdded }); + } catch (error) { + console.error("Error creating catalog:", error); + return errorResponse("Internal server error", 500); + } +} diff --git a/lib/catalog/createSnapshotCatalog.ts b/lib/catalog/createSnapshotCatalog.ts new file mode 100644 index 000000000..ab1ba96c2 --- /dev/null +++ b/lib/catalog/createSnapshotCatalog.ts @@ -0,0 +1,45 @@ +import { Tables } from "@/types/database.types"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; +import { insertCatalogSongs } from "@/lib/supabase/catalog_songs/insertCatalogSongs"; +import { updatePlaycountSnapshot } from "@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot"; +import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; + +const DEFAULT_CATALOG_NAME = "Valuation Catalog"; + +/** + * Creates an account-linked catalog from a valuation snapshot: creates the + * `catalogs` row, links it to the account via `account_catalogs`, adds the + * snapshot's **measured** ISRCs (from `song_measurements`, not the snapshot's + * own `isrcs` column — that's null for album-scoped valuation runs) as + * `catalog_songs`, and records the new catalog on the snapshot (the idempotency + * key for re-claims). + * + * Callers must first confirm the snapshot is owned by `accountId` and not yet + * claimed (`snapshot.catalog` is null). + * + * @param params.accountId - Owning account (already authorized) + * @param params.snapshot - The owned, unclaimed snapshot row + * @param params.name - Optional catalog name; falls back to a default + * @returns The created catalog and the number of songs added + */ +export async function createSnapshotCatalog(params: { + accountId: string; + snapshot: Tables<"playcount_snapshots">; + name?: string; +}): Promise<{ catalog: Tables<"catalogs">; songsAdded: number }> { + const { accountId, snapshot, name } = params; + + const catalog = await insertCatalog(name ?? DEFAULT_CATALOG_NAME); + await insertAccountCatalog({ account: accountId, catalog: catalog.id }); + + const measurements = await selectSongMeasurements({ snapshot: snapshot.id }); + const isrcs = [...new Set(measurements.map(m => m.song))]; + if (isrcs.length > 0) { + await insertCatalogSongs(isrcs.map(isrc => ({ catalog: catalog.id, song: isrc }))); + } + + await updatePlaycountSnapshot(snapshot.id, { catalog: catalog.id }); + + return { catalog, songsAdded: isrcs.length }; +} diff --git a/lib/catalog/validateCreateCatalogBody.ts b/lib/catalog/validateCreateCatalogBody.ts new file mode 100644 index 000000000..b41c058c1 --- /dev/null +++ b/lib/catalog/validateCreateCatalogBody.ts @@ -0,0 +1,47 @@ +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { z } from "zod"; + +export const createCatalogBodySchema = z + .object({ + name: z.string().min(1, "name must not be empty").optional(), + snapshot: z.string().uuid("snapshot must be a valid UUID").optional(), + }) + .refine(data => data.name !== undefined || data.snapshot !== undefined, { + message: "Provide at least one of name or snapshot", + }); + +export type CreateCatalogBody = z.infer; + +/** + * Validates a create-catalog request body. + * + * Accepts `{ name?, snapshot? }`; at least one is required. `snapshot` is a + * completed playcount snapshot id (valuation run) to materialize from. The + * owning account is never taken from the body - it is resolved from the + * request credentials by the handler. + * + * @param body - The parsed request body to validate. + * @returns A NextResponse with a 400 error if validation fails, or the + * validated body if it passes. + */ +export function validateCreateCatalogBody(body: unknown): NextResponse | CreateCatalogBody { + const result = createCatalogBodySchema.safeParse(body); + + if (!result.success) { + const firstError = result.error.issues[0]; + return NextResponse.json( + { + status: "error", + missing_fields: firstError.path, + error: firstError.message, + }, + { + status: 400, + headers: getCorsHeaders(), + }, + ); + } + + return result.data; +} diff --git a/lib/supabase/account_catalogs/insertAccountCatalog.ts b/lib/supabase/account_catalogs/insertAccountCatalog.ts new file mode 100644 index 000000000..1facd5b63 --- /dev/null +++ b/lib/supabase/account_catalogs/insertAccountCatalog.ts @@ -0,0 +1,21 @@ +import supabase from "../serverClient"; + +/** + * Links a catalog to an account by inserting an `account_catalogs` row. + * + * @param params.account - Account id (owner) + * @param params.catalog - Catalog id to link + * @throws Error if the insert fails + */ +export async function insertAccountCatalog(params: { + account: string; + catalog: string; +}): Promise { + const { error } = await supabase + .from("account_catalogs") + .insert({ account: params.account, catalog: params.catalog }); + + if (error) { + throw new Error(`Failed to link account_catalogs: ${error.message}`); + } +} diff --git a/lib/supabase/catalogs/insertCatalog.ts b/lib/supabase/catalogs/insertCatalog.ts new file mode 100644 index 000000000..ecd0c667b --- /dev/null +++ b/lib/supabase/catalogs/insertCatalog.ts @@ -0,0 +1,19 @@ +import supabase from "../serverClient"; +import { Tables } from "@/types/database.types"; + +/** + * Inserts a new `catalogs` row and returns it. + * + * @param name - Display name for the catalog + * @returns The inserted catalog row + * @throws Error if the insert fails + */ +export async function insertCatalog(name: string): Promise> { + const { data, error } = await supabase.from("catalogs").insert({ name }).select().single(); + + if (error || !data) { + throw new Error(`Failed to insert catalog: ${error?.message ?? "no row returned"}`); + } + + return data; +} diff --git a/lib/supabase/catalogs/selectCatalogById.ts b/lib/supabase/catalogs/selectCatalogById.ts new file mode 100644 index 000000000..39f961e3d --- /dev/null +++ b/lib/supabase/catalogs/selectCatalogById.ts @@ -0,0 +1,19 @@ +import supabase from "../serverClient"; +import { Tables } from "@/types/database.types"; + +/** + * Selects a single `catalogs` row by id, or null if none exists. + * + * @param id - Catalog id + * @returns The catalog row, or null if not found + * @throws Error if the query fails + */ +export async function selectCatalogById(id: string): Promise | null> { + const { data, error } = await supabase.from("catalogs").select("*").eq("id", id).maybeSingle(); + + if (error) { + throw new Error(`Failed to fetch catalog: ${error.message}`); + } + + return data; +} diff --git a/lib/supabase/song_measurements/selectSongMeasurements.ts b/lib/supabase/song_measurements/selectSongMeasurements.ts index 24d50bd44..1e118d14b 100644 --- a/lib/supabase/song_measurements/selectSongMeasurements.ts +++ b/lib/supabase/song_measurements/selectSongMeasurements.ts @@ -2,13 +2,14 @@ import supabase from "../serverClient"; import { Tables } from "@/types/database.types"; /** - * Select measurements newest-first. Filter by one song or a batch of songs - * (one is required), and optionally by platform/metric. Uses the - * (song, platform, metric, captured_at DESC) series index; pass `limit: 1` - * for the latest capture, omit for the full series. + * Select measurements newest-first. Filter by one song, a batch of songs, or a + * snapshot (one of the three is required), and optionally by platform/metric. + * Uses the (song, platform, metric, captured_at DESC) series index; pass + * `limit: 1` for the latest capture, omit for the full series. * * @param params.song - Single song ISRC filter * @param params.songs - Batch of song ISRCs (e.g. all tracks on an album) + * @param params.snapshot - Snapshot id filter (all measurements captured for a run) * @param params.platform - Optional platform filter (e.g. "spotify") * @param params.metric - Optional metric filter (e.g. "platform_displayed_play_count") * @param params.limit - Optional cap on returned rows @@ -17,17 +18,19 @@ import { Tables } from "@/types/database.types"; export async function selectSongMeasurements({ song, songs, + snapshot, platform, metric, limit, }: { song?: string; songs?: string[]; + snapshot?: string; platform?: string; metric?: string; limit?: number; }): Promise[]> { - if (!song && (!songs || songs.length === 0)) return []; + if (!song && (!songs || songs.length === 0) && !snapshot) return []; let query = supabase .from("song_measurements") @@ -36,6 +39,7 @@ export async function selectSongMeasurements({ if (song) query = query.eq("song", song); if (songs && songs.length > 0) query = query.in("song", songs); + if (snapshot) query = query.eq("snapshot", snapshot); if (platform) query = query.eq("platform", platform); if (metric) query = query.eq("metric", metric); if (limit) query = query.limit(limit); From a520a1ddac9b7dd4f3bf9d040c2270ecdb7c1dbe Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 17:00:50 -0500 Subject: [PATCH 05/12] fix: LEFT-join artists in catalog-songs read (materialized tracks were hidden) (#681) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: LEFT-join artists in catalog-songs read so materialized tracks surface selectCatalogSongsWithArtists used song_artists!inner -> accounts!inner, so valuation-captured tracks (which have songs + song_measurements but no song_artists yet) were filtered out — a materialized catalog read back as 0 songs (verified live on api#677). Drop the two !inner so artist-less songs return with artists: []; songs!inner stays (catalog_songs.song FK guarantees it). Closes the read-path half of the song_artists follow-up in recoupable/chat#1801. Longer-term (option a): the capture pipeline should also write song_artists. * Update lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts --- lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts b/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts index 8639eb405..47cc14206 100644 --- a/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts +++ b/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts @@ -55,9 +55,9 @@ export async function selectCatalogSongsWithArtists( album, notes, updated_at, - song_artists!inner ( + song_artists ( artist, - accounts!inner ( + accounts ( id, name, timestamp From 709ea0cb339e425cdf4b27cbbd34bfaccf374319 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 17:58:44 -0500 Subject: [PATCH 06/12] feat: add X (Twitter) + LinkedIn to the Composio connector whitelist (chat#1793) (#679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add X (Twitter) + LinkedIn to the Composio connector whitelist (chat#1793) Expand the existing whitelist pattern to two new platforms — no architecture changes: - SUPPORTED_TOOLKITS (getConnectors.ts) + ENABLED_TOOLKITS (getComposioTools.ts) - CONNECTOR_DISPLAY_NAMES: twitter → "X (Twitter)", linkedin → "LinkedIn" - buildAuthConfigs() reads COMPOSIO_TWITTER_AUTH_CONFIG_ID + COMPOSIO_LINKEDIN_AUTH_CONFIG_ID - document both env vars in .env.example TDD: new buildAuthConfigs unit + expanded getConnectors / handler / ENABLED_TOOLKITS assertions, RED before GREEN. Full lib/composio suite green (157 tests). Implements the contract from docs#244. Co-Authored-By: Claude Opus 4.8 (1M context) * chore: fix lint/format — relocate ENABLED_TOOLKITS test block, reformat toolkit array - Move the ENABLED_TOOLKITS describe block below the imports (import/first) - Prettier-format the expanded toolkits array in getConnectors.test.ts Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .env.example | 2 + .../__tests__/buildAuthConfigs.test.ts | 48 +++++++++++++++++++ .../__tests__/getConnectors.test.ts | 22 ++++++++- .../__tests__/getConnectorsHandler.test.ts | 2 + lib/composio/connectors/buildAuthConfigs.ts | 6 +++ lib/composio/connectors/getConnectors.ts | 2 + .../connectors/getConnectorsHandler.ts | 2 + .../__tests__/getComposioTools.test.ts | 9 +++- lib/composio/toolRouter/getComposioTools.ts | 2 + 9 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 lib/composio/connectors/__tests__/buildAuthConfigs.test.ts diff --git a/.env.example b/.env.example index 9d6de84d9..80bca6864 100644 --- a/.env.example +++ b/.env.example @@ -34,6 +34,8 @@ COMPOSIO_INSTAGRAM_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_SHEETS_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_DOCS_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_DRIVE_AUTH_CONFIG_ID= +COMPOSIO_TWITTER_AUTH_CONFIG_ID= +COMPOSIO_LINKEDIN_AUTH_CONFIG_ID= # Slack (coding agent) SLACK_BOT_TOKEN= diff --git a/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts b/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts new file mode 100644 index 000000000..451dc92ed --- /dev/null +++ b/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { buildAuthConfigs } from "../buildAuthConfigs"; + +const AUTH_CONFIG_ENV_KEYS = [ + "COMPOSIO_TIKTOK_AUTH_CONFIG_ID", + "COMPOSIO_INSTAGRAM_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_SHEETS_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_DOCS_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_DRIVE_AUTH_CONFIG_ID", + "COMPOSIO_YOUTUBE_AUTH_CONFIG_ID", + "COMPOSIO_TWITTER_AUTH_CONFIG_ID", + "COMPOSIO_LINKEDIN_AUTH_CONFIG_ID", +]; + +describe("buildAuthConfigs", () => { + let saved: Record; + + beforeEach(() => { + // Snapshot then clear every auth-config env var so the test is isolated + // from whatever .env.local provides. + saved = {}; + for (const key of AUTH_CONFIG_ENV_KEYS) { + saved[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + for (const key of AUTH_CONFIG_ENV_KEYS) { + if (saved[key] === undefined) delete process.env[key]; + else process.env[key] = saved[key]; + } + }); + + it("returns undefined when no auth-config env vars are set", () => { + expect(buildAuthConfigs()).toBeUndefined(); + }); + + it("reads the Twitter and LinkedIn auth configs from env", () => { + process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID = "ac_twitter_123"; + process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID = "ac_linkedin_456"; + + expect(buildAuthConfigs()).toEqual({ + twitter: "ac_twitter_123", + linkedin: "ac_linkedin_456", + }); + }); +}); diff --git a/lib/composio/connectors/__tests__/getConnectors.test.ts b/lib/composio/connectors/__tests__/getConnectors.test.ts index 762c07e07..b637f1026 100644 --- a/lib/composio/connectors/__tests__/getConnectors.test.ts +++ b/lib/composio/connectors/__tests__/getConnectors.test.ts @@ -37,7 +37,16 @@ describe("getConnectors", () => { expect(getComposioClient).toHaveBeenCalled(); expect(mockComposio.create).toHaveBeenCalledWith("account-123", { - toolkits: ["googlesheets", "googledrive", "googledocs", "tiktok", "instagram", "youtube"], + toolkits: [ + "googlesheets", + "googledrive", + "googledocs", + "tiktok", + "instagram", + "youtube", + "twitter", + "linkedin", + ], }); expect(result).toEqual([ { @@ -92,7 +101,16 @@ describe("getConnectors", () => { await getConnectors("account-123"); expect(mockComposio.create).toHaveBeenCalledWith("account-123", { - toolkits: ["googlesheets", "googledrive", "googledocs", "tiktok", "instagram", "youtube"], + toolkits: [ + "googlesheets", + "googledrive", + "googledocs", + "tiktok", + "instagram", + "youtube", + "twitter", + "linkedin", + ], authConfigs: { tiktok: "ac_tiktok_123", instagram: "ac_instagram_456", diff --git a/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts b/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts index d6c3fe31f..bc68d0af6 100644 --- a/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts +++ b/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts @@ -72,6 +72,8 @@ describe("getConnectorsHandler", () => { googlesheets: "Google Sheets", googledrive: "Google Drive", googledocs: "Google Docs", + twitter: "X (Twitter)", + linkedin: "LinkedIn", }, }); }); diff --git a/lib/composio/connectors/buildAuthConfigs.ts b/lib/composio/connectors/buildAuthConfigs.ts index 2c31d571a..4edb893c0 100644 --- a/lib/composio/connectors/buildAuthConfigs.ts +++ b/lib/composio/connectors/buildAuthConfigs.ts @@ -23,5 +23,11 @@ export function buildAuthConfigs(): Record | undefined { if (process.env.COMPOSIO_YOUTUBE_AUTH_CONFIG_ID) { configs.youtube = process.env.COMPOSIO_YOUTUBE_AUTH_CONFIG_ID; } + if (process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID) { + configs.twitter = process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID; + } + if (process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID) { + configs.linkedin = process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID; + } return Object.keys(configs).length > 0 ? configs : undefined; } diff --git a/lib/composio/connectors/getConnectors.ts b/lib/composio/connectors/getConnectors.ts index dcb5fb38e..d692bf40c 100644 --- a/lib/composio/connectors/getConnectors.ts +++ b/lib/composio/connectors/getConnectors.ts @@ -23,6 +23,8 @@ const SUPPORTED_TOOLKITS = [ "tiktok", "instagram", "youtube", + "twitter", + "linkedin", ]; /** diff --git a/lib/composio/connectors/getConnectorsHandler.ts b/lib/composio/connectors/getConnectorsHandler.ts index d92f1293d..86633e32b 100644 --- a/lib/composio/connectors/getConnectorsHandler.ts +++ b/lib/composio/connectors/getConnectorsHandler.ts @@ -12,6 +12,8 @@ const CONNECTOR_DISPLAY_NAMES: Record = { googlesheets: "Google Sheets", googledrive: "Google Drive", googledocs: "Google Docs", + twitter: "X (Twitter)", + linkedin: "LinkedIn", }; /** diff --git a/lib/composio/toolRouter/__tests__/getComposioTools.test.ts b/lib/composio/toolRouter/__tests__/getComposioTools.test.ts index f49aff1d1..ae8d5d718 100644 --- a/lib/composio/toolRouter/__tests__/getComposioTools.test.ts +++ b/lib/composio/toolRouter/__tests__/getComposioTools.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { getComposioTools } from "../getComposioTools"; +import { getComposioTools, ENABLED_TOOLKITS } from "../getComposioTools"; import { getComposioClient } from "../../client"; import { getCallbackUrl } from "../../getCallbackUrl"; @@ -235,3 +235,10 @@ describe("getComposioTools", () => { expect(result).toHaveProperty("COMPOSIO_MULTI_EXECUTE_TOOL"); }); }); + +describe("ENABLED_TOOLKITS", () => { + it("includes twitter and linkedin", () => { + expect(ENABLED_TOOLKITS).toContain("twitter"); + expect(ENABLED_TOOLKITS).toContain("linkedin"); + }); +}); diff --git a/lib/composio/toolRouter/getComposioTools.ts b/lib/composio/toolRouter/getComposioTools.ts index 61f0f887f..7f74b4c2f 100644 --- a/lib/composio/toolRouter/getComposioTools.ts +++ b/lib/composio/toolRouter/getComposioTools.ts @@ -16,6 +16,8 @@ export const ENABLED_TOOLKITS = [ "tiktok", "instagram", "youtube", + "twitter", + "linkedin", ]; const SHARED_ACCOUNT_ID = "recoup-shared-767f498e-e1e9-43c6-a152-a96ae3bd8d07"; From 66cc2fe2518ae526d15f12a70528de93410af815 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 18:30:11 -0500 Subject: [PATCH 07/12] chore: remove unused ALLOWED_ARTIST_CONNECTORS from api (chat#1793) (#680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: allow artists to connect X (Twitter); keep LinkedIn label-only (chat#1793) Add `twitter` to ALLOWED_ARTIST_CONNECTORS — artist-facing social, same class as tiktok/instagram/youtube. `linkedin` is intentionally left out (label/owner-only). TDD: isAllowedArtistConnector.test.ts asserts twitter allowed + linkedin excluded, RED before GREEN. Full lib/composio suite green (157 tests). Co-Authored-By: Claude Opus 4.8 (1M context) * feat: allow artists to connect LinkedIn too (chat#1793) Reversal of the earlier "LinkedIn label/owner-only" call: per owner decision 2026-06-18, LinkedIn is now an artist-facing connector like the others. Add `linkedin` to ALLOWED_ARTIST_CONNECTORS. TDD: flipped the linkedin assertions (now allowed/included), RED before GREEN. Full lib/composio suite green (159 tests). Co-Authored-By: Claude Opus 4.8 (1M context) * chore: remove unused ALLOWED_ARTIST_CONNECTORS from api (chat#1793) The api copy of the artist connector allow-list had no runtime consumer — only its definition, test, and an (also-unused) barrel re-export. The connector routes are unopinionated (allow any connector for any account); the allow-list that actually drives the artist Connectors tab lives in `chat` (`lib/composio/allowedArtistConnectors.ts`). Removing the dead code. Supersedes the earlier plan to add twitter/linkedin to this api constant (decision: owner, 2026-06-18) — the artist allow-list is chat-only. Deletes isAllowedArtistConnector.ts + its test, and the barrel re-export. lib/composio suite green (149); no new tsc errors vs test (198 baseline). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../isAllowedArtistConnector.test.ts | 43 ------------------- lib/composio/connectors/index.ts | 5 --- .../connectors/isAllowedArtistConnector.ts | 16 ------- 3 files changed, 64 deletions(-) delete mode 100644 lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts delete mode 100644 lib/composio/connectors/isAllowedArtistConnector.ts diff --git a/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts b/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts deleted file mode 100644 index 596407d20..000000000 --- a/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { isAllowedArtistConnector, ALLOWED_ARTIST_CONNECTORS } from "../isAllowedArtistConnector"; - -describe("isAllowedArtistConnector", () => { - it("should return true for 'tiktok'", () => { - expect(isAllowedArtistConnector("tiktok")).toBe(true); - }); - - it("should return true for 'instagram'", () => { - expect(isAllowedArtistConnector("instagram")).toBe(true); - }); - - it("should return true for 'youtube'", () => { - expect(isAllowedArtistConnector("youtube")).toBe(true); - }); - - it("should return false for connectors not in ALLOWED_ARTIST_CONNECTORS", () => { - expect(isAllowedArtistConnector("googlesheets")).toBe(false); - expect(isAllowedArtistConnector("googledrive")).toBe(false); - expect(isAllowedArtistConnector("random")).toBe(false); - }); - - it("should return false for empty string", () => { - expect(isAllowedArtistConnector("")).toBe(false); - }); - - it("should be case-sensitive", () => { - expect(isAllowedArtistConnector("TikTok")).toBe(false); - expect(isAllowedArtistConnector("TIKTOK")).toBe(false); - }); -}); - -describe("ALLOWED_ARTIST_CONNECTORS", () => { - it("should include tiktok, instagram, and youtube", () => { - expect(ALLOWED_ARTIST_CONNECTORS).toContain("tiktok"); - expect(ALLOWED_ARTIST_CONNECTORS).toContain("instagram"); - expect(ALLOWED_ARTIST_CONNECTORS).toContain("youtube"); - }); - - it("should be a readonly array", () => { - expect(Array.isArray(ALLOWED_ARTIST_CONNECTORS)).toBe(true); - }); -}); diff --git a/lib/composio/connectors/index.ts b/lib/composio/connectors/index.ts index 848b357ba..be0178edb 100644 --- a/lib/composio/connectors/index.ts +++ b/lib/composio/connectors/index.ts @@ -5,9 +5,4 @@ export { type AuthorizeConnectorOptions, } from "./authorizeConnector"; export { disconnectConnector, type DisconnectConnectorOptions } from "./disconnectConnector"; -export { - ALLOWED_ARTIST_CONNECTORS, - isAllowedArtistConnector, - type AllowedArtistConnector, -} from "./isAllowedArtistConnector"; export { verifyConnectorOwnership } from "./verifyConnectorOwnership"; diff --git a/lib/composio/connectors/isAllowedArtistConnector.ts b/lib/composio/connectors/isAllowedArtistConnector.ts deleted file mode 100644 index f3d83d82d..000000000 --- a/lib/composio/connectors/isAllowedArtistConnector.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * List of toolkit slugs that artists are allowed to connect. - * Only these connectors will be shown in the artist-connectors API. - */ -export const ALLOWED_ARTIST_CONNECTORS = ["tiktok", "instagram", "youtube"] as const; - -export type AllowedArtistConnector = (typeof ALLOWED_ARTIST_CONNECTORS)[number]; - -/** - * Check if a connector slug is an allowed artist connector. - * - * @param slug - */ -export function isAllowedArtistConnector(slug: string): slug is AllowedArtistConnector { - return (ALLOWED_ARTIST_CONNECTORS as readonly string[]).includes(slug); -} From 6fc10cc3659360ee582c045509004764e086d7be Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 18:52:05 -0500 Subject: [PATCH 08/12] fix: enrich valuation-captured songs (artists + notes) so they render in the catalog (#684) * fix: enrich captured songs with artists + notes (root cause) The valuation capture path created songs rows from the Spotify track lookup but discarded track.artists and never ran the manual flow's enrichment, so captured songs had no song_artists and no notes -> the chat catalog view's isCompleteSong filter (artist + notes required, on by default) hid every valuation track (count shown, list empty). mapUnmappedAlbumTracks now carries track.artists through and runs the same enrichment as processSongsInput: linkSongsToArtists (auto-creates the artist account) + queueRedisSongs (queues note generation). TDD: new test asserts artists are linked + queued; lib/research/playcounts + lib/songs 109 tests pass. Root-cause follow-up on recoupable/chat#1801. * style: prettier-format the capture-enrichment test Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/mapUnmappedAlbumTracks.test.ts | 30 ++++++++++++++++ .../playcounts/mapUnmappedAlbumTracks.ts | 34 ++++++++++++++++--- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts b/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts index 954a5851c..714d597f7 100644 --- a/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts +++ b/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts @@ -5,6 +5,8 @@ import generateAccessToken from "@/lib/spotify/generateAccessToken"; import getTracks from "@/lib/spotify/getTracks"; import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; +import { linkSongsToArtists } from "@/lib/songs/linkSongsToArtists"; +import { queueRedisSongs } from "@/lib/songs/queueRedisSongs"; import { SpotifyRateLimitError } from "@/lib/spotify/SpotifyRateLimitError"; vi.mock("@/lib/spotify/generateAccessToken", () => ({ default: vi.fn() })); @@ -13,6 +15,8 @@ vi.mock("@/lib/supabase/songs/upsertSongs", () => ({ upsertSongs: vi.fn() })); vi.mock("@/lib/supabase/song_identifiers/upsertSongIdentifiers", () => ({ upsertSongIdentifiers: vi.fn(), })); +vi.mock("@/lib/songs/linkSongsToArtists", () => ({ linkSongsToArtists: vi.fn() })); +vi.mock("@/lib/songs/queueRedisSongs", () => ({ queueRedisSongs: vi.fn() })); const ALBUMS = [ { @@ -55,6 +59,32 @@ describe("mapUnmappedAlbumTracks", () => { expect([...mapped.entries()]).toEqual([["t_new", "ISRC_NIKES"]]); }); + it("links captured songs to their Spotify artists and queues them for note enrichment", async () => { + vi.mocked(getTracks).mockResolvedValue({ + tracks: [ + { + id: "t_new", + name: "Nikes on My Feet", + external_ids: { isrc: "ISRC_NIKES" }, + artists: [{ id: "a1", name: "Mac Miller" }], + }, + ], + error: null, + } as never); + + await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped", "t_noisrc"])); + + // Root-cause fix: captured songs get the same enrichment as the manual flow — + // artists linked + queued for notes — so they aren't "missing info" in the catalog. + expect(linkSongsToArtists).toHaveBeenCalledWith([ + expect.objectContaining({ + isrc: "ISRC_NIKES", + spotifyArtists: [{ id: "a1", name: "Mac Miller" }], + }), + ]); + expect(queueRedisSongs).toHaveBeenCalledWith([expect.objectContaining({ isrc: "ISRC_NIKES" })]); + }); + it("returns an empty map without Spotify calls when everything is mapped", async () => { const mapped = await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped", "t_new", "t_noisrc"])); diff --git a/lib/research/playcounts/mapUnmappedAlbumTracks.ts b/lib/research/playcounts/mapUnmappedAlbumTracks.ts index e987b0376..e9fdb8fcd 100644 --- a/lib/research/playcounts/mapUnmappedAlbumTracks.ts +++ b/lib/research/playcounts/mapUnmappedAlbumTracks.ts @@ -4,6 +4,10 @@ import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; import { SpotifyAlbumPlayCounts } from "@/lib/apify/spotify/fetchSpotifyAlbumPlayCounts"; import { SpotifyRateLimitError } from "@/lib/spotify/SpotifyRateLimitError"; +import { getSpotifyArtists } from "@/lib/songs/getSpotifyArtists"; +import { linkSongsToArtists } from "@/lib/songs/linkSongsToArtists"; +import { queueRedisSongs } from "@/lib/songs/queueRedisSongs"; +import { SongWithSpotify } from "@/lib/songs/getSongsByIsrc"; /** * Self-mapping bootstrap (chat#1794): resolve ISRCs for actor tracks that have @@ -49,20 +53,35 @@ export async function mapUnmappedAlbumTracks( name: track.name, albumId: context.albumId, albumName: context.albumName, + spotifyArtists: getSpotifyArtists(track.artists ?? []), }, ]; }); if (resolved.length === 0) return new Map(); // Dedupe by ISRC: reissues put the same recording on several albums in one - // batch, and upsertSongs' DO UPDATE cannot affect the same row twice. - const songsByIsrc = new Map( + // batch, and upsertSongs' DO UPDATE cannot affect the same row twice. Carry + // the Spotify artists through for enrichment. + const songsByIsrc = new Map( resolved.map(r => [ r.isrc, - { isrc: r.isrc, name: r.name ?? null, album: r.albumName ?? null }, + { + isrc: r.isrc, + name: r.name ?? null, + album: r.albumName ?? null, + spotifyArtists: r.spotifyArtists, + }, ]), ); - await upsertSongs([...songsByIsrc.values()]); + const songs = [...songsByIsrc.values()]; + + await upsertSongs( + songs.map(song => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { spotifyArtists, ...record } = song; + return record; + }), + ); await upsertSongIdentifiers( resolved.flatMap(r => [ { song: r.isrc, platform: "spotify", identifier_type: "track_id", value: r.trackId }, @@ -72,6 +91,13 @@ export async function mapUnmappedAlbumTracks( ]), ); + // Root cause (chat#1801): give captured songs the same enrichment as the + // manual/CSV flow — link artists (auto-creating the artist account) and + // queue note generation — so valuation tracks aren't "missing info" and + // render in the catalog view rather than being filtered out. + await linkSongsToArtists(songs); + await queueRedisSongs(songs); + return new Map(resolved.map(r => [r.trackId, r.isrc])); } catch (error) { if (error instanceof SpotifyRateLimitError) throw error; From 2fa70298e159a192f63b19d59f581eddf936c282 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Fri, 19 Jun 2026 08:35:33 -0500 Subject: [PATCH 09/12] fix(tasks): let admins fetch any task by id alone (cross-account read) (#689) GET /api/tasks scopes every lookup to the caller's own account. A lookup by `id` alone therefore returns nothing when the caller's key doesn't own the task, which blocks the background worker (customer-prompt-task) from loading a customer's scheduled task config with a shared admin key. When an admin caller queries by `id` with no `account_id` param, drop the account scope so the single task is returned regardless of owner. Non-admin id lookups stay scoped to the authenticated account (no cross-account leak). ValidatedGetTasksQuery.account_id is now optional; selectScheduledActions already filters by account_id only when present. TDD: RED (admin id lookup not cross-account, non-admin not scoped) -> GREEN. Fixes part of recoupable/chat#1810. Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/validateGetTasksQuery.test.ts | 35 +++++++++++++++++++ lib/tasks/validateGetTasksQuery.ts | 16 +++++++-- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/lib/tasks/__tests__/validateGetTasksQuery.test.ts b/lib/tasks/__tests__/validateGetTasksQuery.test.ts index b9d0dda71..21722150a 100644 --- a/lib/tasks/__tests__/validateGetTasksQuery.test.ts +++ b/lib/tasks/__tests__/validateGetTasksQuery.test.ts @@ -119,6 +119,41 @@ describe("validateGetTasksQuery", () => { }); }); + it("lets an admin fetch any task by id alone, dropping account scope", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: "worker_admin_acc", + orgId: null, + authToken: "token", + }); + vi.mocked(checkIsAdmin).mockResolvedValue(true); + + const result = await validateGetTasksQuery( + createMockRequest("http://localhost:3000/api/tasks?id=task_owned_by_someone_else"), + ); + + expect(checkIsAdmin).toHaveBeenCalledWith("worker_admin_acc"); + expect(validateAccountIdOverride).not.toHaveBeenCalled(); + // No account_id key -> selectScheduledActions filters by id only (cross-account read). + expect(result).toEqual({ id: "task_owned_by_someone_else" }); + }); + + it("keeps a non-admin id lookup scoped to the authenticated account", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: "regular_acc", + orgId: null, + authToken: "token", + }); + vi.mocked(checkIsAdmin).mockResolvedValue(false); + + const result = await validateGetTasksQuery( + createMockRequest("http://localhost:3000/api/tasks?id=task_1"), + ); + + expect(checkIsAdmin).toHaveBeenCalledWith("regular_acc"); + expect(validateAccountIdOverride).not.toHaveBeenCalled(); + expect(result).toEqual({ id: "task_1", account_id: "regular_acc" }); + }); + it("returns 403 when non-admin override is denied", async () => { vi.mocked(validateAuthContext).mockResolvedValue({ accountId: "org_owner_acc", diff --git a/lib/tasks/validateGetTasksQuery.ts b/lib/tasks/validateGetTasksQuery.ts index 3e9d56201..f581a438a 100644 --- a/lib/tasks/validateGetTasksQuery.ts +++ b/lib/tasks/validateGetTasksQuery.ts @@ -19,7 +19,9 @@ export const getTasksQuerySchema = z.object({ }); export type GetTasksQuery = z.infer; -export type ValidatedGetTasksQuery = Omit & { account_id: string }; +// account_id is optional: admin callers fetching a single task by `id` are not +// scoped to their own account, so the resolved query may omit account_id entirely. +export type ValidatedGetTasksQuery = Omit & { account_id?: string }; /** * Validates get tasks query parameters from a NextRequest. @@ -55,7 +57,7 @@ export async function validateGetTasksQuery( ); } - let targetAccountId = authResult.accountId; + let targetAccountId: string | undefined = authResult.accountId; if ( validationResult.data.account_id && @@ -76,10 +78,18 @@ export async function validateGetTasksQuery( targetAccountId = overrideResult.accountId; } + } else if (validationResult.data.id && !validationResult.data.account_id) { + // Single-task lookup by id: admins may read any task regardless of owner + // (e.g. the background worker fetching a customer's scheduled task config). + // Non-admins stay scoped to their own account. + const isAdmin = await checkIsAdmin(authResult.accountId); + if (isAdmin) { + targetAccountId = undefined; + } } return { ...validationResult.data, - account_id: targetAccountId, + ...(targetAccountId ? { account_id: targetAccountId } : {}), }; } From cdb34d09f27a50aec51cfffd423139706eaf10e2 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Sat, 20 Jun 2026 17:17:53 -0500 Subject: [PATCH 10/12] =?UTF-8?q?feat(connectors):=20POST=20/api/connector?= =?UTF-8?q?s/files=20=E2=80=94=20stage=20images=20for=20LinkedIn/X=20posts?= =?UTF-8?q?=20(#691)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(connectors): add POST /api/connectors/files (stage image for posts) Connector actions with file_uploadable fields (e.g. LINKEDIN_CREATE_LINKED_IN_POST.images[], TWITTER_CREATION_OF_A_POST) need a Composio { name, mimetype, s3key } descriptor whose s3key already lives in Composio storage. The execute path forwards parameters verbatim and never stages the file, so any s3key 404s. Add POST /api/connectors/files: given { url, toolSlug }, stage the image via composio.files.upload() and return flat { success, name, mimetype, s3key }. The caller passes that descriptor into parameters.images[] on the existing POST /api/connectors/actions. No change to the execute path (Option A). - uploadConnectorFile: calls composio.files.upload({ file: url, toolSlug, toolkitSlug }) where toolkitSlug is derived from the action slug. - validate body (zod { url, toolSlug }) + request (validateAuthContext gate; no account_id — upload is scoped by tool/toolkit, not connection). - handler returns 200 on success, 400 invalid body, 401 unauth, 502 upstream. URL-only input by decision; generic across file_uploadable toolkits (linkedin, twitter). TDD RED→GREEN; connectors suite green (129 tests). Implements recoupable/chat#1809. Docs: recoupable/docs#246. Co-Authored-By: Claude Opus 4.8 (1M context) * style: prettier-format connectors file-upload tests Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(connectors): use shared safeParseJson in file-upload validator Address review (DRY): replace the raw `await request.json()` with the shared `safeParseJson` helper (lib/networking/safeParseJson), matching the other validators. Malformed JSON now yields a clean 400 via body validation instead of throwing into the handler's 502 path. TDD: added a malformed-JSON test (RED on request.json() throw) → GREEN. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- app/api/connectors/files/route.ts | 35 +++++++ .../__tests__/deriveToolkitSlug.test.ts | 16 ++++ .../__tests__/uploadConnectorFile.test.ts | 52 +++++++++++ .../uploadConnectorFileHandler.test.ts | 79 ++++++++++++++++ .../validateUploadConnectorFileBody.test.ts | 45 +++++++++ ...validateUploadConnectorFileRequest.test.ts | 91 +++++++++++++++++++ lib/composio/connectors/deriveToolkitSlug.ts | 15 +++ .../connectors/uploadConnectorFile.ts | 42 +++++++++ .../connectors/uploadConnectorFileHandler.ts | 37 ++++++++ .../validateUploadConnectorFileBody.ts | 45 +++++++++ .../validateUploadConnectorFileRequest.ts | 43 +++++++++ 11 files changed, 500 insertions(+) create mode 100644 app/api/connectors/files/route.ts create mode 100644 lib/composio/connectors/__tests__/deriveToolkitSlug.test.ts create mode 100644 lib/composio/connectors/__tests__/uploadConnectorFile.test.ts create mode 100644 lib/composio/connectors/__tests__/uploadConnectorFileHandler.test.ts create mode 100644 lib/composio/connectors/__tests__/validateUploadConnectorFileBody.test.ts create mode 100644 lib/composio/connectors/__tests__/validateUploadConnectorFileRequest.test.ts create mode 100644 lib/composio/connectors/deriveToolkitSlug.ts create mode 100644 lib/composio/connectors/uploadConnectorFile.ts create mode 100644 lib/composio/connectors/uploadConnectorFileHandler.ts create mode 100644 lib/composio/connectors/validateUploadConnectorFileBody.ts create mode 100644 lib/composio/connectors/validateUploadConnectorFileRequest.ts diff --git a/app/api/connectors/files/route.ts b/app/api/connectors/files/route.ts new file mode 100644 index 000000000..e0e902b47 --- /dev/null +++ b/app/api/connectors/files/route.ts @@ -0,0 +1,35 @@ +import type { NextRequest } from "next/server"; +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { uploadConnectorFileHandler } from "@/lib/composio/connectors/uploadConnectorFileHandler"; + +/** + * OPTIONS handler for CORS preflight requests. + * + * @returns A 200 NextResponse carrying the CORS headers. + */ +export async function OPTIONS() { + return new NextResponse(null, { + status: 200, + headers: getCorsHeaders(), + }); +} + +/** + * POST /api/connectors/files + * + * Stages an image into Composio storage so it can be attached to a connector + * action that accepts a `file_uploadable` field (e.g. + * `LINKEDIN_CREATE_LINKED_IN_POST.images[]`, `TWITTER_CREATION_OF_A_POST`). + * Requires `x-api-key` or `Authorization: Bearer`. + * + * @param request - The incoming request. JSON body: `url` (required — a + * publicly reachable image URL) and `toolSlug` (required — the + * UPPERCASE_SNAKE_CASE action slug the file will be attached to). + * @returns A 200 NextResponse with `{ success, name, mimetype, s3key }`, 400 on + * missing/invalid body, 401 when unauthenticated, or 502 when Composio fails + * to fetch or store the image. + */ +export async function POST(request: NextRequest) { + return uploadConnectorFileHandler(request); +} diff --git a/lib/composio/connectors/__tests__/deriveToolkitSlug.test.ts b/lib/composio/connectors/__tests__/deriveToolkitSlug.test.ts new file mode 100644 index 000000000..74c006f76 --- /dev/null +++ b/lib/composio/connectors/__tests__/deriveToolkitSlug.test.ts @@ -0,0 +1,16 @@ +import { describe, it, expect } from "vitest"; +import { deriveToolkitSlug } from "../deriveToolkitSlug"; + +describe("deriveToolkitSlug", () => { + it("derives linkedin from LINKEDIN_CREATE_LINKED_IN_POST", () => { + expect(deriveToolkitSlug("LINKEDIN_CREATE_LINKED_IN_POST")).toBe("linkedin"); + }); + + it("derives twitter from TWITTER_CREATION_OF_A_POST", () => { + expect(deriveToolkitSlug("TWITTER_CREATION_OF_A_POST")).toBe("twitter"); + }); + + it("lowercases the leading toolkit token for other slugs", () => { + expect(deriveToolkitSlug("GMAIL_SEND_EMAIL")).toBe("gmail"); + }); +}); diff --git a/lib/composio/connectors/__tests__/uploadConnectorFile.test.ts b/lib/composio/connectors/__tests__/uploadConnectorFile.test.ts new file mode 100644 index 000000000..fbb1b72a3 --- /dev/null +++ b/lib/composio/connectors/__tests__/uploadConnectorFile.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { uploadConnectorFile } from "../uploadConnectorFile"; +import { getComposioClient } from "../../client"; + +vi.mock("../../client", () => ({ + getComposioClient: vi.fn(), +})); + +describe("uploadConnectorFile", () => { + const upload = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getComposioClient).mockResolvedValue({ + files: { upload }, + } as unknown as Awaited>); + }); + + it("uploads the url scoped to the tool/toolkit and returns the flat descriptor", async () => { + upload.mockResolvedValue({ + name: "post.png", + mimetype: "image/png", + s3key: "composio/abc123", + }); + + const result = await uploadConnectorFile({ + url: "https://cdn.example.com/post.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + + expect(upload).toHaveBeenCalledWith({ + file: "https://cdn.example.com/post.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + toolkitSlug: "linkedin", + }); + expect(result).toEqual({ + name: "post.png", + mimetype: "image/png", + s3key: "composio/abc123", + }); + }); + + it("propagates upstream upload failures", async () => { + upload.mockRejectedValue(new Error("storage returned HTTP 404")); + await expect( + uploadConnectorFile({ + url: "https://cdn.example.com/post.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }), + ).rejects.toThrow("storage returned HTTP 404"); + }); +}); diff --git a/lib/composio/connectors/__tests__/uploadConnectorFileHandler.test.ts b/lib/composio/connectors/__tests__/uploadConnectorFileHandler.test.ts new file mode 100644 index 000000000..9d148d455 --- /dev/null +++ b/lib/composio/connectors/__tests__/uploadConnectorFileHandler.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; +import { uploadConnectorFileHandler } from "../uploadConnectorFileHandler"; +import { validateUploadConnectorFileRequest } from "../validateUploadConnectorFileRequest"; +import { uploadConnectorFile } from "../uploadConnectorFile"; + +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: vi.fn(() => ({})), +})); + +vi.mock("../validateUploadConnectorFileRequest", () => ({ + validateUploadConnectorFileRequest: vi.fn(), +})); + +vi.mock("../uploadConnectorFile", () => ({ + uploadConnectorFile: vi.fn(), +})); + +const buildRequest = () => + new NextRequest("http://localhost/api/connectors/files", { + method: "POST", + body: JSON.stringify({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }), + }); + +describe("uploadConnectorFileHandler", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns 200 with the flat file descriptor on success", async () => { + vi.mocked(validateUploadConnectorFileRequest).mockResolvedValue({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + vi.mocked(uploadConnectorFile).mockResolvedValue({ + name: "a.png", + mimetype: "image/png", + s3key: "composio/xyz", + }); + + const response = await uploadConnectorFileHandler(buildRequest()); + const body = await response.json(); + + expect(response.status).toBe(200); + expect(body).toEqual({ + success: true, + name: "a.png", + mimetype: "image/png", + s3key: "composio/xyz", + }); + }); + + it("returns the validation/auth error response unchanged", async () => { + const errorResponse = NextResponse.json({ status: "error" }, { status: 401 }); + vi.mocked(validateUploadConnectorFileRequest).mockResolvedValue(errorResponse); + + const response = await uploadConnectorFileHandler(buildRequest()); + + expect(response).toBe(errorResponse); + expect(uploadConnectorFile).not.toHaveBeenCalled(); + }); + + it("returns 502 when the Composio upload fails", async () => { + vi.mocked(validateUploadConnectorFileRequest).mockResolvedValue({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + vi.mocked(uploadConnectorFile).mockRejectedValue(new Error("storage returned HTTP 404")); + + const response = await uploadConnectorFileHandler(buildRequest()); + const body = await response.json(); + + expect(response.status).toBe(502); + expect(body.error).toContain("storage returned HTTP 404"); + }); +}); diff --git a/lib/composio/connectors/__tests__/validateUploadConnectorFileBody.test.ts b/lib/composio/connectors/__tests__/validateUploadConnectorFileBody.test.ts new file mode 100644 index 000000000..3673348ac --- /dev/null +++ b/lib/composio/connectors/__tests__/validateUploadConnectorFileBody.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextResponse } from "next/server"; +import { validateUploadConnectorFileBody } from "../validateUploadConnectorFileBody"; + +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: vi.fn(() => ({})), +})); + +describe("validateUploadConnectorFileBody", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns the validated body for a valid url + toolSlug", () => { + const result = validateUploadConnectorFileBody({ + url: "https://cdn.example.com/post.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + expect(result).toEqual({ + url: "https://cdn.example.com/post.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + }); + + it("returns 400 when url is missing", () => { + const result = validateUploadConnectorFileBody({ toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST" }); + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(400); + }); + + it("returns 400 when url is not a valid URL", () => { + const result = validateUploadConnectorFileBody({ + url: "not-a-url", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(400); + }); + + it("returns 400 when toolSlug is missing", () => { + const result = validateUploadConnectorFileBody({ url: "https://cdn.example.com/post.png" }); + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(400); + }); +}); diff --git a/lib/composio/connectors/__tests__/validateUploadConnectorFileRequest.test.ts b/lib/composio/connectors/__tests__/validateUploadConnectorFileRequest.test.ts new file mode 100644 index 000000000..48544c340 --- /dev/null +++ b/lib/composio/connectors/__tests__/validateUploadConnectorFileRequest.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; +import { validateUploadConnectorFileRequest } from "../validateUploadConnectorFileRequest"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; + +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: vi.fn(() => ({})), +})); + +vi.mock("@/lib/auth/validateAuthContext", () => ({ + validateAuthContext: vi.fn(), +})); + +const buildRequest = (body: unknown) => + new NextRequest("http://localhost/api/connectors/files", { + method: "POST", + body: JSON.stringify(body), + }); + +describe("validateUploadConnectorFileRequest", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns the auth error when authentication fails", async () => { + vi.mocked(validateAuthContext).mockResolvedValue( + NextResponse.json({ status: "error" }, { status: 401 }), + ); + + const result = await validateUploadConnectorFileRequest( + buildRequest({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }), + ); + + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(401); + }); + + it("returns 400 when the body is invalid", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: "acc_1", + orgId: null, + authToken: "t", + }); + + const result = await validateUploadConnectorFileRequest(buildRequest({ toolSlug: "X" })); + + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(400); + }); + + it("returns 400 (not a throw) when the body is malformed JSON", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: "acc_1", + orgId: null, + authToken: "t", + }); + + const malformed = new NextRequest("http://localhost/api/connectors/files", { + method: "POST", + body: "{not json", + }); + + const result = await validateUploadConnectorFileRequest(malformed); + + expect(result).toBeInstanceOf(NextResponse); + if (result instanceof NextResponse) expect(result.status).toBe(400); + }); + + it("returns the validated { url, toolSlug } on success", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: "acc_1", + orgId: null, + authToken: "t", + }); + + const result = await validateUploadConnectorFileRequest( + buildRequest({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }), + ); + + expect(result).toEqual({ + url: "https://cdn.example.com/a.png", + toolSlug: "LINKEDIN_CREATE_LINKED_IN_POST", + }); + }); +}); diff --git a/lib/composio/connectors/deriveToolkitSlug.ts b/lib/composio/connectors/deriveToolkitSlug.ts new file mode 100644 index 000000000..869a3c717 --- /dev/null +++ b/lib/composio/connectors/deriveToolkitSlug.ts @@ -0,0 +1,15 @@ +/** + * Derives the Composio toolkit slug from an action slug. + * + * Action slugs are `UPPERCASE_SNAKE_CASE` and prefixed with their toolkit + * (e.g. `LINKEDIN_CREATE_LINKED_IN_POST` → `linkedin`, + * `TWITTER_CREATION_OF_A_POST` → `twitter`). Composio's file upload is scoped + * to a `{ toolSlug, toolkitSlug }`, so we recover the toolkit from the leading + * token of the action slug. + * + * @param toolSlug - The action slug (e.g. `LINKEDIN_CREATE_LINKED_IN_POST`) + * @returns The lowercase toolkit slug (e.g. `linkedin`) + */ +export function deriveToolkitSlug(toolSlug: string): string { + return toolSlug.split("_")[0].toLowerCase(); +} diff --git a/lib/composio/connectors/uploadConnectorFile.ts b/lib/composio/connectors/uploadConnectorFile.ts new file mode 100644 index 000000000..d6837732b --- /dev/null +++ b/lib/composio/connectors/uploadConnectorFile.ts @@ -0,0 +1,42 @@ +import { getComposioClient } from "../client"; +import { deriveToolkitSlug } from "./deriveToolkitSlug"; + +/** + * A Composio file descriptor, ready to embed in a `file_uploadable` action + * field (e.g. `LINKEDIN_CREATE_LINKED_IN_POST.images[]`). + */ +export interface UploadedConnectorFile { + name: string; + mimetype: string; + s3key: string; +} + +/** + * Stages an image into Composio storage and returns its `{ name, mimetype, s3key }` + * descriptor. + * + * Composio's SDK fetches the URL server-side, uploads the bytes to storage + * (deduplicated by content hash), and returns a storage key that can then be + * passed verbatim into a `file_uploadable` action parameter. The upload is + * scoped to the action's tool/toolkit, derived from `toolSlug`. + * + * @param params.url - Publicly reachable image URL to stage + * @param params.toolSlug - The action slug the file will be attached to + * (e.g. `LINKEDIN_CREATE_LINKED_IN_POST`) + * @returns The Composio file descriptor + * @throws when Composio fails to fetch or store the file (surfaced as 502 upstream) + */ +export async function uploadConnectorFile(params: { + url: string; + toolSlug: string; +}): Promise { + const composio = await getComposioClient(); + + const { name, mimetype, s3key } = await composio.files.upload({ + file: params.url, + toolSlug: params.toolSlug, + toolkitSlug: deriveToolkitSlug(params.toolSlug), + }); + + return { name, mimetype, s3key }; +} diff --git a/lib/composio/connectors/uploadConnectorFileHandler.ts b/lib/composio/connectors/uploadConnectorFileHandler.ts new file mode 100644 index 000000000..6e4156d74 --- /dev/null +++ b/lib/composio/connectors/uploadConnectorFileHandler.ts @@ -0,0 +1,37 @@ +import type { NextRequest } from "next/server"; +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { validateUploadConnectorFileRequest } from "./validateUploadConnectorFileRequest"; +import { uploadConnectorFile } from "./uploadConnectorFile"; + +/** + * Handler for POST /api/connectors/files. + * + * Stages an image (by public URL) into Composio storage and returns a flat + * `{ success, name, mimetype, s3key }` descriptor. The caller passes + * `{ name, mimetype, s3key }` into a `file_uploadable` action parameter + * (e.g. `parameters.images[]` for `LINKEDIN_CREATE_LINKED_IN_POST`) on + * POST /api/connectors/actions. A failure to fetch or store the image surfaces + * as 502 (upstream). + * + * @param request - The incoming request + * @returns A 200 response with `{ success, name, mimetype, s3key }`, or an error. + */ +export async function uploadConnectorFileHandler(request: NextRequest): Promise { + const headers = getCorsHeaders(); + + try { + const validated = await validateUploadConnectorFileRequest(request); + if (validated instanceof NextResponse) { + return validated; + } + + const { name, mimetype, s3key } = await uploadConnectorFile(validated); + + return NextResponse.json({ success: true, name, mimetype, s3key }, { status: 200, headers }); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to upload connector file"; + console.error("Connector file upload error:", error); + return NextResponse.json({ error: message }, { status: 502, headers }); + } +} diff --git a/lib/composio/connectors/validateUploadConnectorFileBody.ts b/lib/composio/connectors/validateUploadConnectorFileBody.ts new file mode 100644 index 000000000..c376ef997 --- /dev/null +++ b/lib/composio/connectors/validateUploadConnectorFileBody.ts @@ -0,0 +1,45 @@ +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { z } from "zod"; + +export const uploadConnectorFileBodySchema = z.object({ + url: z + .string({ message: "url is required" }) + .url("url must be a valid URL (a publicly reachable image link)"), + toolSlug: z + .string({ message: "toolSlug is required" }) + .min(1, "toolSlug cannot be empty (e.g., 'LINKEDIN_CREATE_LINKED_IN_POST')"), +}); + +export type UploadConnectorFileBody = z.infer; + +/** + * Validates request body for POST /api/connectors/files. + * + * Body shape: { url, toolSlug }. + * + * @param body - The request body + * @returns A NextResponse with an error if validation fails, or the validated body if validation passes. + */ +export function validateUploadConnectorFileBody( + body: unknown, +): NextResponse | UploadConnectorFileBody { + const result = uploadConnectorFileBodySchema.safeParse(body); + + if (!result.success) { + const firstError = result.error.issues[0]; + return NextResponse.json( + { + status: "error", + missing_fields: firstError.path, + error: firstError.message, + }, + { + status: 400, + headers: getCorsHeaders(), + }, + ); + } + + return result.data; +} diff --git a/lib/composio/connectors/validateUploadConnectorFileRequest.ts b/lib/composio/connectors/validateUploadConnectorFileRequest.ts new file mode 100644 index 000000000..bf1791c9b --- /dev/null +++ b/lib/composio/connectors/validateUploadConnectorFileRequest.ts @@ -0,0 +1,43 @@ +import type { NextRequest } from "next/server"; +import { NextResponse } from "next/server"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { safeParseJson } from "@/lib/networking/safeParseJson"; +import { validateUploadConnectorFileBody } from "./validateUploadConnectorFileBody"; + +/** + * Validated params for staging a connector file. + */ +export interface UploadConnectorFileParams { + url: string; + toolSlug: string; +} + +/** + * Validates the full POST /api/connectors/files request. + * + * Handles: + * 1. Authentication (x-api-key or Bearer token) — gates the endpoint + * 2. Body validation ({ url, toolSlug }) + * + * The Composio upload is scoped by tool/toolkit, not by connected account, so + * no `account_id` is accepted or used here — authentication just gates access. + * + * @param request - The incoming request + * @returns NextResponse error or validated params + */ +export async function validateUploadConnectorFileRequest( + request: NextRequest, +): Promise { + const authResult = await validateAuthContext(request); + if (authResult instanceof NextResponse) { + return authResult; + } + + const body = await safeParseJson(request); + const validated = validateUploadConnectorFileBody(body); + if (validated instanceof NextResponse) { + return validated; + } + + return { url: validated.url, toolSlug: validated.toolSlug }; +} From 2a7af682fa421bdb646489b499bbe1d369ba8e85 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 23 Jun 2026 07:41:21 -0500 Subject: [PATCH 11/12] feat(artists): account_id override for DELETE /api/artists/{id} (#693) Parse an optional account_id from the request body and thread it into validateAuthContext(request, { accountId }), so a caller with access to multiple accounts (org members / Recoup admins) can delete an artist in another account's context. The resolved account is used for the checkAccountArtistAccess check; a non-admin passing an inaccessible account is still rejected by canAccessAccount (403). Mirrors the existing override pattern on POST /api/artists. chat#1811 Co-authored-by: Claude Opus 4.8 (1M context) --- .../validateDeleteArtistRequest.test.ts | 53 ++++++++++++++++++- lib/artists/validateDeleteArtistRequest.ts | 27 +++++++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/lib/artists/__tests__/validateDeleteArtistRequest.test.ts b/lib/artists/__tests__/validateDeleteArtistRequest.test.ts index c0585f9f2..b39790dbe 100644 --- a/lib/artists/__tests__/validateDeleteArtistRequest.test.ts +++ b/lib/artists/__tests__/validateDeleteArtistRequest.test.ts @@ -55,7 +55,7 @@ describe("validateDeleteArtistRequest", () => { const result = await validateDeleteArtistRequest(request, validArtistId); expect(result).toBe(authError); - expect(validateAuthContext).toHaveBeenCalledWith(request); + expect(validateAuthContext).toHaveBeenCalledWith(request, { accountId: undefined }); }); it("returns 404 when the artist does not exist", async () => { @@ -125,4 +125,55 @@ describe("validateDeleteArtistRequest", () => { requesterAccountId: authenticatedAccountId, }); }); + + describe("account_id override", () => { + const overrideAccountId = "770e8400-e29b-41d4-a716-446655440000"; + + it("passes the account_id override to validateAuthContext and resolves against it", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: overrideAccountId, + authToken: "test-token", + orgId: null, + }); + vi.mocked(selectAccounts).mockResolvedValue([{ id: validArtistId }] as never); + vi.mocked(checkAccountArtistAccess).mockResolvedValue(true); + + const request = new NextRequest(`http://localhost/api/artists/${validArtistId}`, { + method: "DELETE", + headers: { + Authorization: "Bearer test-token", + "Content-Type": "application/json", + }, + body: JSON.stringify({ account_id: overrideAccountId }), + }); + + const result = await validateDeleteArtistRequest(request, validArtistId); + + expect(validateAuthContext).toHaveBeenCalledWith(request, { + accountId: overrideAccountId, + }); + expect(checkAccountArtistAccess).toHaveBeenCalledWith(overrideAccountId, validArtistId); + expect(result).toEqual({ + artistId: validArtistId, + requesterAccountId: overrideAccountId, + }); + }); + + it("returns 400 when account_id is not a valid UUID", async () => { + const request = new NextRequest(`http://localhost/api/artists/${validArtistId}`, { + method: "DELETE", + headers: { + Authorization: "Bearer test-token", + "Content-Type": "application/json", + }, + body: JSON.stringify({ account_id: "not-a-uuid" }), + }); + + const result = await validateDeleteArtistRequest(request, validArtistId); + + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + expect(validateAuthContext).not.toHaveBeenCalled(); + }); + }); }); diff --git a/lib/artists/validateDeleteArtistRequest.ts b/lib/artists/validateDeleteArtistRequest.ts index a69832e45..da237bca8 100644 --- a/lib/artists/validateDeleteArtistRequest.ts +++ b/lib/artists/validateDeleteArtistRequest.ts @@ -1,7 +1,9 @@ import { NextRequest, NextResponse } from "next/server"; +import { z } from "zod"; import { validateAccountParams } from "@/lib/accounts/validateAccountParams"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { safeParseJson } from "@/lib/networking/safeParseJson"; import { checkAccountArtistAccess } from "@/lib/artists/checkAccountArtistAccess"; import { selectAccounts } from "@/lib/supabase/accounts/selectAccounts"; @@ -10,9 +12,17 @@ export interface DeleteArtistRequest { requesterAccountId: string; } +const deleteArtistBodySchema = z.object({ + account_id: z.string().uuid("account_id must be a valid UUID").optional(), +}); + /** * Validates DELETE /api/artists/{id} path params and authentication. * + * Accepts an optional `account_id` in the request body so a caller with access + * to multiple accounts (org members or Recoup admins) can delete an artist in + * another account's context. The override is authorized by `validateAuthContext`. + * * @param request - The incoming request * @param id - The artist account ID from the route * @returns The validated artist ID plus requester context, or a NextResponse error @@ -26,7 +36,22 @@ export async function validateDeleteArtistRequest( return validatedParams; } - const authResult = await validateAuthContext(request); + const body = await safeParseJson(request); + const bodyResult = deleteArtistBodySchema.safeParse(body); + if (!bodyResult.success) { + const firstError = bodyResult.error.issues[0]; + return NextResponse.json( + { + status: "error", + error: firstError.message, + }, + { status: 400, headers: getCorsHeaders() }, + ); + } + + const authResult = await validateAuthContext(request, { + accountId: bodyResult.data.account_id, + }); if (authResult instanceof NextResponse) { return authResult; } From 8b6a3bb763c3fc40054589b660b890121b34dd94 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 23 Jun 2026 09:14:43 -0500 Subject: [PATCH 12/12] =?UTF-8?q?feat(chats):=20admins=20(RECOUP=5FORG)=20?= =?UTF-8?q?can=20access=20any=20chat=20=E2=80=94=20read=20+=20write=20(#69?= =?UTF-8?q?4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(chats): account_id override for GET /api/chats/{id}/messages Parse an optional account_id (or camelCase accountId) query param in validateGetChatMessagesQuery, validate it as a UUID, and thread it into validateChatAccess via a new optional options arg. validateChatAccess forwards it to validateAuthContext(request, { accountId }) and resolves room access against the overridden account, so a caller with access to multiple accounts (org members / Recoup admins) can read another account's chat messages. A non-admin passing an inaccessible account is still rejected by canAccessAccount (403). The override is opt-in per call site: only validateGetChatMessagesQuery passes it, so the other validateChatAccess callers are unchanged. chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(chats): admin bypass (not account_id param) for GET messages Aligns GET /api/chats/{id}/messages with the shipped docs contract — docs#247 rolled back the account_id query param. The chat is identified by the path id and the owner is resolved server-side, so no param is needed. Instead, validateChatAccess gains an opt-in `allowAdmin` flag that grants RECOUP_ORG admins access to any room (mirrors checkAccountArtistAccess). Only the messages read path opts in; chat mutations (update/delete/copy) stay ownership-gated, so admin write access is not silently broadened. - drop account_id/accountId query parsing from validateGetChatMessagesQuery - validateChatAccess: remove accountId override; add allowAdmin + checkIsAdmin bypass - tests: admin bypass grants access; non-admin still 403 even with allowAdmin; mutation paths never consult admin status - mock checkIsAdmin in getChatArtistHandler.test.ts (now a transitive dep) Refs recoupable/chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(chats): drop allowAdmin flag — admins access any chat (read + write) YAGNI/KISS per internal review: RECOUP_ORG admins already have broad cross-account power (delete any artist, read any account), and chat ops are resource-scoped by chatId, so an unconditional admin bypass is the coherent model. Removes the opt-in flag entirely. The admin check now runs ONLY after the ownership check fails, so the common owner path never pays the extra checkIsAdmin lookup (better than both the flag and a top-of-function bypass). Applies across all validateChatAccess call sites (messages + getChatArtist reads; update/delete-trailing/copy mutations), so admins can read and write any account's chats; non-admins are unchanged (403). Refs recoupable/chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(chats): revert validateGetChatMessagesQuery (no change needed) The admin bypass lives entirely in validateChatAccess, which the messages endpoint already delegates to — so validateGetChatMessagesQuery needs no change. Reverts the doc-only edit and the redundant delegation test to keep the PR scoped to validateChatAccess. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/getChatArtistHandler.test.ts | 4 ++ .../__tests__/validateChatAccess.test.ts | 60 +++++++++++++++++++ lib/chats/validateChatAccess.ts | 33 +++++----- 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/lib/chats/__tests__/getChatArtistHandler.test.ts b/lib/chats/__tests__/getChatArtistHandler.test.ts index c88ce650d..7bbf0d388 100644 --- a/lib/chats/__tests__/getChatArtistHandler.test.ts +++ b/lib/chats/__tests__/getChatArtistHandler.test.ts @@ -17,6 +17,10 @@ vi.mock("@/lib/chats/buildGetChatsParams", () => ({ buildGetChatsParams: vi.fn(), })); +vi.mock("@/lib/admins/checkIsAdmin", () => ({ + checkIsAdmin: vi.fn(() => Promise.resolve(false)), +})); + const createRequest = () => new NextRequest("http://localhost/api/chats/chat-id/artist"); describe("getChatArtistHandler", () => { diff --git a/lib/chats/__tests__/validateChatAccess.test.ts b/lib/chats/__tests__/validateChatAccess.test.ts index 4c9255446..cac17ccfa 100644 --- a/lib/chats/__tests__/validateChatAccess.test.ts +++ b/lib/chats/__tests__/validateChatAccess.test.ts @@ -4,6 +4,7 @@ import { validateChatAccess } from "../validateChatAccess"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; import selectRoom from "@/lib/supabase/rooms/selectRoom"; import { buildGetChatsParams } from "@/lib/chats/buildGetChatsParams"; +import { checkIsAdmin } from "@/lib/admins/checkIsAdmin"; vi.mock("@/lib/networking/getCorsHeaders", () => ({ getCorsHeaders: vi.fn(() => ({ "Access-Control-Allow-Origin": "*" })), @@ -21,6 +22,10 @@ vi.mock("@/lib/chats/buildGetChatsParams", () => ({ buildGetChatsParams: vi.fn(), })); +vi.mock("@/lib/admins/checkIsAdmin", () => ({ + checkIsAdmin: vi.fn(), +})); + describe("validateChatAccess", () => { const roomId = "123e4567-e89b-12d3-a456-426614174000"; const accountId = "123e4567-e89b-12d3-a456-426614174001"; @@ -30,6 +35,7 @@ describe("validateChatAccess", () => { beforeEach(() => { vi.clearAllMocks(); + vi.mocked(checkIsAdmin).mockResolvedValue(false); }); it("returns 400 when roomId is invalid uuid", async () => { @@ -159,4 +165,58 @@ describe("validateChatAccess", () => { const result = await validateChatAccess(request, roomId); expect(result).toEqual({ roomId, room, accountId }); }); + + it("grants a Recoup admin access to a room they don't own (admin checked after ownership fails)", async () => { + const room = { + id: roomId, + account_id: "another-account", + artist_id: null, + topic: "Topic", + updated_at: "2026-03-30T00:00:00Z", + }; + + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId, + orgId: null, + authToken: "test-key", + }); + vi.mocked(selectRoom).mockResolvedValue(room); + vi.mocked(buildGetChatsParams).mockResolvedValue({ + params: { account_ids: [accountId] }, + error: null, + }); + vi.mocked(checkIsAdmin).mockResolvedValue(true); + + const result = await validateChatAccess(request, roomId); + + expect(buildGetChatsParams).toHaveBeenCalled(); + expect(checkIsAdmin).toHaveBeenCalledWith(accountId); + expect(result).toEqual({ roomId, room, accountId: "another-account" }); + }); + + it("does not consult admin status when the caller owns the room", async () => { + const room = { + id: roomId, + account_id: accountId, + artist_id: null, + topic: "Topic", + updated_at: "2026-03-30T00:00:00Z", + }; + + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId, + orgId: null, + authToken: "test-key", + }); + vi.mocked(selectRoom).mockResolvedValue(room); + vi.mocked(buildGetChatsParams).mockResolvedValue({ + params: { account_ids: [accountId] }, + error: null, + }); + + const result = await validateChatAccess(request, roomId); + + expect(checkIsAdmin).not.toHaveBeenCalled(); + expect(result).toEqual({ roomId, room, accountId }); + }); }); diff --git a/lib/chats/validateChatAccess.ts b/lib/chats/validateChatAccess.ts index 30f89e5db..de663feaf 100644 --- a/lib/chats/validateChatAccess.ts +++ b/lib/chats/validateChatAccess.ts @@ -3,6 +3,7 @@ import { NextResponse } from "next/server"; import { z } from "zod"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { checkIsAdmin } from "@/lib/admins/checkIsAdmin"; import selectRoom from "@/lib/supabase/rooms/selectRoom"; import { buildGetChatsParams } from "@/lib/chats/buildGetChatsParams"; import type { Tables } from "@/types/database.types"; @@ -18,6 +19,11 @@ const chatIdSchema = z.string().uuid("id must be a valid UUID"); /** * Validates that the authenticated caller can access a chat room. * + * The room is fully identified by `roomId`, so no account override is accepted. + * Access is granted to the room's owner; a Recoup admin (`RECOUP_ORG_ID` member) + * may access any room. The admin check runs only when the ownership check fails, + * so the common owner path never pays the extra lookup. + * * @param request - The incoming request (used for auth context) * @param roomId - The room/chat UUID to validate access for * @returns NextResponse on auth/access failure, or validated access data @@ -49,23 +55,22 @@ export async function validateChatAccess( ); } - const { params, error } = await buildGetChatsParams({ - account_id: accountId, - }); + const { params } = await buildGetChatsParams({ account_id: accountId }); - if (!params) { - return NextResponse.json( - { status: "error", error: error ?? "Access denied" }, - { status: 403, headers: getCorsHeaders() }, - ); + const isOwner = !!params && !!room.account_id && params.account_ids.includes(room.account_id); + if (isOwner) { + return { roomId: room.id, room, accountId }; } - if (!room.account_id || !params.account_ids.includes(room.account_id)) { - return NextResponse.json( - { status: "error", error: "Access denied to this chat" }, - { status: 403, headers: getCorsHeaders() }, - ); + // Non-owner: a Recoup admin may access any chat (read or write). The owner is + // resolved server-side, so no account_id input is needed. Checked only here so + // the owner path above never pays the lookup. + if (await checkIsAdmin(accountId)) { + return { roomId: room.id, room, accountId: room.account_id ?? accountId }; } - return { roomId: room.id, room, accountId }; + return NextResponse.json( + { status: "error", error: "Access denied to this chat" }, + { status: 403, headers: getCorsHeaders() }, + ); }