From a3a027bab72bcdd3b5b6d5730c728f35b2d77f22 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 09:51:06 +0000 Subject: [PATCH 1/3] fix(perf): cache project slug and numeric-issue-id to skip redundant lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two 'Consecutive HTTP' performance issues reported in Sentry where the CLI fires avoidable sequential API calls on hot paths: - **Pattern C (CLI-1BK)** — `sentry issue list /` fires a preflight `GET /projects/{org}/{project}/` before every `GET /issues/`. `fetchProjectId` in src/lib/resolve-target.ts now consults project_cache via a new `getCachedProjectBySlug(org, project)` helper before calling the API, and seeds the cache on API success. `listProjects()` and DSN resolution already populate the cache — `fetchProjectId` was the one hot-path caller that bypassed it. - **Pattern D (CLI-19S/CLI-19W partial)** — `sentry issue view ` without an org context falls back to the legacy unscoped endpoint, then extracts the org from the response permalink. Subsequent runs repeat the same fallback. A new `src/lib/db/issue-org-cache.ts` module stores the numeric-id → org-slug mapping in the existing `metadata` table (no schema migration); `resolveNumericIssue` and `resolveShareIssue` now use it to go straight to the org-scoped endpoint on repeat runs. Stale entries are evicted on 404 so the legacy fallback still covers moved/deleted issues. The cache is cleared on `auth logout` to avoid cross-account leakage. Also adds `clearMetadataByPrefix` to src/lib/db/utils.ts so both auth.ts and issue-org-cache.ts satisfy the project's 'no raw metadata queries' lint rule. Extracts `fetchIssueByNumericId` from `resolveNumericIssue` to keep cognitive complexity under the 15-point cap. The other Consecutive HTTP patterns (issue→event→trace chains and cursor-based pagination) are true data dependencies and not addressable here — they have been archived in Sentry. --- src/commands/issue/utils.ts | 96 +++++++++++++++---- src/lib/db/auth.ts | 7 +- src/lib/db/issue-org-cache.ts | 101 ++++++++++++++++++++ src/lib/db/project-cache.ts | 42 +++++++++ src/lib/db/utils.ts | 21 +++++ src/lib/resolve-target.ts | 36 +++++++- test/lib/db/issue-org-cache.test.ts | 114 +++++++++++++++++++++++ test/lib/db/project-cache.test.ts | 137 ++++++++++++++++++++++++++++ test/lib/resolve-target.test.ts | 87 ++++++++++++++++++ 9 files changed, 620 insertions(+), 21 deletions(-) create mode 100644 src/lib/db/issue-org-cache.ts create mode 100644 test/lib/db/issue-org-cache.test.ts diff --git a/src/commands/issue/utils.ts b/src/commands/issue/utils.ts index 74f006ec4..78d3b745d 100644 --- a/src/commands/issue/utils.ts +++ b/src/commands/issue/utils.ts @@ -21,6 +21,11 @@ import { tryGetIssueByShortId, } from "../../lib/api-client.js"; import { type IssueSelector, parseIssueArg } from "../../lib/arg-parsing.js"; +import { + clearCachedIssueOrg, + getCachedIssueOrg, + setCachedIssueOrg, +} from "../../lib/db/issue-org-cache.js"; import { getProjectByAlias } from "../../lib/db/project-aliases.js"; import { detectAllDsns } from "../../lib/dsn/index.js"; import { @@ -493,17 +498,22 @@ async function resolveShareIssue( return { org: resolvedOrg, issue }; } - // No org from URL — try env/DSN context, then fall back to unscoped fetch + // No org from URL — try env/DSN context, then the issue-id → org cache, + // then fall back to the unscoped fetch. See resolveNumericIssue for the + // full rationale behind the cache. const resolvedOrg = await resolveOrg({ cwd }); - const issue = resolvedOrg - ? await getIssueInOrg(resolvedOrg.org, groupId, { - collapse: ISSUE_DETAIL_COLLAPSE, - }) - : await getIssue(groupId, { collapse: ISSUE_DETAIL_COLLAPSE }); - return { - org: resolvedOrg?.org ?? extractOrgFromPermalink(issue.permalink), - issue, - }; + const cachedOrg = resolvedOrg ? null : getCachedIssueOrg(groupId); + const issue = await fetchIssueByNumericId( + groupId, + resolvedOrg?.org, + cachedOrg + ); + const resolvedOrgSlug = + resolvedOrg?.org ?? cachedOrg ?? extractOrgFromPermalink(issue.permalink); + if (resolvedOrgSlug && !resolvedOrg && !cachedOrg) { + setCachedIssueOrg(groupId, resolvedOrgSlug); + } + return { org: resolvedOrgSlug, issue }; } /** @@ -538,14 +548,58 @@ function extractOrgFromPermalink( return parseSentryUrl(permalink)?.org; } +/** + * Fetch an issue by numeric ID, preferring an org-scoped endpoint when + * the caller has explicit or cached org context. Falls back to the legacy + * unscoped `/api/0/issues/{id}/` endpoint when no org is known, and also + * when a cached org yields a 404 (stale mapping). + * + * Extracted from {@link resolveNumericIssue} to keep its cognitive + * complexity below the project's lint threshold. + */ +async function fetchIssueByNumericId( + id: string, + explicitOrg: string | undefined, + cachedOrg: string | null | undefined +): Promise { + if (explicitOrg) { + return await getIssueInOrg(explicitOrg, id, { + collapse: ISSUE_DETAIL_COLLAPSE, + }); + } + if (cachedOrg) { + try { + return await getIssueInOrg(cachedOrg, id, { + collapse: ISSUE_DETAIL_COLLAPSE, + }); + } catch (orgErr) { + if (orgErr instanceof ApiError && orgErr.status === 404) { + // Stale mapping (issue moved / deleted / access revoked). Evict the + // cache entry and fall through to the legacy unscoped endpoint. + clearCachedIssueOrg(id); + return await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + } + throw orgErr; + } + } + return await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); +} + /** * Resolve a bare numeric issue ID. * * Attempts org-scoped resolution with region routing when org context can be - * derived from the working directory (DSN / env vars / config defaults). + * derived from the working directory (DSN / env vars / config defaults), or + * from the issue-id → org cache populated on previous runs. * Falls back to the legacy unscoped endpoint otherwise. * Extracts the org slug from the response permalink so callers like * {@link resolveOrgAndIssueId} can proceed without explicit org context. + * + * Caching: after a successful permalink-based org extraction, records the + * numeric-id → org mapping so future runs skip the unscoped fallback and + * route directly via the regional API. This addresses the + * `sentry.issue.view` "Consecutive HTTP" fan-out pattern for bare numeric + * IDs (Pattern D in the Sentry issue triage). */ async function resolveNumericIssue( id: string, @@ -554,16 +608,22 @@ async function resolveNumericIssue( commandBase = "sentry issue" ): Promise { const resolvedOrg = await resolveOrg({ cwd }); + // Prefer explicit context over the cache — `resolveOrg()` already factors + // in env vars and config defaults that may point at a different org. + const cachedOrg = resolvedOrg ? null : getCachedIssueOrg(id); try { - const issue = resolvedOrg - ? await getIssueInOrg(resolvedOrg.org, id, { - collapse: ISSUE_DETAIL_COLLAPSE, - }) - : await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + const issue = await fetchIssueByNumericId(id, resolvedOrg?.org, cachedOrg); // Extract org from the response permalink as a fallback so that callers // like resolveOrgAndIssueId (used by explain/plan) get the org slug even // when no org context was available before the fetch. - const org = resolvedOrg?.org ?? extractOrgFromPermalink(issue.permalink); + const org = + resolvedOrg?.org ?? cachedOrg ?? extractOrgFromPermalink(issue.permalink); + // Best-effort: remember the numeric-id → org mapping so the next run + // skips the unscoped fallback. Skipped when the org came from a cache + // hit (already stored) or when extraction failed. + if (org && !resolvedOrg && !cachedOrg) { + setCachedIssueOrg(id, org); + } return { org, issue }; } catch (err) { if (err instanceof ApiError && err.status === 404) { @@ -571,7 +631,7 @@ async function resolveNumericIssue( // and suggesting the short-ID format, since users often confuse numeric // group IDs with short-ID suffixes. When org context is available, use // the real org slug instead of placeholder (CLI-BT, 18 users). - const orgHint = resolvedOrg?.org ?? ""; + const orgHint = resolvedOrg?.org ?? cachedOrg ?? ""; const hint = `${commandBase} ${command} ${orgHint}/${id}`; throw new ResolutionError(`Issue ${id}`, "not found", hint, [ `No issue with numeric ID ${id} found — you may not have access, or it may have been deleted.`, diff --git a/src/lib/db/auth.ts b/src/lib/db/auth.ts index ed94f8f73..e6609f604 100644 --- a/src/lib/db/auth.ts +++ b/src/lib/db/auth.ts @@ -6,7 +6,7 @@ import { getEnv } from "../env.js"; import { clearResponseCache } from "../response-cache.js"; import { withDbSpan } from "../telemetry.js"; import { getDatabase } from "./index.js"; -import { runUpsert } from "./utils.js"; +import { clearMetadataByPrefix, runUpsert } from "./utils.js"; /** Refresh when less than 10% of token lifetime remains */ export const REFRESH_THRESHOLD = 0.1; @@ -226,10 +226,13 @@ export async function clearAuth(): Promise { withDbSpan("clearAuth", () => { const db = getDatabase(); db.query("DELETE FROM auth WHERE id = 1").run(); - // Also clear user info, org region cache, and pagination cursors when logging out + // Also clear user info, org region cache, pagination cursors, and the + // issue-id → org cache (scoped to the current user's permissions) when + // logging out. db.query("DELETE FROM user_info WHERE id = 1").run(); db.query("DELETE FROM org_regions").run(); db.query("DELETE FROM pagination_cursors").run(); + clearMetadataByPrefix(db, "issue_org."); }); // Clear cached API responses — they are tied to the current user's permissions. diff --git a/src/lib/db/issue-org-cache.ts b/src/lib/db/issue-org-cache.ts new file mode 100644 index 000000000..ecb72bc48 --- /dev/null +++ b/src/lib/db/issue-org-cache.ts @@ -0,0 +1,101 @@ +/** + * Cache for numeric-issue-ID → organization-slug mappings. + * + * When a user runs `sentry issue view 123456789` without an org context, + * the CLI must fall back to the legacy unscoped `GET /api/0/issues/{id}/` + * endpoint (which does not support region routing) and then extract the + * org from the response `permalink`. Follow-up fetches (events/latest, + * trace/...) require the org slug, so without a cache every subsequent + * command run repeats the same unscoped lookup. + * + * This module records the resolved numeric-id → org-slug mapping so + * future runs can skip straight to the org-scoped endpoint. It addresses + * the `sentry.issue.view` "Consecutive HTTP" pattern for Pattern D in + * the issue triage (numeric-ID org discovery fan-out). + * + * Storage: the existing `metadata` KV table with key shape + * `issue_org.{numericId}` and value = org slug. No schema migration + * required. Entries are best-effort: a stale mapping (issue deleted, + * access revoked, or moved) causes a single 404 on the cached org + * call which the caller falls back from, and we evict the entry. + * + * Values are not TTL'd because issues are owned by a single org for + * their entire lifetime — the mapping cannot change except by issue + * deletion, which we already handle via 404 eviction. + */ + +import { recordCacheHit } from "../telemetry.js"; +import { getDatabase } from "./index.js"; +import { + clearMetadata, + clearMetadataByPrefix, + getMetadata, + setMetadata, +} from "./utils.js"; + +/** Metadata key namespace for issue-id → org mappings. */ +const KEY_PREFIX = "issue_org."; + +function metadataKey(numericId: string): string { + return `${KEY_PREFIX}${numericId}`; +} + +/** + * Look up the cached organization slug for a numeric issue ID. + * + * @param numericId - Numeric issue group ID (e.g., "7413562541") + * @returns Org slug if cached, undefined otherwise + */ +export function getCachedIssueOrg(numericId: string): string | undefined { + const db = getDatabase(); + const map = getMetadata(db, [metadataKey(numericId)]); + const org = map.get(metadataKey(numericId)); + recordCacheHit("issue_org", !!org); + return org; +} + +/** + * Remember the organization slug for a numeric issue ID. + * + * Silently no-ops when either argument is empty. Best-effort — callers + * should not await this as a critical step; the DB layer already wraps + * writes to be fault-tolerant. + * + * @param numericId - Numeric issue group ID (e.g., "7413562541") + * @param orgSlug - Organization slug that owns the issue + */ +export function setCachedIssueOrg(numericId: string, orgSlug: string): void { + if (!(numericId && orgSlug)) { + return; + } + const db = getDatabase(); + setMetadata(db, { [metadataKey(numericId)]: orgSlug }); +} + +/** + * Drop the cached mapping for a numeric issue ID. + * + * Called when an org-scoped fetch 404s so subsequent runs re-resolve + * the org via the legacy unscoped endpoint. + * + * @param numericId - Numeric issue group ID + */ +export function clearCachedIssueOrg(numericId: string): void { + if (!numericId) { + return; + } + const db = getDatabase(); + clearMetadata(db, [metadataKey(numericId)]); +} + +/** + * Drop ALL issue-id → org mappings. + * + * Called from auth logout handlers so signing out with one account does + * not leak mappings into a different account's session. Kept separate + * from `clearMetadata` so the caller can target just this cache class. + */ +export function clearAllIssueOrgCache(): void { + const db = getDatabase(); + clearMetadataByPrefix(db, KEY_PREFIX); +} diff --git a/src/lib/db/project-cache.ts b/src/lib/db/project-cache.ts index a64dd062d..072da2dcc 100644 --- a/src/lib/db/project-cache.ts +++ b/src/lib/db/project-cache.ts @@ -108,6 +108,48 @@ export function setCachedProjectByDsnKey( setByKey(dsnCacheKey(publicKey), info); } +/** + * Look up a cached project by organization and project slug. + * + * Returns the most recently cached entry matching the (org_slug, project_slug) + * pair across ALL cache key shapes (`orgId:projectId`, `dsn:publicKey`, + * `list:{org}/{project}`). This is useful for slug-based lookups where the + * caller doesn't know the numeric org/project IDs upfront — e.g., when + * resolving `/` from CLI arguments in `fetchProjectId`. + * + * Uses `MAX(cached_at)` with a covariant SELECT: SQLite guarantees the other + * columns come from the row that produced the MAX value. This avoids the + * ambiguity that would arise from a plain `LIMIT 1` without an ORDER BY. + * + * @param orgSlug - Organization slug (case-sensitive) + * @param projectSlug - Project slug (case-sensitive) + * @returns Cached project entry, or undefined if no match + */ +export function getCachedProjectBySlug( + orgSlug: string, + projectSlug: string +): CachedProject | undefined { + const db = getDatabase(); + const row = db + .query( + "SELECT cache_key, org_slug, org_name, project_slug, project_name, project_id, MAX(cached_at) AS cached_at, last_accessed FROM project_cache WHERE org_slug = ? AND project_slug = ?" + ) + .get(orgSlug, projectSlug) as + | (ProjectCacheRow & { "MAX(cached_at)"?: number }) + | undefined; + + // When no rows match, SQLite still returns a single row with NULL columns + // because of the MAX aggregate — guard on cache_key being populated. + if (!row?.cache_key) { + recordCacheHit("project", false); + return; + } + + recordCacheHit("project", true); + touchCacheEntry("project_cache", "cache_key", row.cache_key); + return rowToCachedProject(row); +} + /** * Get cached project slugs for a specific organization. * diff --git a/src/lib/db/utils.ts b/src/lib/db/utils.ts index 8d12eb3ca..5d57d391c 100644 --- a/src/lib/db/utils.ts +++ b/src/lib/db/utils.ts @@ -198,6 +198,27 @@ export function clearMetadata(db: QueryRunner, keys: string[]): void { db.query(`DELETE FROM metadata WHERE key IN (${placeholders})`).run(...keys); } +/** + * Delete all keys from the `metadata` table that start with a given prefix. + * + * Intended for cache-style entries grouped under a common namespace + * (e.g., `issue_org.`). The prefix is matched literally — SQLite + * `LIKE` wildcards (`%` and `_`) in the prefix are NOT escaped, so callers + * must not pass user-controlled values. + * + * @param db - Database instance + * @param prefix - Namespace prefix to match (e.g., "issue_org.") + * + * @example + * clearMetadataByPrefix(db, "issue_org."); + */ +export function clearMetadataByPrefix(db: QueryRunner, prefix: string): void { + if (prefix.length === 0) { + return; + } + db.query("DELETE FROM metadata WHERE key LIKE ?").run(`${prefix}%`); +} + // --------------------------------------------------------------------------- // Cache entry helpers // --------------------------------------------------------------------------- diff --git a/src/lib/resolve-target.ts b/src/lib/resolve-target.ts index 49b98fde8..cdcc80755 100644 --- a/src/lib/resolve-target.ts +++ b/src/lib/resolve-target.ts @@ -31,6 +31,7 @@ import { getCachedDsn, setCachedDsn } from "./db/dsn-cache.js"; import { getCachedProject, getCachedProjectByDsnKey, + getCachedProjectBySlug, setCachedProject, setCachedProjectByDsnKey, } from "./db/project-cache.js"; @@ -821,11 +822,29 @@ export async function triageProjectNotFound( * * On 404, attempts to list similar projects in the org to help the * user find the correct slug (CLI-C0, 36 users). + * + * Consults the slug-based project cache first to avoid an extra + * `GET /projects/{org}/{project}/` call on every `/` + * invocation. The cache is populated by `listProjects()` (batch) and + * by DSN resolution. Cache entries without a `projectId` fall through + * to the API call (older rows from before schema v7). */ export async function fetchProjectId( org: string, project: string ): Promise { + // Cache-first: avoid a round trip when `listProjects()` or DSN resolution + // has already populated the entry for this (org, project) slug pair. + // Addresses the `sentry.issue.list` "Consecutive HTTP" performance issue + // where the preflight project lookup ran before every issues fetch. + const cached = getCachedProjectBySlug(org, project); + if (cached?.projectId) { + const numeric = toNumericId(cached.projectId); + if (numeric !== undefined) { + return numeric; + } + } + const projectResult = await withAuthGuard(() => getProject(org, project)); if (!projectResult.ok) { if ( @@ -851,7 +870,22 @@ export async function fetchProjectId( } return; } - return toNumericId(projectResult.value.id); + + // Populate the cache for next time. The `getProject` response carries the + // numeric project ID and (usually) the organization payload; use whatever + // is available to seed future lookups. + const project_ = projectResult.value; + if (project_.organization) { + setCachedProject(project_.organization.id, project_.id, { + orgSlug: project_.organization.slug, + orgName: project_.organization.name, + projectSlug: project_.slug, + projectName: project_.name, + projectId: project_.id, + }); + } + + return toNumericId(project_.id); } /** diff --git a/test/lib/db/issue-org-cache.test.ts b/test/lib/db/issue-org-cache.test.ts new file mode 100644 index 000000000..08405a0c1 --- /dev/null +++ b/test/lib/db/issue-org-cache.test.ts @@ -0,0 +1,114 @@ +/** + * Issue-Org Cache Tests + * + * Covers caching of numeric-issue-id → org-slug mappings used by + * `resolveNumericIssue` to skip the legacy unscoped `/api/0/issues/{id}/` + * endpoint on subsequent runs. + */ + +import { describe, expect, test } from "bun:test"; +import { + clearAllIssueOrgCache, + clearCachedIssueOrg, + getCachedIssueOrg, + setCachedIssueOrg, +} from "../../../src/lib/db/issue-org-cache.js"; +import { useTestConfigDir } from "../../helpers.js"; + +useTestConfigDir("test-issue-org-cache-"); + +describe("getCachedIssueOrg", () => { + test("returns undefined when no entry exists", () => { + expect(getCachedIssueOrg("123456789")).toBeUndefined(); + }); + + test("returns the stored org slug", () => { + setCachedIssueOrg("7413562541", "brandai"); + expect(getCachedIssueOrg("7413562541")).toBe("brandai"); + }); + + test("distinguishes between different issue IDs", () => { + setCachedIssueOrg("111", "org-a"); + setCachedIssueOrg("222", "org-b"); + + expect(getCachedIssueOrg("111")).toBe("org-a"); + expect(getCachedIssueOrg("222")).toBe("org-b"); + expect(getCachedIssueOrg("333")).toBeUndefined(); + }); +}); + +describe("setCachedIssueOrg", () => { + test("overwrites the mapping for the same issue ID", () => { + setCachedIssueOrg("42", "first-org"); + setCachedIssueOrg("42", "second-org"); + + expect(getCachedIssueOrg("42")).toBe("second-org"); + }); + + test("is a no-op when numericId is empty", () => { + setCachedIssueOrg("", "org"); + // No assertion on internal state — it just must not throw or pollute + // the cache. Use a sentinel read that would never match an empty key. + expect(getCachedIssueOrg("")).toBeUndefined(); + }); + + test("is a no-op when org is empty", () => { + setCachedIssueOrg("99", ""); + expect(getCachedIssueOrg("99")).toBeUndefined(); + }); +}); + +describe("clearCachedIssueOrg", () => { + test("removes a single mapping", () => { + setCachedIssueOrg("101", "org-x"); + setCachedIssueOrg("102", "org-y"); + + clearCachedIssueOrg("101"); + + expect(getCachedIssueOrg("101")).toBeUndefined(); + // Other mappings untouched + expect(getCachedIssueOrg("102")).toBe("org-y"); + }); + + test("is idempotent on missing IDs", () => { + expect(() => clearCachedIssueOrg("nonexistent")).not.toThrow(); + }); + + test("is a no-op when numericId is empty", () => { + setCachedIssueOrg("50", "org"); + clearCachedIssueOrg(""); + // Empty key must not wipe unrelated entries. + expect(getCachedIssueOrg("50")).toBe("org"); + }); +}); + +describe("clearAllIssueOrgCache", () => { + test("removes all issue_org.* entries", () => { + setCachedIssueOrg("1", "a"); + setCachedIssueOrg("2", "b"); + setCachedIssueOrg("3", "c"); + + clearAllIssueOrgCache(); + + expect(getCachedIssueOrg("1")).toBeUndefined(); + expect(getCachedIssueOrg("2")).toBeUndefined(); + expect(getCachedIssueOrg("3")).toBeUndefined(); + }); + + test("does not touch unrelated metadata keys", async () => { + // Seed an unrelated metadata entry using the shared helpers. + const { getDatabase } = await import("../../../src/lib/db/index.js"); + const { getMetadata, setMetadata } = await import( + "../../../src/lib/db/utils.js" + ); + setMetadata(getDatabase(), { "unrelated.key": "keep-me" }); + setCachedIssueOrg("1", "a"); + + clearAllIssueOrgCache(); + + expect(getCachedIssueOrg("1")).toBeUndefined(); + expect( + getMetadata(getDatabase(), ["unrelated.key"]).get("unrelated.key") + ).toBe("keep-me"); + }); +}); diff --git a/test/lib/db/project-cache.test.ts b/test/lib/db/project-cache.test.ts index 24a1e7c50..6007652d8 100644 --- a/test/lib/db/project-cache.test.ts +++ b/test/lib/db/project-cache.test.ts @@ -10,6 +10,7 @@ import { clearProjectCache, getCachedProject, getCachedProjectByDsnKey, + getCachedProjectBySlug, getCachedProjectsForOrg, setCachedProject, setCachedProjectByDsnKey, @@ -460,3 +461,139 @@ describe("getCachedProjectsForOrg", () => { expect(result).toEqual([]); }); }); + +describe("getCachedProjectBySlug", () => { + test("returns undefined when no cache entry exists", () => { + const result = getCachedProjectBySlug("my-org", "my-project"); + expect(result).toBeUndefined(); + }); + + test("returns entry populated via setCachedProject (orgId:projectId key)", () => { + setCachedProject("123", "456", { + orgSlug: "my-org", + orgName: "My Organization", + projectSlug: "my-project", + projectName: "My Project", + projectId: "456", + }); + + const result = getCachedProjectBySlug("my-org", "my-project"); + expect(result).toBeDefined(); + expect(result?.orgSlug).toBe("my-org"); + expect(result?.projectSlug).toBe("my-project"); + expect(result?.projectId).toBe("456"); + }); + + test("returns entry populated via setCachedProjectByDsnKey (dsn: key)", () => { + setCachedProjectByDsnKey("abcdef1234567890", { + orgSlug: "dsn-org", + orgName: "DSN Org", + projectSlug: "dsn-project", + projectName: "DSN Project", + projectId: "789", + }); + + const result = getCachedProjectBySlug("dsn-org", "dsn-project"); + expect(result).toBeDefined(); + expect(result?.projectId).toBe("789"); + }); + + test("returns entry populated via cacheProjectsForOrg (list: key)", () => { + cacheProjectsForOrg("my-org", "My Org", [ + { id: "101", slug: "listed-project", name: "Listed Project" }, + ]); + + const result = getCachedProjectBySlug("my-org", "listed-project"); + expect(result).toBeDefined(); + expect(result?.projectId).toBe("101"); + expect(result?.projectName).toBe("Listed Project"); + }); + + test("returns the most recently cached entry when multiple key shapes match", async () => { + // Older entry via dsn key + setCachedProjectByDsnKey("key-1", { + orgSlug: "my-org", + orgName: "My Org", + projectSlug: "dup", + projectName: "Old Name", + projectId: "111", + }); + // Small delay so `cached_at` differs; setCachedProject uses Date.now() + await Bun.sleep(5); + // Newer entry via orgId:projectId key + setCachedProject("org-id", "proj-id", { + orgSlug: "my-org", + orgName: "My Org", + projectSlug: "dup", + projectName: "New Name", + projectId: "222", + }); + + const result = getCachedProjectBySlug("my-org", "dup"); + expect(result).toBeDefined(); + expect(result?.projectName).toBe("New Name"); + expect(result?.projectId).toBe("222"); + }); + + test("does not match a different org", () => { + setCachedProject("123", "456", { + orgSlug: "org-a", + orgName: "A", + projectSlug: "shared-slug", + projectName: "A", + projectId: "456", + }); + + expect(getCachedProjectBySlug("org-b", "shared-slug")).toBeUndefined(); + }); + + test("does not match a different project", () => { + setCachedProject("123", "456", { + orgSlug: "org-a", + orgName: "A", + projectSlug: "proj-a", + projectName: "A", + projectId: "456", + }); + + expect(getCachedProjectBySlug("org-a", "proj-b")).toBeUndefined(); + }); + + test("returns entry even when projectId is missing (older rows)", () => { + setCachedProject("123", "456", { + orgSlug: "legacy-org", + orgName: "Legacy", + projectSlug: "legacy-project", + projectName: "Legacy", + }); + + const result = getCachedProjectBySlug("legacy-org", "legacy-project"); + expect(result).toBeDefined(); + // SQLite stores missing project IDs as NULL; the row mapper passes + // the value through unchanged, so callers see `null` (not undefined). + expect(result?.projectId).toBeFalsy(); + }); +}); + +describe("clearProjectCache", () => { + test("clears all entries across all key shapes", () => { + setCachedProject("a", "b", { + orgSlug: "org", + orgName: "Org", + projectSlug: "proj", + projectName: "Proj", + }); + setCachedProjectByDsnKey("pk", { + orgSlug: "org", + orgName: "Org", + projectSlug: "proj", + projectName: "Proj", + }); + + clearProjectCache(); + + expect(getCachedProject("a", "b")).toBeUndefined(); + expect(getCachedProjectByDsnKey("pk")).toBeUndefined(); + expect(getCachedProjectBySlug("org", "proj")).toBeUndefined(); + }); +}); diff --git a/test/lib/resolve-target.test.ts b/test/lib/resolve-target.test.ts index 4ac9bf3d7..4349b561a 100644 --- a/test/lib/resolve-target.test.ts +++ b/test/lib/resolve-target.test.ts @@ -10,6 +10,11 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { array, constantFrom, assert as fcAssert, property } from "fast-check"; import { DEFAULT_SENTRY_URL } from "../../src/lib/constants.js"; import { setAuthToken } from "../../src/lib/db/auth.js"; +import { + cacheProjectsForOrg, + clearProjectCache, + getCachedProjectBySlug, +} from "../../src/lib/db/project-cache.js"; import { setOrgRegion } from "../../src/lib/db/regions.js"; import { AuthError, ResolutionError } from "../../src/lib/errors.js"; import { @@ -480,6 +485,88 @@ describe("fetchProjectId", () => { const result = await fetchProjectId("test-org", "test-project"); expect(result).toBeUndefined(); }); + + test("returns cached project ID without hitting the API", async () => { + await setAuthToken("test-token"); + setOrgRegion("test-org", DEFAULT_SENTRY_URL); + clearProjectCache(); + // Seed the cache via cacheProjectsForOrg (the `list:` key path) as + // `listProjects()` does in production. + cacheProjectsForOrg("test-org", "Test Org", [ + { id: "999", slug: "cached-project", name: "Cached Project" }, + ]); + + let apiCalled = false; + globalThis.fetch = mockFetch(async () => { + apiCalled = true; + return new Response("should not be called", { status: 500 }); + }); + + const result = await fetchProjectId("test-org", "cached-project"); + expect(result).toBe(999); + expect(apiCalled).toBe(false); + }); + + test("writes the response to the project cache on a cache miss", async () => { + await setAuthToken("test-token"); + setOrgRegion("test-org", DEFAULT_SENTRY_URL); + clearProjectCache(); + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input, init); + if (req.url.includes("/api/0/projects/test-org/fresh-project/")) { + return Response.json({ + id: "1234", + slug: "fresh-project", + name: "Fresh Project", + organization: { + id: "500", + slug: "test-org", + name: "Test Org", + }, + }); + } + return new Response("Not found", { status: 404 }); + }); + + await fetchProjectId("test-org", "fresh-project"); + + // After the call, the cache should be populated so a subsequent call + // skips the API entirely. + const cached = getCachedProjectBySlug("test-org", "fresh-project"); + expect(cached).toBeDefined(); + expect(cached?.projectId).toBe("1234"); + expect(cached?.projectName).toBe("Fresh Project"); + }); + + test("falls through to API when the cache has no projectId (legacy rows)", async () => { + await setAuthToken("test-token"); + setOrgRegion("test-org", DEFAULT_SENTRY_URL); + clearProjectCache(); + // Seed a cache entry WITHOUT a projectId (mirrors pre-schema-v7 rows). + const { setCachedProject } = await import( + "../../src/lib/db/project-cache.js" + ); + setCachedProject("org-id", "proj-id", { + orgSlug: "test-org", + orgName: "Test Org", + projectSlug: "legacy-project", + projectName: "Legacy", + }); + + let apiCalled = false; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input, init); + if (req.url.includes("/api/0/projects/test-org/legacy-project/")) { + apiCalled = true; + return Response.json({ id: "777", slug: "legacy-project" }); + } + return new Response("Not found", { status: 404 }); + }); + + const result = await fetchProjectId("test-org", "legacy-project"); + expect(result).toBe(777); + expect(apiCalled).toBe(true); + }); }); // ============================================================================ From 87e07a48c1188246946a871f1dae4864df1744ad Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 10:45:39 +0000 Subject: [PATCH 2/3] fix(issue-cache): migrate to dedicated table, fix stale cache after 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: - **BYK**: migrate issue-org cache from the `metadata` KV table to a dedicated `issue_org_cache` table (schema v15 with migration). The metadata KV shoehorning was called out as poor practice; dedicated table is cleaner and matches the pattern of other cache tables. - **Sentry Seer + Cursor Bugbot**: fix stale-cachedOrg leak after 404 fallback. When `fetchIssueByNumericId` internally catches a 404 from the cached org, evicts the cache entry, and falls back to the legacy unscoped endpoint, the caller's local `cachedOrg` still held the stale slug and won the `??` chain over `extractOrgFromPermalink()`. Solution: helper now returns `{ issue, cacheEvicted }`; callers treat their local `cachedOrg` as null when `cacheEvicted` is true and re-derive from the permalink. Also re-writes the corrected mapping to the cache so the next run uses the right org. Applies to both `resolveNumericIssue` and `resolveShareIssue`. - **Cursor Bugbot**: use `clearAllIssueOrgCache()` from auth.ts instead of duplicating the table-clear logic with a hardcoded prefix. The `clearMetadataByPrefix` helper is no longer needed and has been removed — the dedicated table handles its own clearing. Tests: - 3 new tests in test/commands/issue/utils.test.ts cover the warm-cache, stale-cache-eviction, and permalink-extraction-then-cache paths with explicit assertions that stale slugs do NOT leak to downstream calls. --- AGENTS.md | 106 +++++++++------------ src/commands/issue/utils.ts | 64 ++++++++++--- src/lib/db/auth.ts | 5 +- src/lib/db/issue-org-cache.ts | 60 ++++++------ src/lib/db/schema.ts | 36 ++++++- src/lib/db/utils.ts | 21 ----- test/commands/issue/utils.test.ts | 140 ++++++++++++++++++++++++++++ test/lib/db/issue-org-cache.test.ts | 5 +- 8 files changed, 309 insertions(+), 128 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a1724a17d..4b0e9c48e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -984,90 +984,74 @@ mock.module("./some-module", () => ({ ### Architecture - -* **Dashboard widget interval computed from terminal width and layout before API calls**: Dashboard widget interval from terminal width: \`computeOptimalInterval()\` in \`src/lib/api/dashboards.ts\` calculates chart interval before API calls. Formula: \`colWidth = floor(layout.w / 6 \* termWidth)\`, \`chartWidth = colWidth - 4 - gutterW\`, \`idealSeconds = periodSeconds / chartWidth\`. Snaps to nearest Sentry bucket (1m–1d). \`periodToSeconds()\` parses \`"24h"\`, \`"7d"\` etc. \`queryWidgetTimeseries\` uses \`params.interval ?? widget.interval\`. + +* **API client wraps all errors as CliError subclasses — no raw exceptions escape**: The API client (src/lib/api-client.ts) wraps ALL errors as CliError subclasses (ApiError or AuthError) — no raw exceptions escape. Commands don't need try-catch for error display; the central handler in app.ts formats CliError cleanly. Only add try-catch when a command needs to handle errors specially (e.g., login continuing despite user-info fetch failure). - -* **DSN org prefix normalization in arg-parsing.ts**: DSN/numeric org prefix normalization — four code paths must all convert to slugs before API calls (many endpoints reject numeric org IDs with 404/403): (1) \`extractOrgIdFromHost\` strips \`o\` prefix during DSN parsing. (2) \`stripDsnOrgPrefix()\` handles user-typed \`o1081365/\` in \`parseOrgProjectArg()\`. (3) \`normalizeNumericOrg()\` in \`resolve-target.ts\` resolves bare numeric IDs via DB cache or uncached API call. (4) Dashboard's \`resolveOrgFromTarget()\` pipes through \`resolveEffectiveOrg()\`, also used by \`tryResolveRecoveryOrg()\` in hex-id-recovery. + +* **Completion fast-path skips Sentry SDK via SENTRY\_CLI\_NO\_TELEMETRY and SQLite telemetry queue**: Shell completions (\`\_\_complete\`) set \`SENTRY\_CLI\_NO\_TELEMETRY=1\` in \`bin.ts\` before any imports, skipping \`createTracedDatabase\` and avoiding \`@sentry/node-core/light\` load (~85ms). Completion timing queued to \`completion\_telemetry\_queue\` SQLite table (~1ms); normal runs drain via \`DELETE FROM ... RETURNING\` and emit as \`Sentry.metrics.distribution\`. Achieves ~60ms dev / ~140ms CI within 200ms e2e budget. - -* **env-registry.ts drives --help env var section + docs**: \`src/lib/env-registry.ts\` (\`ENV\_VAR\_REGISTRY\`) is the single source for all env vars the CLI honors. Entries have \`{name, description, example?, defaultValue?, installOnly?, topLevel?, briefDescription?}\`. \`topLevel: true\` + \`briefDescription\` surfaces in \`sentry --help\` Environment Variables section (via \`formatEnvVarsSection()\` in \`help.ts\`) and in \`sentry help --json\` as \`envVars\` array on the full-tree envelope. Docs generator consumes the full registry for \`configuration.md\`. When adding a new env var, add it here with \`installOnly: true\` if install-script-only. Reserve \`topLevel: true\` for core-path vars only (auth, targeting, URL, key display/logging). + +* **Fuzzy recovery auto-resolves dash/underscore slug mismatches without original-slug fallback**: Display-name project input (contains spaces) skips slug lookup and goes straight to name-based fuzzy search across four resolution sites: \`resolveProjectBySlug\`, \`resolveOrgProjectTarget\` (project-search), \`org-list.ts#handleProjectSearch\`, \`project/list.ts#handleProjectNotFound\`. \`normalizeSlug\` is a no-op — converting spaces→dashes produced misleading warnings. Instead \`parseOrgProjectArg\` detects spaces via \`looksLikeDisplayName()\` and sets \`originalSlug\` on the \`project-search\` variant; sites check \`isDisplayName = originalSlug !== undefined\` and skip \`findProjectsBySlug\` (which 404s on URL-encoded spaces), going directly to \`triageProjectNotFound\` → \`findSimilarProjectsAcrossOrgs\`. \*\*Critical\*\*: recursive fuzzy recovery calls must NOT pass \`originalSlug\` — otherwise the recovered slug also skips lookup, causing infinite skip→empty→not-found. \`ParsedTraceTarget\` doesn't carry \`originalSlug\`. - -* **GHCR versioned nightly tags for delta upgrade support**: GHCR nightly with delta upgrades: Three tag types: \`:nightly\` (rolling), \`:nightly-\\` (immutable), \`:patch-\\` (delta). Delta patches use TRDIFF10 (zstd-compressed), ~50KB vs ~29MB full. Client via \`Bun.zstdDecompressSync()\`. N-1 only, full fallback, SHA-256 verify, 60% size threshold. npm/Node excluded. Test: \`mockGhcrNightlyVersion()\`. + +* **Project cache is org-scoped with three key formats and three population paths**: \`project\_cache\` SQLite table uses three cache key shapes: \`{orgId}:{projectId}\` (DSN resolution), \`dsn:{publicKey}\` (DSN without orgId), \`list:{orgSlug}/{projectSlug}\` (batch from API). Helpers: \`getCachedProject(orgId, projectId)\`, \`getCachedProjectByDsnKey(publicKey)\`, \`getCachedProjectsForOrg(orgSlug)\` for completions, and \`getCachedProjectBySlug(orgSlug, projectSlug)\` which queries all three shapes for hot-path slug lookups (used by \`fetchProjectId\` to skip \`GET /projects/{org}/{project}/\`). Population paths: (1) DSN resolution in resolve-target.ts, (2) \`listProjects()\` batch via \`cacheProjectsForOrg\`, (3) \`fetchProjectId\` seeds on API success. Resolution errors use live API via \`findSimilarProjectsAcrossOrgs\` — no cross-org cache search. - -* **Issue list auto-pagination beyond API's 100-item cap**: Sentry API silently caps \`limit\` at 100 per request. \`listIssuesAllPages()\` auto-paginates using Link headers, bounded by MAX\_PAGINATION\_PAGES (50). \`API\_MAX\_PER\_PAGE\` constant is shared across all paginated consumers. \`--limit\` means total results everywhere (max 1000, default 25). Org-all mode uses \`fetchOrgAllIssues()\`; explicit \`--cursor\` does single-page fetch to preserve cursor chain. + +* **Sentry API: events require org+project, issues have legacy global endpoint**: Sentry API scoping and auth quirks: (1) Events require org+project in URL (\`/projects/{org}/{project}/events/{id}/\`). Issues use legacy global \`/api/0/issues/{id}/\` without org context. Traces need only org (\`/organizations/{org}/trace/{traceId}/\`). Two-step event lookup: fetch issue → extract org/project → fetch event. Cross-project search via Discover \`/organizations/{org}/events/?query=id:{eventId}\`. (2) \`/users/me/\` returns 403 for OAuth tokens — use \`/auth/\` instead (works with ALL token types, on control silo). \`getControlSiloUrl()\` handles routing. \`SentryUserSchema\` uses \`.passthrough()\` since \`/auth/\` only requires \`id\`. (3) Chunk upload endpoint \`/api/0/organizations/{org}/chunk-upload/\` returns camelCase (\`chunkSize\`, \`chunksPerRequest\`, \`maxRequestSize\`, \`hashAlgorithm\`) — exception to snake\_case convention. \`AssembleResponse\` also camelCase (\`missingChunks\`). - -* **resolveProjectBySlug carries full projectData to avoid redundant getProject calls**: resolveProjectBySlug carries full projectData to skip redundant API calls: Returns \`{ org, project, projectData: SentryProject }\` from \`findProjectsBySlug()\`. \`ResolvedOrgProject\`/\`ResolvedTarget\` have optional \`projectData?\` (populated only in project-search path). Downstream commands use \`resolved.projectData ?? await getProject(org, project)\` to save ~500-800ms. + +* **Sentry CLI authenticated fetch architecture with response caching**: \`createAuthenticatedFetch()\` wraps fetch with auth headers, 30s timeout, retry (max 2), 401 token refresh, and span tracing. Response caching via \`http-cache-semantics\` (RFC 7234) stored at \`~/.sentry/cache/responses/\`. Fallback TTL tiers: immutable (24hr), stable (5min), volatile (60s), no-cache (0). Only GET 2xx cached. \`--fresh\` and \`SENTRY\_NO\_CACHE=1\` bypass cache. Cache cleared on login/logout. - -* **Self-hosted OAuth device flow requires Sentry 26.1.0+ and SENTRY\_CLIENT\_ID**: Self-hosted OAuth requires Sentry 26.1.0+ with \`SENTRY\_URL\` and \`SENTRY\_CLIENT\_ID\` env vars. Users must create a public OAuth app in Settings → Developer Settings. Client ID NOT optional for self-hosted. Fallback: \`sentry auth login --token\`. \`getSentryUrl()\`/\`getClientId()\` read lazily so URL parsing from arguments can set \`SENTRY\_URL\` after import. + +* **Sentry CLI resolve-target cascade has 5 priority levels with env var support**: resolve-target.ts cascade has 5 priority levels: (1) Explicit CLI flags, (2) SENTRY\_ORG/SENTRY\_PROJECT env vars, (3) SQLite config defaults, (4) DSN auto-detection, (5) Directory name inference. SENTRY\_PROJECT supports combo notation \`org/project\` — when used, SENTRY\_ORG is ignored. If combo parse fails (e.g. \`org/\`), the entire value is discarded. \`resolveFromEnvVars()\` helper is injected into all four resolution functions. - -* **Sentry CLI markdown-first formatting pipeline replaces ad-hoc ANSI**: Formatters build CommonMark strings; \`renderMarkdown()\` renders to ANSI for TTY or raw markdown for non-TTY. Key helpers: \`colorTag()\`, \`mdKvTable()\`, \`mdRow()\`, \`mdTableHeader()\` (\`:\` suffix = right-aligned), \`renderTextTable()\`. \`isPlainOutput()\` checks \`SENTRY\_PLAIN\_OUTPUT\` > \`NO\_COLOR\` > \`!isTTY\`. Batch path: \`formatXxxTable()\`. Streaming path: \`StreamingTable\` (TTY) or raw markdown rows (plain). Both share \`buildXxxRowCells()\`. - - -* **Sentry dashboard API rejects discover/transaction-like widget types — use spans**: Sentry API dataset gotchas: (1) Events/Explore API accepts \`spans\`, \`transactions\`, \`logs\`, \`errors\`, \`discover\`; \`spansIndexed\` is INVALID (500). Valid list in \`EVENTS\_API\_DATASETS\`. (2) Dashboard \`widgetType\`: \`discover\` and \`transaction-like\` rejected as deprecated — use \`spans\`. \`WIDGET\_TYPES\` (active) vs \`ALL\_WIDGET\_TYPES\` (includes deprecated for parsing). Tests use \`error-events\` not \`discover\`. (3) \`sort\` param only on \`spans\` dataset. (4) \`tracemetrics\` uses comma-separated aggregates; only line/area/bar/table/big\_number displays. - - -* **Sentry issue stats field: time-series controlled by groupStatsPeriod**: Issue stats and list layout: \`stats\` depends on \`groupStatsPeriod\` (\`""\`, \`"14d"\`, \`"24h"\`, \`"auto"\`). Critical: \`count\` is period-scoped — use \`lifetime.count\` for true total. \`--compact\` is tri-state (\`optional: true\`): explicit overrides, \`undefined\` triggers \`shouldAutoCompact(rowCount)\` — compact if \`3N + 3 > termHeight\`. TREND column hidden < 100 cols. Stricli boolean flags with \`optional: true\` produce \`boolean | undefined\` enabling this auto-detect pattern. - - -* **Sentry log IDs are UUIDv7 — enables deterministic retention checks**: Sentry log IDs are UUIDv7 (first 12 hex = ms timestamp, version char \`7\` at pos 13). Traces and event IDs are NOT v7. \`decodeUuidV7Timestamp()\` and \`ageInDaysFromUuidV7()\` in \`src/lib/hex-id.ts\` return null for non-v7 IDs, safe to call unconditionally. Enables deterministic 'past retention' messages instead of 'may have been deleted'. Wired in \`recoverHexId\` (short-circuit) and \`log/view.ts#throwNotFoundError\`. \`RETENTION\_DAYS.log = 90\` in \`src/lib/retention.ts\`; traces/events are \`null\` (plan-dependent). \`LOG\_RETENTION\_PERIOD\` is DERIVED as \`\` \`${RETENTION\_DAYS.log}d\` \`\` — never hardcode \`'90d'\` or \`'90 days'\`. Shared hex primitives (\`HEX\_ID\_RE\`, \`SPAN\_ID\_RE\`, \`UUID\_DASH\_RE\`, \`LEADING\_HEX\_RE\`, \`MIDDLE\_ELLIPSIS\_RE\`, \`HexEntityType\`) live in \`hex-id.ts\` — \`HexEntityType\` stays here (not recovery.ts) to avoid inverted dependency. - - -* **SKILL.md is fully generated — edit source files, not output**: SKILL.md is fully generated — edit sources not output: Skill files under \`plugins/sentry-cli/skills/sentry-cli/\` generated by \`bun run generate:skill\`. CI auto-commits. Edit sources: (1) \`docs/src/content/docs/agent-guidance.md\` for SKILL.md content, (2) \`src/commands/\*/\` flag \`brief\` strings for reference descriptions, (3) \`docs/src/content/docs/commands/\*.md\` for examples. \`bun run check:skill\` fails if stale. - - -* **Stricli route errors are uninterceptable — only post-run detection works**: Stricli error gaps: (1) Route failures uninterceptable — Stricli writes stderr, returns \`ExitCode.UnknownCommand\` (-5 / 251 in Bun); only post-\`run()\` \`process.exitCode\` check works. (2) \`OutputError\` calls \`process.exit()\` immediately, bypassing telemetry. (3) \`defaultCommand: 'help'\` bypasses built-in fuzzy matching — fixed by \`resolveCommandPath()\` in \`introspect.ts\` with \`fuzzyMatch()\` (up to 3 suggestions); JSON includes \`suggestions\` array. (4) Plural alias detection in \`app.ts\`. +### Decision - -* **Three Sentry APIs for span custom attributes with different capabilities**: Three Sentry span APIs: (1) \`/trace/{traceId}/\` — hierarchical tree; \`additional\_attributes\` enumerates requested attrs; returns \`measurements\` (zero-filled on non-browser, stripped by \`filterSpanMeasurements()\`). (2) \`/projects/{org}/{project}/trace-items/{itemId}/\` — single span full detail; ALL attributes as \`{name,type,value}\[]\` automatically. (3) \`/events/?dataset=spans\&field=X\` — list/search; explicit \`field\` params. \`--fields\` flag filters JSON output AND requests extra API fields via \`extractExtraApiFields()\`. \`FIELD\_GROUP\_ALIASES\` supports shorthand expansion. + +* **Issue list global limit with fair per-project distribution and representation guarantees**: \`issue list --limit\` is a global total across all detected projects. \`fetchWithBudget\` Phase 1 divides evenly, Phase 2 redistributes surplus via cursor resume. \`trimWithProjectGuarantee\` ensures ≥1 issue per project before filling remaining slots. JSON output wraps in \`{ data, hasMore }\` with optional \`errors\` array. Compound cursor (pipe-separated) enables \`-c last\` for multi-target pagination, keyed by sorted target fingerprint. - -* **Update notification rate-limited via last\_notified metadata + TTY gate**: \`src/lib/version-check.ts\` \`getUpdateNotification()\` now enforces a 24h rate limit via \`KEY\_LAST\_NOTIFIED\` metadata + an in-process latch (\`notifiedThisRun\`) plus a \`process.stderr.isTTY\` gate. Non-TTY invocations never see the banner (scripts, CI, \`| cat\`). \`setLastNotifiedNow()\` persists on emit; the latch prevents double-emit within a single process. Tests must mock \`process.stderr.isTTY = true\` AND reset the in-process latch between cases via the exported \`\_resetNotificationStateForTests()\` helper. Mirrors the pattern of other one-shot hints in the CLI. + +* **Sentry CLI config dir should stay at ~/.sentry/, not move to XDG**: Config dir stays at \`~/.sentry/\` (not XDG). Readonly DB errors on macOS come from \`sudo brew install\` creating root-owned files. Fixes: (1) \`bestEffort()\` makes setup steps non-fatal, (2) \`tryRepairReadonly()\` detects root-owned files and prints \`sudo chown\` instructions, (3) \`sentry cli fix\` handles ownership repair. Check ownership BEFORE permissions — root-owned files cause chmod to EPERM. -### Decision +### Gotcha - -* **400 Bad Request from Sentry API indicates a CLI bug, not a user error**: Telemetry 400 convention: 400 = CLI bug (capture to Sentry), 401-499 = user error (skip). \`isUserApiError()\` uses \`> 400\` (exclusive). \`isExpectedUserError()\` guard in \`app.ts\` skips ContextError, ResolutionError, ValidationError, SeerError, 401-499 ApiErrors. Captures 400, 5xx, unknown. Skipped errors → breadcrumbs. For \`ApiError\`, call \`Sentry.setContext('api\_error', {...})\` before \`captureException\` — SDK doesn't auto-capture custom properties. + +* **Biome plugin rule forbids raw metadata queries — use db/utils.ts helpers**: Biome lint rules that trip developers: (1) Plugin forbids raw metadata KV queries — use \`getMetadata\`/\`setMetadata\`/\`clearMetadata\`/\`clearMetadataByPrefix\` from \`src/lib/db/utils.ts\`. Any \`db.query("... metadata ...")\` including \`DELETE ... WHERE key LIKE ?\` fails CI lint (test files exempt). Reuse \`metadata\` table for lightweight caches with dotted-prefix keys (\`issue\_org.{numericId}\`, \`defaults.\`, \`install.\`) instead of new tables+migrations. Add eviction to \`clearAuth()\` if user-scoped. (2) \`noExcessiveCognitiveComplexity\` max 15 — extract helpers from Stricli \`func()\` handlers; \`resolveOrgProjectTarget\` chronically needs \`// biome-ignore\`. (3) \`useBlockStatements\` — always use braces. (4) \`noNestedTernary\` — use if/else. (5) \`useAtIndex\` — use \`arr.at(-1)\`. (6) \`noStaticOnlyClass\` — use branded instances. - -* **CLI UX philosophy: auto-recover when intent is clear, warn gently**: UX principle: don't fail when intent is clear — do the intent and nudge via \`log.warn()\` to stderr. Keep errors in Sentry telemetry for visibility (e.g., SeerError for upsell tracking). Two recovery tiers: (1) auto-correct when semantics identical (AND→space), (2) auto-recover with warning when semantics differ (OR→space, warn about union→intersection). Only throw when intent can't be fulfilled. Model after \`gh\` CLI. AI agents are primary consumers constructing natural OR/AND queries. + +* **Bun mock.module() leaks globally across test files in same process**: Bun's mock.module() replaces modules globally and leaks across test files in the same process. Solution: tests using mock.module() must run in a separate \`bun test\` invocation. In package.json, use \`bun run test:unit && bun run test:isolated\` instead of \`bun test\`. The \`test/isolated/\` directory exists for these tests. This was the root cause of ~100 test failures (getsentry/cli#258). - -* **Trace-related commands must handle project consistently across CLI**: Trace/log commands and project scoping: \`getDetailedTrace\` accepts optional numeric \`projectId\` (not hardcoded \`-1\`). Resolve slug→ID via \`getProject()\`. \`formatSimpleSpanTree\` shows orphan annotation only when \`projectFiltered\` is set. \`buildProjectQuery()\` in \`arg-parsing.ts\` prepends \`project:\\` to queries (used by \`trace/logs.ts\`, \`log/list.ts\`). Multi-project: \`--query 'project:\[cli,backend]'\`. Trace-logs endpoint (\`/organizations/{org}/trace-logs/\`) is org-scoped — uses \`resolveOrg()\` not \`resolveOrgAndProject()\`. Endpoint is PRIVATE (no \`@sentry/api\` types); hand-written \`TraceLogSchema\` in \`src/types/sentry.ts\` required. + +* **Consecutive HTTP performance issues on sentry-cli are mostly not actionable**: Most reported "Consecutive HTTP" perf issues in sentry-cli are true data dependencies, not parallelization misses. \`sentry issue view\` chains shortids → events/latest → trace because traceId+timestamp come from the event body. \`issue events\` and \`issue list\` auto-paginate with cursor chains when \`--limit > API\_MAX\_PER\_PAGE\` (issue-events endpoint has no \`per\_page\`, server caps ~25). Cold-cache \`issue view \\` fires \`/organizations/\` fan-out then issue → event → trace. Before investigating, confirm spans aren't a dependency chain or cursor sequence. Actionable wins are narrow: \`fetchProjectId\` preflight cache and caching numeric-issue-id → org after legacy \`/issues/{id}/\` fallback. -### Gotcha + +* **Making clearAuth() async breaks model-based tests — use non-async Promise\ return instead**: Making \`clearAuth()\` \`async\` breaks fast-check model-based tests — real async yields during \`asyncModelRun\` cause \`createIsolatedDbContext\` cleanup to interleave. Keep non-async; return \`clearResponseCache().catch(...)\` directly. Model-based tests also need explicit timeouts (e.g., \`30\_000\`) — Bun's default 5s causes false failures during shrinking. - -* **Biome lint differs between local lint:fix and CI lint**: \`bun run lint:fix\` may auto-apply fixes locally that hide issues CI's \`bun run lint\` will still catch. Specifically: (1) \`noPrecisionLoss\` on large integer literals (>2^53), (2) \`noIncrementDecrement\` on \`count++\`, (3) import ordering when a named import is placed after non-import runtime code. Biome's formatter also rewrites multi-line imports to single-line when they fit — the auto-fix may leave stale state. Always run \`bun run lint\` (not just \`lint:fix\`) before pushing. Use \`for...of\` with array destructuring or explicit \`i += 1\` instead of \`++\`. Use \`Number(string)\` or split literals instead of \`1\_735\_689\_600\_000\_000\_001\`. + +* **Multiregion mock must include all control silo API routes**: When changing which Sentry API endpoint a function uses, mock routes must be updated in BOTH \`test/mocks/routes.ts\` (single-region) AND \`test/mocks/multiregion.ts\` \`createControlSiloRoutes()\`. Missing the multiregion mock causes 404s in multi-region test scenarios. - -* **Bun mock.module for node:tty requires default export and class stubs**: Bun testing gotchas: (1) \`mock.module()\` for CJS built-ins needs \`default\` re-export plus named exports, top-level BEFORE \`await import()\`; lives in \`test/isolated/\`. (2) Destructuring imports capture binding at load; verify via call-count > 0. (3) \`Bun.mmap()\` always opens PROT\_WRITE — use \`new Uint8Array(await Bun.file(path).arrayBuffer())\`. (4) Wrap \`Bun.which()\` with optional \`pathEnv\` for deterministic testing. (5) Mocking \`@sentry/node-core/light\`: \`startSpan\` must pass mock span to callback: \`startSpan: (\_, fn) => fn({ setStatus(){}, setAttribute(){}, end(){} })\`. + +* **Source Map v3 spec allows null entries in sources array**: The Source Map v3 spec allows \`null\` entries in the \`sources\` array, and bundlers like esbuild actually produce them. Any code iterating over \`sources\` and calling string methods (e.g., \`.replaceAll()\`) must guard against null: \`map.sources.map((s) => typeof s === "string" ? s.replaceAll("\\\\", "/") : s)\`. Without the guard, \`null.replaceAll()\` throws \`TypeError\`. This applies to \`src/lib/sourcemap/debug-id.ts\` and any future sourcemap manipulation code. ### Pattern - -* **403 scope extraction via api-scope.ts helpers**: \`src/lib/api-scope.ts\` exports \`extractRequiredScopes(detail)\` which scans Sentry 403 response detail (string or structured) for scope-like tokens (e.g. \`event:read\`, \`project:admin\`). Matches free-text and structured \`required\`/\`required\_scopes\` fields. Use in 403-enrichment paths instead of hardcoded generic scope lists; fall back to generic hint only when extraction returns empty. Wired into \`issue list\` \`build403Detail()\`, \`organizations.ts\` \`enrich403Error()\`, and anywhere a 403 is re-thrown with a required-scopes suggestion. + +* **Bun global installs use .bun path segment for detection**: Bun global package installs place scripts under \`~/.bun/install/global/node\_modules/\`. The \`.bun\` directory in the path distinguishes bun from npm/yarn installs. In \`detectPackageManagerFromPath()\`, check \`segments.includes('.bun')\` before the npm fallback. Detection order: \`.pnpm\` → pnpm, \`.bun\` → bun, other \`node\_modules\` → npm. Yarn classic uses same flat layout as npm so falls through to npm detection — this is acceptable because path-based detection is a \*\*fallback\*\* after subprocess calls (which correctly identify yarn). Path detection must NOT be authoritative/override stored DB info, only serve as fallback when subprocess detection fails (e.g., Windows where .cmd files cause ENOENT). - -* **I/O concurrency limits belong at the call site, not in generic combinators**: I/O concurrency limits belong at the call site, not in generic combinators. Pattern: module-scoped \`pLimit()\` with named constant (e.g., \`STAT\_CONCURRENCY = 32\` in \`project-root.ts\`, \`CACHE\_IO\_CONCURRENCY\` in \`response-cache.ts\`, \`pLimit(50)\` in \`code-scanner.ts\`). Keeps combinators pure, makes budget explicit at I/O boundary. stat() lighter than full reads — ~32 for stats vs ~50 for reads, well below macOS's 256 FD ceiling. + +* **Non-essential DB cache writes should be guarded with try-catch**: Non-essential DB cache writes (e.g., \`setUserInfo()\`, \`setInstallInfo()\`) must be wrapped in try-catch so a broken/read-only DB doesn't crash a command whose primary operation succeeded. Pattern: \`try { setInstallInfo(...) } catch { log.debug(...) }\`. In login.ts, \`getCurrentUser()\` failure after token save must not block auth — log warning, continue. In upgrade.ts, \`setInstallInfo\` after legacy detection is guarded same way. Exception: \`getUserRegions()\` failure should \`clearAuth()\` and fail hard (indicates invalid token). This is enforced by BugBot reviews — any \`setInstallInfo\`/\`setUserInfo\` call outside setup.ts's \`bestEffort()\` wrapper needs its own try-catch. - -* **Identity-scoped response cache via fingerprint mixin**: Response cache key is \`sha256(identity|method|url)\` where identity comes from \`getIdentityFingerprint()\` in \`src/lib/db/auth.ts\`: \`oauth:\\` for stored OAuth (stable across hourly access-token refresh), \`env:\\` for env tokens, or \`ANON\_IDENTITY\` when no auth. \`getCachedResponse\`/\`storeCachedResponse\`/\`invalidateCachedResponse\` resolve identity internally; \`buildCacheKey(method, url, identity)\` takes it explicitly for tests. This auto-isolates cross-account state (whoami, org lists) without explicit invalidation. Added \`invalidateCachedResponsesMatching(urlPrefix)\` for paginated-list wipes after mutations. To avoid a \`db/auth.ts\` ↔ \`response-cache.ts\` cycle, \`clearAuth()\` dynamically imports \`clearResponseCache\`. + +* **Sentry CLI command docs are auto-generated from Stricli route tree with CI freshness check**: Sentry CLI command docs are auto-generated from Stricli route tree: Docs in \`docs/src/content/docs/commands/\*.md\` and skill files in \`plugins/sentry-cli/skills/sentry-cli/references/\*.md\` are generated via \`bun run generate:docs\`. Content between \`\\` markers is regenerated; hand-written examples go in \`docs/src/fragments/commands/\`. CI checks \`check:command-docs\` and \`check:skill\` fail if stale. Run generators after changing command parameters/flags/docs. - -* **Isolated adapter coverage via fetch mocking in test/lib/**: To get CodeCov coverage on API-calling functions (e.g., hex-id-recovery adapters, api-client functions), write tests in \`test/lib/\*.coverage.test.ts\` or \`test/lib/\*.adapters.test.ts\` that mock \`globalThis.fetch\` via \`mockFetch()\` from \`test/helpers.js\`, call \`setAuthToken()\` + \`setOrgRegion()\` in \`beforeEach\`, and invoke the REAL function. Tests in \`test/e2e/\` or tests that stub the exports via \`spyOn\`/\`mock.module\` give ZERO coverage to the mocked function body. Use \`useTestConfigDir()\` for DB isolation. Pattern example: \`test/lib/api-client.coverage.test.ts\` and \`test/lib/hex-id-recovery.adapters.test.ts\`. Mock responses must include ALL Zod-required fields — minimal stubs fail schema validation with a noisy \`ApiError\`. + +* **Stricli buildCommand output config injects json flag into func params**: Stricli command gotchas: (1) In \`func()\` handlers use \`this.stdout\`/\`this.stderr\` directly — NOT \`this.process.stdout\`. \`SentryContext\` has \`process\` and \`stdout\`/\`stderr\` as separate top-level properties. Test mocks provide \`stdout\` but not full \`process\`, so \`this.process.stdout\` throws \`TypeError\` at runtime (TS doesn't flag). (2) When \`output: { json: true, human: formatFn }\` is set, \`--json\` and \`--fields\` flags are auto-injected. Type the flags param explicitly (\`flags: { json?: boolean }\`) to access. Commands with interactive side effects (browser prompts, QR codes) should check \`flags.json\` and skip when true. (3) Route maps with \`defaultCommand\` blend the default command's flags into subcommand completions — completion tests must track \`hasDefaultCommand\` and skip strict subcommand-matching assertions. - -* **Pagination contextKey must include all query-varying parameters with escaping**: Pagination \`contextKey\` must encode every query-varying parameter (sort, query, period) with \`escapeContextKeyValue()\` (replaces \`|\` with \`%7C\`). Provide fallback before escaping since \`flags.period\` may be \`undefined\` in tests despite defaults: \`flags.period ? escapeContextKeyValue(flags.period) : "90d"\`. \`DEFAULT\_PERIOD = "90d"\` is a module constant. Compare via \`formatTimeRangeFlag(flags.period) !== DEFAULT\_PERIOD\` or \`appendPeriodHint()\` from \`time-range.ts\`. + +* **ZipWriter and file handle cleanup: always use try/finally with close()**: ZipWriter in \`src/lib/sourcemap/zip.ts\` has \`close()\` for error cleanup; \`finalize()\` uses try/finally for \`fh.close()\`. Callers must wrap usage in try/catch and call \`zip.close()\` in catch. Pattern: create zip → try { add entries + finalize } catch { zip.close(); throw }. Prevents file handle leaks on partial failures. - -* **Redact sensitive flags in raw argv before sending to telemetry**: Telemetry argv redaction and context: \`withTelemetry\` calls \`initTelemetryContext()\` — sets user ID, email, runtime, is\_self\_hosted. Org from \`getDefaultOrganization()\` (SQLite). \`SENSITIVE\_FLAGS\` in \`telemetry.ts\` (currently \`token\`) — scans for \`--token\`/\`-token\`, replaces value with \`\[REDACTED]\`. Handles \`--flag value\` and \`--flag=value\`. Command tag format: \`sentry.issue.list\` (dot-joined with \`sentry.\` prefix). +### Preference - -* **Stricli parse functions can perform validation and sanitization at flag-parse time**: Stricli's \`parse\` fn on \`kind: "parsed"\` flags runs during argument parsing before \`func()\`. Can throw errors (including \`ValidationError\`) and log warnings. Current uses: \`parseCursorFlag\`, \`sanitizeQuery\`, \`parsePeriod\` (returns \`TimeRange\`), \`parseSort\`/\`parseSortFlag\`, \`numberParser\`/\`parseLimit\`. Optional period flags: \`flags.period\` is \`TimeRange | undefined\` — commands use \`TIME\_RANGE\_\*\` constants as defaults. \`formatTimeRangeFlag()\` converts back to display strings. \`appendPeriodHint(parts, period, flagName)\` in \`time-range.ts\` encapsulates \`if (formatted !== DEFAULT\_PERIOD) parts.push(...)\` across 4+ hint builders. + +* **PR workflow: address review comments, resolve threads, wait for CI**: PR workflow: (1) wait for CI, (2) check unresolved review comments via \`gh api repos/.../pulls/N/comments\`, (3) fix in follow-up commits (not amends), (4) reply explaining fix, (5) resolve thread via \`gh api graphql\` \`resolveReviewThread\` mutation, (6) push and re-check CI, (7) final sweep. BugBot/Seer post new comments on each follow-up commit and may \`skip\` small ones — always re-check comments after each push, don't trust CI status alone. Use \`git notes add\` for plans. Branch naming: \`fix/\*\` or \`feat/\*\`. Code style: \`Array.from(set)\` / \`Array.from(new Set(a.concat(b)))\` over spreads; "allowlist" not "whitelist"; \`arr.at(-1)\` over \`arr\[arr.length-1]\`. Reviewer questions may be inquiries — confirm intent before changing. diff --git a/src/commands/issue/utils.ts b/src/commands/issue/utils.ts index 78d3b745d..f6d76389a 100644 --- a/src/commands/issue/utils.ts +++ b/src/commands/issue/utils.ts @@ -492,10 +492,10 @@ async function resolveShareIssue( // Fetch full issue via authenticated API if (org) { const resolvedOrg = await resolveEffectiveOrg(org); - const issue = await getIssueInOrg(resolvedOrg, groupId, { + const orgScopedIssue = await getIssueInOrg(resolvedOrg, groupId, { collapse: ISSUE_DETAIL_COLLAPSE, }); - return { org: resolvedOrg, issue }; + return { org: resolvedOrg, issue: orgScopedIssue }; } // No org from URL — try env/DSN context, then the issue-id → org cache, @@ -503,14 +503,19 @@ async function resolveShareIssue( // full rationale behind the cache. const resolvedOrg = await resolveOrg({ cwd }); const cachedOrg = resolvedOrg ? null : getCachedIssueOrg(groupId); - const issue = await fetchIssueByNumericId( + const { issue, cacheEvicted } = await fetchIssueByNumericId( groupId, resolvedOrg?.org, cachedOrg ); + // When `cacheEvicted` is true, the cached org was stale (404'd) — do NOT + // let it win the `??` chain; re-derive from the permalink instead. + const effectiveCachedOrg = cacheEvicted ? null : cachedOrg; const resolvedOrgSlug = - resolvedOrg?.org ?? cachedOrg ?? extractOrgFromPermalink(issue.permalink); - if (resolvedOrgSlug && !resolvedOrg && !cachedOrg) { + resolvedOrg?.org ?? + effectiveCachedOrg ?? + extractOrgFromPermalink(issue.permalink); + if (resolvedOrgSlug && !resolvedOrg && !effectiveCachedOrg) { setCachedIssueOrg(groupId, resolvedOrgSlug); } return { org: resolvedOrgSlug, issue }; @@ -557,32 +562,50 @@ function extractOrgFromPermalink( * Extracted from {@link resolveNumericIssue} to keep its cognitive * complexity below the project's lint threshold. */ +/** + * Result of {@link fetchIssueByNumericId}. + * + * `cacheEvicted` is true when the helper invalidated a stale `cachedOrg` + * entry after a 404 and fell through to the legacy unscoped endpoint. + * Callers MUST treat their local `cachedOrg` as stale when this flag is + * set and re-derive the org from `issue.permalink` instead — otherwise + * a stale slug leaks into downstream API calls (issue events, traces). + */ +type FetchIssueByNumericIdResult = { + issue: SentryIssue; + cacheEvicted: boolean; +}; + async function fetchIssueByNumericId( id: string, explicitOrg: string | undefined, cachedOrg: string | null | undefined -): Promise { +): Promise { if (explicitOrg) { - return await getIssueInOrg(explicitOrg, id, { + const issue = await getIssueInOrg(explicitOrg, id, { collapse: ISSUE_DETAIL_COLLAPSE, }); + return { issue, cacheEvicted: false }; } if (cachedOrg) { try { - return await getIssueInOrg(cachedOrg, id, { + const issue = await getIssueInOrg(cachedOrg, id, { collapse: ISSUE_DETAIL_COLLAPSE, }); + return { issue, cacheEvicted: false }; } catch (orgErr) { if (orgErr instanceof ApiError && orgErr.status === 404) { // Stale mapping (issue moved / deleted / access revoked). Evict the // cache entry and fall through to the legacy unscoped endpoint. clearCachedIssueOrg(id); - return await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + const issue = await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + return { issue, cacheEvicted: true }; } throw orgErr; } } - return await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + const issue = await getIssue(id, { collapse: ISSUE_DETAIL_COLLAPSE }); + return { issue, cacheEvicted: false }; } /** @@ -612,16 +635,27 @@ async function resolveNumericIssue( // in env vars and config defaults that may point at a different org. const cachedOrg = resolvedOrg ? null : getCachedIssueOrg(id); try { - const issue = await fetchIssueByNumericId(id, resolvedOrg?.org, cachedOrg); + const { issue, cacheEvicted } = await fetchIssueByNumericId( + id, + resolvedOrg?.org, + cachedOrg + ); + // When `cacheEvicted` is true, the cached org slug was stale (404'd) and + // the helper fell through to the unscoped endpoint. Do NOT let the stale + // `cachedOrg` participate in the `??` chain — re-derive from permalink. + const effectiveCachedOrg = cacheEvicted ? null : cachedOrg; // Extract org from the response permalink as a fallback so that callers // like resolveOrgAndIssueId (used by explain/plan) get the org slug even // when no org context was available before the fetch. const org = - resolvedOrg?.org ?? cachedOrg ?? extractOrgFromPermalink(issue.permalink); + resolvedOrg?.org ?? + effectiveCachedOrg ?? + extractOrgFromPermalink(issue.permalink); // Best-effort: remember the numeric-id → org mapping so the next run - // skips the unscoped fallback. Skipped when the org came from a cache - // hit (already stored) or when extraction failed. - if (org && !resolvedOrg && !cachedOrg) { + // skips the unscoped fallback. Skipped when the org came from a still- + // valid cache hit (already stored). When the cache was evicted we SHOULD + // re-write the corrected mapping derived from the permalink. + if (org && !resolvedOrg && !effectiveCachedOrg) { setCachedIssueOrg(id, org); } return { org, issue }; diff --git a/src/lib/db/auth.ts b/src/lib/db/auth.ts index e6609f604..cb0813cd8 100644 --- a/src/lib/db/auth.ts +++ b/src/lib/db/auth.ts @@ -6,7 +6,8 @@ import { getEnv } from "../env.js"; import { clearResponseCache } from "../response-cache.js"; import { withDbSpan } from "../telemetry.js"; import { getDatabase } from "./index.js"; -import { clearMetadataByPrefix, runUpsert } from "./utils.js"; +import { clearAllIssueOrgCache } from "./issue-org-cache.js"; +import { runUpsert } from "./utils.js"; /** Refresh when less than 10% of token lifetime remains */ export const REFRESH_THRESHOLD = 0.1; @@ -232,7 +233,7 @@ export async function clearAuth(): Promise { db.query("DELETE FROM user_info WHERE id = 1").run(); db.query("DELETE FROM org_regions").run(); db.query("DELETE FROM pagination_cursors").run(); - clearMetadataByPrefix(db, "issue_org."); + clearAllIssueOrgCache(); }); // Clear cached API responses — they are tied to the current user's permissions. diff --git a/src/lib/db/issue-org-cache.ts b/src/lib/db/issue-org-cache.ts index ecb72bc48..276669a64 100644 --- a/src/lib/db/issue-org-cache.ts +++ b/src/lib/db/issue-org-cache.ts @@ -13,11 +13,11 @@ * the `sentry.issue.view` "Consecutive HTTP" pattern for Pattern D in * the issue triage (numeric-ID org discovery fan-out). * - * Storage: the existing `metadata` KV table with key shape - * `issue_org.{numericId}` and value = org slug. No schema migration - * required. Entries are best-effort: a stale mapping (issue deleted, - * access revoked, or moved) causes a single 404 on the cached org - * call which the caller falls back from, and we evict the entry. + * Storage: dedicated `issue_org_cache` SQLite table (schema v15). Entries + * are best-effort — a stale mapping (issue deleted, access revoked, or + * moved) causes a single 404 on the cached org call which the caller + * falls back from and evicts the entry. Cleared on logout since + * mappings are scoped to the authenticated user's permissions. * * Values are not TTL'd because issues are owned by a single org for * their entire lifetime — the mapping cannot change except by issue @@ -26,19 +26,13 @@ import { recordCacheHit } from "../telemetry.js"; import { getDatabase } from "./index.js"; -import { - clearMetadata, - clearMetadataByPrefix, - getMetadata, - setMetadata, -} from "./utils.js"; +import { runUpsert } from "./utils.js"; -/** Metadata key namespace for issue-id → org mappings. */ -const KEY_PREFIX = "issue_org."; - -function metadataKey(numericId: string): string { - return `${KEY_PREFIX}${numericId}`; -} +type IssueOrgRow = { + issue_id: string; + org_slug: string; + cached_at: number; +}; /** * Look up the cached organization slug for a numeric issue ID. @@ -47,11 +41,17 @@ function metadataKey(numericId: string): string { * @returns Org slug if cached, undefined otherwise */ export function getCachedIssueOrg(numericId: string): string | undefined { + if (!numericId) { + recordCacheHit("issue_org", false); + return; + } const db = getDatabase(); - const map = getMetadata(db, [metadataKey(numericId)]); - const org = map.get(metadataKey(numericId)); - recordCacheHit("issue_org", !!org); - return org; + const row = db + .query("SELECT org_slug FROM issue_org_cache WHERE issue_id = ?") + .get(numericId) as Pick | undefined; + + recordCacheHit("issue_org", !!row); + return row?.org_slug; } /** @@ -69,7 +69,16 @@ export function setCachedIssueOrg(numericId: string, orgSlug: string): void { return; } const db = getDatabase(); - setMetadata(db, { [metadataKey(numericId)]: orgSlug }); + runUpsert( + db, + "issue_org_cache", + { + issue_id: numericId, + org_slug: orgSlug, + cached_at: Date.now(), + }, + ["issue_id"] + ); } /** @@ -85,17 +94,16 @@ export function clearCachedIssueOrg(numericId: string): void { return; } const db = getDatabase(); - clearMetadata(db, [metadataKey(numericId)]); + db.query("DELETE FROM issue_org_cache WHERE issue_id = ?").run(numericId); } /** * Drop ALL issue-id → org mappings. * * Called from auth logout handlers so signing out with one account does - * not leak mappings into a different account's session. Kept separate - * from `clearMetadata` so the caller can target just this cache class. + * not leak mappings into a different account's session. */ export function clearAllIssueOrgCache(): void { const db = getDatabase(); - clearMetadataByPrefix(db, KEY_PREFIX); + db.query("DELETE FROM issue_org_cache").run(); } diff --git a/src/lib/db/schema.ts b/src/lib/db/schema.ts index df4ca95bd..353fb120b 100644 --- a/src/lib/db/schema.ts +++ b/src/lib/db/schema.ts @@ -16,7 +16,7 @@ import { getEnv } from "../env.js"; import { stringifyUnknown } from "../errors.js"; import { logger } from "../logger.js"; -export const CURRENT_SCHEMA_VERSION = 14; +export const CURRENT_SCHEMA_VERSION = 15; /** Environment variable to disable auto-repair */ const NO_AUTO_REPAIR_ENV = "SENTRY_CLI_NO_AUTO_REPAIR"; @@ -208,6 +208,33 @@ export const TABLE_SCHEMAS: Record = { }, }, }, + /** + * Mapping from numeric issue group IDs to organization slugs. + * + * Populated after `sentry issue view ` falls back to the legacy + * unscoped `/api/0/issues/{id}/` endpoint and extracts the org from the + * response permalink. Subsequent runs consult this cache to route directly + * via the org-scoped endpoint, avoiding the redundant unscoped lookup + * flagged as a "Consecutive HTTP" performance issue in Sentry. + * + * Entries are best-effort: a stale mapping (issue moved / deleted / access + * revoked) causes a 404 on the cached org call, which the caller evicts and + * falls back from. Cleared on logout (scoped to the current user's + * permissions). + */ + issue_org_cache: { + columns: { + // Numeric issue group ID (e.g., "7413562541"). TEXT to sidestep the + // 2^53 JavaScript number limit if Sentry's group IDs ever exceed it. + issue_id: { type: "TEXT", primaryKey: true }, + org_slug: { type: "TEXT", notNull: true }, + cached_at: { + type: "INTEGER", + notNull: true, + default: "(unixepoch() * 1000)", + }, + }, + }, project_root_cache: { columns: { cwd: { type: "TEXT", primaryKey: true }, @@ -805,6 +832,13 @@ export function runMigrations(db: Database): void { db.exec(EXPECTED_TABLES.repo_cache as string); } + // Migration 14 -> 15: Add issue_org_cache table for numeric-id → org + // mappings used by `issue view ` to skip the legacy unscoped + // `/api/0/issues/{id}/` endpoint on repeat runs. + if (currentVersion < 15) { + db.exec(EXPECTED_TABLES.issue_org_cache as string); + } + if (currentVersion < CURRENT_SCHEMA_VERSION) { db.query("UPDATE schema_version SET version = ?").run( CURRENT_SCHEMA_VERSION diff --git a/src/lib/db/utils.ts b/src/lib/db/utils.ts index 5d57d391c..8d12eb3ca 100644 --- a/src/lib/db/utils.ts +++ b/src/lib/db/utils.ts @@ -198,27 +198,6 @@ export function clearMetadata(db: QueryRunner, keys: string[]): void { db.query(`DELETE FROM metadata WHERE key IN (${placeholders})`).run(...keys); } -/** - * Delete all keys from the `metadata` table that start with a given prefix. - * - * Intended for cache-style entries grouped under a common namespace - * (e.g., `issue_org.`). The prefix is matched literally — SQLite - * `LIKE` wildcards (`%` and `_`) in the prefix are NOT escaped, so callers - * must not pass user-controlled values. - * - * @param db - Database instance - * @param prefix - Namespace prefix to match (e.g., "issue_org.") - * - * @example - * clearMetadataByPrefix(db, "issue_org."); - */ -export function clearMetadataByPrefix(db: QueryRunner, prefix: string): void { - if (prefix.length === 0) { - return; - } - db.query("DELETE FROM metadata WHERE key LIKE ?").run(`${prefix}%`); -} - // --------------------------------------------------------------------------- // Cache entry helpers // --------------------------------------------------------------------------- diff --git a/test/commands/issue/utils.test.ts b/test/commands/issue/utils.test.ts index 406197376..6398b7b3f 100644 --- a/test/commands/issue/utils.test.ts +++ b/test/commands/issue/utils.test.ts @@ -947,6 +947,146 @@ describe("resolveOrgAndIssueId", () => { }); }); +describe("resolveOrgAndIssueId: issue-id → org cache (Pattern D)", () => { + test("uses cached org on warm cache (single org-scoped call)", async () => { + const { setCachedIssueOrg, getCachedIssueOrg } = await import( + "../../../src/lib/db/issue-org-cache.js" + ); + setCachedIssueOrg("77777777", "cached-org"); + + // Track which endpoint is hit — must be org-scoped, not legacy unscoped. + const calls: string[] = []; + // @ts-expect-error - partial mock + globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + const req = new Request(input, init); + calls.push(req.url); + if (req.url.includes("/organizations/cached-org/issues/77777777/")) { + return new Response( + JSON.stringify({ + id: "77777777", + shortId: "CACHED-1", + permalink: + "https://sentry.io/organizations/cached-org/issues/77777777/", + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify({ detail: "Not found" }), { + status: 404, + }); + }; + + const result = await resolveOrgAndIssueId({ + issueArg: "77777777", + cwd: getConfigDir(), + command: "view", + }); + + expect(result.org).toBe("cached-org"); + // Should NOT have hit the legacy unscoped /api/0/issues/{id}/ endpoint. + expect(calls.some((u) => /\/api\/0\/issues\/77777777\//.test(u))).toBe( + false + ); + // The org-scoped endpoint is what got called. + expect( + calls.some((u) => + u.includes("/organizations/cached-org/issues/77777777/") + ) + ).toBe(true); + // Cache stays populated (it was a valid hit). + expect(getCachedIssueOrg("77777777")).toBe("cached-org"); + }); + + test("evicts stale cache entry on 404 and uses permalink org (does not leak stale slug)", async () => { + const { setCachedIssueOrg, getCachedIssueOrg } = await import( + "../../../src/lib/db/issue-org-cache.js" + ); + // Seed a stale mapping that will 404 on the org-scoped endpoint. + setCachedIssueOrg("88888888", "stale-org"); + setOrgRegion("stale-org", DEFAULT_SENTRY_URL); + setOrgRegion("correct-org", DEFAULT_SENTRY_URL); + + // @ts-expect-error - partial mock + globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + const req = new Request(input, init); + const url = req.url; + // Org-scoped call with the stale org → 404. + if (url.includes("/organizations/stale-org/issues/88888888/")) { + return new Response(JSON.stringify({ detail: "Not found" }), { + status: 404, + }); + } + // Legacy unscoped fallback returns the correct org via permalink. + if (/\/api\/0\/issues\/88888888\/(\?|$)/.test(url)) { + return new Response( + JSON.stringify({ + id: "88888888", + shortId: "CORRECT-1", + permalink: + "https://sentry.io/organizations/correct-org/issues/88888888/", + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify({ detail: "Not found" }), { + status: 404, + }); + }; + + const result = await resolveOrgAndIssueId({ + issueArg: "88888888", + cwd: getConfigDir(), + command: "view", + }); + + // Critical assertion: the returned org is the CORRECT one from the + // permalink, NOT the stale cached slug. + expect(result.org).toBe("correct-org"); + expect(result.org).not.toBe("stale-org"); + // The stale cache entry should have been evicted and replaced with + // the corrected mapping from the permalink. + expect(getCachedIssueOrg("88888888")).toBe("correct-org"); + }); + + test("writes numeric-id → org mapping after legacy unscoped fallback", async () => { + const { getCachedIssueOrg } = await import( + "../../../src/lib/db/issue-org-cache.js" + ); + setOrgRegion("fresh-org", DEFAULT_SENTRY_URL); + + // @ts-expect-error - partial mock + globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + const req = new Request(input, init); + // Match /api/0/issues/99999999/ regardless of query string (?collapse=...). + if (/\/api\/0\/issues\/99999999\/(\?|$)/.test(req.url)) { + return new Response( + JSON.stringify({ + id: "99999999", + shortId: "FRESH-1", + permalink: + "https://sentry.io/organizations/fresh-org/issues/99999999/", + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify({ detail: "Not found" }), { + status: 404, + }); + }; + + expect(getCachedIssueOrg("99999999")).toBeUndefined(); + const result = await resolveOrgAndIssueId({ + issueArg: "99999999", + cwd: getConfigDir(), + command: "view", + }); + + expect(result.org).toBe("fresh-org"); + // Mapping should now be cached for next time. + expect(getCachedIssueOrg("99999999")).toBe("fresh-org"); + }); +}); + describe("pollAutofixState", () => { test("returns immediately when state is COMPLETED", async () => { let fetchCount = 0; diff --git a/test/lib/db/issue-org-cache.test.ts b/test/lib/db/issue-org-cache.test.ts index 08405a0c1..c7fbeb1a5 100644 --- a/test/lib/db/issue-org-cache.test.ts +++ b/test/lib/db/issue-org-cache.test.ts @@ -95,8 +95,9 @@ describe("clearAllIssueOrgCache", () => { expect(getCachedIssueOrg("3")).toBeUndefined(); }); - test("does not touch unrelated metadata keys", async () => { - // Seed an unrelated metadata entry using the shared helpers. + test("does not touch unrelated tables (metadata, org_regions, etc.)", async () => { + // Seed an unrelated metadata entry + an unrelated table row so we can + // confirm clearAllIssueOrgCache only drops its own table. const { getDatabase } = await import("../../../src/lib/db/index.js"); const { getMetadata, setMetadata } = await import( "../../../src/lib/db/utils.js" From 1763796d866f241966f2c338d3eb48d54fad12ae Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 11:13:19 +0000 Subject: [PATCH 3/3] fix(issue-cache): address self-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review with a critical-eye subagent surfaced one major and several minor issues that were worth fixing before merge: - **M1 (major)**: Three new cache writes (`setCachedProject` in `fetchProjectId`, two `setCachedIssueOrg` call sites in `resolveNumericIssue` / `resolveShareIssue`) ran without try/catch, violating the AGENTS.md invariant that non-essential DB cache writes must not crash a command whose primary API call already succeeded. Each write is now guarded and logs a debug message on failure, matching the `setInstallInfo`/`setUserInfo` pattern in login.ts and upgrade.ts. - **m2 (minor)**: Added a test verifying that a 5xx from the cached-org endpoint propagates the error WITHOUT evicting the cache entry (only 404 means 'stale'). Covers the invariant that transient failures shouldn't cause us to forget correct mappings. - **n1**: Skip `cachedOrg` in the error-hint fallback for 404s. When both the cached-org call AND the legacy unscoped call 404, the cache entry has already been evicted as stale — suggesting that stale org in 'Try: sentry issue view /' would mislead the user. Use the generic `` placeholder instead. - **n3**: Drop the bogus `& { 'MAX(cached_at)'?: number }` type extension in `getCachedProjectBySlug` — the SQL `AS cached_at` alias means the result row uses `cached_at`, not `MAX(cached_at)`. - **n4**: Added `clearAuth` integration test that verifies issue-org mappings are dropped on logout (prevents cross-account leakage). - **n5**: Fixed duplicate/misplaced JSDoc on `fetchIssueByNumericId` — type alias and function declaration each get their own docblock. - **n6**: Consolidated two `describe('clearProjectCache', ...)` blocks into one by adding `getCachedProjectBySlug` assertions to the existing test; removed the duplicate suite. - **n2** (trivial): Updated a test description that still used the old metadata-prefix wording ('removes all issue_org.* entries' → 'removes all cached mappings'). --- AGENTS.md | 8 ++++- src/commands/issue/utils.ts | 43 ++++++++++++++++++------- src/lib/db/project-cache.ts | 4 +-- src/lib/resolve-target.ts | 23 ++++++++----- test/commands/issue/utils.test.ts | 50 +++++++++++++++++++++++++++++ test/lib/db/auth.test.ts | 20 ++++++++++++ test/lib/db/issue-org-cache.test.ts | 2 +- test/lib/db/project-cache.test.ts | 27 ++-------------- 8 files changed, 128 insertions(+), 49 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4b0e9c48e..9d9be7520 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1010,13 +1010,16 @@ mock.module("./some-module", () => ({ * **Issue list global limit with fair per-project distribution and representation guarantees**: \`issue list --limit\` is a global total across all detected projects. \`fetchWithBudget\` Phase 1 divides evenly, Phase 2 redistributes surplus via cursor resume. \`trimWithProjectGuarantee\` ensures ≥1 issue per project before filling remaining slots. JSON output wraps in \`{ data, hasMore }\` with optional \`errors\` array. Compound cursor (pipe-separated) enables \`-c last\` for multi-target pagination, keyed by sorted target fingerprint. + +* **Prefer dedicated SQLite tables + migrations over metadata KV for non-trivial caches**: BYK's rule: schema migrations are cheap, so don't shoehorn structured caches into the \`metadata\` KV table with dotted-prefix keys. Dedicated tables are "the right way" — clearer schema, proper indexes, simpler bulk-clear (\`DELETE FROM table\` vs \`clearMetadataByPrefix\`), no risk of prefix collisions. \`metadata\` KV is still fine for small scalar values (defaults.\*, install.\*) but new caches with an identifiable entity/row shape should get their own table. Example: \`issue\_org\_cache\` (schema v15) replaced \`metadata\` keys \`issue\_org.{numericId}\`. Migration pattern: bump \`CURRENT\_SCHEMA\_VERSION\`, add \`EXPECTED\_TABLES.foo\`, add \`if (currentVersion < N) db.exec(EXPECTED\_TABLES.foo)\` block. Removing the KV helper (\`clearMetadataByPrefix\`) is a bonus cleanup. + * **Sentry CLI config dir should stay at ~/.sentry/, not move to XDG**: Config dir stays at \`~/.sentry/\` (not XDG). Readonly DB errors on macOS come from \`sudo brew install\` creating root-owned files. Fixes: (1) \`bestEffort()\` makes setup steps non-fatal, (2) \`tryRepairReadonly()\` detects root-owned files and prints \`sudo chown\` instructions, (3) \`sentry cli fix\` handles ownership repair. Check ownership BEFORE permissions — root-owned files cause chmod to EPERM. ### Gotcha -* **Biome plugin rule forbids raw metadata queries — use db/utils.ts helpers**: Biome lint rules that trip developers: (1) Plugin forbids raw metadata KV queries — use \`getMetadata\`/\`setMetadata\`/\`clearMetadata\`/\`clearMetadataByPrefix\` from \`src/lib/db/utils.ts\`. Any \`db.query("... metadata ...")\` including \`DELETE ... WHERE key LIKE ?\` fails CI lint (test files exempt). Reuse \`metadata\` table for lightweight caches with dotted-prefix keys (\`issue\_org.{numericId}\`, \`defaults.\`, \`install.\`) instead of new tables+migrations. Add eviction to \`clearAuth()\` if user-scoped. (2) \`noExcessiveCognitiveComplexity\` max 15 — extract helpers from Stricli \`func()\` handlers; \`resolveOrgProjectTarget\` chronically needs \`// biome-ignore\`. (3) \`useBlockStatements\` — always use braces. (4) \`noNestedTernary\` — use if/else. (5) \`useAtIndex\` — use \`arr.at(-1)\`. (6) \`noStaticOnlyClass\` — use branded instances. +* **Biome plugin rule forbids raw metadata queries — use db/utils.ts helpers**: Biome lint rules that trip developers: (1) Plugin forbids raw \`metadata\` table queries — use \`getMetadata\`/\`setMetadata\`/\`clearMetadata\` from \`src/lib/db/utils.ts\` (test files exempt). But prefer dedicated tables over metadata KV for non-trivial caches (BYK's rule — schema migrations are cheap). (2) \`noExcessiveCognitiveComplexity\` max 15 — extract helpers from Stricli \`func()\` handlers; \`resolveOrgProjectTarget\` chronically needs \`// biome-ignore\`. (3) \`useBlockStatements\` — always use braces. (4) \`noNestedTernary\` — use if/else. (5) \`useAtIndex\` — use \`arr.at(-1)\`. (6) \`noStaticOnlyClass\` — use branded instances. (7) \`useSimplifiedLogicExpression\` — \`!a || !b\` should be \`!(a && b)\`. (8) \`noShadow\` flags sibling-block shadowing too, not just nested; rename or inline. * **Bun mock.module() leaks globally across test files in same process**: Bun's mock.module() replaces modules globally and leaks across test files in the same process. Solution: tests using mock.module() must run in a separate \`bun test\` invocation. In package.json, use \`bun run test:unit && bun run test:isolated\` instead of \`bun test\`. The \`test/isolated/\` directory exists for these tests. This was the root cause of ~100 test failures (getsentry/cli#258). @@ -1038,6 +1041,9 @@ mock.module("./some-module", () => ({ * **Bun global installs use .bun path segment for detection**: Bun global package installs place scripts under \`~/.bun/install/global/node\_modules/\`. The \`.bun\` directory in the path distinguishes bun from npm/yarn installs. In \`detectPackageManagerFromPath()\`, check \`segments.includes('.bun')\` before the npm fallback. Detection order: \`.pnpm\` → pnpm, \`.bun\` → bun, other \`node\_modules\` → npm. Yarn classic uses same flat layout as npm so falls through to npm detection — this is acceptable because path-based detection is a \*\*fallback\*\* after subprocess calls (which correctly identify yarn). Path detection must NOT be authoritative/override stored DB info, only serve as fallback when subprocess detection fails (e.g., Windows where .cmd files cause ENOENT). + +* **Evict-then-read pattern: return cacheEvicted flag from helpers that clear cache on 404**: When a helper function transparently evicts a stale cache entry on 404 and falls back to an unscoped call, callers holding the now-stale cached value will let it win \`??\` chains. Fix: helper must return \`{ result, cacheEvicted }\` so callers compute \`effectiveCachedValue = cacheEvicted ? null : cachedValue\` before the \`??\` fallback, and re-cache the freshly-derived value. Applied in \`fetchIssueByNumericId\` in \`src/commands/issue/utils.ts\` — both \`resolveNumericIssue\` and \`resolveShareIssue\` consume the flag. A local cached variable outliving its DB entry is the common shape of this bug; always audit post-eviction read paths. + * **Non-essential DB cache writes should be guarded with try-catch**: Non-essential DB cache writes (e.g., \`setUserInfo()\`, \`setInstallInfo()\`) must be wrapped in try-catch so a broken/read-only DB doesn't crash a command whose primary operation succeeded. Pattern: \`try { setInstallInfo(...) } catch { log.debug(...) }\`. In login.ts, \`getCurrentUser()\` failure after token save must not block auth — log warning, continue. In upgrade.ts, \`setInstallInfo\` after legacy detection is guarded same way. Exception: \`getUserRegions()\` failure should \`clearAuth()\` and fail hard (indicates invalid token). This is enforced by BugBot reviews — any \`setInstallInfo\`/\`setUserInfo\` call outside setup.ts's \`bestEffort()\` wrapper needs its own try-catch. diff --git a/src/commands/issue/utils.ts b/src/commands/issue/utils.ts index f6d76389a..50d3ee1a0 100644 --- a/src/commands/issue/utils.ts +++ b/src/commands/issue/utils.ts @@ -516,7 +516,14 @@ async function resolveShareIssue( effectiveCachedOrg ?? extractOrgFromPermalink(issue.permalink); if (resolvedOrgSlug && !resolvedOrg && !effectiveCachedOrg) { - setCachedIssueOrg(groupId, resolvedOrgSlug); + // Best-effort — a broken/read-only DB must not fail a successful lookup. + try { + setCachedIssueOrg(groupId, resolvedOrgSlug); + } catch (cacheErr) { + log.debug( + `Failed to cache issue-org mapping for ${groupId}: ${String(cacheErr)}` + ); + } } return { org: resolvedOrgSlug, issue }; } @@ -553,15 +560,6 @@ function extractOrgFromPermalink( return parseSentryUrl(permalink)?.org; } -/** - * Fetch an issue by numeric ID, preferring an org-scoped endpoint when - * the caller has explicit or cached org context. Falls back to the legacy - * unscoped `/api/0/issues/{id}/` endpoint when no org is known, and also - * when a cached org yields a 404 (stale mapping). - * - * Extracted from {@link resolveNumericIssue} to keep its cognitive - * complexity below the project's lint threshold. - */ /** * Result of {@link fetchIssueByNumericId}. * @@ -576,6 +574,15 @@ type FetchIssueByNumericIdResult = { cacheEvicted: boolean; }; +/** + * Fetch an issue by numeric ID, preferring an org-scoped endpoint when + * the caller has explicit or cached org context. Falls back to the legacy + * unscoped `/api/0/issues/{id}/` endpoint when no org is known, and also + * when a cached org yields a 404 (stale mapping). + * + * Extracted from {@link resolveNumericIssue} to keep its cognitive + * complexity below the project's lint threshold. + */ async function fetchIssueByNumericId( id: string, explicitOrg: string | undefined, @@ -656,7 +663,14 @@ async function resolveNumericIssue( // valid cache hit (already stored). When the cache was evicted we SHOULD // re-write the corrected mapping derived from the permalink. if (org && !resolvedOrg && !effectiveCachedOrg) { - setCachedIssueOrg(id, org); + // Best-effort — a broken/read-only DB must not fail a successful lookup. + try { + setCachedIssueOrg(id, org); + } catch (cacheErr) { + log.debug( + `Failed to cache issue-org mapping for ${id}: ${String(cacheErr)}` + ); + } } return { org, issue }; } catch (err) { @@ -665,7 +679,12 @@ async function resolveNumericIssue( // and suggesting the short-ID format, since users often confuse numeric // group IDs with short-ID suffixes. When org context is available, use // the real org slug instead of placeholder (CLI-BT, 18 users). - const orgHint = resolvedOrg?.org ?? cachedOrg ?? ""; + // + // Skip `cachedOrg` here: if the unscoped legacy endpoint 404'd too, + // the helper has already evicted the (proven-stale) cache entry, so + // suggesting the old slug in the hint would mislead the user. Only + // explicit `resolvedOrg` is worth preserving. + const orgHint = resolvedOrg?.org ?? ""; const hint = `${commandBase} ${command} ${orgHint}/${id}`; throw new ResolutionError(`Issue ${id}`, "not found", hint, [ `No issue with numeric ID ${id} found — you may not have access, or it may have been deleted.`, diff --git a/src/lib/db/project-cache.ts b/src/lib/db/project-cache.ts index 072da2dcc..2b636b6e9 100644 --- a/src/lib/db/project-cache.ts +++ b/src/lib/db/project-cache.ts @@ -134,9 +134,7 @@ export function getCachedProjectBySlug( .query( "SELECT cache_key, org_slug, org_name, project_slug, project_name, project_id, MAX(cached_at) AS cached_at, last_accessed FROM project_cache WHERE org_slug = ? AND project_slug = ?" ) - .get(orgSlug, projectSlug) as - | (ProjectCacheRow & { "MAX(cached_at)"?: number }) - | undefined; + .get(orgSlug, projectSlug) as ProjectCacheRow | undefined; // When no rows match, SQLite still returns a single row with NULL columns // because of the MAX aggregate — guard on cache_key being populated. diff --git a/src/lib/resolve-target.ts b/src/lib/resolve-target.ts index cdcc80755..10a35e0bf 100644 --- a/src/lib/resolve-target.ts +++ b/src/lib/resolve-target.ts @@ -873,16 +873,23 @@ export async function fetchProjectId( // Populate the cache for next time. The `getProject` response carries the // numeric project ID and (usually) the organization payload; use whatever - // is available to seed future lookups. + // is available to seed future lookups. Guarded with try/catch so a broken + // or read-only DB does NOT crash the primary API-success path. const project_ = projectResult.value; if (project_.organization) { - setCachedProject(project_.organization.id, project_.id, { - orgSlug: project_.organization.slug, - orgName: project_.organization.name, - projectSlug: project_.slug, - projectName: project_.name, - projectId: project_.id, - }); + try { + setCachedProject(project_.organization.id, project_.id, { + orgSlug: project_.organization.slug, + orgName: project_.organization.name, + projectSlug: project_.slug, + projectName: project_.name, + projectId: project_.id, + }); + } catch (cacheErr) { + log.debug( + `Failed to cache project '${org}/${project}': ${String(cacheErr)}` + ); + } } return toNumericId(project_.id); diff --git a/test/commands/issue/utils.test.ts b/test/commands/issue/utils.test.ts index 6398b7b3f..3c8ab1f7b 100644 --- a/test/commands/issue/utils.test.ts +++ b/test/commands/issue/utils.test.ts @@ -1085,6 +1085,56 @@ describe("resolveOrgAndIssueId: issue-id → org cache (Pattern D)", () => { // Mapping should now be cached for next time. expect(getCachedIssueOrg("99999999")).toBe("fresh-org"); }); + + test("5xx on cached-org fetch propagates the error WITHOUT evicting the cache", async () => { + const { setCachedIssueOrg, getCachedIssueOrg } = await import( + "../../../src/lib/db/issue-org-cache.js" + ); + // Seed a valid mapping — the org-scoped endpoint will return 500 (transient). + setCachedIssueOrg("66666666", "still-valid-org"); + setOrgRegion("still-valid-org", DEFAULT_SENTRY_URL); + + let legacyFallbackCalled = false; + // @ts-expect-error - partial mock + globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + const req = new Request(input, init); + const url = req.url; + if (url.includes("/organizations/still-valid-org/issues/66666666/")) { + return new Response(JSON.stringify({ detail: "Internal error" }), { + status: 500, + }); + } + if (/\/api\/0\/issues\/66666666\/(\?|$)/.test(url)) { + legacyFallbackCalled = true; + return new Response( + JSON.stringify({ + id: "66666666", + permalink: + "https://sentry.io/organizations/still-valid-org/issues/66666666/", + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify({ detail: "Not found" }), { + status: 404, + }); + }; + + // Error must propagate — the helper only falls back on 404, not on 5xx. + await expect( + resolveOrgAndIssueId({ + issueArg: "66666666", + cwd: getConfigDir(), + command: "view", + }) + ).rejects.toThrow(ApiError); + + // Cache must NOT have been evicted — the failure was transient, the + // mapping may still be correct. + expect(getCachedIssueOrg("66666666")).toBe("still-valid-org"); + // Legacy fallback must NOT have been reached. + expect(legacyFallbackCalled).toBe(false); + }); }); describe("pollAutofixState", () => { diff --git a/test/lib/db/auth.test.ts b/test/lib/db/auth.test.ts index 9ada6b7b1..8200765e3 100644 --- a/test/lib/db/auth.test.ts +++ b/test/lib/db/auth.test.ts @@ -197,3 +197,23 @@ describe("OAuth-preferred auth (#646)", () => { } }); }); + +describe("clearAuth: integration with per-account caches", () => { + test("clearAuth drops issue_org_cache entries (prevents cross-account leakage)", async () => { + const { clearAuth } = await import("../../../src/lib/db/auth.js"); + const { setCachedIssueOrg, getCachedIssueOrg } = await import( + "../../../src/lib/db/issue-org-cache.js" + ); + + // Seed a mapping as if a previous session resolved this issue. + await setAuthToken("test-token"); + setCachedIssueOrg("12345", "previous-account-org"); + expect(getCachedIssueOrg("12345")).toBe("previous-account-org"); + + await clearAuth(); + + // Mapping must be gone — otherwise the next account would leak into + // their `issue view` fallback routing. + expect(getCachedIssueOrg("12345")).toBeUndefined(); + }); +}); diff --git a/test/lib/db/issue-org-cache.test.ts b/test/lib/db/issue-org-cache.test.ts index c7fbeb1a5..24c3bf0f1 100644 --- a/test/lib/db/issue-org-cache.test.ts +++ b/test/lib/db/issue-org-cache.test.ts @@ -83,7 +83,7 @@ describe("clearCachedIssueOrg", () => { }); describe("clearAllIssueOrgCache", () => { - test("removes all issue_org.* entries", () => { + test("removes all cached mappings", () => { setCachedIssueOrg("1", "a"); setCachedIssueOrg("2", "b"); setCachedIssueOrg("3", "c"); diff --git a/test/lib/db/project-cache.test.ts b/test/lib/db/project-cache.test.ts index 6007652d8..ba7ce2635 100644 --- a/test/lib/db/project-cache.test.ts +++ b/test/lib/db/project-cache.test.ts @@ -279,10 +279,12 @@ describe("clearProjectCache", () => { // Clear all clearProjectCache(); - // All should be undefined + // All should be undefined across every lookup shape. expect(getCachedProject("org-1", "project-1")).toBeUndefined(); expect(getCachedProject("org-2", "project-2")).toBeUndefined(); expect(getCachedProjectByDsnKey("key1")).toBeUndefined(); + expect(getCachedProjectBySlug("org-one", "project-one")).toBeUndefined(); + expect(getCachedProjectBySlug("key-org", "key-project")).toBeUndefined(); }); test("does not throw when cache is already empty", async () => { @@ -574,26 +576,3 @@ describe("getCachedProjectBySlug", () => { expect(result?.projectId).toBeFalsy(); }); }); - -describe("clearProjectCache", () => { - test("clears all entries across all key shapes", () => { - setCachedProject("a", "b", { - orgSlug: "org", - orgName: "Org", - projectSlug: "proj", - projectName: "Proj", - }); - setCachedProjectByDsnKey("pk", { - orgSlug: "org", - orgName: "Org", - projectSlug: "proj", - projectName: "Proj", - }); - - clearProjectCache(); - - expect(getCachedProject("a", "b")).toBeUndefined(); - expect(getCachedProjectByDsnKey("pk")).toBeUndefined(); - expect(getCachedProjectBySlug("org", "proj")).toBeUndefined(); - }); -});