From 347bdd15c426fea8618e37b218c83e8daa6e716c Mon Sep 17 00:00:00 2001 From: Vaijanath Rao Date: Sun, 21 Jun 2026 10:34:12 -0700 Subject: [PATCH] chore: make SpotBugs (effort=Max, threshold=Low) clean again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpotBugs is already enabled at effort=Max/threshold=Low (fb-contrib + findsecbugs) and bound to the verify phase — but the model-backed CI jobs run mvn test / mvn package, which never reach verify, so the audit Tier-1–3 fixes (merged in #258) introduced 6 findings that went uncaught. Resolve all 6: - ITC_INHERITANCE_TYPE_CHECKING (JsonParameters.withScalar): replace the instanceof Float + instanceof Double pair with a single `instanceof Number && !Double.isFinite(...)` check (also tidier). - EI_EXPOSE_REP (ChatMessage.getToolCalls): return a fresh Collections.unmodifiableList view instead of the stored list, mirroring getParts — the internal list is never exposed. - THROWS_METHOD_THROWS_RUNTIMEEXCEPTION x3 (LlamaModel.completeBatch / completeBatchWithStats / chatBatch): the re-throw of the captured first failure is intentional — narrow suppression with rationale in spotbugs-exclude.xml. - OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER (ChatMessage public constructor): the List param is the deliberate public-API type; widening to Collection would be a binary-incompatible change — narrow suppression with rationale. `mvn spotbugs:check` is green; 152 affected Java tests, Spotless, Javadoc clean. TODO.md updated (the "flip the pom / ~65 findings" note was stale; notes that verify is not reached by the test jobs). Co-Authored-By: Claude Opus 4.8 --- TODO.md | 10 ++++++- spotbugs-exclude.xml | 30 +++++++++++++++++++ .../llama/parameters/JsonParameters.java | 12 ++++---- .../ladenthin/llama/value/ChatMessage.java | 4 ++- 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/TODO.md b/TODO.md index 5ea451d1..c969e3ee 100644 --- a/TODO.md +++ b/TODO.md @@ -435,7 +435,15 @@ Feel free to contribute fixes — PRs welcome. - **Null-safety refinement.** JSpecify + NullAway are now enforced at compile time in **strict JSpecify mode** with the extra options `CheckOptionalEmptiness`, `AcknowledgeRestrictiveAnnotations`, `AcknowledgeAndroidRecent`, `AssertsEnabled` (see `pom.xml`); `@NullMarked` on the three packages via `package-info.java`; JDK module exports in `.mvn/jvm.config`. The legacy `org.jetbrains.annotations` dep has been removed; all nullability annotations are JSpecify. Public-API methods that may legitimately have no value use `Optional` rather than `@Nullable T` (`ChatResponse.getFirstMessage`, `ChatMessage.getParts`, `ChatRequest.buildToolsJson`). Open follow-up: review remaining unannotated public API surfaces for places where `@Nullable` would be more precise than the implicit non-null default. -- **SpotBugs `effort=Max` + `threshold=Low`** — currently default effort/threshold. Raising both surfaces ~65 remaining findings (was 90; the cross-repo `OPM_OVERLY_PERMISSIVE_METHOD` suppression in `07109cc` silenced 25 of them pending the package refactor — see below). Top remaining patterns: `DRE_DECLARED_RUNTIME_EXCEPTION` 20, `WEM_WEAK_EXCEPTION_MESSAGING` 14. The BAF/sb/plugin playbook applies: flip pom, run `spotbugs:check`, fix at source where reasonable + narrow `` with rationale for structural false positives. Cross-cutting (tracked in [`../workspace/crossrepostatus.md`](../workspace/crossrepostatus.md)). +- **SpotBugs `effort=Max` + `threshold=Low`** — **DONE (already enabled in `pom.xml`)**, with fb-contrib + + findsecbugs, bound to `verify`. The legacy "flip the pom / ~65 findings" note is stale: only a handful + of unexcluded findings remain at any time, and `spotbugs:check` is kept green. Most recent pass fixed the + 6 introduced by the audit Tier-1–3 fixes — `withScalar` uses a single `instanceof Number` (no + `ITC_INHERITANCE_TYPE_CHECKING`); `ChatMessage.getToolCalls` returns a fresh unmodifiable view (no + `EI_EXPOSE_REP`); the `LlamaModel` batch methods' deliberate re-throw and the `ChatMessage` public + constructor's `List` param carry narrow `` rationale suppressions. + **Note:** `spotbugs:check` is bound to the `verify` phase, which the model-backed CI test jobs + (`mvn test` / `mvn package`) do not reach — run `mvn verify` (or a dedicated job) to gate it in CI. - **Drop the project-wide `OPM_OVERLY_PERMISSIVE_METHOD` suppression in `spotbugs-exclude.xml`** once the package-architecture refactor lands diff --git a/spotbugs-exclude.xml b/spotbugs-exclude.xml index f753ee0b..26929e34 100644 --- a/spotbugs-exclude.xml +++ b/spotbugs-exclude.xml @@ -347,6 +347,36 @@ SPDX-License-Identifier: MIT + + + + + + + + + + + + + + + + + +