diff --git a/CHANGELOG.md b/CHANGELOG.md index fb584d53..45d98733 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 6615094a..c1206b7f 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 5d939a4e..455395e8 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 ee913f52..f6d5028c 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 cc712556..332d6cb7 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 d0e302db..68f8e8f6 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