From dfee25086b1880a5138e326b7e45f3b1a33e45f2 Mon Sep 17 00:00:00 2001 From: Ned Wolpert Date: Thu, 11 Jun 2026 18:08:38 -0700 Subject: [PATCH] build(sonar): tune quality gate for false positives and fix genuine findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first SonarCloud analysis flagged the full backlog against new-code thresholds. Overall health already passes (~88% coverage, ~2% duplication); the noise came from rules that conflict with the project's conventions plus a couple of real findings. Triaged all 407 issues and addressed the legitimate ones without deleting correct defensive code. Config (version-controlled, rationale in docs/adr/0017): - Suppress java:S4449 project-wide (demands JSR-305 javax.annotation.Nullable; we standardize on JSpecify, enforced by Error Prone). - Suppress java:S2699 on **/*Test.java (persistence/in-memory suites delegate assertions to shared testkit *Scenarios classes; gated by JaCoCo + mutation). - Exclude DynamoDB *Item beans and module-info.java from coverage (structural SDK-driven POJOs / no executable statements). Genuine fixes: - Annotate Micronaut admin controller @Body params @Nullable, matching the Spring adapter, so the existing null-guards are no longer flagged as dead code (java:S2583/S2589) — the framework can inject a null body. - CeremonyScenarios: Optional.orElseThrow() instead of get() (java:S3655). Co-Authored-By: Claude Opus 4.8 (1M context) --- build.gradle.kts | 29 +++++++++++++++ .../0017-sonarqube-cloud-static-analysis.md | 36 +++++++++++++++++-- .../micronaut/PkAuthAdminController.java | 13 ++++--- .../pkauth/testkit/CeremonyScenarios.java | 5 +-- 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 62a6d1d..87ef208 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -17,6 +17,35 @@ sonar { property("sonar.organization", "codeheadsystems") property("sonar.host.url", "https://sonarcloud.io") property("sonar.exclusions", "examples/**,clients/passkeys-browser/dist/**") + + // Coverage exclusions: structural code with no meaningful logic to unit-test. The DynamoDB + // Enhanced Client item beans are mandatory mutable POJOs (no-arg ctor + getters/setters) + // driven entirely by the SDK's bean mapper, and module-info has no executable statements. + // Excluding them keeps the coverage signal honest instead of padding it with trivial tests. + property( + "sonar.coverage.exclusions", + "**/persistence/dynamodb/*Item.java,**/module-info.java", + ) + + // Documented false-positive suppressions (see docs/adr/0017). These are kept in the build + // (version-controlled, reviewed) rather than the SonarCloud UI so the rationale travels with + // the code. Real defects in these rules' domains are still caught: null discipline by Error + // Prone + JSpecify, and test effectiveness by the per-module JaCoCo floors plus mutation + // testing — these two rules are redundant with gates we already enforce. + property("sonar.issue.ignore.multicriteria", "jspecifyNullable,delegatedAssertions") + // S4449 demands JSR-305 `javax.annotation.Nullable`; the project standardizes on JSpecify + // (CONTRIBUTING.md §7), enforced at compile time by Error Prone. Two annotation systems for + // the same contract — we follow JSpecify, so this rule is noise here. + property("sonar.issue.ignore.multicriteria.jspecifyNullable.ruleKey", "java:S4449") + property("sonar.issue.ignore.multicriteria.jspecifyNullable.resourceKey", "**/*.java") + // S2699 ("tests should include assertions") cannot see across a call: the persistence and + // in-memory tests deliberately delegate their assertions to shared testkit `*Scenarios` + // classes so one suite runs against every backend. The assertions exist, just not inline. + property("sonar.issue.ignore.multicriteria.delegatedAssertions.ruleKey", "java:S2699") + property( + "sonar.issue.ignore.multicriteria.delegatedAssertions.resourceKey", + "**/*Test.java", + ) } } diff --git a/docs/adr/0017-sonarqube-cloud-static-analysis.md b/docs/adr/0017-sonarqube-cloud-static-analysis.md index cb3284f..6b72583 100644 --- a/docs/adr/0017-sonarqube-cloud-static-analysis.md +++ b/docs/adr/0017-sonarqube-cloud-static-analysis.md @@ -40,7 +40,39 @@ Adopt **SonarQube Cloud** via the `org.sonarqube` Gradle plugin, driven from CI. - **Negative — fork PRs get no analysis.** GitHub withholds secrets from fork PRs, so the `sonar` job is skipped there. External contributors' branches are covered once merged to `main`; the standard trade-off for an open-source repo. - **Negative — heavier CI job.** The `sonar` job runs the full `build` (including the Docker-backed Testcontainers integration tests) so coverage exists when the scanner reads it. Acceptable on `ubuntu-latest`; can be optimized later by sharing coverage artifacts with the existing `build` job. +## New-code quality gate and false-positive policy + +The default **Sonar way** gate scores the *new-code period* (Clean as You Code). On the very first +analysis that period is the whole history, so the entire backlog is judged against strict new-code +thresholds (80% coverage, A ratings, <3% duplication) — which is why the initial run reported red +even though the project's *overall* numbers are healthy (≈88% coverage, ≈2% duplication). This is an +onboarding artifact, not a regression. + +Two things keep the gate meaningful instead of noisy: + +- **New Code definition.** Set the project's New Code to **"Previous version"** in SonarCloud + (Administration → New Code). Because all modules share one `gradle.properties` version, each + release bounds a clean new-code window, and the gate then measures genuinely new changes rather + than the onboarding backlog. *This is a server-side setting and cannot live in the repo.* +- **Documented false-positive suppressions live in `build.gradle.kts`**, not the SonarCloud UI, so + the rationale is version-controlled and reviewed: + - `java:S4449` (demands JSR-305 `javax.annotation.Nullable`) is suppressed project-wide — we + standardize on **JSpecify**, enforced by Error Prone (CONTRIBUTING.md §7). + - `java:S2699` ("tests should include assertions") is suppressed on `**/*Test.java` — the + persistence/in-memory suites delegate assertions to shared testkit `*Scenarios` classes so one + suite runs against every backend; the rule cannot see across the call. Test effectiveness is + already gated by the per-module JaCoCo floors and mutation testing. + - DynamoDB `*Item.java` beans and `module-info.java` are excluded from **coverage** — mandatory + structural code (SDK-driven POJOs / no executable statements) with nothing meaningful to test. + +Reliability findings flagged on intentional defensive null-guards (e.g. `S2583`/`S2589` where a +framework can still inject null despite a `@NullMarked` package) are fixed *in code* by annotating +the parameter `@Nullable` to match reality — see the Micronaut admin controller's `@Body` params — +never by deleting the guard. + ## Open follow-ups -- If we want the quality gate to block merges, wire the Sonar status check into branch protection. -- If CI time becomes a concern, have the `sonar` job download JaCoCo artifacts from the `build` job instead of rebuilding. +- Once the New Code definition is set and the gate is green, optionally wire the Sonar status check + into branch protection to block merges. +- If CI time becomes a concern, have the `sonar` job download JaCoCo artifacts from the `build` job + instead of rebuilding. diff --git a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthAdminController.java b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthAdminController.java index b43ff70..252a3db 100644 --- a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthAdminController.java +++ b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthAdminController.java @@ -30,6 +30,7 @@ import io.micronaut.http.annotation.Produces; import io.micronaut.scheduling.TaskExecutors; import io.micronaut.scheduling.annotation.ExecuteOn; +import org.jspecify.annotations.Nullable; /** * Admin controller mounting the brief §6.9 endpoints under {@code /auth/admin/**}. All @@ -76,7 +77,9 @@ public HttpResponse listCredentials(HttpRequest request) { /** Renames the credential identified by its base64url-encoded id. */ @Patch("/credentials/{credentialId}") public HttpResponse renameCredential( - HttpRequest request, @PathVariable String credentialId, @Body RenameCredential body) { + HttpRequest request, + @PathVariable String credentialId, + @Body @Nullable RenameCredential body) { UserHandle actor = PkAuthJwtAuthenticationFilter.attachedUserHandle(request); if (actor == null) return HttpResponse.status(HttpStatus.UNAUTHORIZED); CredentialId id = CredentialId.fromB64Url(credentialId); @@ -111,7 +114,7 @@ public HttpResponse remainingBackupCodes(HttpRequest request) { @Post("/email/start-verification") public HttpResponse startEmailVerification( - HttpRequest request, @Body StartEmailVerification body) { + HttpRequest request, @Body @Nullable StartEmailVerification body) { UserHandle actor = PkAuthJwtAuthenticationFilter.attachedUserHandle(request); if (actor == null) return HttpResponse.status(HttpStatus.UNAUTHORIZED); return map(adminService.startEmailVerification(actor, actor, body == null ? "" : body.email())); @@ -119,7 +122,7 @@ public HttpResponse startEmailVerification( /** Unauthenticated. */ @Post("/email/complete-verification") - public HttpResponse finishEmailVerification(@Body FinishEmailVerification body) { + public HttpResponse finishEmailVerification(@Body @Nullable FinishEmailVerification body) { return toMicronaut( AdminResponseMapper.toResponse( adminService.finishEmailVerification(body == null ? "" : body.token()), @@ -128,7 +131,7 @@ public HttpResponse finishEmailVerification(@Body FinishEmailVerification bod @Post("/phone/start-verification") public HttpResponse startPhoneVerification( - HttpRequest request, @Body StartPhoneVerification body) { + HttpRequest request, @Body @Nullable StartPhoneVerification body) { UserHandle actor = PkAuthJwtAuthenticationFilter.attachedUserHandle(request); if (actor == null) return HttpResponse.status(HttpStatus.UNAUTHORIZED); return map(adminService.startPhoneVerification(actor, actor, body == null ? "" : body.phone())); @@ -136,7 +139,7 @@ public HttpResponse startPhoneVerification( @Post("/phone/complete-verification") public HttpResponse finishPhoneVerification( - HttpRequest request, @Body FinishPhoneVerification body) { + HttpRequest request, @Body @Nullable FinishPhoneVerification body) { UserHandle actor = PkAuthJwtAuthenticationFilter.attachedUserHandle(request); if (actor == null) return HttpResponse.status(HttpStatus.UNAUTHORIZED); return map( diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/CeremonyScenarios.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/CeremonyScenarios.java index e6f7bbb..a4864aa 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/CeremonyScenarios.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/CeremonyScenarios.java @@ -65,8 +65,9 @@ public void registrationThenAssertionBumpsSignCount() { Optional stored = singleCredentialFor(handle); assertThat(stored).isPresent(); - assertThat(stored.get().signCount()).isZero(); - CredentialId credentialId = stored.get().credentialId(); + CredentialRecord credential = stored.orElseThrow(); + assertThat(credential.signCount()).isZero(); + CredentialId credentialId = credential.credentialId(); AssertionResult first = assertOnce(handle); assertThat(first)