Skip to content

chore: 🤖 resolve SonarCloud quality gate failures - #374

Open
prashantasdeveloper wants to merge 3 commits into
alphafrom
fix/sonar-quality-gate
Open

prashantasdeveloper wants to merge 3 commits into
alphafrom
fix/sonar-quality-gate

Conversation

@prashantasdeveloper

Copy link
Copy Markdown
Contributor

Resolves the SonarCloud Quality Gate failure reported on master (check run 105569982946).

Failing conditions, all addressed: C Security Rating on New Code, 40 Code Smells, 3 New Code Smells, 66.7% Security Hotspots Reviewed, C Security Review Rating — 46 issues + 1 unreviewed hotspot, pulled from the SonarCloud API.

Security — S6505, 6 issues

CI installs ran yarn --immutable, which executes dependency lifecycle scripts.

The obvious fix (--mode=skip-build) would have broken the build, so it was verified against a scratch Yarn 4 project first: --mode=skip-build also suppresses the root postinstall — the script that dedupes @polkadot for the SDK. Each install is therefore now two steps:

- run: yarn --immutable --mode=skip-build   # dependency scripts blocked
- run: yarn run postinstall                 # our own trusted script still runs

Verified end-to-end from a clean node_modules: install → postinstall → lint → build → test all pass. (Note: with the current lockfile those nested @polkadot copies are no longer created at all, so postinstall is currently a defensive no-op.)

The same treatment is applied to scripts/integration-test.sh, where the yarn call installs the cloned polymesh-dev-env test harness — that package has no postinstall, so no follow-up step is needed there.

Security hotspot

sequoia-pgp/fast-forward@v1 is pinned to ea7628b. The v1 branch, the v1.0.0 tag and that SHA are all the same commit, so behavior is unchanged.

Code smells

  • Mark never-reassigned members readonly (currentId is left mutable — it is a counter)
  • node: protocol imports, Number.isNaN / Number.parseInt, Date.now(), Array.prototype.flatMap over lodash flatten
  • Avoid an object literal as a default parameter in ApiArrayResponse
  • String.raw for the regex literals in jest.config.js
  • Sort the Dockerfile apk packages; [[ ]] in the shell scripts; explicit return in the integration-test cleanup trap
  • Extract cloneRepo / buildAndPushImage out of the Jenkinsfile node block to keep control-flow nesting within 3 levels

One real bug

subscriptions.service.spec.ts called expect(...) with no matcher, so the batchBumpNonce test asserted nothing and passed unconditionally. Completing the assertion made it fail — findAll() is not stubbed in that block and returned a bare mock. Rewritten to assert the actual delegation to incrementNonces, matching the adjacent batchMarkAsDone test.

Reviewer note — 9 smells suppressed, not code-fixed

The *.repo.spec.ts files Sonar reports as having no tests (S2187) do run real tests — they delegate to shared suites (NotificationRepo.test(repo)) whose it blocks live in *.repo.suite.ts and which Sonar's static analysis cannot see through. Deleting them would delete real coverage, so sonar-project.properties ignores S2187 for those paths.

This project uses SonarCloud Automatic Analysis (there is no scanner step in CI), so please confirm the gate actually honours sonar.issue.ignore.multicriteria. If those 9 still report, the fallback is marking them "Won't Fix" in the SonarCloud UI.

Verification

yarn lint, yarn build clean. 1087/1087 tests pass across 105 suites.

Addresses every open issue the quality gate reported on master.

Security (New Code rating C -> A):
- disable dependency lifecycle scripts on CI installs with
  `yarn --immutable --mode=skip-build`, running the repo's own trusted
  postinstall as an explicit step so the @PolkaDot dedupe still happens
- pin sequoia-pgp/fast-forward to a full commit SHA (v1.0.0), clearing
  the outstanding security hotspot

Code smells:
- mark never-reassigned members readonly
- prefer node: protocol imports, Number.isNaN / Number.parseInt,
  Date.now() and Array.prototype.flatMap over lodash flatten
- avoid an object literal as a default parameter in ApiArrayResponse
- use String.raw for the regex literals in jest.config.js
- sort the Dockerfile apk packages, use `[[ ]]` in the shell scripts and
  give the integration-test cleanup trap an explicit return
- extract cloneRepo/buildAndPushImage out of the Jenkinsfile node block
  to keep control flow nesting within 3 levels

Tests:
- complete the vacuous assertion in the batchBumpNonce spec, which
  called expect() with no matcher and so asserted nothing

S2187 is ignored for the datastore repo specs: they delegate to shared
suites (`XRepo.test(repo)`) whose `it` blocks Sonar's static analysis
cannot see, making "add some tests to this file" a false positive.
The Jenkins pipeline is no longer used, so drop it along with the
CODEOWNERS entry that pointed at it.

This also removes the three S134 control flow nesting smells the
previous commit refactored the file to satisfy.
`./github` is syntactically valid but never matches, so changes under
`.github/` were silently falling through to the catch-all owners rather
than requiring SRE review. Use a root-anchored `/.github/` instead.

Also replace the individually named catch-all owners with the Middleware
team. Three of the four no longer resolve, which left a single valid
reviewer on every path.
@sonarqubecloud

Copy link
Copy Markdown

This branch has not been deployed

No deployments
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.

1 participant