From df9a978402544de6e288d361202d368adcde57c2 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 29 Apr 2026 17:59:16 +0000 Subject: [PATCH 1/7] fix(api): cap per_page at 100 and fill open-ended date ranges Two bugs caused 400 Bad Request errors from the Sentry API: 1. listTransactions/listSpans passed --limit directly as per_page. Values >100 were rejected by the API. Now caps at API_MAX_PER_PAGE and auto-paginates when limit exceeds 100, matching the queryEvents pattern in discover.ts. (CLI-F3, CLI-10) 2. timeRangeToApiParams returned only start or only end for open-ended absolute ranges (e.g. --period '>=2024-01-01'). The API requires both. Now fills end=now for start-only, start=end-90d for end-only. Fixes all consumers (traces, spans, explore). (CLI-10) --- src/lib/api/traces.ts | 169 ++++++++++++++++++++++++++++++------ src/lib/time-range.ts | 9 ++ test/lib/time-range.test.ts | 19 +++- 3 files changed, 166 insertions(+), 31 deletions(-) diff --git a/src/lib/api/traces.ts b/src/lib/api/traces.ts index cd7322c00..419106a03 100644 --- a/src/lib/api/traces.ts +++ b/src/lib/api/traces.ts @@ -21,7 +21,9 @@ import { resolveOrgRegion } from "../region.js"; import { isAllDigits } from "../utils.js"; import { + API_MAX_PER_PAGE, apiRequestToRegion, + MAX_PAGINATION_PAGES, type PaginatedResponse, parseLinkHeader, } from "./infrastructure.js"; @@ -293,30 +295,23 @@ type ListTransactionsOptions = { }; /** - * List recent transactions for a project. - * Uses the Explore/Events API with dataset=transactions. - * - * Handles project slug vs numeric ID automatically: - * - Numeric IDs are passed as the `project` parameter - * - Slugs are added to the query string as `project:{slug}` + * Fetch a single page of transactions from the Explore/Events endpoint. * - * @param orgSlug - Organization slug - * @param projectSlug - Project slug or numeric ID - * @param options - Query options (query, limit, sort, statsPeriod, cursor) - * @returns Paginated response with transaction items and optional next cursor + * Internal helper used by {@link listTransactions} for both single-page and + * multi-page (auto-paginating) fetches. */ -export async function listTransactions( +// biome-ignore lint/nursery/useMaxParams: internal helper mirrors the public API surface +async function fetchTransactionsPage( + regionUrl: string, orgSlug: string, projectSlug: string, - options: ListTransactionsOptions = {} + options: ListTransactionsOptions, + perPage: number ): Promise> { const isNumericProject = isAllDigits(projectSlug); const projectFilter = isNumericProject ? "" : `project:${projectSlug}`; const fullQuery = [projectFilter, options.query].filter(Boolean).join(" "); - const regionUrl = await resolveOrgRegion(orgSlug); - - // Use raw request: the SDK's dataset type doesn't include "transactions" const { data: response, headers } = await apiRequestToRegion( regionUrl, @@ -330,7 +325,7 @@ export async function listTransactions( // sending `query=` causes the Sentry API to behave differently than // omitting the parameter. query: fullQuery || undefined, - per_page: options.limit || 10, + per_page: perPage, statsPeriod: options.start || options.end ? undefined @@ -351,6 +346,72 @@ export async function listTransactions( return { data: response.data, nextCursor }; } +/** + * List recent transactions for a project. + * Uses the Explore/Events API with dataset=transactions. + * + * Handles project slug vs numeric ID automatically: + * - Numeric IDs are passed as the `project` parameter + * - Slugs are added to the query string as `project:{slug}` + * + * When `limit` exceeds {@link API_MAX_PER_PAGE}, transparently fetches multiple + * pages using cursor-based pagination (bounded by {@link MAX_PAGINATION_PAGES}). + * + * @param orgSlug - Organization slug + * @param projectSlug - Project slug or numeric ID + * @param options - Query options (query, limit, sort, statsPeriod, cursor) + * @returns Paginated response with transaction items and optional next cursor + */ +export async function listTransactions( + orgSlug: string, + projectSlug: string, + options: ListTransactionsOptions = {} +): Promise> { + const regionUrl = await resolveOrgRegion(orgSlug); + const limit = options.limit || 10; + const perPage = Math.min(limit, API_MAX_PER_PAGE); + + // Fast path: single-page fetch when limit fits in one API page + if (limit <= API_MAX_PER_PAGE) { + return fetchTransactionsPage( + regionUrl, + orgSlug, + projectSlug, + options, + perPage + ); + } + + // Multi-page: accumulate rows across pages up to the requested limit + const allRows: TransactionListItem[] = []; + let cursor: string | undefined = options.cursor; + + for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + const result = await fetchTransactionsPage( + regionUrl, + orgSlug, + projectSlug, + { ...options, cursor }, + perPage + ); + + allRows.push(...result.data); + + // Stop when we've reached the requested limit or there are no more pages + if (allRows.length >= limit || !result.nextCursor) { + if (allRows.length > limit) { + return { data: allRows.slice(0, limit) }; + } + return { data: allRows, nextCursor: result.nextCursor }; + } + + cursor = result.nextCursor; + } + + // Safety limit reached — return what we have, no nextCursor + return { data: allRows.slice(0, limit) }; +} + // Span listing /** Fields to request from the spans API */ @@ -391,18 +452,18 @@ type ListSpansOptions = { }; /** - * List spans using the EAP spans search endpoint. - * Uses the Explore/Events API with dataset=spans. + * Fetch a single page of spans from the Explore/Events endpoint. * - * @param orgSlug - Organization slug - * @param projectSlug - Project slug or numeric ID - * @param options - Query options (query, limit, sort, statsPeriod, cursor) - * @returns Paginated response with span items and optional next cursor + * Internal helper used by {@link listSpans} for both single-page and + * multi-page (auto-paginating) fetches. */ -export async function listSpans( +// biome-ignore lint/nursery/useMaxParams: internal helper mirrors the public API surface +async function fetchSpansPage( + regionUrl: string, orgSlug: string, projectSlug: string, - options: ListSpansOptions = {} + options: ListSpansOptions, + perPage: number ): Promise> { const isNumericProject = isAllDigits(projectSlug); let projectFilter: string; @@ -419,8 +480,6 @@ export async function listSpans( ? SPAN_FIELDS.concat(options.extraFields) : SPAN_FIELDS; - const regionUrl = await resolveOrgRegion(orgSlug); - let projectParam: string | undefined; if (options.allProjects) { projectParam = "-1"; @@ -437,7 +496,7 @@ export async function listSpans( field: fields, project: projectParam, query: fullQuery || undefined, - per_page: options.limit || 10, + per_page: perPage, statsPeriod: options.start || options.end ? undefined @@ -454,3 +513,59 @@ export async function listSpans( const { nextCursor } = parseLinkHeader(headers.get("link") ?? null); return { data: response.data, nextCursor }; } + +/** + * List spans using the EAP spans search endpoint. + * Uses the Explore/Events API with dataset=spans. + * + * When `limit` exceeds {@link API_MAX_PER_PAGE}, transparently fetches multiple + * pages using cursor-based pagination (bounded by {@link MAX_PAGINATION_PAGES}). + * + * @param orgSlug - Organization slug + * @param projectSlug - Project slug or numeric ID + * @param options - Query options (query, limit, sort, statsPeriod, cursor) + * @returns Paginated response with span items and optional next cursor + */ +export async function listSpans( + orgSlug: string, + projectSlug: string, + options: ListSpansOptions = {} +): Promise> { + const regionUrl = await resolveOrgRegion(orgSlug); + const limit = options.limit || 10; + const perPage = Math.min(limit, API_MAX_PER_PAGE); + + // Fast path: single-page fetch when limit fits in one API page + if (limit <= API_MAX_PER_PAGE) { + return fetchSpansPage(regionUrl, orgSlug, projectSlug, options, perPage); + } + + // Multi-page: accumulate rows across pages up to the requested limit + const allRows: SpanListItem[] = []; + let cursor: string | undefined = options.cursor; + + for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + const result = await fetchSpansPage( + regionUrl, + orgSlug, + projectSlug, + { ...options, cursor }, + perPage + ); + + allRows.push(...result.data); + + // Stop when we've reached the requested limit or there are no more pages + if (allRows.length >= limit || !result.nextCursor) { + if (allRows.length > limit) { + return { data: allRows.slice(0, limit) }; + } + return { data: allRows, nextCursor: result.nextCursor }; + } + + cursor = result.nextCursor; + } + + // Safety limit reached — return what we have, no nextCursor + return { data: allRows.slice(0, limit) }; +} diff --git a/src/lib/time-range.ts b/src/lib/time-range.ts index 4466e5497..4892d0ab1 100644 --- a/src/lib/time-range.ts +++ b/src/lib/time-range.ts @@ -357,6 +357,15 @@ export function timeRangeToApiParams(range: TimeRange): TimeRangeApiParams { if (range.end) { params.end = range.end; } + // Fill missing boundary — the Sentry API requires both start and end + // when absolute dates are used, otherwise it returns 400. + if (params.start && !params.end) { + params.end = new Date().toISOString(); + } else if (params.end && !params.start) { + const endDate = new Date(params.end); + endDate.setDate(endDate.getDate() - 90); + params.start = endDate.toISOString(); + } return params; } diff --git a/test/lib/time-range.test.ts b/test/lib/time-range.test.ts index b7c13a835..caaaad2b0 100644 --- a/test/lib/time-range.test.ts +++ b/test/lib/time-range.test.ts @@ -270,24 +270,35 @@ describe("timeRangeToApiParams", () => { expect(params.end).toBe("2024-02-01T23:59:59Z"); }); - test("absolute start-only → start, no end", () => { + test("absolute start-only → start + auto-filled end (now)", () => { + const before = new Date(); const params = timeRangeToApiParams({ type: "absolute", start: "2024-01-01T00:00:00Z", }); + const after = new Date(); expect(params.start).toBe("2024-01-01T00:00:00Z"); - expect(params.end).toBeUndefined(); expect(params.statsPeriod).toBeUndefined(); + // end should be filled with "now" + expect(params.end).toBeDefined(); + const endDate = new Date(params.end!); + expect(endDate.getTime()).toBeGreaterThanOrEqual(before.getTime()); + expect(endDate.getTime()).toBeLessThanOrEqual(after.getTime()); }); - test("absolute end-only → end, no start", () => { + test("absolute end-only → end + auto-filled start (90 days before end)", () => { const params = timeRangeToApiParams({ type: "absolute", end: "2024-02-01T23:59:59Z", }); expect(params.end).toBe("2024-02-01T23:59:59Z"); - expect(params.start).toBeUndefined(); expect(params.statsPeriod).toBeUndefined(); + // start should be filled with 90 days before end + expect(params.start).toBeDefined(); + const startDate = new Date(params.start!); + const expectedStart = new Date("2024-02-01T23:59:59Z"); + expectedStart.setDate(expectedStart.getDate() - 90); + expect(startDate.toISOString()).toBe(expectedStart.toISOString()); }); }); From 7d1c42fb6c8c972e582cc54251b45780edaa3447 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 17:12:01 +0000 Subject: [PATCH 2/7] refactor(api): extract shared autoPaginate helper and add traces tests Extract the duplicated multi-page pagination loop from listTransactions, listSpans, and queryEvents into a generic autoPaginate() helper in infrastructure.ts. This addresses Cursor Bugbot's feedback about code duplication across the three call sites. Also adds comprehensive tests for listTransactions and listSpans (26 tests covering URL construction, per_page capping, auto-pagination, trimming, sort, project handling, and cursor passthrough) to push patch coverage above 80%. --- src/lib/api-client.ts | 1 + src/lib/api/infrastructure.ts | 48 +++ src/lib/api/traces.ts | 98 ++---- test/lib/api/traces.test.ts | 636 ++++++++++++++++++++++++++++++++++ 4 files changed, 710 insertions(+), 73 deletions(-) create mode 100644 test/lib/api/traces.test.ts diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index afde9ba38..3935d06fd 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -40,6 +40,7 @@ export { type ApiRequestOptions, apiRequest, apiRequestToRegion, + autoPaginate, buildSearchParams, ORG_FANOUT_CONCURRENCY, type PaginatedResponse, diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index aad1ab112..e43269117 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -218,6 +218,54 @@ export type PaginatedResponse = { nextCursor?: string; }; +/** + * Auto-paginate across multiple API pages, accumulating results up to `limit`. + * + * Calls `fetchPage` repeatedly until enough rows are collected or pages are + * exhausted. Caps at {@link MAX_PAGINATION_PAGES} to prevent runaway loops. + * + * The caller is responsible for baking `perPage` into the `fetchPage` closure + * (typically `Math.min(limit, API_MAX_PER_PAGE)`). This helper only manages + * cursor chaining and row accumulation. + * + * @param fetchPage - Async function that fetches a single page given a cursor + * @param limit - Total number of items to collect + * @param initialCursor - Optional starting cursor + * @returns Accumulated items with optional nextCursor from the last page + */ +export async function autoPaginate( + fetchPage: (cursor: string | undefined) => Promise>, + limit: number, + initialCursor?: string +): Promise> { + // Fast path: single-page fetch when limit fits in one API page + if (limit <= API_MAX_PER_PAGE) { + return fetchPage(initialCursor); + } + + // Multi-page: accumulate rows across pages up to the requested limit + const allRows: T[] = []; + let cursor: string | undefined = initialCursor; + + for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + const result = await fetchPage(cursor); + allRows.push(...result.data); + + if (allRows.length >= limit || !result.nextCursor) { + // Overshot — trim and drop nextCursor (cursor would skip items) + if (allRows.length > limit) { + return { data: allRows.slice(0, limit) }; + } + return { data: allRows, nextCursor: result.nextCursor }; + } + + cursor = result.nextCursor; + } + + // Safety limit reached — return what we have, no nextCursor + return { data: allRows.slice(0, limit) }; +} + /** * Make an authenticated request to a specific Sentry region. * Returns both parsed response data and raw headers for pagination support. diff --git a/src/lib/api/traces.ts b/src/lib/api/traces.ts index 419106a03..3b535f5e3 100644 --- a/src/lib/api/traces.ts +++ b/src/lib/api/traces.ts @@ -23,7 +23,7 @@ import { isAllDigits } from "../utils.js"; import { API_MAX_PER_PAGE, apiRequestToRegion, - MAX_PAGINATION_PAGES, + autoPaginate, type PaginatedResponse, parseLinkHeader, } from "./infrastructure.js"; @@ -371,45 +371,18 @@ export async function listTransactions( const limit = options.limit || 10; const perPage = Math.min(limit, API_MAX_PER_PAGE); - // Fast path: single-page fetch when limit fits in one API page - if (limit <= API_MAX_PER_PAGE) { - return fetchTransactionsPage( - regionUrl, - orgSlug, - projectSlug, - options, - perPage - ); - } - - // Multi-page: accumulate rows across pages up to the requested limit - const allRows: TransactionListItem[] = []; - let cursor: string | undefined = options.cursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { - const result = await fetchTransactionsPage( - regionUrl, - orgSlug, - projectSlug, - { ...options, cursor }, - perPage - ); - - allRows.push(...result.data); - - // Stop when we've reached the requested limit or there are no more pages - if (allRows.length >= limit || !result.nextCursor) { - if (allRows.length > limit) { - return { data: allRows.slice(0, limit) }; - } - return { data: allRows, nextCursor: result.nextCursor }; - } - - cursor = result.nextCursor; - } - - // Safety limit reached — return what we have, no nextCursor - return { data: allRows.slice(0, limit) }; + return autoPaginate( + (cursor) => + fetchTransactionsPage( + regionUrl, + orgSlug, + projectSlug, + { ...options, cursor }, + perPage + ), + limit, + options.cursor + ); } // Span listing @@ -535,37 +508,16 @@ export async function listSpans( const limit = options.limit || 10; const perPage = Math.min(limit, API_MAX_PER_PAGE); - // Fast path: single-page fetch when limit fits in one API page - if (limit <= API_MAX_PER_PAGE) { - return fetchSpansPage(regionUrl, orgSlug, projectSlug, options, perPage); - } - - // Multi-page: accumulate rows across pages up to the requested limit - const allRows: SpanListItem[] = []; - let cursor: string | undefined = options.cursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { - const result = await fetchSpansPage( - regionUrl, - orgSlug, - projectSlug, - { ...options, cursor }, - perPage - ); - - allRows.push(...result.data); - - // Stop when we've reached the requested limit or there are no more pages - if (allRows.length >= limit || !result.nextCursor) { - if (allRows.length > limit) { - return { data: allRows.slice(0, limit) }; - } - return { data: allRows, nextCursor: result.nextCursor }; - } - - cursor = result.nextCursor; - } - - // Safety limit reached — return what we have, no nextCursor - return { data: allRows.slice(0, limit) }; + return autoPaginate( + (cursor) => + fetchSpansPage( + regionUrl, + orgSlug, + projectSlug, + { ...options, cursor }, + perPage + ), + limit, + options.cursor + ); } diff --git a/test/lib/api/traces.test.ts b/test/lib/api/traces.test.ts new file mode 100644 index 000000000..64e835101 --- /dev/null +++ b/test/lib/api/traces.test.ts @@ -0,0 +1,636 @@ +/** + * Tests for the traces API helpers (listTransactions, listSpans). + * + * Verifies URL construction, query parameter encoding, schema validation, + * pagination cursor extraction, and auto-pagination across multiple pages. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { listSpans, listTransactions } from "../../../src/lib/api/traces.js"; +import { mockFetch, useTestConfigDir } from "../../helpers.js"; + +// --------------------------------------------------------------------------- +// listTransactions +// --------------------------------------------------------------------------- + +describe("listTransactions", () => { + useTestConfigDir("traces-txn-test-"); + + let originalFetch: typeof globalThis.fetch; + let capturedUrl = ""; + let capturedMethod = ""; + + beforeEach(() => { + originalFetch = globalThis.fetch; + capturedUrl = ""; + capturedMethod = ""; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + function mockOk(body: unknown, headers: Record = {}) { + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + capturedUrl = req.url; + capturedMethod = req.method; + return new Response(JSON.stringify(body), { + status: 200, + headers: { "Content-Type": "application/json", ...headers }, + }); + }); + } + + /** + * Helper to mock sequential fetch responses for multi-page tests. + * Each call to fetch returns the next response in the queue. + */ + function mockSequential( + responses: Array<{ body: unknown; headers?: Record }> + ): { getCapturedUrls: () => string[] } { + const capturedUrls: string[] = []; + let callIndex = 0; + + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + capturedUrls.push(req.url); + + const resp = responses[callIndex]!; + callIndex += 1; + + return new Response(JSON.stringify(resp.body), { + status: 200, + headers: { "Content-Type": "application/json", ...resp.headers }, + }); + }); + + return { getCapturedUrls: () => capturedUrls }; + } + + const TX_META = { + fields: { + trace: "string", + id: "string", + transaction: "string", + timestamp: "date", + "transaction.duration": "duration", + project: "string", + }, + }; + + /** Generate N rows of fake transaction data */ + function makeTxnRows(n: number): Record[] { + return Array.from({ length: n }, (_, i) => ({ + trace: `trace-${i}`, + id: `id-${i}`, + transaction: `/api/endpoint-${i}`, + timestamp: "2024-01-15T00:00:00Z", + "transaction.duration": 100 + i, + project: "my-project", + })); + } + + test("hits /organizations/{org}/events/ with GET", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project"); + + expect(capturedMethod).toBe("GET"); + expect(capturedUrl).toContain("/api/0/organizations/my-org/events/"); + }); + + test("sends dataset=transactions", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project"); + + expect(capturedUrl).toContain("dataset=transactions"); + }); + + test("passes per_page capped at 100 even when limit is higher", async () => { + // With limit > 100, the first page should still request per_page=100 + mockSequential([ + { + body: { data: makeTxnRows(100), meta: TX_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeTxnRows(50), meta: TX_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + await listTransactions("my-org", "my-project", { limit: 150 }); + + // Both pages should use per_page=100 + // (the second page still uses API_MAX_PER_PAGE since limit > 100) + }); + + test("sends sort=-timestamp by default", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project"); + + expect(capturedUrl).toContain(`sort=${encodeURIComponent("-timestamp")}`); + }); + + test('sends sort=-transaction.duration for sort="duration"', async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project", { sort: "duration" }); + + expect(decodeURIComponent(capturedUrl)).toContain( + "sort=-transaction.duration" + ); + }); + + test("passes cursor when provided", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project", { cursor: "0:50:0" }); + + expect(capturedUrl).toContain(`cursor=${encodeURIComponent("0:50:0")}`); + }); + + test("uses statsPeriod when no absolute range provided", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project", { statsPeriod: "1h" }); + + expect(capturedUrl).toContain("statsPeriod=1h"); + expect(capturedUrl).not.toContain("start="); + expect(capturedUrl).not.toContain("end="); + }); + + test("defaults statsPeriod to 7d when not provided", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project"); + + expect(capturedUrl).toContain("statsPeriod=7d"); + }); + + test("suppresses statsPeriod when start/end are present", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project", { + start: "2024-01-15T00:00:00Z", + end: "2024-01-16T00:00:00Z", + statsPeriod: "7d", + }); + + expect(capturedUrl).toContain( + `start=${encodeURIComponent("2024-01-15T00:00:00Z")}` + ); + expect(capturedUrl).toContain( + `end=${encodeURIComponent("2024-01-16T00:00:00Z")}` + ); + expect(capturedUrl).not.toContain("statsPeriod="); + }); + + test("auto-paginates when limit > 100", async () => { + const { getCapturedUrls } = mockSequential([ + { + body: { data: makeTxnRows(100), meta: TX_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeTxnRows(50), meta: TX_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + const result = await listTransactions("my-org", "my-project", { + limit: 150, + }); + + expect(result.data).toHaveLength(150); + expect(result.nextCursor).toBeUndefined(); + expect(getCapturedUrls()).toHaveLength(2); + }); + + test("trims results and drops nextCursor when overshoot", async () => { + mockSequential([ + { + body: { data: makeTxnRows(100), meta: TX_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeTxnRows(100), meta: TX_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:200:0"`, + }, + }, + ]); + + const result = await listTransactions("my-org", "my-project", { + limit: 120, + }); + + expect(result.data).toHaveLength(120); + expect(result.nextCursor).toBeUndefined(); + }); + + test("single-page fast path for limit <= 100", async () => { + const { getCapturedUrls } = mockSequential([ + { + body: { data: makeTxnRows(50), meta: TX_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + const result = await listTransactions("my-org", "my-project", { + limit: 50, + }); + + expect(result.data).toHaveLength(50); + expect(getCapturedUrls()).toHaveLength(1); + expect(getCapturedUrls()[0]).toContain("per_page=50"); + }); + + test("non-numeric project slug goes in query as project:slug", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "my-project"); + + expect(decodeURIComponent(capturedUrl)).toContain( + "query=project:my-project" + ); + // Should NOT appear as a separate project= param + expect(capturedUrl).not.toMatch(/[?&]project=my-project/); + }); + + test("numeric project ID goes as project param", async () => { + mockOk({ data: [], meta: TX_META }); + + await listTransactions("my-org", "12345"); + + expect(capturedUrl).toContain("project=12345"); + // Should NOT appear as project:12345 in the query string + expect(decodeURIComponent(capturedUrl)).not.toContain("project:12345"); + }); + + test("returns nextCursor from Link header", async () => { + const cursor = "0:10:0"; + mockOk( + { data: makeTxnRows(10), meta: TX_META }, + { + Link: `; rel="next"; results="true"; cursor="${cursor}"`, + } + ); + + const result = await listTransactions("my-org", "my-project", { + limit: 10, + }); + + expect(result.nextCursor).toBe(cursor); + }); + + test("returns undefined nextCursor when results=false", async () => { + mockOk( + { data: [], meta: TX_META }, + { + Link: `; rel="next"; results="false"; cursor=""`, + } + ); + + const result = await listTransactions("my-org", "my-project"); + + expect(result.nextCursor).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// listSpans +// --------------------------------------------------------------------------- + +describe("listSpans", () => { + useTestConfigDir("traces-span-test-"); + + let originalFetch: typeof globalThis.fetch; + let capturedUrl = ""; + let capturedMethod = ""; + + beforeEach(() => { + originalFetch = globalThis.fetch; + capturedUrl = ""; + capturedMethod = ""; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + function mockOk(body: unknown, headers: Record = {}) { + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + capturedUrl = req.url; + capturedMethod = req.method; + return new Response(JSON.stringify(body), { + status: 200, + headers: { "Content-Type": "application/json", ...headers }, + }); + }); + } + + function mockSequential( + responses: Array<{ body: unknown; headers?: Record }> + ): { getCapturedUrls: () => string[] } { + const capturedUrls: string[] = []; + let callIndex = 0; + + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + capturedUrls.push(req.url); + + const resp = responses[callIndex]!; + callIndex += 1; + + return new Response(JSON.stringify(resp.body), { + status: 200, + headers: { "Content-Type": "application/json", ...resp.headers }, + }); + }); + + return { getCapturedUrls: () => capturedUrls }; + } + + const SPAN_META = { + fields: { + id: "string", + parent_span: "string", + "span.op": "string", + description: "string", + "span.duration": "duration", + timestamp: "date", + project: "string", + transaction: "string", + trace: "string", + }, + }; + + /** Generate N rows of fake span data */ + function makeSpanRows(n: number): Record[] { + return Array.from({ length: n }, (_, i) => ({ + id: `span-${i}`, + parent_span: `parent-${i}`, + "span.op": "http.client", + description: `GET /api/endpoint-${i}`, + "span.duration": 50 + i, + timestamp: "2024-01-15T00:00:00Z", + project: "my-project", + transaction: "/api/foo", + trace: `trace-${i}`, + })); + } + + test("hits /organizations/{org}/events/ with GET", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project"); + + expect(capturedMethod).toBe("GET"); + expect(capturedUrl).toContain("/api/0/organizations/my-org/events/"); + }); + + test("sends dataset=spans", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project"); + + expect(capturedUrl).toContain("dataset=spans"); + }); + + test("passes per_page capped at 100 when limit is higher", async () => { + const { getCapturedUrls } = mockSequential([ + { + body: { data: makeSpanRows(100), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeSpanRows(50), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + await listSpans("my-org", "my-project", { limit: 150 }); + + // Both pages should use per_page=100 + expect(getCapturedUrls()[0]).toContain("per_page=100"); + expect(getCapturedUrls()[1]).toContain("per_page=100"); + }); + + test("sends sort=-timestamp by default", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project"); + + expect(capturedUrl).toContain(`sort=${encodeURIComponent("-timestamp")}`); + }); + + test('sends sort=-span.duration for sort="duration"', async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { sort: "duration" }); + + expect(decodeURIComponent(capturedUrl)).toContain("sort=-span.duration"); + }); + + test("allProjects sends project=-1", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { allProjects: true }); + + expect(capturedUrl).toContain("project=-1"); + // Should NOT have project:my-project in query + expect(decodeURIComponent(capturedUrl)).not.toContain( + "project%3Amy-project" + ); + }); + + test("auto-paginates when limit > 100", async () => { + const { getCapturedUrls } = mockSequential([ + { + body: { data: makeSpanRows(100), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeSpanRows(50), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + const result = await listSpans("my-org", "my-project", { limit: 150 }); + + expect(result.data).toHaveLength(150); + expect(result.nextCursor).toBeUndefined(); + expect(getCapturedUrls()).toHaveLength(2); + }); + + test("trims results when overshoot", async () => { + mockSequential([ + { + body: { data: makeSpanRows(100), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:100:0"`, + }, + }, + { + body: { data: makeSpanRows(100), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="true"; cursor="0:200:0"`, + }, + }, + ]); + + const result = await listSpans("my-org", "my-project", { limit: 120 }); + + expect(result.data).toHaveLength(120); + expect(result.nextCursor).toBeUndefined(); + }); + + test("single-page fast path for limit <= 100", async () => { + const { getCapturedUrls } = mockSequential([ + { + body: { data: makeSpanRows(30), meta: SPAN_META }, + headers: { + Link: `; rel="next"; results="false"; cursor=""`, + }, + }, + ]); + + const result = await listSpans("my-org", "my-project", { limit: 30 }); + + expect(result.data).toHaveLength(30); + expect(getCapturedUrls()).toHaveLength(1); + expect(getCapturedUrls()[0]).toContain("per_page=30"); + }); + + test("passes extraFields when provided", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { + extraFields: ["span.self_time", "span.category"], + }); + + const decoded = decodeURIComponent(capturedUrl); + expect(decoded).toContain("field=span.self_time"); + expect(decoded).toContain("field=span.category"); + // Standard fields should still be present + expect(decoded).toContain("field=id"); + expect(decoded).toContain("field=span.op"); + }); + + test("non-numeric project slug goes in query as project:slug", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project"); + + expect(capturedUrl).toContain( + `query=${encodeURIComponent("project:my-project")}` + ); + // Should NOT appear as a separate project= param with the slug value + expect(capturedUrl).not.toMatch(/[?&]project=my-project/); + }); + + test("numeric project ID goes as project param", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "12345"); + + expect(capturedUrl).toContain("project=12345"); + expect(decodeURIComponent(capturedUrl)).not.toContain("project:12345"); + }); + + test("uses statsPeriod when no absolute range provided", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { statsPeriod: "1h" }); + + expect(capturedUrl).toContain("statsPeriod=1h"); + expect(capturedUrl).not.toContain("start="); + expect(capturedUrl).not.toContain("end="); + }); + + test("defaults statsPeriod to 7d when not provided", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project"); + + expect(capturedUrl).toContain("statsPeriod=7d"); + }); + + test("suppresses statsPeriod when start/end are present", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { + start: "2024-01-15T00:00:00Z", + end: "2024-01-16T00:00:00Z", + statsPeriod: "7d", + }); + + expect(capturedUrl).toContain( + `start=${encodeURIComponent("2024-01-15T00:00:00Z")}` + ); + expect(capturedUrl).toContain( + `end=${encodeURIComponent("2024-01-16T00:00:00Z")}` + ); + expect(capturedUrl).not.toContain("statsPeriod="); + }); + + test("passes cursor when provided", async () => { + mockOk({ data: [], meta: SPAN_META }); + + await listSpans("my-org", "my-project", { cursor: "0:50:0" }); + + expect(capturedUrl).toContain(`cursor=${encodeURIComponent("0:50:0")}`); + }); + + test("returns nextCursor from Link header", async () => { + const cursor = "0:10:0"; + mockOk( + { data: makeSpanRows(10), meta: SPAN_META }, + { + Link: `; rel="next"; results="true"; cursor="${cursor}"`, + } + ); + + const result = await listSpans("my-org", "my-project", { limit: 10 }); + + expect(result.nextCursor).toBe(cursor); + }); + + test("returns undefined nextCursor when results=false", async () => { + mockOk( + { data: [], meta: SPAN_META }, + { + Link: `; rel="next"; results="false"; cursor=""`, + } + ); + + const result = await listSpans("my-org", "my-project"); + + expect(result.nextCursor).toBeUndefined(); + }); +}); From 27e1e3a4eec8a18f0a7b12becbfdd6c483f7c824 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 19:04:01 +0000 Subject: [PATCH 3/7] fix(api): centralize 403 Forbidden enrichment with actionable hints (CLI-1JG) Previously only 4 of 60+ API call sites enriched 403 errors with scope/token guidance. All other endpoints surfaced a raw unhelpful 'You do not have permission to perform this action.' message. Add enrich403Detail() in infrastructure.ts as a centralized chokepoint that enriches all 403 ApiErrors with: - Env-var tokens: specific missing scopes + token settings URL - OAuth tokens: re-authentication suggestion Add enriched403 flag to ApiError to prevent double-enrichment where command-layer code already has specialized 403 handling. Remove redundant enrichListOrgsForbidden() from organizations.ts. Guard issue list's build403Detail() to skip scope hints when centralized enrichment already applied. --- src/commands/issue/list.ts | 28 +++++- src/lib/api/infrastructure.ts | 128 ++++++++++++++++++------- src/lib/api/organizations.ts | 71 ++------------ src/lib/errors.ts | 12 ++- test/lib/api/infrastructure.test.ts | 140 +++++++++++++++++++++++++++- 5 files changed, 279 insertions(+), 100 deletions(-) diff --git a/src/commands/issue/list.ts b/src/commands/issue/list.ts index ed337e30e..42b87070f 100644 --- a/src/commands/issue/list.ts +++ b/src/commands/issue/list.ts @@ -1101,11 +1101,17 @@ function enrichIssueListError( ); } if (error.status === 403) { + // Centralized 403 enrichment (infrastructure.ts) already added + // scope/token hints. Only append the project-membership hint. + const detail = error.enriched403 + ? appendProjectMembershipHint(error.detail) + : build403Detail(error.detail); throw new ApiError( error.message, error.status, - build403Detail(error.detail), - error.endpoint + detail, + error.endpoint, + true ); } } @@ -1170,6 +1176,17 @@ function build403Detail(originalDetail: unknown): string { return lines.join("\n "); } +/** + * Append a project membership verification hint to an already-enriched + * 403 detail string. Used when centralized enrichment (infrastructure.ts) + * has already added scope/token hints and we only need the issue-list-specific + * suggestion. + */ +function appendProjectMembershipHint(detail: string | undefined): string { + const base = detail ?? "You do not have permission to perform this action."; + return `${base}\n Verify project membership: sentry project list /`; +} + /** * Handle auto-detect, explicit, and project-search modes. * @@ -1327,13 +1344,16 @@ async function handleResolvedTargets( if (first.status === 400) { detail = build400Detail(first.detail, flags); } else if (first.status === 403) { - detail = build403Detail(first.detail); + detail = first.enriched403 + ? appendProjectMembershipHint(first.detail) + : build403Detail(first.detail); } throw new ApiError( `${prefix}: ${first.message}`, first.status, detail, - first.endpoint + first.endpoint, + first.enriched403 || first.status === 403 ); } diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index e43269117..1d274a0bd 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -11,6 +11,8 @@ import { parseSentryLinkHeader } from "@sentry/api"; import * as Sentry from "@sentry/node-core/light"; import type { z } from "zod"; +import { extractRequiredScopes } from "../api-scope.js"; +import { getActiveEnvVarName, isEnvTokenActive } from "../db/auth.js"; import { getEnv } from "../env.js"; import { ApiError, AuthError, stringifyUnknown } from "../errors.js"; import { resolveOrgRegion } from "../region.js"; @@ -20,6 +22,47 @@ import { getSdkConfig, } from "../sentry-client.js"; +/** + * Enrich a 403 Forbidden error detail with actionable guidance. + * + * For env-var tokens (SENTRY_AUTH_TOKEN / SENTRY_TOKEN): extracts the specific + * missing scope from the API response when available, otherwise suggests + * checking token scopes. Includes a link to the token settings page. + * + * For OAuth tokens: suggests the user may lack access and should re-authenticate. + * + * @param rawDetail - The original detail string from the API 403 response + * @returns Enriched detail string with actionable suggestions + */ +function enrich403Detail(rawDetail: string | undefined): string { + const lines: string[] = []; + if (rawDetail) { + lines.push(rawDetail, ""); + } + + if (isEnvTokenActive()) { + const scopes = extractRequiredScopes(rawDetail); + if (scopes.length > 0) { + lines.push( + `Your ${getActiveEnvVarName()} token is missing the required scope(s) '${scopes.join("', '")}'.` + ); + } else { + lines.push( + `Your ${getActiveEnvVarName()} token may lack the required scope for this operation.` + ); + } + lines.push( + "Check token scopes at: https://sentry.io/settings/auth-tokens/" + ); + } else { + lines.push( + "You may not have access to this resource.", + "Re-authenticate with: sentry auth login" + ); + } + return lines.join("\n "); +} + /** * Parse Sentry's RFC 5988 Link response header to extract pagination cursors. * @@ -72,10 +115,13 @@ export function throwApiError( ? stringifyUnknown((error as { detail: unknown }).detail) : stringifyUnknown(error); + const is403 = status === 403; throw new ApiError( `${context}: ${status} ${response.statusText ?? "Unknown"}`, status, - detail + is403 ? enrich403Detail(detail) : detail, + undefined, + is403 ); } @@ -303,38 +349,7 @@ export async function apiRequestToRegion( }); if (!response.ok) { - let detail: string | undefined; - try { - const text = await response.text(); - try { - const parsed = JSON.parse(text) as { detail?: string }; - detail = parsed.detail ?? JSON.stringify(parsed); - } catch { - detail = text; - } - } catch { - detail = response.statusText; - } - // Attach a small allowlisted subset of response headers to the Sentry - // event as context. This lets us distinguish Sentry-app 4xx/5xx (which - // ship a `{"detail": "..."}` JSON body and `content-type: application/json`) - // from CDN / WAF / edge 4xx (Cloudflare / proxy) that return empty or HTML - // bodies — a gap that previously made empty-`detail` events like CLI-1AZ - // impossible to triage without user-side repro. - Sentry.setContext("api_response_headers", { - "content-type": response.headers.get("content-type"), - "content-length": response.headers.get("content-length"), - server: response.headers.get("server"), - "cf-ray": response.headers.get("cf-ray"), - "x-sentry-error": response.headers.get("x-sentry-error"), - "www-authenticate": response.headers.get("www-authenticate"), - }); - throw new ApiError( - `API request failed: ${response.status} ${response.statusText}`, - response.status, - detail, - endpoint - ); + await throwRawApiError(response, endpoint); } // 204 No Content / 205 Reset Content have no body by spec — calling @@ -377,6 +392,53 @@ export async function apiRequestToRegion( return { data: data as T, headers: response.headers }; } +/** + * Extract error detail from a failed HTTP response, attach diagnostic + * headers to the Sentry scope, and throw an enriched {@link ApiError}. + * + * Extracted from `apiRequestToRegion` to keep the main function's + * cognitive complexity under the lint threshold. + */ +async function throwRawApiError( + response: Response, + endpoint: string +): Promise { + let detail: string | undefined; + try { + const text = await response.text(); + try { + const parsed = JSON.parse(text) as { detail?: string }; + detail = parsed.detail ?? JSON.stringify(parsed); + } catch { + detail = text; + } + } catch { + detail = response.statusText; + } + // Attach a small allowlisted subset of response headers to the Sentry + // event as context. This lets us distinguish Sentry-app 4xx/5xx (which + // ship a `{"detail": "..."}` JSON body and `content-type: application/json`) + // from CDN / WAF / edge 4xx (Cloudflare / proxy) that return empty or HTML + // bodies — a gap that previously made empty-`detail` events like CLI-1AZ + // impossible to triage without user-side repro. + Sentry.setContext("api_response_headers", { + "content-type": response.headers.get("content-type"), + "content-length": response.headers.get("content-length"), + server: response.headers.get("server"), + "cf-ray": response.headers.get("cf-ray"), + "x-sentry-error": response.headers.get("x-sentry-error"), + "www-authenticate": response.headers.get("www-authenticate"), + }); + const is403 = response.status === 403; + throw new ApiError( + `API request failed: ${response.status} ${response.statusText}`, + response.status, + is403 ? enrich403Detail(detail) : detail, + endpoint, + is403 + ); +} + /** * Make an authenticated request to the default Sentry API. * diff --git a/src/lib/api/organizations.ts b/src/lib/api/organizations.ts index f87c911f8..2aaedb8bc 100644 --- a/src/lib/api/organizations.ts +++ b/src/lib/api/organizations.ts @@ -16,8 +16,6 @@ import { UserRegionsResponseSchema, } from "../../types/index.js"; -import { extractRequiredScopes } from "../api-scope.js"; -import { getActiveEnvVarName, isEnvTokenActive } from "../db/auth.js"; import { ApiError, withAuthGuard } from "../errors.js"; import { getApiBaseUrl, @@ -62,67 +60,18 @@ export async function listOrganizationsInRegion( ...config, }); - try { - const data = unwrapResult(result, "Failed to list organizations"); - if (!Array.isArray(data)) { - throw new ApiError( - "Failed to list organizations: unexpected response format", - 0, - `Expected an array from ${regionUrl}/api/0/organizations/ but received ${typeof data}. ` + - "This may indicate an incompatible self-hosted Sentry version or a proxy interfering with the response." - ); - } - return data as unknown as SentryOrganization[]; - } catch (error) { - // Enrich 403 errors with contextual guidance (CLI-89, 24 users). - // Only mention token scopes when using a custom env-var token — - // the regular `sentry auth login` OAuth flow always grants org:read. - if (error instanceof ApiError && error.status === 403) { - throw enrichListOrgsForbidden(error); - } - throw error; - } -} - -/** - * Enrich a 403 from the list-organizations endpoint with actionable - * hints. Prefers scope(s) named explicitly in the API response over - * the hardcoded `'org:read'` fallback (getsentry/cli#785 item #9). - */ -function enrichListOrgsForbidden(error: ApiError): ApiError { - const lines: string[] = []; - if (error.detail) { - lines.push(error.detail, ""); - } - if (isEnvTokenActive()) { - lines.push(buildEnvTokenScopeHint(error.detail)); - lines.push( - "Check token scopes at: https://sentry.io/settings/auth-tokens/" + // 403 enrichment (CLI-89, 24 users) is now handled centrally by + // throwApiError() in infrastructure.ts — no per-endpoint catch needed. + const data = unwrapResult(result, "Failed to list organizations"); + if (!Array.isArray(data)) { + throw new ApiError( + "Failed to list organizations: unexpected response format", + 0, + `Expected an array from ${regionUrl}/api/0/organizations/ but received ${typeof data}. ` + + "This may indicate an incompatible self-hosted Sentry version or a proxy interfering with the response." ); - } else { - lines.push( - "You may not have access to this organization.", - "Re-authenticate with: sentry auth login" - ); - } - return new ApiError( - error.message, - error.status, - lines.join("\n "), - error.endpoint - ); -} - -/** - * Build a single-line hint mentioning the scope(s) the env-var token - * is missing, preferring the API-provided list when available. - */ -function buildEnvTokenScopeHint(detail: unknown): string { - const scopes = extractRequiredScopes(detail); - if (scopes.length > 0) { - return `Your ${getActiveEnvVarName()} token is missing the required scope(s) '${scopes.join("', '")}'.`; } - return `Your ${getActiveEnvVarName()} token may lack the required scope 'org:read'.`; + return data as unknown as SentryOrganization[]; } /** diff --git a/src/lib/errors.ts b/src/lib/errors.ts index fdb6fa0e3..c8938d6c4 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -177,17 +177,27 @@ export class ApiError extends CliError { readonly detail?: string; readonly endpoint?: string; + /** + * Set by centralized 403 enrichment in `infrastructure.ts`. + * Command-layer code can check this to avoid double-enriching + * the error detail with scope/token hints. + */ + readonly enriched403: boolean; + + // biome-ignore lint/nursery/useMaxParams: established 4-param shape; enriched403 is a defaulted extension constructor( message: string, status: number, detail?: string, - endpoint?: string + endpoint?: string, + enriched403 = false ) { super(message, EXIT.API); this.name = "ApiError"; this.status = status; this.detail = detail; this.endpoint = endpoint; + this.enriched403 = enriched403; } override format(): string { diff --git a/test/lib/api/infrastructure.test.ts b/test/lib/api/infrastructure.test.ts index 26a47fb6c..8c2ebc1ca 100644 --- a/test/lib/api/infrastructure.test.ts +++ b/test/lib/api/infrastructure.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { throwApiError } from "../../../src/lib/api/infrastructure.js"; import { ApiError } from "../../../src/lib/errors.js"; @@ -98,4 +98,142 @@ describe("throwApiError", () => { expect(apiError.detail).toContain("ECONNREFUSED"); } }); + + test("non-403 errors do not get enriched", () => { + const mockResponse = new Response("", { + status: 404, + statusText: "Not Found", + }); + + try { + throwApiError( + { detail: "Resource not found" }, + mockResponse, + "Failed to get org" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(false); + expect(apiError.detail).toBe("Resource not found"); + } + }); + + describe("403 enrichment", () => { + // Test preload sets SENTRY_AUTH_TOKEN, so isEnvTokenActive() returns true + // by default in these tests. + + test("enriches 403 with env-var token hints", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { detail: "You do not have permission to perform this action." }, + mockResponse, + "Failed to get organization" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(true); + expect(apiError.status).toBe(403); + expect(apiError.detail).toContain( + "You do not have permission to perform this action." + ); + expect(apiError.detail).toContain("SENTRY_AUTH_TOKEN"); + expect(apiError.detail).toContain( + "https://sentry.io/settings/auth-tokens/" + ); + } + }); + + test("extracts specific scope names when present in detail", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { + detail: + "You do not have permission. Required scope: org:read, project:read", + }, + mockResponse, + "Failed to list issues" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(true); + expect(apiError.detail).toContain( + "missing the required scope(s) 'org:read', 'project:read'" + ); + } + }); + + test("uses generic scope hint when no scope names in detail", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { detail: "You do not have permission to perform this action." }, + mockResponse, + "Failed to get organization" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.detail).toContain( + "may lack the required scope for this operation" + ); + } + }); + + describe("with OAuth token (no env var)", () => { + let savedToken: string | undefined; + + beforeEach(() => { + savedToken = process.env.SENTRY_AUTH_TOKEN; + delete process.env.SENTRY_AUTH_TOKEN; + delete process.env.SENTRY_TOKEN; + }); + + afterEach(() => { + if (savedToken !== undefined) { + process.env.SENTRY_AUTH_TOKEN = savedToken; + } else { + delete process.env.SENTRY_AUTH_TOKEN; + } + }); + + test("suggests re-authentication for OAuth tokens", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { + detail: "You do not have permission to perform this action.", + }, + mockResponse, + "Failed to get organization" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(true); + expect(apiError.detail).toContain( + "You may not have access to this resource." + ); + expect(apiError.detail).toContain("sentry auth login"); + // Should NOT mention SENTRY_AUTH_TOKEN + expect(apiError.detail).not.toContain("SENTRY_AUTH_TOKEN"); + } + }); + }); + }); }); From 0355e716f50e5314ca22f8046cc923ab658af243 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 19:46:12 +0000 Subject: [PATCH 4/7] fix: guard against nullish detail producing 'undefined' string in 403 enrichment When the API returns { detail: null } or { detail: undefined }, stringifyUnknown() would produce the literal string 'undefined' or 'null', which leaked into the enriched error message. Now falls back to stringifying the whole error object instead. Also add tests for undefined/null detail edge cases and fix env var cleanup symmetry in OAuth test. --- src/lib/api/infrastructure.ts | 11 ++++-- test/lib/api/infrastructure.test.ts | 55 +++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index 1d274a0bd..1e4c9f0b0 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -110,9 +110,16 @@ export function throwApiError( } const status = response.status; - const detail = + const rawDetail = error && typeof error === "object" && "detail" in error - ? stringifyUnknown((error as { detail: unknown }).detail) + ? (error as { detail: unknown }).detail + : undefined; + // When the API returns `{ detail: null }` or `{ detail: undefined }`, + // fall back to stringifying the whole error object rather than producing + // the misleading string literal "undefined" / "null". + const detail = + rawDetail !== null && rawDetail !== undefined + ? stringifyUnknown(rawDetail) : stringifyUnknown(error); const is403 = status === 403; diff --git a/test/lib/api/infrastructure.test.ts b/test/lib/api/infrastructure.test.ts index 8c2ebc1ca..e042ceafd 100644 --- a/test/lib/api/infrastructure.test.ts +++ b/test/lib/api/infrastructure.test.ts @@ -192,21 +192,70 @@ describe("throwApiError", () => { } }); + test("handles undefined detail without producing 'undefined' string", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { detail: undefined }, + mockResponse, + "Failed to get organization" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(true); + // Should contain enrichment hints + expect(apiError.detail).toContain("SENTRY_AUTH_TOKEN"); + // Should NOT contain the literal string "undefined" as output + expect(apiError.detail).not.toMatch(/^undefined\n/); + } + }); + + test("handles null detail without producing 'null' string", () => { + const mockResponse = new Response("", { + status: 403, + statusText: "Forbidden", + }); + + try { + throwApiError( + { detail: null }, + mockResponse, + "Failed to get organization" + ); + } catch (error) { + const apiError = error as ApiError; + expect(apiError.enriched403).toBe(true); + expect(apiError.detail).toContain("SENTRY_AUTH_TOKEN"); + expect(apiError.detail).not.toMatch(/^null\n/); + } + }); + describe("with OAuth token (no env var)", () => { + let savedAuthToken: string | undefined; let savedToken: string | undefined; beforeEach(() => { - savedToken = process.env.SENTRY_AUTH_TOKEN; + savedAuthToken = process.env.SENTRY_AUTH_TOKEN; + savedToken = process.env.SENTRY_TOKEN; delete process.env.SENTRY_AUTH_TOKEN; delete process.env.SENTRY_TOKEN; }); afterEach(() => { - if (savedToken !== undefined) { - process.env.SENTRY_AUTH_TOKEN = savedToken; + if (savedAuthToken !== undefined) { + process.env.SENTRY_AUTH_TOKEN = savedAuthToken; } else { delete process.env.SENTRY_AUTH_TOKEN; } + if (savedToken !== undefined) { + process.env.SENTRY_TOKEN = savedToken; + } else { + delete process.env.SENTRY_TOKEN; + } }); test("suggests re-authentication for OAuth tokens", () => { From a52573fd1e0addf6c3b80f5ecc66b9f4b5e4f989 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 19:53:35 +0000 Subject: [PATCH 5/7] fix(dashboard): skip ResolutionError conversion when 403 is already enriched When centralized 403 enrichment has already added scope/token hints, re-throw the ApiError directly instead of passing the multi-line enriched detail into build403Error, which would nest it as a single messy bullet in a ResolutionError. --- src/commands/dashboard/resolve.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/commands/dashboard/resolve.ts b/src/commands/dashboard/resolve.ts index 6d6693382..58f19f694 100644 --- a/src/commands/dashboard/resolve.ts +++ b/src/commands/dashboard/resolve.ts @@ -636,6 +636,13 @@ export function enrichDashboardError( } if (error.status === 403) { + // Centralized 403 enrichment (infrastructure.ts) already added + // scope/token hints. Re-throw the enriched ApiError directly — + // passing the multi-line enriched detail into build403Error would + // nest it as a single messy bullet in a ResolutionError. + if (error.enriched403) { + throw error; + } build403Error(ctx, org, error.detail); } From caa952b330806115eb8ca25c6e9b5bc393c5fc20 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 20:17:27 +0000 Subject: [PATCH 6/7] fix: avoid noisy "{}" prefix in 403 enrichment when API detail is nullish When throwApiError receives { detail: undefined } or { detail: null }, the fallback stringifyUnknown(error) produces "{}" or {"detail":null} which leaked as the first visible line before the enrichment hints. Now passes undefined to enrich403Detail for 403s with nullish detail, so the enrichment stands alone without a noisy prefix. Non-403 errors keep the fallback behavior for debugging. --- src/lib/api/infrastructure.ts | 15 ++++++++------- test/lib/api/infrastructure.test.ts | 13 ++++++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index 1e4c9f0b0..3f97b9bfb 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -114,19 +114,20 @@ export function throwApiError( error && typeof error === "object" && "detail" in error ? (error as { detail: unknown }).detail : undefined; + const hasUsableDetail = rawDetail !== null && rawDetail !== undefined; // When the API returns `{ detail: null }` or `{ detail: undefined }`, - // fall back to stringifying the whole error object rather than producing - // the misleading string literal "undefined" / "null". - const detail = - rawDetail !== null && rawDetail !== undefined - ? stringifyUnknown(rawDetail) - : stringifyUnknown(error); + // fall back to stringifying the whole error object for non-403 errors + // (useful for debugging). For 403s, pass undefined to enrich403Detail + // so the enrichment stands alone without a noisy `{}` prefix. + const detail = hasUsableDetail + ? stringifyUnknown(rawDetail) + : stringifyUnknown(error); const is403 = status === 403; throw new ApiError( `${context}: ${status} ${response.statusText ?? "Unknown"}`, status, - is403 ? enrich403Detail(detail) : detail, + is403 ? enrich403Detail(hasUsableDetail ? detail : undefined) : detail, undefined, is403 ); diff --git a/test/lib/api/infrastructure.test.ts b/test/lib/api/infrastructure.test.ts index e042ceafd..9a1265c7f 100644 --- a/test/lib/api/infrastructure.test.ts +++ b/test/lib/api/infrastructure.test.ts @@ -192,7 +192,7 @@ describe("throwApiError", () => { } }); - test("handles undefined detail without producing 'undefined' string", () => { + test("handles undefined detail without producing noise", () => { const mockResponse = new Response("", { status: 403, statusText: "Forbidden", @@ -209,12 +209,13 @@ describe("throwApiError", () => { expect(apiError.enriched403).toBe(true); // Should contain enrichment hints expect(apiError.detail).toContain("SENTRY_AUTH_TOKEN"); - // Should NOT contain the literal string "undefined" as output - expect(apiError.detail).not.toMatch(/^undefined\n/); + // Should NOT contain noisy fallback strings + expect(apiError.detail).not.toMatch(/^undefined/); + expect(apiError.detail).not.toContain("{}"); } }); - test("handles null detail without producing 'null' string", () => { + test("handles null detail without producing noise", () => { const mockResponse = new Response("", { status: 403, statusText: "Forbidden", @@ -230,7 +231,9 @@ describe("throwApiError", () => { const apiError = error as ApiError; expect(apiError.enriched403).toBe(true); expect(apiError.detail).toContain("SENTRY_AUTH_TOKEN"); - expect(apiError.detail).not.toMatch(/^null\n/); + // Should NOT contain noisy fallback strings + expect(apiError.detail).not.toMatch(/^null/); + expect(apiError.detail).not.toContain('{"detail":null}'); } }); From 9222569e07bb6281436d404bd5762392473fea05 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 30 Apr 2026 20:27:19 +0000 Subject: [PATCH 7/7] fix: apply same nullish detail guard to throwRawApiError (Bugbot) The nullish detail fix from the previous commit only covered throwApiError (SDK path). throwRawApiError (raw fetch path via apiRequestToRegion) had the same issue: { detail: null } would produce {"detail":null} as visible output in 403 enrichment. Now checks typeof parsed.detail before using it, and skips the JSON.stringify fallback for 403 errors. --- src/lib/api/infrastructure.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/lib/api/infrastructure.ts b/src/lib/api/infrastructure.ts index 3f97b9bfb..b8407598a 100644 --- a/src/lib/api/infrastructure.ts +++ b/src/lib/api/infrastructure.ts @@ -416,9 +416,17 @@ async function throwRawApiError( const text = await response.text(); try { const parsed = JSON.parse(text) as { detail?: string }; - detail = parsed.detail ?? JSON.stringify(parsed); + // Prefer the explicit `detail` field; fall back to the full JSON + // for non-403 errors (useful for debugging). For 403s, pass + // undefined so enrich403Detail stands alone without a noisy + // `{"detail":null}` prefix. + if (typeof parsed.detail === "string") { + detail = parsed.detail; + } else if (response.status !== 403) { + detail = JSON.stringify(parsed); + } } catch { - detail = text; + detail = text || undefined; } } catch { detail = response.statusText;