From f9de12af331843ab6b0d1e8beb1ca307e278ee07 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 21:39:25 +0000 Subject: [PATCH] fix: send JSON, not the redacted debug view, on every streaming generation Splitting the parameter object's single renderer into toJson() (the wire form) and toString() (a redacted debug view, deliberately not valid JSON) turned every surviving toString() payload call site into a silent trap. Six were repointed in LlamaModel; LlamaIterator was missed. Every streaming path -- generate(), generateChat(), the LlamaIterable paths and the Kotlin generateFlow / generateChatFlow -- therefore handed the native parser InferenceParameters{keys=[cache_prompt, prompt, stream], values=redacted} where a request body belonged. Nothing local could see it. Every test that exercises streaming is model-gated and self-skips without a GGUF, so a green `mvn test` with 269 skips said nothing about it; it surfaced on the first full-matrix CI run, on all five model-backed Java jobs at once. That is the failure mode the redacted form was designed for -- an unparseable body dying at the parser rather than a plausible-looking one succeeding with different values -- so the design held; the call site did not. Three model-gated tests passed params.toString() the same way and are repointed too, and the class javadoc that still described toString as "consumed by the native server" is corrected -- it would have sent the next reader back into the same trap. The guard is an ArchUnit rule: no class outside the parameters package may call a parameter object's toString() at all, not merely at a known call site. Two notes on its shape, both found by running it rather than reasoning about it. The parameters package itself is scoped out because JsonParameters is package-private with public subclasses, so javac emits a synthetic bridge toString() that appears in bytecode and in no source file. And it matches an explicit call only: implicit string concatenation lowers to a concat factory with no toString() call site to see -- documented on the rule rather than left to be rediscovered. Falsified by reintroducing the bug: the rule reports both call sites and names them. Verified: mvn test 1755/0 failures (13 ArchUnit rules), SpotBugs 0, PIT 337/337 at 100%, spotless and javadoc:jar clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH --- CHANGELOG.md | 15 ++++++++ .../net/ladenthin/llama/LlamaIterator.java | 6 ++- .../llama/parameters/InferenceParameters.java | 6 ++- .../net/ladenthin/llama/ChatAdvancedTest.java | 4 +- .../net/ladenthin/llama/ChatScenarioTest.java | 2 +- .../llama/LlamaArchitectureTest.java | 38 +++++++++++++++++++ 6 files changed, 64 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb584d53a..45d98733f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -191,6 +191,21 @@ from version 5.0.0 onward. Pre-fork releases (`1.x`–`4.2.0`) were authored by llama.cpp misparses is a trap with a warning label on it, and this is a major-version window. ### Fixed +- **Every streaming generation sent the native parser an unparseable body.** Splitting the parameter + object's single renderer into `toJson()` (the wire form) and `toString()` (a redacted debug view, + deliberately not valid JSON) turned every surviving `toString()` payload call site into a silent + trap. Six were repointed; `LlamaIterator` was missed, so `generate()`, `generateChat()`, the + `LlamaIterable` paths and the Kotlin `generateFlow` / `generateChatFlow` all shipped + `InferenceParameters{keys=[…], values=redacted}` where a request body belonged. It is caught by an + ArchUnit rule now — no class outside the `parameters` package may call a parameter object's + `toString()` at all — and the stale class javadoc that described `toString` as "consumed by the + native server" is corrected. + + Nothing local could see it: every test that exercises streaming is model-gated and self-skips + without a GGUF, so a green `mvn test` with 269 skips said nothing about it. It surfaced on the + first full-matrix CI run, on all five model-backed test jobs at once — which is the behaviour the + redacted form was designed for, an unparseable body failing loudly at the parser rather than a + plausible-looking one succeeding with different values. - **A caller-supplied JSON fragment could inject sibling fields into a request body.** `InferenceParameters` stored every value as a raw string and built the request by concatenating `"key": value` pairs, so a fragment passed to `withJsonSchema` / `withResponseFormat` / diff --git a/llama/src/main/java/net/ladenthin/llama/LlamaIterator.java b/llama/src/main/java/net/ladenthin/llama/LlamaIterator.java index 6615094a5..c1206b7f3 100644 --- a/llama/src/main/java/net/ladenthin/llama/LlamaIterator.java +++ b/llama/src/main/java/net/ladenthin/llama/LlamaIterator.java @@ -52,9 +52,11 @@ public final class LlamaIterator implements Iterator, AutoCloseable // is not mutated — InferenceParameters is immutable and withStream returns a // new instance with the flag set. InferenceParameters streamingParams = parameters.withStream(true); + // toJson(), never toString(): toString() is the redacted debug view and is deliberately + // not valid JSON, so passing it here would send the native parser a body it rejects. taskId = chat - ? model.requestChatCompletion(streamingParams.toString()) - : model.requestCompletion(streamingParams.toString()); + ? model.requestChatCompletion(streamingParams.toJson()) + : model.requestCompletion(streamingParams.toJson()); } @Override diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java index 5d939a4e2..455395e82 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java @@ -47,8 +47,10 @@ * *

