fix: resolve silent error swallowing and unsafe type coercion#1082
Merged
Conversation
Three categories of fixes, superseding #1079: 1. fix(release): return [] instead of undefined when resolveProjectIds() fails. The old behavior returned undefined, which the caller treated as 'no filter' — returning ALL org releases mislabeled with the requested project name. Now returns an empty array so the API returns no results rather than wrong results. 2. fix(trace): replace Number(projectData.id) with toNumericId() in trace/view.ts. NaN passes through ?? unchanged (NaN ?? -1 === NaN), so invalid IDs could silently disable project filtering. Also tighten the equivalent validation in releases.ts (Number.isInteger instead of Number.isFinite, avoiding circular import from resolve-target.ts). 3. chore: add log.debug() to 6 silent catch blocks in issue/view.ts, issue/utils.ts, log/view.ts, release/list.ts, and span-tree.ts. Uses module-level logger.withTag() per codebase convention.
Contributor
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Contributor
Codecov Results 📊❌ Patch coverage is 23.81%. Project has 4847 uncovered lines. Files with missing lines (5)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 81.52% 81.49% -0.03%
==========================================
Files 374 374 —
Lines 26174 26190 +16
Branches 17080 17082 +2
==========================================
+ Hits 21337 21343 +6
- Misses 4837 4847 +10
- Partials 1769 1769 —Generated by Codecov Action |
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple cases where errors were silently swallowed or invalid numeric coercions could lead to incorrect/unscoped API queries, improving correctness and debuggability across release, trace, issue, log, and span-tree flows.
Changes:
- Adjusts release list project ID resolution and adds debug logging to avoid misleading org-wide release output when project resolution fails.
- Replaces unsafe
Number()coercion with validated numeric parsing for trace project filtering, and tightens project ID validation in release API helpers. - Adds
log.debug()to previously silent catch blocks in several commands/libs to improve diagnostics without changing user-facing output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/commands/release/list.ts | Adds module-level logger, logs resolution failures, and changes project ID resolution behavior used for project-scoped release listing. |
| src/lib/api/releases.ts | Tightens numeric project ID validation before passing project query param. |
| src/commands/trace/view.ts | Uses toNumericId() for safer project ID handling when filtering traces by project. |
| src/commands/issue/view.ts | Adds debug logging for non-blocking enrichment failures (latest event, replay IDs). |
| src/commands/issue/utils.ts | Logs DSN resolution failures before falling back to full search. |
| src/commands/log/view.ts | Logs debug info when log item detail fetch fails inside non-blocking enrichment. |
| src/lib/span-tree.ts | Logs debug info when trace/span-tree fetching fails, keeping fallback UX intact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+360
to
364
| log.debug(`Failed to resolve project ID for ${org}/${project}`, error); | ||
| return []; | ||
| } | ||
| } | ||
|
|
Member
Author
There was a problem hiding this comment.
Fixed in 151b556. fetchReleasesForTarget() now checks if (ids.length === 0) return [] before calling listReleasesPaginated(), so we never rely on the API's behavior with empty arrays.
…queries Address review findings: 1. Success path: toNumericId() returning undefined also leaked org-wide releases (Warden finding). Now returns [] with a debug log when the project ID is non-numeric. 2. Caller: fetchReleasesForTarget() now short-circuits and returns [] when resolveProjectIds() yields an empty array, rather than passing project: [] to the API (whose behavior with empty arrays is unverified). This eliminates any ambiguity about API semantics.
listReleasesForProject() had the same undefined-fallback bug as resolveProjectIds(): when the project ID was non-numeric, project: undefined was passed to the API, silently returning all org releases instead of project-scoped ones. Unlike resolveProjectIds (best-effort), this function is a direct API call where the caller explicitly requested a specific project. Throwing ValidationError is the correct behavior — if getProject succeeded but the ID is malformed, that's a data integrity issue that should surface, not silently widen the query.
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
Supersedes #1079 (Cursor BugBot). That PR had one real bug fix and five logging-only changes that didn't fix the bugs they described. This PR addresses all issues properly.
Fix 1:
fix(release)— Prevent misleading fallback in release listFiles:
src/commands/release/list.tsRoot Cause:
resolveProjectIds()returnedundefinedon error. The caller passedproject: undefinedto the API, which returned all releases in the entire org. Each release was then stamped withtargetProject: t.project— the user's requested project name. Users saw org-wide releases mislabeled as belonging to their project.Fix: Return
[](empty array) on both error and invalid-ID paths. The callerfetchReleasesForTarget()short-circuits on emptyidsand returns[]— no results rather than wrong results. Return type tightened fromPromise<number[] | undefined>toPromise<number[]>to enforce this at the type level.Fix 2:
fix(trace)— UsetoNumericId()for project ID validationFiles:
src/commands/trace/view.ts,src/lib/api/releases.tsRoot Cause:
Number(projectData.id)returnsNaNfor undefined/non-numeric values. Critically,NaN ?? -1evaluates toNaN(not-1), becauseNaNis not nullish. This could silently passNaNto the trace API asprojectId, disabling project filtering unpredictably.Fix:
trace/view.ts: ReplacedNumber()withtoNumericId()which returnsundefinedfor invalid inputs, correctly triggering the?? -1fallback.releases.ts:listReleasesForProject()now throwsValidationErrorwhen the project ID is non-numeric, rather than silently widening to an org-wide query. This is the correct behavior for a direct API function where the caller explicitly requested a specific project. (Cannot importtoNumericIdfromresolve-target.tsdue to circular dependency throughapi-client.js.)Fix 3:
chore— Addlog.debug()to silent catch blocksFiles:
src/commands/issue/view.ts,src/commands/issue/utils.ts,src/commands/log/view.ts,src/lib/span-tree.tsAdded
log.debug()to 6 silent catch blocks per AGENTS.md catch-block rules. Uses module-levellogger.withTag()per the dominant codebase convention (~35 instances), not function-scoped duplicates.Differences from #1079
release/list.tsdata corruptionlog.debug()— bug still produces wrong output[]on error + short-circuits caller — no results beats wrong resultsreleases.tsNumber()ValidationErrorfor non-numeric project IDs instead of silently widening querylogger.withTag()inside each function)const logper codebase conventiontoNumericIdintoreleases.ts