fix: three bug fixes — sourcemap JSON.parse crash, log per_page cap, dashboard pagination overshoot#1118
Draft
cursor[bot] wants to merge 3 commits into
Draft
fix: three bug fixes — sourcemap JSON.parse crash, log per_page cap, dashboard pagination overshoot#1118cursor[bot] wants to merge 3 commits into
cursor[bot] wants to merge 3 commits into
Conversation
…map files The JSON.parse call at line 155 of debug-id.ts was unguarded — if a .map file contained malformed JSON (truncated, corrupt, or not a sourcemap), the entire sourcemap inject command crashed with an opaque SyntaxError. Every other JSON.parse in the codebase is wrapped in try/catch. This adds the same pattern, throwing a descriptive ValidationError that includes the file path so users know which .map file is problematic. Co-authored-by: Miguel Betegón <miguelbetegongarcia@gmail.com>
Both listLogs() and listTraceLogs() passed the user's --limit value directly as per_page to the Sentry API. When limit > 100, the API silently caps at 100 items, but the caller's hasMore check (logs.length >= flags.limit) evaluates to false, hiding remaining data. This violates the project convention documented in AGENTS.md: 'Never pass a per_page value larger than API_MAX_PER_PAGE to the API.' Cap both calls with Math.min(limit, API_MAX_PER_PAGE) to match the pattern used in autoPaginate() and other list commands. Co-authored-by: Miguel Betegón <miguelbetegongarcia@gmail.com>
…list When multi-page fetching accumulates more results than flags.limit, the excess items are trimmed with .slice(0, flags.limit) but the API cursor from the last fetched page is still stored. On '-c next', those trimmed items are skipped entirely. This matches the autoPaginate() pattern in infrastructure.ts (lines 419-420) which explicitly drops nextCursor when results overshoot to prevent skipping items. Co-authored-by: Miguel Betegón <miguelbetegongarcia@gmail.com>
Contributor
|
Contributor
Codecov Results 📊❌ Patch coverage is 71.43%. Project has 5049 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.24% 81.24% —%
==========================================
Files 388 388 —
Lines 26910 26914 +4
Branches 17481 17483 +2
==========================================
+ Hits 21861 21865 +4
- Misses 5049 5049 —
- Partials 1822 1822 —Generated by Codecov Action |
Comment on lines
+202
to
208
| const overshot = results.length > flags.limit; | ||
| const hasMore = overshot || !!nextCursor; | ||
| // When multi-page fetch overshoots the limit, the API cursor points past | ||
| // trimmed items — storing it would skip them on '-c next'. Drop it to | ||
| // match autoPaginate() behavior (see infrastructure.ts lines 419-420). | ||
| const cursorToStore = !overshot && hasMore ? nextCursor : undefined; | ||
|
|
There was a problem hiding this comment.
hasMore = true when overshot but no cursor stored causes -c next to throw a ValidationError
When the multi-page fetch overshoots flags.limit, hasMore is set to true and the paginationHint prints a -c next hint, but cursorToStore is undefined, so advancePaginationState stores no next entry in the stack. Running --cursor next then hits state.index + 1 >= state.stack.length in resolveCursor and throws "No next page saved for this query" — the opposite of what hasMore: true implies.
Evidence
overshot = true→cursorToStore = undefined(line 208);hasMore = true(line 203).advancePaginationState(..., direction='first', undefined)storesstack = ['']with no next entry (pagination.ts:265:nextCursor ? ['', nextCursor] : ['']).outputData.hasMore = truecausespaginationHintto emit the-c nexthint (revisions.ts line 229).resolveCursor(..., 'next')inpagination.ts:198checksstate.index + 1 >= state.stack.length→1 >= 1→ throwsValidationError('No next page saved for this query.').autoPaginate()(infrastructure.ts line 419) never exposeshasMoreto callers, so the cited comparison does not apply here wherehasMoreis surfaced directly in CLI output.
Identified by Warden find-bugs · LHQ-E68
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
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
Three independent bug fixes discovered via code analysis of the codebase. Each fix is a separate commit.
1.
fix(sourcemap): Guard JSON.parse in injectDebugId against malformed .map filesRoot cause:
JSON.parse(mapContent)insrc/lib/sourcemap/debug-id.ts:155was the only unguardedJSON.parsein the codebase. Malformed.mapfiles crash thesourcemap injectcommand with an opaqueSyntaxError.Reproduction: Run
sentry sourcemap injecton a directory containing a.jsfile whose companion.maphas invalid JSON.Fix: Wrapped in try/catch that throws a
ValidationErrorwith the file path, matching the pattern used ininline-sourcemap.tsand throughout the rest of the codebase.2.
fix(logs): Cap per_page at API_MAX_PER_PAGE in log list APIsRoot cause: Both
listLogs()andlistTraceLogs()insrc/lib/api/logs.tspassed the user's--limitdirectly asper_page. Whenlimit > 100, the API silently caps at 100, but the caller'shasMorecheck (logs.length >= flags.limit) evaluates tofalse, hiding remaining data.Reproduction: Run
sentry log list --limit 200on a project with >100 logs. Only ~100 are returned and the hint says there are no more.Fix: Cap both calls with
Math.min(limit, API_MAX_PER_PAGE), matching the project convention documented in AGENTS.md: "Never pass a per_page value larger than API_MAX_PER_PAGE to the API."3.
fix(dashboard): Drop nextCursor on pagination overshoot in revisions listRoot cause: In
src/commands/dashboard/revisions.ts, when multi-page fetching accumulates more results thanflags.limit, the excess is trimmed but the API cursor from the last fetched page is still stored. On-c next, those trimmed items are silently skipped.Reproduction: Run
sentry dashboard revisions <id> --limit Nwhere N causes multi-page overshoot (e.g.,--limit 150with 100 items per page, where 2 pages return 160 items total).Fix: Drop
nextCursorwhen results overshoot the limit, matching theautoPaginate()pattern ininfrastructure.ts(lines 419-420).