Merge pull request #263 from vaiju1981/chore/spotbugs-effort-max

chore: make SpotBugs (effort=Max, threshold=Low) clean again

Author: bernardladenthin

TODO.md

@@ -442,7 +442,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<T>` 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 `<Match>` 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 `<Match>` 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

spotbugs-exclude.xml

@@ -347,6 +347,36 @@ SPDX-License-Identifier: MIT
         <Method name="invoke"/>
     </Match>
 
+    <!--
+        LlamaModel.completeBatch / completeBatchWithStats / chatBatch dispatch N async requests, then
+        deliberately join EVERY future before re-throwing the first captured failure (a
+        CompletionException) — so a partial failure never abandons sibling requests running unobserved.
+        Re-throwing the captured RuntimeException is the intended behaviour; fb-contrib flags any
+        `throw <RuntimeException>` as THROWS_METHOD_THROWS_RUNTIMEEXCEPTION.
+    -->
+    <Match>
+        <Class name="net.ladenthin.llama.LlamaModel"/>
+        <Bug pattern="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION"/>
+        <Or>
+            <Method name="completeBatch"/>
+            <Method name="completeBatchWithStats"/>
+            <Method name="chatBatch"/>
+        </Or>
+    </Match>
+
+    <!--
+        ChatMessage's public constructor intentionally accepts List<ToolCall> (the natural,
+        ergonomic public-API type, matching the parts constructor). fb-contrib notes the parameter
+        is only consumed as a Collection (defensively copied into an ArrayList) and suggests widening
+        it; doing so on a public constructor would be a binary-incompatible API change for no caller
+        benefit, so the concrete List type is kept deliberately.
+    -->
+    <Match>
+        <Class name="net.ladenthin.llama.value.ChatMessage"/>
+        <Bug pattern="OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER"/>
+        <Method name="&lt;init&gt;"/>
+    </Match>
+
     <!--
         ChatMessage.requireNonNull is a precondition guard whose only
         meaningful state to report is the parameter name itself (the value

src/main/java/net/ladenthin/llama/parameters/JsonParameters.java

@@ -139,13 +139,11 @@ protected final <T extends JsonParameters> T withRaw(String key, String value) {
      */
     @SuppressWarnings("TypeParameterUnusedInFormals")
     protected final <T extends JsonParameters> T withScalar(String key, Object value) {
-        // String.valueOf(Float.NaN)/Infinity produce "NaN"/"Infinity", which are NOT valid JSON
-        // tokens and would make the whole request body unparseable by the native nlohmann parser.
-        // Reject at the source so the caller gets a clear error instead of an opaque downstream failure.
-        if (value instanceof Float && !Float.isFinite((Float) value)) {
-            throw new IllegalArgumentException(key + " must be a finite number but was " + value);
-        }
-        if (value instanceof Double && !Double.isFinite((Double) value)) {
+        // String.valueOf on a non-finite float/double yields "NaN"/"Infinity" — invalid JSON tokens
+        // the native nlohmann parser rejects. Reject at the source so the caller gets a clear error
+        // instead of an opaque downstream failure. Integer/Long are always finite (a no-op here);
+        // Boolean is not a Number and is skipped.
+        if (value instanceof Number && !Double.isFinite(((Number) value).doubleValue())) {
             throw new IllegalArgumentException(key + " must be a finite number but was " + value);
         }
         return withPut(key, String.valueOf(value));

src/main/java/net/ladenthin/llama/value/ChatMessage.java

@@ -186,7 +186,9 @@ public Optional<String> getToolCallId() {
      * @return the calls list, never {@code null}; empty when the message is not a tool-call turn
      */
     public List<ToolCall> getToolCalls() {
-        return toolCalls;
+        // Return a fresh unmodifiable view (mirrors getParts) so the stored list is never
+        // exposed directly — the caller cannot mutate this value type's internal state.
+        return Collections.unmodifiableList(toolCalls);
     }
 
     /**