From 8a25d9e6b1c30511c4c0789277767cdc0c985a74 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:38:57 +0000 Subject: [PATCH 01/10] fix(db): guard corrupt cache JSON and log silent catch blocks Add a shared safeParseJson() helper and use it in getPaginationState() and getCachedDetection() so corrupt cached JSON (partial writes, manual edits, schema drift) is treated as a cache miss instead of crashing the command with an unhandled SyntaxError. Pagination state additionally validates Array.isArray and clears the bad row. Replace three silent catch blocks in sentry-client.ts (token refresh, response cache write, post-mutation invalidation) with log.debug() so errors are visible in debug output per the AGENTS.md no-silent-catch rule. Consolidates the cache-hardening fixes from #908, #947, and #1044. --- src/lib/db/dsn-cache.ts | 28 ++++++------ src/lib/db/json.ts | 53 +++++++++++++++++++++++ src/lib/db/pagination.ts | 16 ++++++- src/lib/sentry-client.ts | 12 +++--- test/lib/db/cache-corruption.test.ts | 64 ++++++++++++++++++++++++++++ test/lib/db/json.test.ts | 32 ++++++++++++++ 6 files changed, 186 insertions(+), 19 deletions(-) create mode 100644 src/lib/db/json.ts create mode 100644 test/lib/db/cache-corruption.test.ts create mode 100644 test/lib/db/json.test.ts diff --git a/src/lib/db/dsn-cache.ts b/src/lib/db/dsn-cache.ts index 3fe87281d..e00099c8b 100644 --- a/src/lib/db/dsn-cache.ts +++ b/src/lib/db/dsn-cache.ts @@ -15,6 +15,7 @@ import type { } from "../dsn/types.js"; import { recordCacheHit } from "../telemetry.js"; import { getDatabase, maybeCleanupCaches } from "./index.js"; +import { safeParseJson } from "./json.js"; import { runUpsert, touchCacheEntry } from "./utils.js"; /** Cache TTL in milliseconds (24 hours) */ @@ -362,20 +363,26 @@ export async function getCachedDetection( return; } - // Parse source mtimes and validate - const sourceMtimes = JSON.parse(row.source_mtimes_json) as Record< - string, - number - >; + // Parse all cached JSON columns up front. Corruption (partial write, manual + // edit, schema drift) is treated as a cache miss so detection re-runs from + // scratch instead of crashing. + const sourceMtimes = safeParseJson>( + row.source_mtimes_json + ); + const dirMtimes = row.dir_mtimes_json + ? safeParseJson>(row.dir_mtimes_json) + : {}; + const allDsns = safeParseJson(row.all_dsns_json); + if (sourceMtimes === undefined || dirMtimes === undefined || !allDsns) { + recordCacheHit("dsn-detection", false); + return; + } + if (!(await validateSourceMtimes(projectRoot, sourceMtimes))) { recordCacheHit("dsn-detection", false); return; } - // Parse directory mtimes and validate (if present) - const dirMtimes = row.dir_mtimes_json - ? (JSON.parse(row.dir_mtimes_json) as Record) - : {}; if (!(await validateDirMtimes(projectRoot, dirMtimes))) { recordCacheHit("dsn-detection", false); return; @@ -385,9 +392,6 @@ export async function getCachedDetection( // Cache is valid - update last access time touchCacheEntry("dsn_cache", "directory", projectRoot); - // Parse and return cached detection - const allDsns = JSON.parse(row.all_dsns_json) as DetectedDsn[]; - return { fingerprint: row.fingerprint, allDsns, diff --git a/src/lib/db/json.ts b/src/lib/db/json.ts new file mode 100644 index 000000000..8641e036d --- /dev/null +++ b/src/lib/db/json.ts @@ -0,0 +1,53 @@ +/** + * Safe JSON parsing for values read from the SQLite cache layer. + * + * Cached JSON columns can be corrupted by partial writes, manual DB edits, or + * incompatible schema migrations. A bare `JSON.parse` on such data throws a + * `SyntaxError` that crashes whatever command triggered the cache read. This + * module centralizes the defensive parse so every cache reader treats + * corruption as a cache miss instead of a fatal error. + */ + +import { logger } from "../logger.js"; + +const log = logger.withTag("db-json"); + +/** + * Parse a JSON string read from the cache, returning `undefined` on failure. + * + * Failure modes handled: + * - `raw` is `null`/`undefined` (column was never written) → `undefined`. + * - `JSON.parse` throws (corrupt JSON) → logged at debug level → `undefined`. + * - An optional `validate` predicate rejects the parsed shape → `undefined`. + * + * Callers should treat `undefined` as a cache miss and recompute the value. + * + * @typeParam T - Expected shape of the parsed value. + * @param raw - Raw JSON string from a SQLite column (may be null/undefined). + * @param validate - Optional type guard run against the parsed value; when it + * returns `false` the result is discarded and `undefined` is returned. + * @returns The parsed value, or `undefined` if parsing/validation failed. + */ +export function safeParseJson( + raw: string | null | undefined, + validate?: (value: unknown) => value is T +): T | undefined { + if (raw === null || raw === undefined) { + return; + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch (error) { + log.debug("Failed to parse cached JSON; treating as cache miss", error); + return; + } + + if (validate && !validate(parsed)) { + log.debug("Cached JSON failed validation; treating as cache miss"); + return; + } + + return parsed as T; +} diff --git a/src/lib/db/pagination.ts b/src/lib/db/pagination.ts index c702918d9..bb7359c45 100644 --- a/src/lib/db/pagination.ts +++ b/src/lib/db/pagination.ts @@ -16,6 +16,7 @@ import { ValidationError } from "../errors.js"; import { CURSOR_KEYWORDS } from "../list-command.js"; import { getApiBaseUrl } from "../sentry-client.js"; import { getDatabase } from "./index.js"; +import { safeParseJson } from "./json.js"; import { runUpsert } from "./utils.js"; /** Default TTL for stored cursors: 5 minutes */ @@ -84,7 +85,20 @@ export function getPaginationState( return; } - const stack = JSON.parse(row.cursor_stack) as string[]; + // Corrupt cursor stacks (partial writes, manual edits, migration drift) must + // not crash paginated commands. Treat any parse/validation failure as a fresh + // first page by deleting the bad row and returning a cache miss. + const stack = safeParseJson( + row.cursor_stack, + (value): value is string[] => Array.isArray(value) + ); + if (!stack) { + db.query( + "DELETE FROM pagination_cursors WHERE command_key = ? AND context = ?" + ).run(commandKey, contextKey); + return; + } + return { stack, index: row.page_index }; } diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 026e8400b..3531f1d3f 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -188,8 +188,8 @@ async function handleUnauthorized(headers: Headers): Promise { headers.set(RETRY_MARKER_HEADER, "1"); return true; } - } catch { - // Token refresh failed + } catch (error) { + log.debug("Token refresh failed after 401", error); } return false; } @@ -420,8 +420,8 @@ function cacheResponse( fullUrl, requestHeaders, response.clone() as Response - ).catch(() => { - // Non-fatal: cache write failures don't affect the response + ).catch((error) => { + log.debug("Response cache write failed", error); }); } @@ -448,8 +448,8 @@ async function invalidateAfterMutation( await Promise.all( prefixes.map((prefix) => invalidateCachedResponsesMatching(prefix)) ); - } catch { - /* best-effort: mutation already succeeded upstream */ + } catch (error) { + log.debug("Post-mutation cache invalidation failed", error); } } diff --git a/test/lib/db/cache-corruption.test.ts b/test/lib/db/cache-corruption.test.ts new file mode 100644 index 000000000..d8eea0144 --- /dev/null +++ b/test/lib/db/cache-corruption.test.ts @@ -0,0 +1,64 @@ +/** + * Corruption-resilience tests for the SQLite cache readers. + * + * Cached JSON columns can be corrupted by partial writes, manual edits, or + * schema drift. These tests assert that the readers treat such corruption as a + * cache miss (returning undefined and clearing the bad row where applicable) + * instead of throwing a SyntaxError that would crash the command. + */ + +import { describe, expect, test } from "vitest"; +import { getDatabase } from "../../../src/lib/db/index.js"; +import { getPaginationState } from "../../../src/lib/db/pagination.js"; +import { useTestConfigDir } from "../../helpers.js"; + +useTestConfigDir("cache-corruption-"); + +const CMD_KEY = "corrupt-list"; +const CTX_KEY = "org:test|sort:date"; + +/** Insert a raw pagination row with the given (possibly invalid) cursor_stack. */ +function insertPaginationRow(cursorStack: string): void { + const db = getDatabase(); + db.query( + `INSERT OR REPLACE INTO pagination_cursors + (command_key, context, cursor_stack, page_index, expires_at) + VALUES (?, ?, ?, ?, ?)` + ).run(CMD_KEY, CTX_KEY, cursorStack, 0, Date.now() + 60_000); +} + +function paginationRowCount(): number { + const db = getDatabase(); + const row = db + .query( + "SELECT COUNT(*) AS n FROM pagination_cursors WHERE command_key = ? AND context = ?" + ) + .get(CMD_KEY, CTX_KEY) as { n: number }; + return row.n; +} + +describe("getPaginationState corruption handling", () => { + test("returns undefined and deletes the row on unparseable JSON", () => { + insertPaginationRow("{not valid json"); + expect(paginationRowCount()).toBe(1); + + expect(getPaginationState(CMD_KEY, CTX_KEY)).toBeUndefined(); + expect(paginationRowCount()).toBe(0); + }); + + test("returns undefined and deletes the row when JSON is not an array", () => { + insertPaginationRow(JSON.stringify({ not: "an array" })); + expect(paginationRowCount()).toBe(1); + + expect(getPaginationState(CMD_KEY, CTX_KEY)).toBeUndefined(); + expect(paginationRowCount()).toBe(0); + }); + + test("returns the parsed stack for valid array JSON", () => { + insertPaginationRow(JSON.stringify(["", "cursor-1"])); + + const state = getPaginationState(CMD_KEY, CTX_KEY); + expect(state?.stack).toEqual(["", "cursor-1"]); + expect(state?.index).toBe(0); + }); +}); diff --git a/test/lib/db/json.test.ts b/test/lib/db/json.test.ts new file mode 100644 index 000000000..f30110d80 --- /dev/null +++ b/test/lib/db/json.test.ts @@ -0,0 +1,32 @@ +/** + * Unit tests for the safeParseJson cache helper. + */ + +import { describe, expect, test } from "vitest"; +import { safeParseJson } from "../../../src/lib/db/json.js"; + +describe("safeParseJson", () => { + test("parses valid JSON", () => { + expect(safeParseJson<{ a: number }>('{"a":1}')).toEqual({ a: 1 }); + expect(safeParseJson("[1,2,3]")).toEqual([1, 2, 3]); + }); + + test("returns undefined for null/undefined input", () => { + expect(safeParseJson(null)).toBeUndefined(); + expect(safeParseJson(undefined)).toBeUndefined(); + }); + + test("returns undefined for unparseable JSON instead of throwing", () => { + expect(safeParseJson("{not json")).toBeUndefined(); + expect(safeParseJson("")).toBeUndefined(); + }); + + test("returns undefined when the validator rejects the parsed value", () => { + const isStringArray = (v: unknown): v is string[] => + Array.isArray(v) && v.every((x) => typeof x === "string"); + + expect(safeParseJson('["a","b"]', isStringArray)).toEqual(["a", "b"]); + expect(safeParseJson('{"a":1}', isStringArray)).toBeUndefined(); + expect(safeParseJson("[1,2]", isStringArray)).toBeUndefined(); + }); +}); From d48dfec50071545d1255b27bc5264cfc1bc96af8 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:04 +0000 Subject: [PATCH 02/10] fix(events): use autoPaginate so overshoot never skips events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit listIssueEvents hand-rolled its pagination loop and, when a page overshot the requested limit, trimmed the data but kept nextCursor — which pointed past the trimmed tail and made -c next navigation skip events. Refactor onto the shared autoPaginate() helper and, because the events endpoint has no per-page param and its single page can still overshoot, trim-and-drop-nextCursor in the wrapper. Add a property test pinning autoPaginate's multi-page trim-drops-cursor invariant and a focused regression test for the events overshoot case. Consolidates #947's event-pagination fix. --- src/lib/api/events.ts | 46 +++++---- test/lib/api/events-overshoot.test.ts | 95 ++++++++++++++++++ test/lib/auto-paginate.property.test.ts | 122 ++++++++++++++++++++++++ 3 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 test/lib/api/events-overshoot.test.ts create mode 100644 test/lib/auto-paginate.property.test.ts diff --git a/src/lib/api/events.ts b/src/lib/api/events.ts index 22e2f5c91..3e2a1058c 100644 --- a/src/lib/api/events.ts +++ b/src/lib/api/events.ts @@ -18,8 +18,8 @@ import { ApiError, AuthError } from "../errors.js"; import { API_MAX_PER_PAGE, + autoPaginate, getOrgSdkConfig, - MAX_PAGINATION_PAGES, ORG_FANOUT_CONCURRENCY, type PaginatedResponse, unwrapPaginatedResult, @@ -198,8 +198,10 @@ export type ListIssueEventsOptions = { * List events for a specific issue. * * Uses the SDK's `listAnIssue_sEvents` endpoint with region-aware routing. - * When `limit` exceeds {@link API_MAX_PER_PAGE} (100), auto-paginates through - * multiple API calls to fill the requested limit, bounded by {@link MAX_PAGINATION_PAGES}. + * When `limit` exceeds {@link API_MAX_PER_PAGE} (100), {@link autoPaginate} + * fetches multiple API pages to fill the requested limit. Because the endpoint + * has no per-page parameter, a page may overshoot `limit`; the result is trimmed + * and `nextCursor` dropped so cursor navigation never skips events. * * @param orgSlug - Organization slug for region routing * @param issueId - Numeric issue ID @@ -215,11 +217,9 @@ export async function listIssueEvents( const config = await getOrgSdkConfig(orgSlug); - const allEvents: IssueEvent[] = []; - let currentCursor = cursor; - let nextCursor: string | undefined; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { + const fetchPage = async ( + pageCursor: string | undefined + ): Promise> => { const result = await listAnIssue_sEvents({ ...config, path: { @@ -229,7 +229,7 @@ export async function listIssueEvents( query: { query: query || undefined, full, - cursor: currentCursor, + cursor: pageCursor, statsPeriod, start, end, @@ -240,22 +240,20 @@ export async function listIssueEvents( result, "Failed to list issue events" ); + return { + data: paginated.data as IssueEvent[], + nextCursor: paginated.nextCursor, + }; + }; - allEvents.push(...(paginated.data as IssueEvent[])); - nextCursor = paginated.nextCursor; + const result = await autoPaginate(fetchPage, limit, cursor); - if (allEvents.length >= limit || !nextCursor) { - break; - } - currentCursor = nextCursor; + // The issue events endpoint has no per-page parameter, so a single page can + // already overshoot `limit` (autoPaginate's fast path returns it untrimmed). + // When trimming we MUST drop nextCursor — it points past the trimmed tail and + // would make `-c next` skip the events between the trim point and that cursor. + if (result.data.length > limit) { + return { data: result.data.slice(0, limit) }; } - - // Trim to exact limit. Unlike listIssuesAllPages (which controls per_page), - // the issue events endpoint has no per-page parameter, so the API may return - // more items than requested. We preserve nextCursor so the command-level - // cursor stack can navigate to subsequent pages. - const trimmed = - allEvents.length > limit ? allEvents.slice(0, limit) : allEvents; - - return { data: trimmed, nextCursor }; + return result; } diff --git a/test/lib/api/events-overshoot.test.ts b/test/lib/api/events-overshoot.test.ts new file mode 100644 index 000000000..c40f6005e --- /dev/null +++ b/test/lib/api/events-overshoot.test.ts @@ -0,0 +1,95 @@ +/** + * Regression tests for listIssueEvents pagination overshoot. + * + * The issue events endpoint has no per-page parameter, so a single API page can + * return more events than the caller's `limit`. When that happens listIssueEvents + * MUST trim to `limit` AND drop nextCursor — returning a cursor that points past + * the trimmed tail would make `-c next` navigation skip the events between the + * trim point and that cursor. + */ + +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { listIssueEvents } from "../../../src/lib/api/events.js"; +import { setAuthToken } from "../../../src/lib/db/auth.js"; +import { setOrgRegion } from "../../../src/lib/db/regions.js"; +import { mockFetch, useTestConfigDir } from "../../helpers.js"; + +useTestConfigDir("test-events-overshoot-"); +let originalFetch: typeof globalThis.fetch; + +beforeEach(async () => { + originalFetch = globalThis.fetch; + await setAuthToken("test-token"); + setOrgRegion("test-org", "https://sentry.io"); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +/** Build a Sentry pagination Link header advertising a next page. */ +function nextLinkHeader(cursor: string): string { + return `; rel="next"; results="true"; cursor="${cursor}"`; +} + +function makeEvents(count: number): Array<{ id: string }> { + return Array.from({ length: count }, (_unused, i) => ({ id: `evt-${i}` })); +} + +describe("listIssueEvents overshoot handling", () => { + test("trims to limit and drops nextCursor when a page overshoots", async () => { + // Single page returns 5 events with a next cursor, but caller wants 2. + globalThis.fetch = mockFetch( + async () => + new Response(JSON.stringify(makeEvents(5)), { + status: 200, + headers: { + "Content-Type": "application/json", + Link: nextLinkHeader("page2"), + }, + }) + ); + + const result = await listIssueEvents("test-org", "123", { limit: 2 }); + + expect(result.data).toHaveLength(2); + expect(result.data[0]?.id).toBe("evt-0"); + expect(result.data[1]?.id).toBe("evt-1"); + // Critical: cursor dropped so no events are skipped on the next navigation. + expect(result.nextCursor).toBeUndefined(); + }); + + test("preserves nextCursor when the page exactly fills the limit", async () => { + globalThis.fetch = mockFetch( + async () => + new Response(JSON.stringify(makeEvents(2)), { + status: 200, + headers: { + "Content-Type": "application/json", + Link: nextLinkHeader("page2"), + }, + }) + ); + + const result = await listIssueEvents("test-org", "123", { limit: 2 }); + + expect(result.data).toHaveLength(2); + // No trim occurred, so the cursor remains valid for the next page. + expect(result.nextCursor).toBe("page2"); + }); + + test("returns all events with no cursor when under the limit", async () => { + globalThis.fetch = mockFetch( + async () => + new Response(JSON.stringify(makeEvents(3)), { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + ); + + const result = await listIssueEvents("test-org", "123", { limit: 25 }); + + expect(result.data).toHaveLength(3); + expect(result.nextCursor).toBeUndefined(); + }); +}); diff --git a/test/lib/auto-paginate.property.test.ts b/test/lib/auto-paginate.property.test.ts new file mode 100644 index 000000000..8e8665136 --- /dev/null +++ b/test/lib/auto-paginate.property.test.ts @@ -0,0 +1,122 @@ +/** + * Property-based tests for autoPaginate(). + * + * autoPaginate has two paths with DIFFERENT trimming contracts: + * + * - Fast path (`limit <= API_MAX_PER_PAGE`): returns a single page verbatim, + * WITHOUT trimming — so a page that overshoots `limit` is returned as-is with + * its nextCursor. Callers whose endpoint can overshoot a single page (e.g. + * `listIssueEvents`, which has no per-page param) must trim-and-drop-cursor + * themselves; that behavior is covered by `events-overshoot.test.ts`. + * - Multi-page path (`limit > API_MAX_PER_PAGE`): accumulates across pages and, + * on overshoot, trims to `limit` AND drops nextCursor so cursor navigation + * never skips the rows between the trim point and the dropped cursor. + * + * These tests pin the multi-page contract, the source of the original bug class. + */ + +import { asyncProperty, assert as fcAssert, integer, nat } from "fast-check"; +import { describe, expect, test } from "vitest"; +import { + API_MAX_PER_PAGE, + autoPaginate, + type PaginatedResponse, +} from "../../src/lib/api/infrastructure.js"; +import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; + +/** + * Build a paginated fetcher over a fixed pool of sequential numbers, served in + * pages of `pageSize` (capped at API_MAX_PER_PAGE to mimic a real endpoint). + */ +function makePagedFetcher(total: number, pageSize: number) { + const cappedPageSize = Math.min(pageSize, API_MAX_PER_PAGE); + return (cursor: string | undefined): Promise> => { + const offset = cursor ? Number(cursor) : 0; + const data = Array.from( + { length: Math.min(cappedPageSize, Math.max(0, total - offset)) }, + (_unused, i) => offset + i + ); + const nextOffset = offset + data.length; + const nextCursor = nextOffset < total ? String(nextOffset) : undefined; + return Promise.resolve({ data, nextCursor }); + }; +} + +// Multi-page path requires limit > API_MAX_PER_PAGE. Real callers cap perPage at +// min(limit, API_MAX_PER_PAGE) and the multi-page cap is MAX_PAGINATION_PAGES, so +// the achievable ceiling is API_MAX_PER_PAGE * MAX_PAGINATION_PAGES. We keep the +// limit comfortably under that ceiling and use realistic (large) page sizes so +// the page-cap safety path is not the thing under test here. +const multiPageLimit = integer({ + min: API_MAX_PER_PAGE + 1, + max: API_MAX_PER_PAGE * 4, +}); +const pageSizeArb = integer({ + min: Math.floor(API_MAX_PER_PAGE / 2), + max: API_MAX_PER_PAGE, +}); + +describe("property: autoPaginate multi-page contract", () => { + test("never returns more than limit items", () => { + fcAssert( + asyncProperty( + nat(API_MAX_PER_PAGE * 5), + multiPageLimit, + pageSizeArb, + async (total, limit, pageSize) => { + const result = await autoPaginate( + makePagedFetcher(total, pageSize), + limit + ); + expect(result.data.length).toBeLessThanOrEqual(limit); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("drops nextCursor whenever the result is trimmed to limit", () => { + fcAssert( + asyncProperty( + nat(API_MAX_PER_PAGE * 5), + multiPageLimit, + pageSizeArb, + async (total, limit, pageSize) => { + const result = await autoPaginate( + makePagedFetcher(total, pageSize), + limit + ); + // When more rows exist than we returned, the data must be exactly + // `limit` long AND nextCursor undefined — a non-undefined cursor would + // skip the rows between the trim point and that cursor. + if (total > limit) { + expect(result.data.length).toBe(limit); + expect(result.nextCursor).toBeUndefined(); + } + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("accumulated rows are contiguous from offset 0 (no skips/dupes)", () => { + fcAssert( + asyncProperty( + nat(API_MAX_PER_PAGE * 5), + multiPageLimit, + pageSizeArb, + async (total, limit, pageSize) => { + const result = await autoPaginate( + makePagedFetcher(total, pageSize), + limit + ); + const expectedLen = Math.min(total, limit); + expect(result.data).toEqual( + Array.from({ length: expectedLen }, (_unused, i) => i) + ); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); +}); From 8ed120db02c59f48236aad1bdfd4307f3a03fff1 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:10 +0000 Subject: [PATCH 03/10] fix(formatters): clamp negative relative times to '0m ago' Clock skew, scheduled/future timestamps, or bad API data could make diffMs negative and render as '-5m ago'. Clamp to >= 0 so future dates display as '0m ago'. Consolidates #1044's relative-time fix. --- src/lib/formatters/time-utils.ts | 4 +++- test/lib/formatters/human.utils.test.ts | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/lib/formatters/time-utils.ts b/src/lib/formatters/time-utils.ts index 3202d8d1a..16529223a 100644 --- a/src/lib/formatters/time-utils.ts +++ b/src/lib/formatters/time-utils.ts @@ -31,7 +31,9 @@ export function formatRelativeTime(dateString: string | undefined): string { const date = new Date(dateString); const now = Date.now(); - const diffMs = now - date.getTime(); + // Clamp to >= 0 so clock skew, scheduled/future timestamps, or bad API data + // render as "0m ago" instead of a negative duration like "-5m ago". + const diffMs = Math.max(0, now - date.getTime()); const diffMins = Math.floor(diffMs / 60_000); const diffHours = Math.floor(diffMs / 3_600_000); const diffDays = Math.floor(diffMs / 86_400_000); diff --git a/test/lib/formatters/human.utils.test.ts b/test/lib/formatters/human.utils.test.ts index 176794216..d6801b9f1 100644 --- a/test/lib/formatters/human.utils.test.ts +++ b/test/lib/formatters/human.utils.test.ts @@ -139,6 +139,15 @@ describe("formatRelativeTime", () => { // Should be "0m ago" or similar for very recent times expect(result.trim()).toMatch(/^\d+[mhd] ago$|^[A-Z][a-z]{2} \d{1,2}$/); }); + + test("clamps future timestamps to '0m ago' instead of negative", () => { + // Clock skew / scheduled timestamps can be in the future; diffMs is clamped + // to >= 0 so we never render "-5m ago". + const fiveMinutesAhead = new Date(Date.now() + 5 * 60_000).toISOString(); + const result = stripAnsi(formatRelativeTime(fiveMinutesAhead)); + expect(result.trim()).toBe("0m ago"); + expect(result).not.toContain("-"); + }); }); // Token Masking From 66fb25f177462a39e07912d435fcfedc4cf3c54d Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:19 +0000 Subject: [PATCH 04/10] fix(seer): normalize canceled status and fix root-cause/no-solution extraction - normalizeAgentStatus(): map 'canceled'/'cancelled' to CANCELLED and 'need_more_information' to NEED_MORE_INFORMATION. The US spelling 'canceled' previously became 'CANCELED' via toUpperCase() and never matched the British 'CANCELLED' terminal status, so polling spun until timeout. - searchContainersForRootCauses(): require causes.length > 0 so an empty legacy causes:[] array no longer short-circuits the fallthrough to agent artifacts. - extractNoSolutionReason(): also read the step-level description (current API shape) before the artifact-level reason (legacy), mirroring extractSolution. Consolidates the seer fixes from #973 and #1023. Relates to #958. --- src/lib/api/seer.ts | 15 +++++- src/types/seer.ts | 70 ++++++++++++++++++++---- test/lib/api-client.seer.test.ts | 42 +++++++++++++++ test/types/seer.test.ts | 93 ++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+), 12 deletions(-) diff --git a/src/lib/api/seer.ts b/src/lib/api/seer.ts index c96127410..d0437f473 100644 --- a/src/lib/api/seer.ts +++ b/src/lib/api/seer.ts @@ -19,8 +19,14 @@ const EXPLORER_MODE_PARAMS = { mode: "explorer" }; * Normalize agent status values to the uppercase format used throughout the CLI. * * The agent endpoint returns lowercase statuses (`processing`, `completed`, - * `error`, `awaiting_user_input`) while the CLI expects uppercase - * (`PROCESSING`, `COMPLETED`, `ERROR`, `WAITING_FOR_USER_RESPONSE`). + * `error`, `awaiting_user_input`, `canceled`) while the CLI expects uppercase + * (`PROCESSING`, `COMPLETED`, `ERROR`, `WAITING_FOR_USER_RESPONSE`, `CANCELLED`). + * + * Explicit cases are required for any status whose CLI name differs from a naive + * `toUpperCase()`. In particular `canceled` (US spelling) must map to `CANCELLED` + * (British, used in TERMINAL_STATUSES) — otherwise `isTerminalStatus("CANCELED")` + * returns false and polling spins until timeout. `awaiting_user_input` maps to + * `WAITING_FOR_USER_RESPONSE`. */ function normalizeAgentStatus(status: string): string { switch (status) { @@ -30,8 +36,13 @@ function normalizeAgentStatus(status: string): string { return "COMPLETED"; case "error": return "ERROR"; + case "canceled": + case "cancelled": + return "CANCELLED"; case "awaiting_user_input": return "WAITING_FOR_USER_RESPONSE"; + case "need_more_information": + return "NEED_MORE_INFORMATION"; default: return status.toUpperCase(); } diff --git a/src/types/seer.ts b/src/types/seer.ts index 74fb3193b..d7803714d 100644 --- a/src/types/seer.ts +++ b/src/types/seer.ts @@ -253,7 +253,14 @@ function searchContainersForRootCauses( containers: WithCauses[] ): RootCause[] | null { for (const container of containers) { - if (container.key === "root_cause_analysis" && container.causes) { + // Require a non-empty causes array. An empty `causes: []` is truthy and + // would short-circuit here, blocking fallthrough to the agent artifact + // format (`searchBlocksForAgentRootCause`) when only that has data. + if ( + container.key === "root_cause_analysis" && + container.causes && + container.causes.length > 0 + ) { return container.causes; } } @@ -380,7 +387,7 @@ function findNoSolutionReason(artifacts: ArtifactEntry[]): string | undefined { } /** - * Search containers (blocks or steps) for a no-solution reason. + * Search containers (blocks or steps) for a no-solution reason in artifacts. */ function searchContainersForNoSolutionReason( containers: WithArtifacts[] @@ -396,6 +403,29 @@ function searchContainersForNoSolutionReason( return; } +/** + * Search containers for a step-level no-solution reason. + * + * The current Seer API returns solution data directly on steps with + * `key === "solution"`. When no fix is produced the step has an empty/missing + * `solution` array but its `description` field carries the reason. This mirrors + * {@link searchContainersForStepLevelSolution} for the no-solution path. + */ +function searchContainersForStepLevelNoSolutionReason( + containers: StepWithSolution[] +): string | undefined { + for (const container of containers) { + if ( + container.key === "solution" && + (!container.solution || container.solution.length === 0) && + container.description + ) { + return container.description; + } + } + return; +} + /** * Search an array of containers (blocks or steps) for a solution artifact. */ @@ -513,30 +543,34 @@ export function extractSolution(state: AutofixState): SolutionArtifact | null { /** * Extract the reason why no solution was produced. * - * When Seer completes analysis but cannot produce a code fix, the API - * returns a solution artifact with `data: null` and a `reason` string. - * This function searches blocks and steps for that reason. + * Searches blocks and steps for a no-solution reason in two formats: + * 1. Step-level: `step.key === "solution"` with an empty/missing `solution[]` + * and a `description` explaining why (current API). + * 2. Artifact-level: `artifact.key === "solution"` with a `reason` field + * (legacy format, kept as fallback). + * + * Step-level is checked first to match {@link extractSolution}'s ordering. * - * @param state - Autofix state (may contain blocks or steps with artifacts) + * @param state - Autofix state (may contain blocks or steps with solution data) * @returns Reason string if found, undefined otherwise */ export function extractNoSolutionReason( state: AutofixState ): string | undefined { const stateWithExtras = state as AutofixState & { - blocks?: WithArtifacts[]; - steps?: WithArtifacts[]; + blocks?: (WithArtifacts & StepWithSolution)[]; + steps?: (WithArtifacts & StepWithSolution)[]; }; if (stateWithExtras.blocks) { - const reason = searchContainersForNoSolutionReason(stateWithExtras.blocks); + const reason = findNoSolutionReasonInContainers(stateWithExtras.blocks); if (reason) { return reason; } } if (stateWithExtras.steps) { - const reason = searchContainersForNoSolutionReason(stateWithExtras.steps); + const reason = findNoSolutionReasonInContainers(stateWithExtras.steps); if (reason) { return reason; } @@ -545,6 +579,22 @@ export function extractNoSolutionReason( return; } +/** + * Resolve a no-solution reason for a single container list, preferring the + * step-level `description` (current API) over the artifact-level `reason` + * (legacy). Extracted to keep {@link extractNoSolutionReason} flat and within + * the cognitive-complexity budget. + */ +function findNoSolutionReasonInContainers( + containers: (WithArtifacts & StepWithSolution)[] +): string | undefined { + const stepLevel = searchContainersForStepLevelNoSolutionReason(containers); + if (stepLevel) { + return stepLevel; + } + return searchContainersForNoSolutionReason(containers); +} + /** * Extract file paths examined during root cause analysis. * diff --git a/test/lib/api-client.seer.test.ts b/test/lib/api-client.seer.test.ts index cf0615d5f..afcbcfe3f 100644 --- a/test/lib/api-client.seer.test.ts +++ b/test/lib/api-client.seer.test.ts @@ -155,6 +155,48 @@ describe("getAutofixState", () => { expect(result?.status).toBe("WAITING_FOR_USER_RESPONSE"); }); + test("normalizes US spelling 'canceled' to CANCELLED for terminal status match", async () => { + globalThis.fetch = async () => + new Response( + JSON.stringify({ + autofix: { + run_id: 1, + status: "canceled", + blocks: [], + updated_at: "2025-01-01T00:00:00Z", + }, + }), + { + status: 200, + headers: { "Content-Type": "application/json" }, + } + ); + + const result = await getAutofixState("test-org", "123456789"); + expect(result?.status).toBe("CANCELLED"); + }); + + test("normalizes need_more_information to NEED_MORE_INFORMATION", async () => { + globalThis.fetch = async () => + new Response( + JSON.stringify({ + autofix: { + run_id: 1, + status: "need_more_information", + blocks: [], + updated_at: "2025-01-01T00:00:00Z", + }, + }), + { + status: 200, + headers: { "Content-Type": "application/json" }, + } + ); + + const result = await getAutofixState("test-org", "123456789"); + expect(result?.status).toBe("NEED_MORE_INFORMATION"); + }); + test("returns null when autofix is null", async () => { globalThis.fetch = async () => new Response(JSON.stringify({ autofix: null }), { diff --git a/test/types/seer.test.ts b/test/types/seer.test.ts index db049216f..f202b4145 100644 --- a/test/types/seer.test.ts +++ b/test/types/seer.test.ts @@ -254,6 +254,42 @@ describe("extractRootCauses", () => { expect(causes[0]?.description).toBe("Configuration error"); expect(causes[0]?.relevant_repos).toBeUndefined(); }); + + test("falls through to agent artifacts when legacy causes array is empty", () => { + const state = { + run_id: 789, + status: "COMPLETED", + blocks: [ + { + key: "root_cause_analysis", + status: "COMPLETED", + causes: [], + artifacts: [], + }, + { + id: "block-2", + message: { role: "assistant", content: "Found it" }, + timestamp: "2025-01-01T00:00:00Z", + artifacts: [ + { + key: "root_cause", + data: { + one_line_description: "Race condition in auth middleware", + five_whys: ["Token refresh not atomic"], + relevant_repo: "org/api-server", + }, + reason: "", + }, + ], + }, + ], + } as unknown as AutofixState; + + const causes = extractRootCauses(state); + expect(causes).toHaveLength(1); + expect(causes[0]?.description).toBe("Race condition in auth middleware"); + expect(causes[0]?.relevant_repos).toEqual(["org/api-server"]); + }); }); describe("extractNoSolutionReason", () => { @@ -345,6 +381,63 @@ describe("extractNoSolutionReason", () => { const state: AutofixState = { run_id: 1, status: "COMPLETED" }; expect(extractNoSolutionReason(state)).toBeUndefined(); }); + + test("extracts reason from step-level description when solution is empty", () => { + const state = { + run_id: 1, + status: "NEED_MORE_INFORMATION", + blocks: [ + { + key: "solution", + description: + "Cannot produce a fix: the issue is in a third-party library", + solution: [], + artifacts: [], + }, + ], + } as unknown as AutofixState; + + expect(extractNoSolutionReason(state)).toBe( + "Cannot produce a fix: the issue is in a third-party library" + ); + }); + + test("extracts reason from step-level when solution field is missing", () => { + const state = { + run_id: 1, + status: "NEED_MORE_INFORMATION", + steps: [ + { + key: "solution", + description: "Infrastructure-level issue, no code change applicable", + artifacts: [], + }, + ], + } as unknown as AutofixState; + + expect(extractNoSolutionReason(state)).toBe( + "Infrastructure-level issue, no code change applicable" + ); + }); + + test("prefers step-level reason over artifact-level reason", () => { + const state = { + run_id: 1, + status: "COMPLETED", + blocks: [ + { + key: "solution", + description: "Step-level reason", + solution: [], + artifacts: [ + { key: "solution", data: null, reason: "Artifact-level reason" }, + ], + }, + ], + } as unknown as AutofixState; + + expect(extractNoSolutionReason(state)).toBe("Step-level reason"); + }); }); describe("extractSolution", () => { From 6253ac0e79cd34a835293d9135ac1af124da1248 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:26 +0000 Subject: [PATCH 05/10] fix(release): only fall back to local git on missing repo integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set-commits default mode caught any ApiError with status 400 from setCommitsAuto and treated it as 'no repository integration', falling back to local git and poisoning the cache. But setCommitsAuto internally calls setCommitsWithRefs, which can return 400 for unrelated reasons (invalid refs, bad release state) — those were masked. Export NO_REPO_INTEGRATIONS_MESSAGE from releases.ts and narrow the catch to the exact client-side message. Unrelated 400s now propagate. Matched on message (not detail) because this error is constructed client-side with detail undefined. Consolidates #1023's set-commits fix. --- src/commands/release/set-commits.ts | 13 ++++- src/lib/api-client.ts | 1 + src/lib/api/releases.ts | 19 +++++-- test/commands/release/set-commits.test.ts | 69 ++++++++++++++++++++++- 4 files changed, 94 insertions(+), 8 deletions(-) diff --git a/src/commands/release/set-commits.ts b/src/commands/release/set-commits.ts index f1917b506..b7b5d6fcd 100644 --- a/src/commands/release/set-commits.ts +++ b/src/commands/release/set-commits.ts @@ -6,6 +6,7 @@ import type { SentryContext } from "../../context.js"; import { + NO_REPO_INTEGRATIONS_MESSAGE, setCommitsAuto, setCommitsLocal, setCommitsWithRefs, @@ -134,7 +135,17 @@ async function setCommitsDefault( clearRepoIntegrationCache(org); return release; } catch (error) { - if (error instanceof ApiError && error.status === 400) { + // Only fall back to local git when the org genuinely has no repository + // integration. setCommitsAuto internally calls setCommitsWithRefs, which can + // return a server 400 for unrelated reasons (invalid refs, bad release + // state) — those must propagate, not be masked as "no integration" (which + // would also poison the cache). Match on the exact client-side message since + // the API exposes no stable error code for this case. + if ( + error instanceof ApiError && + error.status === 400 && + error.message === NO_REPO_INTEGRATIONS_MESSAGE + ) { cacheNoRepoIntegration(org); log.warn( "Could not auto-discover commits (no repository integration). " + diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index 87f750b87..d2e51c1e6 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -120,6 +120,7 @@ export { listReleaseDeploys, listReleasesForProject, listReleasesPaginated, + NO_REPO_INTEGRATIONS_MESSAGE, type ReleaseSortValue, setCommitsAuto, setCommitsLocal, diff --git a/src/lib/api/releases.ts b/src/lib/api/releases.ts index ee64aae4f..127a8b161 100644 --- a/src/lib/api/releases.ts +++ b/src/lib/api/releases.ts @@ -402,6 +402,18 @@ async function getPreviousReleaseCommit( * Set commits on a release using auto-discovery mode. * * Lists the org's repositories from the Sentry API, matches against the +/** + * Message of the client-side {@link ApiError} thrown by {@link setCommitsAuto} + * when the org has no repository integrations. Exported so callers can + * distinguish this specific no-repo-integration case from unrelated 400s + * returned by the server (e.g. invalid commit refs), which must not be masked + * as "no integration". Matched on `message` (not `detail`) because this error is + * constructed client-side with `detail: undefined`. + */ +export const NO_REPO_INTEGRATIONS_MESSAGE = + "No repository integrations configured for this organization."; + +/** * local git remote URL to find the corresponding Sentry repo, then sends * a refs payload with the HEAD commit SHA. This is the equivalent of the * reference sentry-cli's `--auto` mode. @@ -470,12 +482,7 @@ export async function setCommitsAuto( if (!foundAnyRepos) { const endpoint = `organizations/${orgSlug}/releases/${encodeURIComponent(version)}/`; - throw new ApiError( - "No repository integrations configured for this organization.", - 400, - undefined, - endpoint - ); + throw new ApiError(NO_REPO_INTEGRATIONS_MESSAGE, 400, undefined, endpoint); } throw new ValidationError( diff --git a/test/commands/release/set-commits.test.ts b/test/commands/release/set-commits.test.ts index 1d9f7efee..443323ba2 100644 --- a/test/commands/release/set-commits.test.ts +++ b/test/commands/release/set-commits.test.ts @@ -19,9 +19,10 @@ vi.mock("../../../src/lib/api-client.js", async (importOriginal) => { ); }); +import { NO_REPO_INTEGRATIONS_MESSAGE } from "../../../src/lib/api/releases.js"; // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as apiClient from "../../../src/lib/api-client.js"; -import { ValidationError } from "../../../src/lib/errors.js"; +import { ApiError, ValidationError } from "../../../src/lib/errors.js"; vi.mock("../../../src/lib/resolve-target.js", async (importOriginal) => { const actual = @@ -265,6 +266,72 @@ describe("release set-commits (default mode)", () => { resolveOrgSpy.mockRestore(); }); + test("propagates unrelated 400 errors from setCommitsAuto", async () => { + resolveOrgSpy.mockResolvedValue({ org: "my-org" }); + setCommitsAutoSpy.mockRejectedValue( + new ApiError("Invalid commit SHA.", 400, undefined, "releases/1.0.0/") + ); + + const repoRoot = new URL("../../..", import.meta.url).pathname.replace( + /\/$/, + "" + ); + const { context } = createMockContext(repoRoot); + const func = await setCommitsCommand.loader(); + await expect( + func.call( + context, + { + auto: false, + local: false, + clear: false, + commit: undefined, + "initial-depth": 20, + json: true, + }, + "1.0.0" + ) + ).rejects.toThrow("Invalid commit SHA."); + + expect(setCommitsAutoSpy).toHaveBeenCalled(); + expect(setCommitsLocalSpy).not.toHaveBeenCalled(); + }); + + test("falls back to local only on the no-repo-integration 400", async () => { + resolveOrgSpy.mockResolvedValue({ org: "my-org" }); + setCommitsAutoSpy.mockRejectedValue( + new ApiError( + NO_REPO_INTEGRATIONS_MESSAGE, + 400, + undefined, + "releases/1.0.0/" + ) + ); + setCommitsLocalSpy.mockResolvedValue(sampleRelease); + + const repoRoot = new URL("../../..", import.meta.url).pathname.replace( + /\/$/, + "" + ); + const { context } = createMockContext(repoRoot); + const func = await setCommitsCommand.loader(); + await func.call( + context, + { + auto: false, + local: false, + clear: false, + commit: undefined, + "initial-depth": 20, + json: true, + }, + "1.0.0" + ); + + expect(setCommitsAutoSpy).toHaveBeenCalled(); + expect(setCommitsLocalSpy).toHaveBeenCalled(); + }); + test("falls back to local on ValidationError from auto", async () => { resolveOrgSpy.mockResolvedValue({ org: "my-org" }); setCommitsAutoSpy.mockRejectedValue( From 48b80b60e9f5a4e0f6a2286d38738bfd64bca3ae Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:33 +0000 Subject: [PATCH 06/10] test: complete env isolation in terminalLink withTTY helper withTTY only saved/restored SENTRY_PLAIN_OUTPUT, so in CI (where NO_COLOR=1) isPlainOutput() returned true before the TTY check and terminalLink returned plain text, failing the TTY tests. Save/restore NO_COLOR and FORCE_COLOR too. Consolidates #1044/#947's test isolation fix. --- test/lib/formatters/colors.test.ts | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/test/lib/formatters/colors.test.ts b/test/lib/formatters/colors.test.ts index 2f0e32793..310f54f7c 100644 --- a/test/lib/formatters/colors.test.ts +++ b/test/lib/formatters/colors.test.ts @@ -114,21 +114,36 @@ describe("fixabilityColor", () => { }); describe("terminalLink", () => { - /** Save/restore isTTY and env around TTY-specific tests */ + function restoreEnv(key: string, saved: string | undefined): void { + if (saved !== undefined) { + process.env[key] = saved; + } else { + delete process.env[key]; + } + } + + /** + * Save/restore isTTY and ALL plain-output env vars around TTY-specific tests. + * `isPlainOutput()` precedence is SENTRY_PLAIN_OUTPUT > NO_COLOR > FORCE_COLOR + * > !isTTY — so in CI (where NO_COLOR=1) failing to clear NO_COLOR/FORCE_COLOR + * makes terminalLink return plain text even with isTTY forced true. + */ function withTTY(fn: () => void): void { const savedTTY = process.stdout.isTTY; const savedPlain = process.env.SENTRY_PLAIN_OUTPUT; + const savedNoColor = process.env.NO_COLOR; + const savedForceColor = process.env.FORCE_COLOR; process.stdout.isTTY = true; delete process.env.SENTRY_PLAIN_OUTPUT; + delete process.env.NO_COLOR; + delete process.env.FORCE_COLOR; try { fn(); } finally { process.stdout.isTTY = savedTTY; - if (savedPlain !== undefined) { - process.env.SENTRY_PLAIN_OUTPUT = savedPlain; - } else { - delete process.env.SENTRY_PLAIN_OUTPUT; - } + restoreEnv("SENTRY_PLAIN_OUTPUT", savedPlain); + restoreEnv("NO_COLOR", savedNoColor); + restoreEnv("FORCE_COLOR", savedForceColor); } } From e245a774f7ed59506d14f9902df11881d7c39478 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:39:44 +0000 Subject: [PATCH 07/10] chore: enforce no-silent-catch (advisory) and document automated-PR rules - Extend script/check-error-patterns.ts with a silent-catch scan (empty, comment-only, or return-only catch blocks that don't surface the error). It is advisory by default given a pre-existing backlog; set SENTRY_STRICT_SILENT_CATCH=1 to enforce. References to the caught error or a log/throw exclude a block. - Document automated-fix-PR rules in AGENTS.md: check existing open PRs/issues first, rebase before review, separate correctness from opinion, prefer shared helpers (autoPaginate, safeParseJson). These address the duplication/staleness that produced five overlapping BugBot PRs. - Remove a now-unused biome-ignore suppression in formatters/local.ts that broke repo-wide lint under biome 2.3.8 (stale-base drift). --- AGENTS.md | 40 +++++++++++- script/check-error-patterns.ts | 108 +++++++++++++++++++++++++++++++++ src/lib/formatters/local.ts | 1 - 3 files changed, 147 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4c1136be6..725e88f0d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -510,7 +510,7 @@ CliError (base, exitCode=1) - Pass `alternatives: []` when defaults are irrelevant (e.g., for missing Trace ID, Event ID) - Use `" and "` in `resource` for plural grammar: `"Trace ID and span ID"` → "are required" -**CI enforcement:** `bun run check:errors` scans for `ContextError` with multiline commands and `CliError` with ad-hoc "Try:" strings. +**CI enforcement:** `pnpm run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory). ```typescript // Usage examples @@ -554,6 +554,12 @@ catch (error) { Use `logger.withTag("command-name")` for tagged logging in command files. +**CI enforcement:** `pnpm run check:errors` includes a silent-catch scan that flags +`catch` blocks which are empty, comment-only, or return-only without surfacing the +error. It is currently **advisory** (warns, does not fail CI) because of a pre-existing +backlog; run with `SENTRY_STRICT_SILENT_CATCH=1` to enforce. Do not add new silent +catches — they will appear in the scan output during review. + ### Auto-Recovery for Wrong Entity Types When a user provides the wrong type of identifier (e.g., an issue short ID @@ -961,6 +967,38 @@ mock.module("./some-module", () => ({ | Add documentation | `docs/src/content/docs/` | | Hand-written command doc content | `docs/src/fragments/commands/` | +## Automated Fix PRs (BugBot / agents) + +Automated bug-fix PRs (e.g. Cursor BugBot) must follow these rules to avoid the +duplication and staleness that caused five overlapping PRs to pile up: + +1. **Check for existing work first.** Before opening a PR, search open PRs and + recently-closed PRs/issues for the same file + symbol: + ```bash + gh pr list --state open --search "in:title " + gh issue list --state all --search "" + ``` + If an open PR already touches the target function, **comment on it** or extend + it instead of opening a duplicate. Multiple BugBot PRs independently re-fixed + the same `JSON.parse` guard, `withTTY` helper, and pagination code. + +2. **Rebase before review.** A PR that is many commits behind `main` may fail CI + on unrelated drift (e.g. a lint error in a file the PR never touched) and its + fix may already be superseded. Rebase onto `main` and re-verify the bug still + exists before requesting review. Verify against current `main`, not the + snapshot the PR was generated from. + +3. **Separate correctness fixes from opinion.** A real bug (wrong output, crash, + skipped data) is in scope. A subjective UX change (different hint wording, + different default) is **not** a bug — `main`'s current behavior is often + deliberate. Do not bundle UX opinions into bug-fix PRs; they waste review + cycles and are usually dropped. + +4. **Prefer shared helpers over re-deriving fixes.** If a correct implementation + already exists (e.g. `autoPaginate()` for pagination, `safeParseJson()` for + cached JSON), use it rather than hand-rolling a one-off fix. The recurring + pagination-overshoot and parse-crash bugs were classes solved once centrally. + ## Long-term Knowledge diff --git a/script/check-error-patterns.ts b/script/check-error-patterns.ts index c0aa31cba..20a7dafea 100644 --- a/script/check-error-patterns.ts +++ b/script/check-error-patterns.ts @@ -10,6 +10,12 @@ * 2. `new CliError(... "Try:" ...)` — ad-hoc "Try:" strings * → Should use ResolutionError with structured hint/suggestions * + * 3. Silent catch blocks — `catch { ... }` whose body has no logging, no + * re-throw, and at most a bare `return`. Errors must be surfaced via + * `log.debug`/`log.warn` (or re-thrown) per AGENTS.md. Biome's + * `noEmptyBlockStatements` only catches syntactically empty `catch {}`; + * this catches comment-only and return-only blocks too. + * * Usage: * tsx script/check-error-patterns.ts * @@ -27,8 +33,20 @@ const CONTEXT_ERROR_RE = /new ContextError\(/g; const TRY_PATTERN_RE = /["'`]Try:/; const files = await glob("src/**/*.ts"); + +/** Hard violations — these fail CI. */ const violations: Violation[] = []; +/** + * Advisory silent-catch findings. Reported as warnings but do NOT fail CI yet: + * the repo has a pre-existing backlog of intentional best-effort catches (e.g. + * UI teardown, cleanup paths). The check surfaces them for incremental cleanup + * and so new ones are visible in review. Set SENTRY_STRICT_SILENT_CATCH=1 to + * promote them to hard failures once the backlog is cleared. + */ +const silentCatchWarnings: Violation[] = []; +const STRICT_SILENT_CATCH = process.env.SENTRY_STRICT_SILENT_CATCH === "1"; + /** Characters that open a nesting level in JavaScript source. */ function isOpener(ch: string): boolean { return ch === "(" || ch === "[" || ch === "{"; @@ -255,10 +273,100 @@ function checkAdHocTryPatterns(content: string, filePath: string): void { } } +/** Matches the start of a catch block in both statement and promise form. */ +const CATCH_RE = + /\bcatch\s*(?:\(\s*(\w+)[^)]*\)\s*)?\{|\.catch\(\s*(?:\(\s*(\w+)[^)]*\)|(\w+))\s*=>\s*\{/g; + +/** Tokens inside a catch body that prove the error is surfaced (not silenced). */ +const SURFACING_RE = + /\b(?:log|logger|console)\s*\.|[^.]\bthrow\b|captureException|reportError/; + +/** A catch body consisting solely of a single `return ...;` statement. */ +const RETURN_ONLY_RE = /^return\b[^;]*;?$/; + +/** + * Return the source of a balanced `{...}` block given the index of its opening + * brace, skipping strings so braces inside literals don't break depth tracking. + */ +function readBlock(content: string, openBraceIdx: number): string { + let depth = 0; + let i = openBraceIdx; + while (i < content.length) { + const { next, ch } = advanceToken(content, i); + if (ch === "{") { + depth += 1; + } else if (ch === "}") { + depth -= 1; + if (depth === 0) { + return content.slice(openBraceIdx + 1, i); + } + } + i = next; + } + return content.slice(openBraceIdx + 1); +} + +/** + * Strip line and block comments from a snippet so comment-only catch bodies are + * treated as empty. + */ +function stripComments(snippet: string): string { + return snippet.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/[^\n]*/g, ""); +} + +/** + * Detect silent catch blocks: catch bodies that, after removing comments, are + * empty or contain only a bare `return;`/`return ;` with no logging or + * re-throw. These hide errors and violate the AGENTS.md no-silent-catch rule. + */ +function checkSilentCatch(content: string, filePath: string): void { + let match = CATCH_RE.exec(content); + while (match !== null) { + const openBraceIdx = match.index + match[0].length - 1; + const errorParam = match[1] ?? match[2] ?? match[3]; + const body = readBlock(content, openBraceIdx); + const code = stripComments(body).trim(); + // A body that references the caught error identifier (forwarding it to a + // handler, attaching it, etc.) is not "silent" even if it lacks an explicit + // log/throw — avoids false positives like `return handleFetchError(error)`. + const usesError = + errorParam !== undefined && new RegExp(`\\b${errorParam}\\b`).test(code); + const returnOnly = RETURN_ONLY_RE.test(code); + const silent = + !(SURFACING_RE.test(code) || usesError) && + (code.length === 0 || returnOnly); + if (silent) { + const line = content.slice(0, match.index).split("\n").length; + const target = STRICT_SILENT_CATCH ? violations : silentCatchWarnings; + target.push({ + file: filePath, + line, + message: + "Silent catch block. Add log.debug()/log.warn() or re-throw — errors must not vanish (AGENTS.md).", + }); + } + match = CATCH_RE.exec(content); + } +} + for (const filePath of files) { const content = await readFile(filePath, "utf-8"); checkContextErrorNewlines(content, filePath); checkAdHocTryPatterns(content, filePath); + checkSilentCatch(content, filePath); +} + +if (silentCatchWarnings.length > 0) { + console.warn( + `⚠ ${silentCatchWarnings.length} silent catch block(s) found (advisory; not failing CI).` + ); + console.warn( + " Add log.debug()/log.warn() or re-throw. Run with SENTRY_STRICT_SILENT_CATCH=1 to enforce.\n" + ); + for (const v of silentCatchWarnings) { + console.warn(` ${v.file}:${v.line}`); + } + console.warn(""); } if (violations.length === 0) { diff --git a/src/lib/formatters/local.ts b/src/lib/formatters/local.ts index 183eb9d8e..8ccce8d73 100644 --- a/src/lib/formatters/local.ts +++ b/src/lib/formatters/local.ts @@ -14,7 +14,6 @@ import { * `JSON.stringify` only escapes C0 (U+0000–U+001F) per RFC 8259; * C1 and BiDi pass through unescaped. */ -// biome-ignore lint/suspicious/noControlCharactersInRegex: stripping C1 control chars from untrusted data const JSON_UNSAFE_RE = /[\x80-\x9f\u200e\u200f\u202a-\u202e\u2066-\u2069]/g; /** BiDi-only regex for the full `sanitize()` function. */ From 817389de08b9a04a7f7aa20b738232bb7e0f1b21 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 11:52:21 +0000 Subject: [PATCH 08/10] fix(db): validate cached DSN detection JSON shape Address Warden find-bugs: getCachedDetection parsed allDsns and the mtimes records via safeParseJson without a validator, so a structurally-corrupt row (e.g. an object where an array is expected) passed the truthy !allDsns check and flowed downstream typed as DetectedDsn[], risking a runtime TypeError. Add isArray / isMtimesRecord type guards so any shape mismatch is treated as a cache miss, matching getPaginationState's Array.isArray guard. --- src/lib/db/dsn-cache.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/lib/db/dsn-cache.ts b/src/lib/db/dsn-cache.ts index e00099c8b..7aaaa7782 100644 --- a/src/lib/db/dsn-cache.ts +++ b/src/lib/db/dsn-cache.ts @@ -308,6 +308,16 @@ async function validateDirMtimes( return true; } +/** Type guard: value is a plain object usable as a mtimes record. */ +function isMtimesRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** Type guard: value is an array (used for the cached DSN list). */ +function isArray(value: unknown): value is DetectedDsn[] { + return Array.isArray(value); +} + /** * Get cached full detection result if valid. * @@ -363,16 +373,15 @@ export async function getCachedDetection( return; } - // Parse all cached JSON columns up front. Corruption (partial write, manual - // edit, schema drift) is treated as a cache miss so detection re-runs from - // scratch instead of crashing. - const sourceMtimes = safeParseJson>( - row.source_mtimes_json - ); + // Parse all cached JSON columns up front, validating their shape. Corruption + // (partial write, manual edit, schema drift) — including a structurally wrong + // value like an object where an array is expected — is treated as a cache miss + // so detection re-runs from scratch instead of crashing downstream. + const sourceMtimes = safeParseJson(row.source_mtimes_json, isMtimesRecord); const dirMtimes = row.dir_mtimes_json - ? safeParseJson>(row.dir_mtimes_json) + ? safeParseJson(row.dir_mtimes_json, isMtimesRecord) : {}; - const allDsns = safeParseJson(row.all_dsns_json); + const allDsns = safeParseJson(row.all_dsns_json, isArray); if (sourceMtimes === undefined || dirMtimes === undefined || !allDsns) { recordCacheHit("dsn-detection", false); return; From 8865da1a3797ac8f1bbd09e6faeaea30f662a3f3 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 12:09:11 +0000 Subject: [PATCH 09/10] fix(events): cap per_page and preserve cursor so pagination never stalls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Cursor BugBot (high severity): the previous fix dropped nextCursor on overshoot, which stranded every event past the first page — 'sentry issue events' could only ever show the first --limit events. Cap page size with per_page = min(limit, API_MAX_PER_PAGE) (Sentry accepts per_page on this route at runtime though it's absent from the OpenAPI spec) so the server cursor is page-aligned and never overshoots. Trim defensively to limit but PRESERVE nextCursor: the events cursor is offset-based, so resuming re-includes any trimmed tail instead of skipping it, and -c next can advance. This resolves both the original skip bug (#947) and the stall regression. Update tests to assert per_page is sent (capped at 100) and the cursor is preserved on overshoot. --- src/lib/api/events.ts | 82 +++++++++++++-------------- test/lib/api/events-overshoot.test.ts | 59 +++++++++++++++---- 2 files changed, 87 insertions(+), 54 deletions(-) diff --git a/src/lib/api/events.ts b/src/lib/api/events.ts index 3e2a1058c..155fcb5ab 100644 --- a/src/lib/api/events.ts +++ b/src/lib/api/events.ts @@ -18,7 +18,6 @@ import { ApiError, AuthError } from "../errors.js"; import { API_MAX_PER_PAGE, - autoPaginate, getOrgSdkConfig, ORG_FANOUT_CONCURRENCY, type PaginatedResponse, @@ -198,10 +197,17 @@ export type ListIssueEventsOptions = { * List events for a specific issue. * * Uses the SDK's `listAnIssue_sEvents` endpoint with region-aware routing. - * When `limit` exceeds {@link API_MAX_PER_PAGE} (100), {@link autoPaginate} - * fetches multiple API pages to fill the requested limit. Because the endpoint - * has no per-page parameter, a page may overshoot `limit`; the result is trimmed - * and `nextCursor` dropped so cursor navigation never skips events. + * + * Page size is capped at `min(limit, {@link API_MAX_PER_PAGE})` via `per_page`, + * which Sentry accepts on this route at runtime even though it is absent from + * the OpenAPI spec. Capping page size means the server-issued `nextCursor` is + * aligned to a page boundary, so cursor navigation never skips events. + * + * As defense-in-depth (in case the server ignores `per_page`), the result is + * trimmed to `limit` but `nextCursor` is PRESERVED: the events cursor is + * offset-based, so resuming from it re-includes any trimmed tail rather than + * skipping it. Preserving the cursor is also what lets `-c next` advance at all + * when a page is larger than `limit` — dropping it would strand later events. * * @param orgSlug - Organization slug for region routing * @param issueId - Numeric issue ID @@ -216,44 +222,36 @@ export async function listIssueEvents( const { limit = 25, query, full, cursor, statsPeriod, start, end } = options; const config = await getOrgSdkConfig(orgSlug); + const perPage = Math.min(limit, API_MAX_PER_PAGE); - const fetchPage = async ( - pageCursor: string | undefined - ): Promise> => { - const result = await listAnIssue_sEvents({ - ...config, - path: { - organization_id_or_slug: orgSlug, - issue_id: Number(issueId), - }, - query: { - query: query || undefined, - full, - cursor: pageCursor, - statsPeriod, - start, - end, - }, - }); - - const paginated = unwrapPaginatedResult( - result, - "Failed to list issue events" - ); - return { - data: paginated.data as IssueEvent[], - nextCursor: paginated.nextCursor, - }; - }; + const result = await listAnIssue_sEvents({ + ...config, + path: { + organization_id_or_slug: orgSlug, + issue_id: Number(issueId), + }, + query: { + query: query || undefined, + full, + cursor, + statsPeriod, + start, + end, + // `per_page` is accepted at runtime but absent from the generated query + // type, so widen via cast. + per_page: perPage, + } as Parameters[0]["query"], + }); - const result = await autoPaginate(fetchPage, limit, cursor); + const paginated = unwrapPaginatedResult( + result, + "Failed to list issue events" + ); + const events = paginated.data as IssueEvent[]; - // The issue events endpoint has no per-page parameter, so a single page can - // already overshoot `limit` (autoPaginate's fast path returns it untrimmed). - // When trimming we MUST drop nextCursor — it points past the trimmed tail and - // would make `-c next` skip the events between the trim point and that cursor. - if (result.data.length > limit) { - return { data: result.data.slice(0, limit) }; - } - return result; + // Trim to limit but keep nextCursor (see JSDoc): the offset-based events + // cursor re-includes any trimmed tail on the next page, so navigation neither + // skips nor stalls. + const data = events.length > limit ? events.slice(0, limit) : events; + return { data, nextCursor: paginated.nextCursor }; } diff --git a/test/lib/api/events-overshoot.test.ts b/test/lib/api/events-overshoot.test.ts index c40f6005e..7256d6128 100644 --- a/test/lib/api/events-overshoot.test.ts +++ b/test/lib/api/events-overshoot.test.ts @@ -1,11 +1,16 @@ /** - * Regression tests for listIssueEvents pagination overshoot. + * Regression tests for listIssueEvents pagination. * - * The issue events endpoint has no per-page parameter, so a single API page can - * return more events than the caller's `limit`. When that happens listIssueEvents - * MUST trim to `limit` AND drop nextCursor — returning a cursor that points past - * the trimmed tail would make `-c next` navigation skip the events between the - * trim point and that cursor. + * Two bugs framed this behavior: + * 1. The original skip bug: trimming an overshooting page while returning the + * server's next-page cursor skipped the trimmed tail on `-c next`. + * 2. The overcorrection: dropping the cursor on overshoot stranded all events + * past the first `limit`, so `sentry issue events` could never page forward. + * + * The fix caps page size with `per_page = min(limit, API_MAX_PER_PAGE)` so the + * server cursor is page-aligned, and trims defensively while PRESERVING the + * cursor (the events cursor is offset-based, so resuming re-includes any trimmed + * tail). These tests pin both the `per_page` request and the cursor preservation. */ import { afterEach, beforeEach, describe, expect, test } from "vitest"; @@ -36,9 +41,39 @@ function makeEvents(count: number): Array<{ id: string }> { return Array.from({ length: count }, (_unused, i) => ({ id: `evt-${i}` })); } -describe("listIssueEvents overshoot handling", () => { - test("trims to limit and drops nextCursor when a page overshoots", async () => { - // Single page returns 5 events with a next cursor, but caller wants 2. +describe("listIssueEvents pagination", () => { + test("sends per_page capped at the requested limit", async () => { + let capturedUrl = ""; + globalThis.fetch = mockFetch(async (input, init) => { + capturedUrl = new Request(input!, init).url; + return new Response(JSON.stringify(makeEvents(2)), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }); + + await listIssueEvents("test-org", "123", { limit: 2 }); + + expect(new URL(capturedUrl).searchParams.get("per_page")).toBe("2"); + }); + + test("caps per_page at API_MAX_PER_PAGE for very large limits", async () => { + let capturedUrl = ""; + globalThis.fetch = mockFetch(async (input, init) => { + capturedUrl = new Request(input!, init).url; + return new Response(JSON.stringify(makeEvents(10)), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }); + + await listIssueEvents("test-org", "123", { limit: 5000 }); + + expect(new URL(capturedUrl).searchParams.get("per_page")).toBe("100"); + }); + + test("trims to limit but PRESERVES nextCursor when a page overshoots", async () => { + // Server ignores per_page and returns 5 with a next cursor; caller wants 2. globalThis.fetch = mockFetch( async () => new Response(JSON.stringify(makeEvents(5)), { @@ -55,8 +90,9 @@ describe("listIssueEvents overshoot handling", () => { expect(result.data).toHaveLength(2); expect(result.data[0]?.id).toBe("evt-0"); expect(result.data[1]?.id).toBe("evt-1"); - // Critical: cursor dropped so no events are skipped on the next navigation. - expect(result.nextCursor).toBeUndefined(); + // Cursor preserved so `-c next` can advance; the offset-based events cursor + // re-includes the trimmed tail rather than skipping it. + expect(result.nextCursor).toBe("page2"); }); test("preserves nextCursor when the page exactly fills the limit", async () => { @@ -74,7 +110,6 @@ describe("listIssueEvents overshoot handling", () => { const result = await listIssueEvents("test-org", "123", { limit: 2 }); expect(result.data).toHaveLength(2); - // No trim occurred, so the cursor remains valid for the next page. expect(result.nextCursor).toBe("page2"); }); From 5ca856bf9fd22b2c20a361cfb26ab331e6453f3e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 12:55:08 +0000 Subject: [PATCH 10/10] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20restore=20multi-page=20events=20accumulation,=20tig?= =?UTF-8?q?hten=20validators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M2: listIssueEvents now auto-paginates across multiple API calls for limit > 100 again (per AGENTS.md requirement), while still sending per_page to cap page size and preserving nextCursor on overshoot. M1: isMtimesRecord now validates all values are numbers, not just that the value is a non-array object. M3: AGENTS.md references changed from pnpm run to bun run for consistency with the rest of the file. --- AGENTS.md | 4 +- src/lib/api/events.ts | 90 ++++++++++++++++++++++++----------------- src/lib/db/dsn-cache.ts | 11 ++++- 3 files changed, 65 insertions(+), 40 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 725e88f0d..0287d7041 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -510,7 +510,7 @@ CliError (base, exitCode=1) - Pass `alternatives: []` when defaults are irrelevant (e.g., for missing Trace ID, Event ID) - Use `" and "` in `resource` for plural grammar: `"Trace ID and span ID"` → "are required" -**CI enforcement:** `pnpm run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory). +**CI enforcement:** `bun run check:errors` scans for `ContextError` with multiline commands, `CliError` with ad-hoc "Try:" strings, and silent `catch` blocks (advisory). ```typescript // Usage examples @@ -554,7 +554,7 @@ catch (error) { Use `logger.withTag("command-name")` for tagged logging in command files. -**CI enforcement:** `pnpm run check:errors` includes a silent-catch scan that flags +**CI enforcement:** `bun run check:errors` includes a silent-catch scan that flags `catch` blocks which are empty, comment-only, or return-only without surfacing the error. It is currently **advisory** (warns, does not fail CI) because of a pre-existing backlog; run with `SENTRY_STRICT_SILENT_CATCH=1` to enforce. Do not add new silent diff --git a/src/lib/api/events.ts b/src/lib/api/events.ts index 155fcb5ab..ecf08f830 100644 --- a/src/lib/api/events.ts +++ b/src/lib/api/events.ts @@ -19,6 +19,7 @@ import { ApiError, AuthError } from "../errors.js"; import { API_MAX_PER_PAGE, getOrgSdkConfig, + MAX_PAGINATION_PAGES, ORG_FANOUT_CONCURRENCY, type PaginatedResponse, unwrapPaginatedResult, @@ -197,17 +198,20 @@ export type ListIssueEventsOptions = { * List events for a specific issue. * * Uses the SDK's `listAnIssue_sEvents` endpoint with region-aware routing. + * When `limit` exceeds {@link API_MAX_PER_PAGE} (100), auto-paginates through + * multiple API calls to fill the requested limit, bounded by + * {@link MAX_PAGINATION_PAGES}. * - * Page size is capped at `min(limit, {@link API_MAX_PER_PAGE})` via `per_page`, - * which Sentry accepts on this route at runtime even though it is absent from - * the OpenAPI spec. Capping page size means the server-issued `nextCursor` is - * aligned to a page boundary, so cursor navigation never skips events. + * Page size is capped at `min(limit, API_MAX_PER_PAGE)` via `per_page`, which + * Sentry accepts on this route at runtime even though it is absent from the + * OpenAPI spec. Capping page size keeps the server-issued `nextCursor` aligned + * to a page boundary, preventing the skip bug where trim + keep-cursor would + * jump past items. * - * As defense-in-depth (in case the server ignores `per_page`), the result is - * trimmed to `limit` but `nextCursor` is PRESERVED: the events cursor is + * When trimming to `limit`, `nextCursor` is PRESERVED: the events cursor is * offset-based, so resuming from it re-includes any trimmed tail rather than - * skipping it. Preserving the cursor is also what lets `-c next` advance at all - * when a page is larger than `limit` — dropping it would strand later events. + * skipping it. This prevents both the original skip bug and the stall + * (drop-cursor) regression. * * @param orgSlug - Organization slug for region routing * @param issueId - Numeric issue ID @@ -224,34 +228,48 @@ export async function listIssueEvents( const config = await getOrgSdkConfig(orgSlug); const perPage = Math.min(limit, API_MAX_PER_PAGE); - const result = await listAnIssue_sEvents({ - ...config, - path: { - organization_id_or_slug: orgSlug, - issue_id: Number(issueId), - }, - query: { - query: query || undefined, - full, - cursor, - statsPeriod, - start, - end, - // `per_page` is accepted at runtime but absent from the generated query - // type, so widen via cast. - per_page: perPage, - } as Parameters[0]["query"], - }); + const allEvents: IssueEvent[] = []; + let currentCursor = cursor; + let nextCursor: string | undefined; - const paginated = unwrapPaginatedResult( - result, - "Failed to list issue events" - ); - const events = paginated.data as IssueEvent[]; + for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + const result = await listAnIssue_sEvents({ + ...config, + path: { + organization_id_or_slug: orgSlug, + issue_id: Number(issueId), + }, + query: { + query: query || undefined, + full, + cursor: currentCursor, + statsPeriod, + start, + end, + // `per_page` is accepted at runtime but absent from the generated query + // type, so widen via cast. + per_page: perPage, + } as Parameters[0]["query"], + }); + + const paginated = unwrapPaginatedResult( + result, + "Failed to list issue events" + ); + + allEvents.push(...(paginated.data as IssueEvent[])); + nextCursor = paginated.nextCursor; + + if (allEvents.length >= limit || !nextCursor) { + break; + } + currentCursor = nextCursor; + } - // Trim to limit but keep nextCursor (see JSDoc): the offset-based events - // cursor re-includes any trimmed tail on the next page, so navigation neither - // skips nor stalls. - const data = events.length > limit ? events.slice(0, limit) : events; - return { data, nextCursor: paginated.nextCursor }; + // Trim to limit but PRESERVE nextCursor. The events cursor is offset-based, + // so resuming from it re-includes any trimmed tail rather than skipping it. + // Preserving the cursor lets `-c next` advance; dropping it would strand all + // events past the first page. + const data = allEvents.length > limit ? allEvents.slice(0, limit) : allEvents; + return { data, nextCursor }; } diff --git a/src/lib/db/dsn-cache.ts b/src/lib/db/dsn-cache.ts index 7aaaa7782..deb5cbdfc 100644 --- a/src/lib/db/dsn-cache.ts +++ b/src/lib/db/dsn-cache.ts @@ -308,9 +308,16 @@ async function validateDirMtimes( return true; } -/** Type guard: value is a plain object usable as a mtimes record. */ +/** Type guard: value is a plain object with all-numeric values (mtimes record). */ function isMtimesRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + Object.values(value as Record).every( + (v) => typeof v === "number" + ) + ); } /** Type guard: value is an array (used for the cached DSN list). */