fix: consolidate Cursor BugBot PRs (#908, #947, #973, #1023, #1044)#1051
Merged
Conversation
Add a shared safeParseJson() helper and use it in getPaginationState() and getCachedDetection() so corrupt cached JSON (partial writes, manual edits, schema drift) is treated as a cache miss instead of crashing the command with an unhandled SyntaxError. Pagination state additionally validates Array.isArray and clears the bad row. Replace three silent catch blocks in sentry-client.ts (token refresh, response cache write, post-mutation invalidation) with log.debug() so errors are visible in debug output per the AGENTS.md no-silent-catch rule. Consolidates the cache-hardening fixes from #908, #947, and #1044.
listIssueEvents hand-rolled its pagination loop and, when a page overshot the requested limit, trimmed the data but kept nextCursor — which pointed past the trimmed tail and made -c next navigation skip events. Refactor onto the shared autoPaginate() helper and, because the events endpoint has no per-page param and its single page can still overshoot, trim-and-drop-nextCursor in the wrapper. Add a property test pinning autoPaginate's multi-page trim-drops-cursor invariant and a focused regression test for the events overshoot case. Consolidates #947's event-pagination fix.
Clock skew, scheduled/future timestamps, or bad API data could make diffMs negative and render as '-5m ago'. Clamp to >= 0 so future dates display as '0m ago'. Consolidates #1044's relative-time fix.
…xtraction - normalizeAgentStatus(): map 'canceled'/'cancelled' to CANCELLED and 'need_more_information' to NEED_MORE_INFORMATION. The US spelling 'canceled' previously became 'CANCELED' via toUpperCase() and never matched the British 'CANCELLED' terminal status, so polling spun until timeout. - searchContainersForRootCauses(): require causes.length > 0 so an empty legacy causes:[] array no longer short-circuits the fallthrough to agent artifacts. - extractNoSolutionReason(): also read the step-level description (current API shape) before the artifact-level reason (legacy), mirroring extractSolution. Consolidates the seer fixes from #973 and #1023. Relates to #958.
set-commits default mode caught any ApiError with status 400 from setCommitsAuto and treated it as 'no repository integration', falling back to local git and poisoning the cache. But setCommitsAuto internally calls setCommitsWithRefs, which can return 400 for unrelated reasons (invalid refs, bad release state) — those were masked. Export NO_REPO_INTEGRATIONS_MESSAGE from releases.ts and narrow the catch to the exact client-side message. Unrelated 400s now propagate. Matched on message (not detail) because this error is constructed client-side with detail undefined. Consolidates #1023's set-commits fix.
…ules - Extend script/check-error-patterns.ts with a silent-catch scan (empty, comment-only, or return-only catch blocks that don't surface the error). It is advisory by default given a pre-existing backlog; set SENTRY_STRICT_SILENT_CATCH=1 to enforce. References to the caught error or a log/throw exclude a block. - Document automated-fix-PR rules in AGENTS.md: check existing open PRs/issues first, rebase before review, separate correctness from opinion, prefer shared helpers (autoPaginate, safeParseJson). These address the duplication/staleness that produced five overlapping BugBot PRs. - Remove a now-unused biome-ignore suppression in formatters/local.ts that broke repo-wide lint under biome 2.3.8 (stale-base drift).
This was referenced Jun 3, 2026
Contributor
|
Contributor
Codecov Results 📊✅ Patch coverage is 89.80%. Project has 4337 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.97% 82.07% +0.1%
==========================================
Files 334 335 +1
Lines 24157 24193 +36
Branches 15807 15839 +32
==========================================
+ Hits 19802 19856 +54
- Misses 4355 4337 -18
- Partials 1659 1658 -1Generated by Codecov Action |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Address Warden find-bugs: getCachedDetection parsed allDsns and the mtimes records via safeParseJson without a validator, so a structurally-corrupt row (e.g. an object where an array is expected) passed the truthy !allDsns check and flowed downstream typed as DetectedDsn[], risking a runtime TypeError. Add isArray / isMtimesRecord type guards so any shape mismatch is treated as a cache miss, matching getPaginationState's Array.isArray guard.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 817389d. Configure here.
Address Cursor BugBot (high severity): the previous fix dropped nextCursor on overshoot, which stranded every event past the first page — 'sentry issue events' could only ever show the first --limit events. Cap page size with per_page = min(limit, API_MAX_PER_PAGE) (Sentry accepts per_page on this route at runtime though it's absent from the OpenAPI spec) so the server cursor is page-aligned and never overshoots. Trim defensively to limit but PRESERVE nextCursor: the events cursor is offset-based, so resuming re-includes any trimmed tail instead of skipping it, and -c next can advance. This resolves both the original skip bug (#947) and the stall regression. Update tests to assert per_page is sent (capped at 100) and the cursor is preserved on overshoot.
…, tighten validators
M2: listIssueEvents now auto-paginates across multiple API calls for limit > 100
again (per AGENTS.md requirement), while still sending per_page to cap page
size and preserving nextCursor on overshoot.
M1: isMtimesRecord now validates all values are numbers, not just that the value
is a non-array object.
M3: AGENTS.md references changed from pnpm run to bun run for consistency with
the rest of the file.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
228
to
238
| const config = await getOrgSdkConfig(orgSlug); | ||
| const perPage = Math.min(limit, API_MAX_PER_PAGE); | ||
|
|
||
| const allEvents: IssueEvent[] = []; | ||
| let currentCursor = cursor; | ||
| let nextCursor: string | undefined; | ||
|
|
||
| for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { | ||
| for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { | ||
| const result = await listAnIssue_sEvents({ | ||
| ...config, | ||
| path: { |
Contributor
There was a problem hiding this comment.
Bug: When limit > 100, listIssueEvents returns a nextCursor that points past all fetched items, not the trimmed limit, causing users to permanently skip events on the next page request.
Severity: HIGH
Suggested Fix
When the number of fetched items exceeds the requested limit, trim the results to the limit and drop the nextCursor. This prevents clients from skipping items. The implementation should align with the logic in the autoPaginate helper, which handles this "overshoot" case by returning next: undefined.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/lib/api/events.ts#L226-L238
Potential issue: The `listIssueEvents` function incorrectly handles pagination when a
`limit` greater than the page size (100) is requested. It accumulates events from
multiple pages, trims the result to the specified `limit`, but returns the `nextCursor`
from the last full page fetched. For example, with `limit=150`, it fetches 200 items but
returns a cursor pointing to item 201. When a client uses this cursor for the next page,
items 151-200 are permanently skipped. This contradicts the behavior of the general
`autoPaginate` helper, which correctly drops the cursor in such overshoot scenarios.
Did we get this right? 👍 / 👎 to inform future reviews.
| * re-throw. These hide errors and violate the AGENTS.md no-silent-catch rule. | ||
| */ | ||
| function checkSilentCatch(content: string, filePath: string): void { | ||
| let match = CATCH_RE.exec(content); |
There was a problem hiding this comment.
CATCH_RE global regex lastIndex not reset between files, causing missed matches
Because CATCH_RE is a module-level /g regex and lastIndex is never reset inside checkSilentCatch, every call after the first will start scanning from where the previous file left off, silently skipping catch blocks in all subsequent files.
Evidence
CATCH_REis declared at module scope with/gflag (line 277–278), makinglastIndexstateful across calls.checkSilentCatchis called once per file inside thefor (const filePath of files)loop (line 356).- The function never resets
CATCH_RE.lastIndex = 0before beginning itsexecloop. - After the first file exhausts the regex,
lastIndexstays at or near the end of that file's content length; the next file's content is a fresh string, butexecstarts atlastIndexwhich is almost certainly past the end, returningnullimmediately. - The same pattern is safe in
checkAdHocTryPatterns(line 253 area) only because that function usesString.prototype.matchAllor a freshly constructed regex — not a shared global one.
Identified by Warden find-bugs · 3PA-UHS
BYK
added a commit
that referenced
this pull request
Jun 3, 2026
…e entries (#1056) ## Summary Fixes the failing `main` build caused by a ~10% flaky unit test (`response-cache.test.ts` → `--fresh round-trip: stale entry is replaced by fresh response`) that exposed a **real production bug** in the HTTP response cache. ## Root cause In `src/lib/response-cache.ts`, each cache write fires `cleanupCache()` **fire-and-forget** at 10% probability (`CLEANUP_PROBABILITY = 0.1`). A second write to the **same key** overwrote the file with a **non-atomic** `writeFile`. The first write's async cleanup could then read the file **mid-overwrite** (torn read), fail to `JSON.parse` the truncated content, and **delete it** as "corrupted" in `collectEntryMetadata` — silently losing a valid cache entry. **Proof:** A 300-iteration repro failed ~10% of the time (matching `CLEANUP_PROBABILITY`), and on every miss the cache directory was **empty** — the valid entry was deleted, not expired. This is a genuine correctness bug beyond tests: any two rapid writes to the same cache key (paginated fetches, concurrent CLI invocations sharing the cache dir) can race so cleanup's torn read deletes a valid entry. ## Why PR checks didn't catch it The flake is ~10%, so any single CI "Unit Tests" run passes ~90% of the time. PR #1051's Unit Tests check landed in the lucky 90%; the post-merge `main` run hit the unlucky 10%. The bug is **pre-existing** — the cleanup code predates #1051, which never touched `response-cache.ts`. ## Fix 1. **Atomic writes** — new `atomicWriteCacheFile()` writes to a unique temp file (`<key>.<pid>.<uuid>.tmp`) then `rename`s it into place. `rename` is atomic on POSIX (same filesystem) and near-atomic on Windows (same volume), so a concurrent reader sees a complete old or new file, never a torn one. 2. **Cleanup hardening** — `collectEntryMetadata()` now separates failure modes: - **transient read failure** (locking, AV scanner, ENOENT from a concurrent sweep) → **skip**, never delete. - **fully read but unparseable** → genuine corruption (atomic writes preclude torn reads), so the file is marked expired and reclaimed by `deleteExpiredEntries`. This keeps corrupt files visible to `MAX_CACHE_ENTRIES` eviction instead of leaking past the count. 3. **Temp-file sweep** — `cleanupCache()` removes orphaned `.tmp` files older than 60s, so a crash between `writeFile` and `rename` can't leak temp files. 4. **Diagnostics** — previously-silent catch blocks now `log.debug()` the suppressed error (per AGENTS.md no-silent-catch rule). ## Verification - Amplified repro: **300/300 pass** (was ~10% failing before). - Actual test file: **10/10 runs green** (36 tests, +2 new regression tests). - `pnpm run lint`: clean. `pnpm exec tsc --noEmit`: exit 0. - Full unit suite: 325 files, all passing (one unrelated pre-existing property flake noted below). ## Out of scope (follow-up) During full-suite runs I observed an unrelated, pre-existing flaky **property** test: `auto-paginate.property.test.ts:94` (counterexample `[393,392,98]` — an `autoPaginate` `nextCursor` trim bug when `total > limit`). It passes in isolation and is independent of this change. Tracked as a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates the five open Cursor BugBot PRs (#908, #947, #973, #1023, #1044)
into a single, de-duplicated, rebased PR. Each genuinely-needed fix is verified
against current
main; overlapping fixes use the best variant; stale andsubjective changes are dropped.
Why consolidate
The five BugBot PRs heavily overlapped and had gone stale (7–113 commits behind
main): the cacheJSON.parseguard appeared 3×, thewithTTYtest fix 2×, thepagination guard 3×. Several also failed CI only on unrelated base drift. This PR
lands the real fixes once and adds guardrails so the duplication/staleness
doesn't recur.
Fixes included
Cache hardening (
fix(db)) — from #908/#947/#1044safeParseJson()db helper; used ingetPaginationState()(withArray.isArrayvalidation) andgetCachedDetection()so corrupt cached JSONis a cache miss, not a crash.
log.debug()added to three silent catch blocks insentry-client.ts.Event pagination (
fix(events)) — from #947listIssueEventsnow uses the sharedautoPaginate()helper and dropsnextCursorwhen a page overshootslimit, so-c nextnever skips events.Adds a property test for the
autoPaginatetrim-drops-cursor invariant plus afocused overshoot regression test.
Relative time (
fix(formatters)) — from #1044formatRelativeTimeclampsdiffMsto>= 0("0m ago" instead of "-5m ago").Seer (
fix(seer)) — from #973/#1023, relates to #958normalizeAgentStatusmapscanceled/cancelled→CANCELLED(fixes apolling-timeout bug) and
need_more_information→NEED_MORE_INFORMATION.searchContainersForRootCausesrequirescauses.length > 0so an emptylegacy array no longer blocks the agent-artifact fallthrough.
extractNoSolutionReasonreads the step-leveldescription(current API)before the artifact-level
reason(legacy).Release set-commits (
fix(release)) — from #1023NO_REPO_INTEGRATIONS_MESSAGEso unrelated 400s (invalid refs, bad release state) propagate instead of being
masked as "no integration".
Test isolation (
test) — from #1044/#947withTTYnow saves/restoresNO_COLORandFORCE_COLOR(CI setsNO_COLOR=1).Systemic guardrails
script/check-error-patterns.tsgains an advisory silent-catch scan(
SENTRY_STRICT_SILENT_CATCH=1to enforce).AGENTS.mddocuments automated-fix-PR rules: check existing PRs/issues first,rebase before review, separate correctness from opinion, prefer shared helpers.
biome-ignoreinformatters/local.tsthat brokerepo-wide lint under biome 2.3.8.
Dropped (intentionally)
issue list"lifetime" collapse removal (fix: resolve 3 bugs — missing issue stats, seer polling timeout, no-solution reason #973) — superseded onmainbythe
shouldCollapseLifetimeparam; issuesentry issue list --json --fieldsomitscount,firstSeen,lastSeen,userCount(missingexpand=stats) #969 is already closed.is:resolved→ bare list (fix: 3 bug fixes — seer root cause extraction, release set-commits masking, issue selector hint #1023) — subjective UX;main's current behavior is deliberate.Follow-up
Testing
pnpm run typecheck✓pnpm run lint✓ (766 files)pnpm run check:errors✓ ·pnpm run check:deps✓pnpm run test:unit✓ (325 files, 7419 passed, 13 skipped)Closes #908, #947, #973, #1023, #1044.