{@code equals}/{@code hashCode} are generated by Lombok with {@code callSuper=true} * so the parent {@link JsonParameters} parameters map participates in equality. - * {@code toString} is inherited from {@link JsonParameters} and emits the accumulated - * parameters as a JSON object string consumed by the native server. + * {@code toString} is inherited from {@link JsonParameters} and is a redacted debug + * view — deliberately not valid JSON, because a parameter set carries the prompt, the + * message history and the tool definitions. The wire form consumed by the native server is + * {@code toJson()}. */ @SuppressWarnings("unused") @EqualsAndHashCode(callSuper = true) diff --git a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java index ee913f52f..f6d5028c3 100644 --- a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java +++ b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java @@ -141,7 +141,7 @@ public void testSetNProbsStreamingJsonHasProbabilities() { .withNProbs(3) .withStream(true); - int taskId = model.requestCompletion(params.toString()); + int taskId = model.requestCompletion(params.toJson()); boolean foundProbabilities = false; int tokens = 0; @@ -288,7 +288,7 @@ public void testRequestCompletionDirectStreaming() { .withTemperature(0.0f) .withStream(true); - int taskId = model.requestCompletion(params.toString()); + int taskId = model.requestCompletion(params.toJson()); StringBuilder sb = new StringBuilder(); int tokens = 0; diff --git a/llama/src/test/java/net/ladenthin/llama/ChatScenarioTest.java b/llama/src/test/java/net/ladenthin/llama/ChatScenarioTest.java index cc712556b..332d6cb7e 100644 --- a/llama/src/test/java/net/ladenthin/llama/ChatScenarioTest.java +++ b/llama/src/test/java/net/ladenthin/llama/ChatScenarioTest.java @@ -193,7 +193,7 @@ public void testRequestChatCompletionDirectStreaming() { .withTemperature(0.0f) .withStream(true); - int taskId = model.requestChatCompletion(params.toString()); + int taskId = model.requestChatCompletion(params.toJson()); StringBuilder sb = new StringBuilder(); int tokens = 0; diff --git a/llama/src/test/java/net/ladenthin/llama/LlamaArchitectureTest.java b/llama/src/test/java/net/ladenthin/llama/LlamaArchitectureTest.java index d0e302db9..68f8e8f65 100644 --- a/llama/src/test/java/net/ladenthin/llama/LlamaArchitectureTest.java +++ b/llama/src/test/java/net/ladenthin/llama/LlamaArchitectureTest.java @@ -3,8 +3,12 @@ // SPDX-License-Identifier: MIT package net.ladenthin.llama; +import static com.tngtech.archunit.core.domain.JavaCall.Predicates.target; +import static com.tngtech.archunit.core.domain.JavaClass.Predicates.assignableTo; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAPackage; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAnyPackage; +import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name; +import static com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With.owner; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.fields; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; import static com.tngtech.archunit.library.Architectures.layeredArchitecture; @@ -211,6 +215,40 @@ public class LlamaArchitectureTest { .callMethod(Thread.class, "sleep", long.class, int.class) .allowEmptyShould(true); + /** + * A parameter object's {@code toString()} must never reach a wire payload. It is the redacted + * debug view ({@code InferenceParameters{keys=[...], values=redacted}}) and is deliberately not + * valid JSON; {@code toJson()} is the serializer. The two used to be the same method, so every + * call site that was correct before the split is a silent trap after it — and this one bit: + * {@code LlamaIterator} kept calling {@code toString()} and sent the native parser an + * unparseable body on every streaming generation, which no local run could see because every + * test that exercises streaming is model-gated and self-skips without a GGUF. + * + *

The {@code parameters} package itself is scoped out, and not as a convenience: because + * {@code JsonParameters} is package-private and its subclasses are public, javac emits a + * synthetic bridge {@code toString()} in each subclass that does nothing but + * {@code invokespecial} the supertype's. That call exists in the bytecode and in no source file, + * so a rule covering the package would fail on a method nobody can edit. + * + *

Limitation worth knowing: this catches an explicit {@code toString()} call, not an implicit + * one through string concatenation, which the compiler lowers to {@code StringBuilder.append} or + * an {@code invokedynamic} string-concat factory and leaves no {@code toString()} call site to + * match. Concatenating a parameter object into a request body would still slip through — but + * that shape does not occur here, and the redacted form is unparseable precisely so that it + * fails loudly at the parser rather than sending a different body. + */ + @ArchTest + static final ArchRule parameterToStringIsNeverAWirePayload = noClasses() + .that() + .resideInAPackage("net.ladenthin.llama..") + .and() + .resideOutsideOfPackage("net.ladenthin.llama.parameters..") + .should() + .callMethodWhere(target(name("toString")) + .and(target(owner(assignableTo("net.ladenthin.llama.parameters.JsonParameters"))))) + .because("toString() is the redacted debug view; the wire serializer is toJson()") + .allowEmptyShould(true); + /** * Per-module banned import: the foundation contracts ({@code args}, {@code callback}, * {@code exception}) and the {@code loader} infrastructure must stay free of the Jackson