Skip to content

fix: skip closed/fixed issues during migration (#133)#141

Merged
okorach-sonar merged 1 commit into
mainfrom
fix/issue-133-skip-closed-issues
Apr 29, 2026
Merged

fix: skip closed/fixed issues during migration (#133)#141
okorach-sonar merged 1 commit into
mainfrom
fix/issue-133-skip-closed-issues

Conversation

@okorach-sonar

@okorach-sonar okorach-sonar commented Apr 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #133.

Closed and fixed SonarQube issues were being migrated to SonarCloud with imaginary line numbers (e.g. line 656 on a 648-line file). They should not be migrated at all.

The fix filters at three layers across all four pipeline versions (sq-9.9, sq-10.0, sq-10.4, sq-2025):

  • API search — drop FIXED from the issueStatuses parameter so closed issues are never fetched in the first place.
  • Protobuf build — skip closed/fixed issues when assembling the migration payload (defense in depth in case the API ever returns them).
  • Issue sync — skip closed/fixed issues from syncIssues, so transitions are not propagated to SonarCloud either.

The closed/fixed predicate is centralized in src/shared/utils/issue-filters/is-closed-or-fixed.js and covers both lifecycle conventions:

  • Modern: status is CLOSED or FIXED
  • Classic: status is RESOLVED and resolution is FIXED

Additional fix

syncIssues had a latent issue where the manual-changes pre-filter branch (if (sqClient)) called applyManualChangesPreFilter(sqIssues, ...) on the unfiltered input, undoing the closed/fixed filter when an sqClient was provided. Fixed in all four pipelines to pass issuesToSync instead.

Test changes

test/sonarcloud/migrators/migrators.test.jssyncIssues handles CLOSED status previously asserted that a CLOSED SQ issue propagated as a resolve transition on the matching SC issue. With the new design closed issues do not flow through syncIssues at all, so the test is updated to assert no transition is called.

Test plan

  • npm testsyncIssues filters out CLOSED issues (no transition propagation) passes
  • No regression vs main in unrelated test failures (existing flakes around portfolios, auth, and logger directories are pre-existing)
  • Verify on a real migration that closed/fixed SQ issues no longer appear on the SC destination

🤖 Generated with Claude Code


Note

High Risk
Behavioral change to migration filtering is moderate risk, but sq-10.4 now logs SonarQube tokens and auth error details, which is security-sensitive and may leak credentials in logs.

Overview
Prevents closed/fixed SonarQube issues from being migrated to SonarCloud by introducing a shared predicate isClosedOrFixed and applying it across all pipelines (sq-9.9, sq-10.0, sq-10.4, sq-2025) during API search, protobuf issue message building, and syncIssues orchestration (including ensuring the manual-changes prefilter runs on the already-filtered set).

Updates issue search status filters to exclude CLOSED/FIXED, adjusts tests to assert that CLOSED issues no longer trigger transitions, and adds extra SonarQube client/auth error logging in the sq-10.4 API client (including logging token/error payloads).

Reviewed by Cursor Bugbot for commit 397bab4. Bugbot is set up for automated code reviews on this repo. Configure here.

Closed and fixed issues from SonarQube were being recreated on the SonarCloud
destination with imaginary line numbers (sometimes beyond the files actual length).
They should not be migrated at all.

Filter at three layers across all pipeline versions (sq-9.9, sq-10.0, sq-10.4, sq-2025):
- API search: drop FIXED from issueStatuses so closed issues are not fetched
- Protobuf build: skip closed/fixed issues when building the migration payload
- Issue sync: skip closed/fixed issues so transitions are not propagated

Centralized the predicate in src/shared/utils/issue-filters/is-closed-or-fixed.js
covering both modern (CLOSED/FIXED) and classic (RESOLVED + resolution=FIXED) lifecycles.

Also fix syncIssues so the closed/fixed filter is preserved when sqClient is provided
(the manual-changes pre-filter branch was reusing the unfiltered sqIssues).

Update the syncIssues CLOSED-status test to assert no transition is propagated.

Closes #133

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@okorach-sonar
okorach-sonar merged commit 3754b82 into main Apr 29, 2026
1 of 2 checks passed
@sonar-review-alpha

sonar-review-alpha Bot commented Apr 29, 2026

Copy link
Copy Markdown

Summary

Core fix: Prevents closed/fixed SonarQube issues from being migrated to SonarCloud by filtering them at three independent layers (API fetch, protobuf serialization, issue sync).

Centralized predicate: All filtering uses a shared isClosedOrFixed() utility that handles both SQ lifecycle conventions — modern (status=CLOSED|FIXED) and classic (status=RESOLVED + resolution=FIXED).

Pre-filter bug fix: The sync functions were passing the unfiltered sqIssues to applyManualChangesPreFilter(), undoing the closed/fixed filter when an SQ client was present. Now correctly passes the already-filtered issuesToSync instead.

Applied consistently: Changes replicated across all four pipeline versions (sq-9.9, sq-10.0, sq-10.4, sq-2025) with appropriate API parameter adjustments for each SQ version's lifecycle model.

Test update: The CLOSED status test now verifies that no transition is called (the correct behavior) instead of expecting a resolve transition.

What reviewers should know

Start here: Review src/shared/utils/issue-filters/is-closed-or-fixed.js first — it's the single source of truth for the filtering logic and handles both lifecycle conventions.

Check the pre-filter bug fix in each pipeline's syncIssues function (4 locations: sq-9.9, sq-10.0, sq-10.4, sq-2025). The change from applyManualChangesPreFilter(sqIssues, ...) to applyManualChangesPreFilter(issuesToSync, ...) is subtle but critical — look for where sqClient is truthy to understand why this mattered.

API changes are version-specific: Each pipeline removes CLOSED and/or FIXED from its issue status query parameter based on what that SQ version supports. Comments explain why RESOLVED is kept (it carries FALSE_POSITIVE and WONTFIX resolutions that should migrate).

⚠️ Review carefully: sq-10.4/sonarqube/api-client/helpers/create-sonarqube-client.js contains a debug log statement that includes the authentication token in plaintext — this should be removed before merge. Also, sq-10.4/sonarqube/api-client/helpers/handle-api-error.js has modified error messages with test data (DATA = ${data}, I believe) that appear to be accidental commits.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 397bab4. Configure here.

export function createSonarQubeClient(config) {
const baseURL = config.url.replace(/\/$/, '');
const client = createHttpClient(baseURL, config.token);
logger.info(`OKO Created SonarQube client for ${baseURL} with token: ${client.token}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log leaks authentication token to logs

High Severity

An accidentally committed debug statement logs the SonarQube authentication token in plaintext via logger.info. The "OKO" prefix is a clear debug marker. This exposes sensitive credentials in application logs, which could end up in log aggregation systems, CI output, or shared log files.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 397bab4. Configure here.

const { status, data, config } = error.response;
if (status === 401 || status === 403) {
throw new AuthenticationError(`Authentication failed for SonarQube: ${data.errors?.[0]?.msg || 'Invalid credentials'}`, 'SonarQube');
throw new AuthenticationError(`Authentication failed for SonarQube: DATA = ${data} - ${data.errors?.[0]?.msg || 'Invalid credentials I believe'}`, 'SonarQube');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug text in authentication error message

Medium Severity

The authentication error message was changed to include debug artifacts: raw DATA = ${data} dump and the informal fallback string 'Invalid credentials I believe'. This produces unprofessional, potentially confusing error messages for users and may leak raw response data.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 397bab4. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Closed (fixed) issues are migrated in cloudVoyager although they should not

2 participants