Fix Scorecard SonarQube "Open Issues" metric pulling - #4274
Conversation
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 3:59 PM UTC Commit: |
|
ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| } | ||
| catch { | ||
| throw new Error( | ||
| `SonarQube project '${projectKey}' is not accessible or the project key is missing or invalid`, |
There was a problem hiding this comment.
[medium] error handling
The bare catch block around the project-access check swallows all errors indiscriminately and replaces them with a misleading project is not accessible message. This catches not only HTTP 4xx responses (the intended case) but also: (1) configuration errors from resolveInstance (e.g., SonarQube instance unknown not found in configuration would be masked), (2) network failures, (3) server errors (HTTP 500/503). For case (1), this is a behavioral regression — before this change, passing an invalid instanceName would throw a clear configuration error; now it throws a misleading project not accessible error.
Suggested fix: Narrow the catch to only handle the fetchApi HTTP error case. Check if (error instanceof Error && error.message.includes(SonarQube API error)) and only rethrow the project-accessibility message for that case, re-throwing the original error otherwise.
| instanceName, | ||
| ); | ||
|
|
||
| return data.paging?.total ?? data.total ?? 0; |
There was a problem hiding this comment.
[medium] scope-creep
The claimed intent is to handle inaccessible projects, but the change also silently alters return-value parsing from data.total to data.paging?.total ?? data.total ?? 0. This is a separate behavioral change not mentioned in the PR title, body, or changeset description.
Suggested fix: Either revert the return expression to data.total if the pre-flight check alone solves the bug, or document why the parsing change was needed.
| ); | ||
|
|
||
| return data.paging?.total ?? data.total ?? 0; | ||
| } |
There was a problem hiding this comment.
[low] error handling / silent fallback
data.paging?.total ?? data.total ?? 0 silently returns 0 when neither paging.total nor total is present in the API response. If the SonarQube API response structure changes unexpectedly, this will produce a silent 0 rather than a visible failure.
| 'https://sonarcloud.io/api/issues/search?componentKeys=my-project&statuses=OPEN,CONFIRMED,REOPENED&ps=1', | ||
| expect.any(Object), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[low] test adequacy
The test throws when project access check fails only tests the HTTP 404 case. There is no test covering the scenario where an invalid instanceName is passed to getOpenIssuesCount — this would reveal the regression where the configuration error (instance not found) is now incorrectly masked as project not accessible.
| this.logger.debug(`Fetching open issues count for project ${projectKey}`); | ||
|
|
||
| // Additional check to ensure the project is accessible | ||
| try { |
There was a problem hiding this comment.
[low] indentation
The method body of getOpenIssuesCount is indented at 6 spaces, while every other method in this class uses 4 spaces. This inconsistency was introduced by the diff.
Suggested fix: Re-indent the entire getOpenIssuesCount method body to use 4-space indentation, matching the sibling methods.
| throw new Error( | ||
| `SonarQube project '${projectKey}' is not accessible or the project key is missing or invalid`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[low] brace placement
The catch keyword is placed on a new line after the closing brace. The standard JavaScript/TypeScript convention (and what prettier enforces) is same-line style (} catch {).
Suggested fix: Move catch onto the same line as the closing brace: } catch {.
| this.logger.debug(`Fetching open issues count for project ${projectKey}`); | ||
|
|
||
| // Additional check to ensure the project is accessible | ||
| try { |
There was a problem hiding this comment.
[low] intent-clarity
The pre-flight call to /api/components/show runs on every invocation, adding latency and load to the happy path. The changeset describes fixing an issue when the project is inaccessible but the implementation adds overhead to all calls.
|
🤖 Finished Retro · ✅ Success · Started 1:51 PM UTC · Completed 2:05 PM UTC Commit: |
Retro: PR #4274 — Fix Scorecard SonarQube "Open Issues" metric pullingTimeline
Review quality assessmentThe review agent performed well. Its 2 medium-severity findings were substantive and actionable:
The 5 low-severity findings (silent fallback to 0, missing test for non-HTTP errors, indentation inconsistency, brace placement, missing linked issue) were all valid. The challenger appropriately merged one pair and downgraded two findings. No false positives were observed. Cost: $4.29, 28 turns — reasonable for the finding quality. Evidence for existing issues
ConclusionThe workflow performed well overall. The review agent caught real issues with appropriate severity, and the CI pipeline correctly blocked the formatting violation. No new proposals are warranted — the improvement opportunities identified are already tracked in existing open issues. |
1 participant
Hey, I just made a Pull Request!
✔️ Checklist