You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review Round 3 — 15 human review comments addressed:
#1: login.ts — Replace await .catch() with proper try/await/catch blocks
#2: whoami.ts + all commands — Export FRESH_ALIASES constant from list-command.ts
to reduce boilerplate; update 15 command files to use it
#3: response-cache.ts — Bump immutable TTL from 1hr to 24hr (events/traces
never change once created)
#4–6: response-cache.ts — Restructure URL_TIER_PATTERNS as Record<TtlTier, RegExp[]>,
combine duplicate regex patterns into single alternations
#7: response-cache.ts — Replace localeCompare with simple < comparison for
ASCII URL query param sorting
#8: response-cache.ts — Remove try-catch in normalizeUrl (URLs reaching the
cache already came from a fetch, always valid)
#9: response-cache.ts — Link immutableMinTimeToLive to FALLBACK_TTL_MS.immutable
instead of hardcoded magic number
#10: response-cache.ts — Use Object.fromEntries(headers.entries()) instead of
manual forEach loop
#11: response-cache.ts — Remove unnecessary await on fire-and-forget unlink in
catch block
#12: response-cache.ts — Add expiresAt field to CacheEntry for O(1) expiry
checks during cleanup (no CachePolicy deserialization needed)
#13–15: response-cache.ts — Parallelize cache I/O (collectEntryMetadata,
deleteExpiredEntries, evictExcessEntries) using p-limit-style concurrency
limiter (max 8 concurrent)
***SentryAPI: eventsrequireorg+project, issueshavelegacyglobalendpoint**: SentryAPIscoping: Eventsrequireorg+projectinURLpath (\`/projects/{org}/{project}/events/{id}/\`). Issues use legacy global endpoint (\`/api/0/issues/{id}/\`) without org context. Traces need only org (\`/organizations/{org}/trace/{traceId}/\`). Two-step lookup for events: fetch issue → extract org/project from response → fetch event. Cross-project event search possible via Discover endpoint \`/organizations/{org}/events/\` with \`query=id:{eventId}\`.
* **Sentry CLI has two distribution channels with different runtimes**: Sentry CLI ships two ways: (1) Standalone binary via \`Bun.build()\` with \`compile: true\`. (2) npm package via esbuild producing CJS \`dist/bin.cjs\` for Node 22+, with Bun API polyfills from \`script/node-polyfills.ts\`. \`Bun.$\` has NO polyfill — use \`execSync\` instead. \`require()\` in ESM is safe (Bun native, esbuild resolves at bundle time).
* **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).
* **Making clearAuth() async breaks model-based tests — use non-async Promise\<void> return instead**: Making \`clearAuth()\`\`async\` breaks fast-check model-based tests. Any real async yield (macrotask like \`Bun.sleep(1)\` or \`rm()\`) during \`asyncModelRun\` causes \`createIsolatedDbContext\` cleanup to interleave, switching the DB. Microtasks (\`Promise.resolve()\`) are fine. Fix: keep \`clearAuth()\` non-async, return \`clearResponseCache().catch(...)\` directly — DB ops execute synchronously, callers can optionally \`await\` the returned promise. Model-based tests should NOT await it. Related: model-based tests are inherently slow (5-20s with shrinking) and need explicit timeouts (e.g., \`30\_000\`) to avoid false failures from Bun's default 5s timeout. Don't chase 'data corruption' bugs that are actually timeout-interrupted shrinking runs.
* **Multiregion mock must include all control silo API routes**: When changing which Sentry API endpoint a function uses (e.g., switching getCurrentUser() from /users/me/ to /auth/), the mock route must be updated in BOTH test/mocks/routes.ts (single-region) AND test/mocks/multiregion.ts createControlSiloRoutes() (multi-region). Missing the multiregion mock causes 404s in multi-region test scenarios. The multiregion control silo mock serves auth, user info, and region discovery routes. Cursor Bugbot caught this gap when /api/0/auth/ was added to routes.ts but not multiregion.ts.
* **pagination\_cursors table schema mismatch requires repair migration**: The pagination\_cursors SQLite table may have wrong PK (single-column instead of composite). Migration 5→6 detects via \`hasCompositePrimaryKey()\` and drops/recreates. \`repairWrongPrimaryKeys()\` in \`repairSchema()\` handles auto-repair. \`isSchemaError()\` catches 'on conflict clause does not match' to trigger repair. Data loss acceptable since cursors are ephemeral (5-min TTL).
* **Returning bare promises loses async function from error stack traces**: When an \`async\` function returns another promise without \`await\`, the calling function disappears from error stack traces if the inner promise rejects. A function that drops \`async\` and does \`return someAsyncCall()\` loses its frame entirely. Fix: keep the function \`async\` and use \`return await someAsyncCall()\`. This matters for debugging — the intermediate function name in the stack trace helps locate which code path triggered the failure. ESLint rule \`no-return-await\` is outdated; modern engines optimize \`return await\` in async functions.
* **Sentry /users/me/ endpoint returns 403 for OAuth tokens — use /auth/ instead**: The Sentry \`/users/me/\` endpoint returns 403 for OAuth tokens. Use \`/auth/\` instead — it works with ALL token types and lives on the control silo. In the CLI, \`getControlSiloUrl()\` handles routing correctly. \`SentryUserSchema\` (with \`.passthrough()\`) handles the \`/auth/\` response since it only requires \`id\`.
* **PR workflow: address review comments, resolve threads, wait for CI**: User's PR workflow after creation: (1) Wait for CI checks to pass, (2) Check for unresolved review comments via \`gh api\` for PR review comments, (3) Fix issues in follow-up commits (not amends), (4) Reply to the comment thread explaining the fix, (5) Resolve the thread programmatically via \`gh api graphql\` with \`resolveReviewThread\` mutation, (6) Push and wait for CI again, (7) Final sweep for any remaining unresolved comments. Use \`git notes add\` to attach implementation plans to commits. Branch naming: \`fix/descriptive-slug\` or \`feat/descriptive-slug\`.
* **Progress message format: 'N and counting (up to M)...' pattern**: User prefers progress messages that frame the limit as a ceiling rather than an expected total. Format: \`Fetching issues, 30 and counting (up to 50)...\` — not \`Fetching issues... 30/50\`. The 'up to' framing makes it clear the denominator is a max, not an expected count, avoiding confusion when fewer items exist than the limit. For multi-target fetches, include target count: \`Fetching issues from 10 projects, 30 and counting (up to 50)...\`. Initial message before any results: \`Fetching issues (up to 50)...\` or \`Fetching issues from 10 projects (up to 50)...\`.
0 commit comments