From cc475a80c8cb9dd54cecbf7aa53a15f1f4695e4c Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Mon, 14 Sep 2026 20:42:57 -0600 Subject: [PATCH 01/15] feat(ai): stop CORS headers on /api/inference/v1, deprecate superseded ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FR-030 requires this family to emit no cross-origin headers: the credential is a long-lived bearer token with the full authority of its owner, and advertising CORS invites putting it in browser JavaScript. It was not being met. dotCMS ships CORS on by default (`api.cors.default.Access-Control-Allow-Origin=*`) and `CorsFilter` is a global `@Provider` that applies the `default` mapping to every resource without one of its own, so these endpoints answered with `Access-Control-Allow-Origin: *`. Adds a `@NoCors` marker the filter honors, applied to all four resources. Additive: a resource that does not ask for the exemption keeps dotCMS's existing behaviour, which `CorsFilterTest` pins from both sides. Per-resource `api.cors..*` configuration was the alternative and is worse — it keys on the class's simple name, so a rename disables it silently, and it cannot express "emit nothing" since an entry with an empty value emits an empty header. Also marks the three superseded operations deprecated (FR-034) — text/generate, image/generate and completions/rawPrompt, GET and POST where both exist, five operations in all. `@Deprecated(since = ...)` without `forRemoval`, per ADR-0020's pattern minus the removal commitment this spec deliberately does not make; each carries a javadoc pointer to its replacement, and the regenerated openapi.yaml marks all five `deprecated: true`. Co-Authored-By: Claude Opus 5 (1M context) --- .../dotcms/ai/rest/CompletionsResource.java | 4 + .../com/dotcms/ai/rest/ImageResource.java | 8 ++ .../java/com/dotcms/ai/rest/TextResource.java | 8 ++ .../rest/ChatCompletionsResource.java | 2 + .../inference/rest/EmbeddingsResource.java | 2 + .../dotcms/inference/rest/ImagesResource.java | 2 + .../dotcms/inference/rest/ModelsResource.java | 2 + .../com/dotcms/rest/annotation/NoCors.java | 32 ++++++++ .../java/com/dotcms/rest/api/CorsFilter.java | 22 ++++- .../main/webapp/WEB-INF/openapi/openapi.yaml | 5 ++ .../com/dotcms/rest/api/CorsFilterTest.java | 80 +++++++++++++++++++ 11 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 dotCMS/src/main/java/com/dotcms/rest/annotation/NoCors.java create mode 100644 dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java index f36da6942d0e..cdec0ae4be9e 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java @@ -111,7 +111,11 @@ public final Response summarizeFromContent(@Context final HttpServletRequest req * @param response the HTTP response * @param formIn the form data containing the prompt * @return a Response object containing the generated completions + * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the + * OpenAI-compatible shape any standard client and agent framework already + * speaks. Not marked {@code forRemoval}: no removal date is committed. */ + @Deprecated(since = "Sep 14th, 26") @Path("/rawPrompt") @POST @JSONP diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java index b3aa0cc54fde..ed1347ee9810 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java @@ -61,7 +61,11 @@ public final Response indexByInode(@Context final HttpServletRequest request, * @param prompt the prompt to generate images from * @return a Response object containing the generated images * @throws IOException if an I/O error occurs + * @deprecated Superseded by {@code POST /api/inference/v1/images/generations}, which answers in the + * OpenAI-compatible shape any standard client and agent framework already + * speaks. Not marked {@code forRemoval}: no removal date is committed. */ + @Deprecated(since = "Sep 14th, 26") @GET @JSONP @Path("/generate") @@ -82,7 +86,11 @@ public final Response indexByInode(@Context final HttpServletRequest request, * @param aiImageRequestDTO the AIImageRequestDTO containing the prompt and other parameters * @return a Response object containing the generated images * @throws IOException if an I/O error occurs + * @deprecated Superseded by {@code POST /api/inference/v1/images/generations}, which answers in the + * OpenAI-compatible shape any standard client and agent framework already + * speaks. Not marked {@code forRemoval}: no removal date is committed. */ + @Deprecated(since = "Sep 14th, 26") @POST @Path("/generate") @Produces(MediaType.APPLICATION_JSON) diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java index c49ce73adecb..e27945f95f95 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java @@ -43,7 +43,11 @@ public class TextResource { * @param prompt the prompt to generate text from * @return a Response object containing the generated text * @throws IOException if an I/O error occurs + * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the + * OpenAI-compatible shape any standard client and agent framework already + * speaks. Not marked {@code forRemoval}: no removal date is committed. */ + @Deprecated(since = "Sep 14th, 26") @Path("/generate") @GET @Produces(MediaType.APPLICATION_JSON) @@ -62,7 +66,11 @@ public Response doGet(@Context final HttpServletRequest request, * @param form the form data containing the prompt * @return a Response object containing the generated text * @throws IOException if an I/O error occurs + * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the + * OpenAI-compatible shape any standard client and agent framework already + * speaks. Not marked {@code forRemoval}: no removal date is committed. */ + @Deprecated(since = "Sep 14th, 26") @Path("/generate") @POST @Produces(MediaType.APPLICATION_JSON) diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index cfe426f2ac3c..6d430f14ed3e 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -18,6 +18,7 @@ import com.dotcms.inference.rest.view.InferenceErrorView; import com.dotcms.rest.WebResource; import com.dotcms.rest.annotation.NoCache; +import com.dotcms.rest.annotation.NoCors; import com.dotcms.rest.api.v1.DotObjectMapperProvider; import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.util.Logger; @@ -91,6 +92,7 @@ */ @Path("/inference/v1/chat") @Tag(name = "AI", description = "AI-powered content generation and analysis endpoints") +@NoCors public class ChatCompletionsResource { /** Media type a streamed completion is served as. */ diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java index d478c1c3c4e8..99e4539cb677 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java @@ -13,6 +13,7 @@ import com.dotcms.inference.rest.view.InferenceErrorView; import com.dotcms.rest.WebResource; import com.dotcms.rest.annotation.NoCache; +import com.dotcms.rest.annotation.NoCors; import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.util.Logger; import com.fasterxml.jackson.databind.JsonNode; @@ -72,6 +73,7 @@ */ @Path("/inference/v1/embeddings") @Tag(name = "AI", description = "AI-powered content generation and analysis endpoints") +@NoCors public class EmbeddingsResource { /** Section of the site's {@code providerConfig} JSON that configures embeddings. */ diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java index 21d5ac2ebb7b..6675a50eb44f 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java @@ -14,6 +14,7 @@ import com.dotcms.inference.rest.view.InferenceErrorView; import com.dotcms.rest.WebResource; import com.dotcms.rest.annotation.NoCache; +import com.dotcms.rest.annotation.NoCors; import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.util.Logger; import com.liferay.portal.model.User; @@ -86,6 +87,7 @@ */ @Path("/inference/v1/images") @Tag(name = "AI", description = "AI-powered content generation and analysis endpoints") +@NoCors public class ImagesResource { /** Section of the site's {@code providerConfig} JSON that configures image generation. */ diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java index e2f2f4a31207..579d1f2bc605 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java @@ -10,6 +10,7 @@ import com.dotcms.inference.rest.view.ModelListView; import com.dotcms.rest.WebResource; import com.dotcms.rest.annotation.NoCache; +import com.dotcms.rest.annotation.NoCors; import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.util.Logger; import com.liferay.portal.model.User; @@ -68,6 +69,7 @@ */ @Path("/inference/v1/models") @Tag(name = "AI", description = "AI-powered content generation and analysis endpoints") +@NoCors public class ModelsResource { /** Section of the site's {@code providerConfig} JSON the listed models come from. */ diff --git a/dotCMS/src/main/java/com/dotcms/rest/annotation/NoCors.java b/dotCMS/src/main/java/com/dotcms/rest/annotation/NoCors.java new file mode 100644 index 000000000000..6ae6e4a7d102 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/rest/annotation/NoCors.java @@ -0,0 +1,32 @@ +package com.dotcms.rest.annotation; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks a REST resource that must never receive cross-origin response headers. + * + *

dotCMS ships CORS switched on for every endpoint through the {@code api.cors.default.*} + * properties, so a resource that says nothing gets {@code Access-Control-Allow-Origin: *}. That is + * the right default for the delivery APIs a browser is meant to call. It is the wrong one for an + * endpoint whose credential is a long-lived bearer token carrying the full authority of its owner: + * advertising cross-origin support invites putting that token in browser JavaScript, where any + * script on the page can read it.

+ * + *

Annotating the resource is what makes the exemption survive: the alternative is per-resource + * {@code api.cors..*} configuration, which is keyed on the class's simple name and + * so breaks silently the moment the class is renamed, and which cannot express "emit nothing" + * anyway — an entry with an empty value emits an empty header rather than none.

+ * + * @see com.dotcms.rest.api.CorsFilter + */ +@Documented +@Inherited +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.RUNTIME) +public @interface NoCors { +} diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/CorsFilter.java b/dotCMS/src/main/java/com/dotcms/rest/api/CorsFilter.java index fbfa2dbb420b..bd19da6c7ad0 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/CorsFilter.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/CorsFilter.java @@ -14,6 +14,7 @@ import javax.ws.rs.container.ContainerResponseFilter; import javax.ws.rs.core.MultivaluedMap; import com.dotmarketing.util.Config; +import com.dotcms.rest.annotation.NoCors; import com.dotmarketing.util.Logger; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -59,11 +60,19 @@ Map> loadHeaders() { public void filter(ContainerRequestContext requestContext, ContainerResponseContext responseContext) throws IOException { final MultivaluedMap headers = responseContext.getHeaders(); String resource = "unknown"; + Object matched = null; try{ - resource = requestContext.getUriInfo().getMatchedResources().get(0).getClass().getSimpleName().toLowerCase(); + matched = requestContext.getUriInfo().getMatchedResources().get(0); + resource = matched.getClass().getSimpleName().toLowerCase(); }catch(Exception e) { Logger.debug(this.getClass(), () -> e.getMessage()); } + // A resource can decline cross-origin support outright. Checked before the mapping lookup + // because the default mapping applies to everything that has no mapping of its own, so + // "emit nothing" is not something the configuration can express. + if (optsOutOfCors(matched)) { + return; + } getHeaders(resource) .stream() .forEach(entry-> { @@ -76,6 +85,17 @@ public void filter(ContainerRequestContext requestContext, ContainerResponseCont } + /** + * Answers whether the matched resource has declined cross-origin headers. + * + * @param matchedResource the resource instance this request matched, or null when unknown + * @return whether the resource is annotated {@link NoCors} + */ + protected boolean optsOutOfCors(final Object matchedResource) { + return matchedResource != null + && matchedResource.getClass().isAnnotationPresent(NoCors.class); + } + protected List getHeaders(final String mapping) { List corsHeaders = headerMap.get().containsKey(mapping) ? headerMap.get().get(mapping) : headerMap.get().get(CORS_DEFAULT); return corsHeaders != null ? corsHeaders : ImmutableList.of() ; diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index f23cc9f6b583..15ab85dbed6f 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -3053,6 +3053,7 @@ paths: - AI /v1/ai/completions/rawPrompt: post: + deprecated: true description: Processes raw prompts directly through the AI service without content preprocessing. Supports both streaming and non-streaming responses. operationId: rawPrompt @@ -3215,6 +3216,7 @@ paths: - AI /v1/ai/image/generate: get: + deprecated: true operationId: indexByInode_1 parameters: - in: query @@ -3229,6 +3231,7 @@ paths: tags: - AI post: + deprecated: true operationId: handleImageRequest requestBody: content: @@ -3468,6 +3471,7 @@ paths: - AI /v1/ai/text/generate: get: + deprecated: true operationId: doGet parameters: - in: query @@ -3482,6 +3486,7 @@ paths: tags: - AI post: + deprecated: true operationId: doPost requestBody: content: diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java new file mode 100644 index 000000000000..6c47608aac83 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java @@ -0,0 +1,80 @@ +package com.dotcms.rest.api; + +import com.dotcms.inference.rest.ChatCompletionsResource; +import com.dotcms.inference.rest.EmbeddingsResource; +import com.dotcms.inference.rest.ImagesResource; +import com.dotcms.inference.rest.ModelsResource; +import com.dotcms.rest.annotation.NoCors; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Covers the {@link NoCors} opt-out, which is the only thing standing between the + * {@code /api/inference/v1} family and the cross-origin headers dotCMS emits by default. + * + *

Worth its own test because the gap it closes is invisible: {@code api.cors.default.*} ships + * enabled, so a resource that says nothing is served {@code Access-Control-Allow-Origin: *} and + * nothing in the resource's own code hints at it. A regression here would not fail any of this + * family's other tests — they call the resource methods directly and never reach a response + * filter — and would quietly advertise to browsers that a long-lived bearer token is welcome in + * page JavaScript.

+ */ +public class CorsFilterTest { + + /** A resource that has said nothing about CORS, as most of dotCMS has. */ + private static class OrdinaryResource { + } + + @NoCors + private static class OptedOutResource { + } + + /** + * Given each resource of the inference family + * When the CORS filter decides whether to add its headers + * Then every one of them is exempt + * + *

FR-030. Asserted per class rather than on one of them: the annotation is applied by hand + * to each resource, so a fifth operation added later without it is exactly the mistake this + * catches, and testing a single representative would not.

+ */ + @Test + public void test_everyInferenceResource_optsOutOfCors() { + final CorsFilter filter = new CorsFilter(); + + for (final Object resource : new Object[]{ + new ChatCompletionsResource(), + new EmbeddingsResource(), + new ImagesResource(), + new ModelsResource()}) { + assertTrue(resource.getClass().getSimpleName() + + " must not receive cross-origin headers: its credential is a " + + "long-lived bearer token, and advertising CORS invites putting it " + + "in browser JavaScript", + filter.optsOutOfCors(resource)); + } + } + + /** + * Given a resource that has not opted out, and the unmatched case + * When the CORS filter decides + * Then the opt-out does not apply, so dotCMS's default behaviour is untouched + * + *

The other half of FR-030's guarantee, and the one that keeps this change additive: the + * exemption has to be something a resource asks for, not something the filter now does to + * everyone. A null stands in for a request that matched no resource, which the filter already + * tolerates.

+ */ + @Test + public void test_resourcesThatDidNotOptOut_areUnaffected() { + final CorsFilter filter = new CorsFilter(); + + assertFalse("A resource that never asked to be exempt must keep dotCMS's default " + + "cross-origin behaviour", filter.optsOutOfCors(new OrdinaryResource())); + assertFalse("An unmatched request must not blow up the filter", + filter.optsOutOfCors(null)); + assertTrue(filter.optsOutOfCors(new OptedOutResource())); + } +} From 6c2a9d24508899c663346ae241109a9c6f2160ea Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Tue, 15 Sep 2026 11:16:55 -0600 Subject: [PATCH 02/15] docs(ai): record why the flat request price under-counts a stream T046. The cost annotation on the streaming path is a flat price for an operation whose real cost varies by orders of magnitude. The comment states why it is not fixed with a larger constant, that token counts are the unit matching what the provider bills, and what a real design has to settle first. Co-Authored-By: Claude Opus 5 (1M context) --- .../dotcms/inference/rest/ChatCompletionsResource.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index 6d430f14ed3e..f043b3fb0e96 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -188,6 +188,16 @@ public class ChatCompletionsResource { @JSONP @NoCache @InferenceEndpoint + // One flat price for an operation whose real cost varies by orders of magnitude, and it + // under-counts a stream worst of all: a streamed completion holds a request thread for the + // whole generation rather than for one round trip, and the tokens it spends are unknown until + // it ends. Deliberately not "fixed" here by picking a larger constant — a bigger flat number + // is the same mistake scaled, and it would silently re-price every non-streaming caller too. + // Pricing this honestly means charging on what a request actually consumed, token counts being + // the unit that matches what the provider bills, which is a design this endpoint cannot settle + // on its own: it needs a decision about where usage is metered, what happens when a stream + // fails halfway, and how that reconciles with the per-site spend attribution FR-032 puts out + // of scope. Until that exists, the flat price stands and is known to be wrong for streams. @RequestCost(Price.HTTP_FETCH) @Path("/completions") @Consumes(MediaType.APPLICATION_JSON) From 6d58d727b0ee0cfe6de87e7f50967d2860ef7078 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Tue, 15 Sep 2026 11:31:43 -0600 Subject: [PATCH 03/15] feat(ai): prove client conformance, report provider rate limits as 429 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of Phase 8's remaining tasks, plus a gap the docs work uncovered. T089 — client-library conformance test, the automated half of SC-008. Every other test in this family reads back the view objects dotCMS produced, which cannot catch a payload that satisfies our own assertions and still chokes a real client: a required field typed as a string instead of a number, an enum value the library maps to a constant, a field it refuses to ignore. This test reads none of our types. It serializes what the endpoint returned exactly as the REST layer would, serves those bytes from WireMock, and points an unmodified LangChain4j OpenAI client at them, asserting on the library's own result types. "No adapter" is demonstrated by the absence of code between the two. Streaming is covered by replaying dotCMS's real SSE bytes through the library's own stream reader, where frame boundaries and the [DONE] sentinel would fail. T090 — FR-031. Every provider failure was answered as 502, rate limits included, so a throttled client was told the provider was broken rather than busy and backed off on the wrong schedule. A rate limit is now 429 in the standard shape. The cause chain is walked rather than the top exception inspected, because the fallback chain wraps as it walks a site's models and the wrapped case is the one that reaches production. FR-031's other half cannot be met: the provider client surfaces a failed call as an exception carrying a status code and a message and nothing else, so a provider's Retry-After is discarded before dotCMS can see it. Relaying it needs a custom HTTP client under the provider abstraction. Recorded as an open residual in the spec and as a known limitation in the docs rather than dropped quietly. T082 — docs/backend/INFERENCE_API.md, linked from CLAUDE.md and docs/README.md. T081 — checked, not skipped: no raw collections in the touched files, no rawtypes or unchecked warnings from javac, @Override already present, and Logger/Config used throughout. Recorded as having nothing to do. Tests: 67 unit, 77 integration across eleven classes, all green. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 1 + docs/README.md | 1 + docs/backend/INFERENCE_API.md | 315 +++++++++++ .../client/langchain4j/InferenceAIClient.java | 4 +- .../inference/model/InferenceError.java | 46 ++ .../rest/ChatCompletionsResource.java | 2 +- .../inference/rest/EmbeddingsResource.java | 3 +- .../dotcms/inference/rest/ImagesResource.java | 2 +- .../InferenceErrorTranslationTest.java | 130 +++++ .../src/test/java/com/dotcms/MainSuite2b.java | 2 + .../rest/InferenceClientConformanceTest.java | 531 ++++++++++++++++++ .../37431-openai-compatible-inference/spec.md | 2 +- 12 files changed, 1034 insertions(+), 5 deletions(-) create mode 100644 docs/backend/INFERENCE_API.md create mode 100644 dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 76a2579afcdb..76f942cd1045 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -120,6 +120,7 @@ How it's built + upgrade re-apply notes: [.specify/CUSTOMIZATIONS.md](.specify/C - [Java Standards](docs/backend/JAVA_STANDARDS.md) — Coding patterns, immutables, exceptions, utilities - [When to Use Virtual Threads](docs/backend/VIRTUAL_THREADS.md) — Socket I/O yes, file I/O no; carrier pinning - [REST API Patterns](docs/backend/REST_API_PATTERNS.md) — JAX-RS, Swagger, @Schema rules +- [Inference API](docs/backend/INFERENCE_API.md) — OpenAI-compatible `/api/inference/v1` family: chat completions, streaming, models, embeddings, images; bearer-only auth, per-site resolution, limits - [Maven Build System](docs/backend/MAVEN_BUILD_SYSTEM.md) — Dependency management - [Configuration Patterns](docs/backend/CONFIGURATION_PATTERNS.md) — Config.getProperty() usage - [Database Patterns](docs/backend/DATABASE_PATTERNS.md) — DotConnect, transactions diff --git a/docs/README.md b/docs/README.md index d6ca3f4edf1a..1e61cfdd7019 100644 --- a/docs/README.md +++ b/docs/README.md @@ -16,6 +16,7 @@ Standards that apply to all development: Backend-specific patterns and standards: - **JAVA_STANDARDS.md** - Java development patterns and API usage - **MAVEN_BUILD_SYSTEM.md** - Dependency and plugin management +- **[INFERENCE_API.md](backend/INFERENCE_API.md)** - OpenAI-compatible `/api/inference/v1` endpoint family: operations, bearer-only auth, site resolution, limits, errors ### `/docs/frontend/` - Angular/TypeScript Development Frontend-specific patterns and standards. **Start at the index: [frontend/README.md](frontend/README.md)** — it lists every doc and when to load it. diff --git a/docs/backend/INFERENCE_API.md b/docs/backend/INFERENCE_API.md new file mode 100644 index 000000000000..c1464f30b05e --- /dev/null +++ b/docs/backend/INFERENCE_API.md @@ -0,0 +1,315 @@ +# OpenAI-Compatible Inference API (`/api/inference/v1`) + +`/api/inference/v1` is a REST endpoint family that speaks the OpenAI chat-completions wire format — the request and response JSON shapes published by OpenAI and served by most AI gateways (OpenRouter, LiteLLM, Vercel AI Gateway, Ollama, vLLM). Because the format is the one standard client libraries and agent frameworks already emit, pointing such a client at dotCMS takes exactly two settings: a base URL and a dotCMS API token. No dotCMS-specific client code is written. + +The endpoints sit in front of dotAI, which is already a multi-provider gateway with per-site credentials held in App secrets, model fallback chains, and provider selection. What the family adds is the door: an application that previously had to embed a provider key of its own can route through dotCMS instead, so the site's credentials stay server-side, the provider can be swapped centrally, and every call can be attributed to a site. + +**This is not a replacement for `/api/v1/ai/*`.** Those endpoints stay and keep their behavior. Retrieval-augmented completions, semantic search, the vector-corpus operations and provider administration have no equivalent verb in the OpenAI format and remain first-class there. Three `/api/v1/ai/*` operations are superseded by this family and are now `@Deprecated` — see [Superseded endpoints](#superseded-endpoints). + +Source: `dotCMS/src/main/java/com/dotcms/inference/`. Design intent lives in `specs/37431-openai-compatible-inference/`; the published contract is the generated `openapi.yaml`. + +--- + +## Operations + +| Operation | Path | Notes | +|---|---|---| +| Chat completion | `POST /api/inference/v1/chat/completions` | Multi-turn messages, tool calling, optional SSE streaming | +| Model listing | `GET /api/inference/v1/models` | The models the resolved site configured for chat | +| Embeddings | `POST /api/inference/v1/embeddings` | One string or an array of strings | +| Image generation | `POST /api/inference/v1/images/generations` | Always base64, never a URL | + +The trailing `v1` is the **wire-protocol** version, not dotCMS's own `/api/v1` resource versioning. A second wire format would be added as a sibling under the same version (`/api/inference/v1/responses`), not as `/api/inference/v2`. OpenAI's legacy non-chat `POST /completions` is deliberately absent. + +--- + +## Authentication + +**Bearer token only.** Every endpoint requires `Authorization: Bearer `. Session cookies and basic authentication are refused with a `401`, even though the surrounding dotCMS filters would otherwise accept them — `BearerOnlyAuthFilter` enforces this for the whole family, and each resource calls the same check directly so it also holds when a resource method is invoked outside the JAX-RS chain. + +Any authenticated user is accepted, backend or frontend, matching the existing dotAI endpoints. + +**These endpoints are server-side only. Never put the token in browser JavaScript.** The token carries the full authority of the user it was issued for, so a token that reaches a browser is a token that can be read out of it. This is why **no CORS headers are emitted**: each resource carries the `@NoCors` marker (`com.dotcms.rest.annotation.NoCors`), which `com.dotcms.rest.api.CorsFilter` honors by skipping cross-origin headers entirely, so a browser blocks a cross-origin call before it reaches the endpoint. Refusing session and basic credentials is what keeps that a real control rather than a formality — if an ambient browser credential authenticated here, any page the user had open would be one fetch away from spending the site's AI budget. + +--- + +## Model selection + +`model` is **required on every request**. There is no implicit default, no reserved alias and no sentinel name — a request that omits it is refused with a `400` naming the field. + +The value is validated against what the resolved site has configured **for that capability**, for every caller including administrators: + +| Endpoint | Validated against the site's | +|---|---| +| `chat/completions` | `chat` section of `providerConfig` (including every fallback-chain entry) | +| `embeddings` | `embeddings` section | +| `images/generations` | `image` section | + +A model the site has not configured for that capability is refused with a `404`, so a chat model sent to the images endpoint is refused even though the same site configured it perfectly well for chat. Nothing is passed through to the provider. + +`GET /api/inference/v1/models` is the discovery mechanism: it lists exactly the names `chat/completions` accepts, in configured order, the first being the site's primary model. No provider is contacted and nothing synthetic is added. A caller that wants "whatever this site runs" reads the list and takes the first entry. + +Because the model is required and validated, swapping a site's provider is **not** invisible to callers: a caller pinning the old vendor's model gets an explicit `404` and has to re-read the model list. That is deliberate. + +Within the chat capability, the site's own configuration and fallback chain decide which model actually runs. The `model` field of the response names the model that served the request, which may be a later entry in the chain than the one requested. + +--- + +## Site resolution + +Site resolution is **standard dotCMS resolution** — this family invents no semantics of its own: + +1. An explicit override, given as the `X-dotCMS-Site` request header or the `siteId` query parameter (a site identifier or a host name). The header wins when both are present. An override is resolved **as the caller**, so a site the caller cannot read is refused with a `403`. +2. Otherwise the request's host name. +3. Otherwise the default site, exactly as everywhere else in dotCMS. Server-side callers routinely present a host name that is no site alias — internal DNS, a container service name, `localhost` — so refusing those would break the deployments this family exists for. + +A default-site fallback is **logged** with the unmatched host name, at warn level, stating that the default site's credentials will be spent. + +The legacy `host` and `host_id` request parameters, which act as a de-facto site override elsewhere in dotCMS, are **ignored** here. + +Every response — success, refusal, streamed or not — carries the serving site in the **`X-dotCMS-Resolved-Site`** response header, holding the site identifier. It is a header rather than a body field so that payloads still deserialize into a standard client's own types, and so that it survives on errors and on streams. `ResolvedSiteHeaderFilter` writes it from an attribute the resource publishes the instant the site is known, before anything that can fail. + +A site with no dotAI configuration of its own inherits the system-level configuration, as every other dotAI caller does. Only when neither the site nor the system level has any configuration is a completion refused; the model listing returns an empty array in that case, never another site's models. + +--- + +## Configuration + +All four keys are read through `Config` at the point of use (`com.dotcms.inference.model.InferenceLimits`), so changing one takes effect without restarting the node. + +| Key | Default | What it bounds | +|---|---|---| +| `DOT_INFERENCE_MAX_CONCURRENT_STREAMS` | `50` | Streamed completions in flight on this node. A streamed completion parks a request thread for the whole generation, so concurrency — not request rate — is the scarce resource here. Over the ceiling, a streaming request is refused with `429`. | +| `DOT_INFERENCE_COMPLETION_TIMEOUT_SECONDS` | `300` | Hard ceiling on one streamed completion. A provider that stops producing events ends the stream with an error event instead of parking the thread indefinitely. | +| `DOT_INFERENCE_MAX_REQUEST_BYTES` | `1048576` (1 MiB) | Largest request body accepted. A declared `Content-Length` over the ceiling is refused with `413` before the body is read. | +| `DOT_INFERENCE_MAX_IMAGES_PER_REQUEST` | `10` | Most images one generation request may ask for. Ten is the ceiling the OpenAI images API itself documents for `n`. Over it, a `400` naming `n`. | + +> Earlier task notes refer to "the three config keys". There are four: `DOT_INFERENCE_MAX_IMAGES_PER_REQUEST` was added later, because image generation is priced per image and per-site spend quotas are out of scope — without a ceiling, one accepted request is an unbounded bill. + +Each endpoint also registers `@RequestCost(Price.HTTP_FETCH)`, the band used for operations that make one remote round trip, so the instance-wide rate-limit backstop prices them. That price is known to under-count a stream; it is not a per-site AI spend quota, which is out of scope. + +--- + +## `POST /chat/completions` + +Non-streaming, with tool declarations: + +```bash +curl -X POST "https://demo.dotcms.com/api/inference/v1/chat/completions" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{ + "model": "gpt-4o", + "messages": [ + { "role": "system", "content": "You are a helpful assistant." }, + { "role": "user", "content": "What is the weather in Bogota?" } + ], + "tools": [ + { "type": "function", "function": { + "name": "get_weather", + "description": "Current weather for a city", + "parameters": { "type": "object", "properties": { "city": { "type": "string" } }, "required": ["city"] } } } + ], + "tool_choice": "auto", + "temperature": 0.7, + "max_tokens": 1024 + }' +``` + +```json +{ + "id": "chatcmpl-…", "object": "chat.completion", "created": 1789000000, "model": "gpt-4o", + "choices": [ { "index": 0, "finish_reason": "tool_calls", + "message": { "role": "assistant", "content": null, + "tool_calls": [ { "id": "call_1", "type": "function", + "function": { "name": "get_weather", "arguments": "{\"city\":\"Bogota\"}" } } ] } } ], + "usage": { "prompt_tokens": 82, "completion_tokens": 17, "total_tokens": 99 } +} +``` + +The next turn appends the assistant's tool-call message and a `tool` message carrying the result, correlated by `tool_call_id`: + +```bash +curl -X POST "https://demo.dotcms.com/api/inference/v1/chat/completions" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{ + "model": "gpt-4o", + "messages": [ + { "role": "system", "content": "You are a helpful assistant." }, + { "role": "user", "content": "What is the weather in Bogota?" }, + { "role": "assistant", "content": null, + "tool_calls": [ { "id": "call_1", "type": "function", + "function": { "name": "get_weather", "arguments": "{\"city\":\"Bogota\"}" } } ] }, + { "role": "tool", "tool_call_id": "call_1", "content": "{\"tempC\":19}" } + ] + }' +``` + +`temperature`, `max_tokens`, `top_p` and `stop` are passed through to the provider, because they change what the caller gets and what the site pays. Incidental fields a standard client sends by default are ignored. `n` is the exception among the fields this family does not honor: asking for more than one choice is refused with a `400` rather than silently ignored. `response_format` is accepted. + +### Streaming + +Set `"stream": true` and the answer is served as `text/event-stream`: + +```bash +curl -N -X POST "https://demo.dotcms.com/api/inference/v1/chat/completions" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{ + "model": "gpt-4o", + "messages": [ { "role": "user", "content": "Write a haiku about caching." } ], + "stream": true, + "stream_options": { "include_usage": true } + }' +``` + +``` +data: {"id":"chatcmpl-…","object":"chat.completion.chunk","created":1789000000,"model":"gpt-4o","choices":[{"index":0,"delta":{"role":"assistant","content":""}}]} + +data: {"id":"chatcmpl-…","object":"chat.completion.chunk","created":1789000000,"model":"gpt-4o","choices":[{"index":0,"delta":{"content":"Cold bytes"}}]} + +data: {"id":"chatcmpl-…","object":"chat.completion.chunk","created":1789000000,"model":"gpt-4o","choices":[{"index":0,"delta":{},"finish_reason":"stop"}]} + +data: [DONE] +``` + +Every chunk carries `id`, `object`, `created` and `model`, fixed for the life of the stream so a client can correlate the fragments it stitches together. Tool calls stream incrementally: each fragment carries the call's `index`, and `id`, `type` and `function.name` arrive on the first fragment, so a standard reader reassembles the arguments as they land. + +Token usage on a stream is emitted **only** when the request asks for it through `stream_options.include_usage`. It arrives as a final chunk with an empty `choices` array, immediately before `[DONE]`. dotCMS builds that chunk itself from the counts the provider abstraction reports and does not forward the option upstream; a usage chunk a provider volunteers unasked is suppressed, because the empty `choices` array is exactly what breaks readers that assume every chunk carries a choice. + +**A stream that fails after it has begun** — a provider error mid-generation, or `DOT_INFERENCE_COMPLETION_TIMEOUT_SECONDS` firing — emits the standard error object as its final event and closes **without** `[DONE]`: + +``` +data: {"error":{"message":"The model provider failed to complete the request","type":"api_error","param":null,"code":null}} +``` + +Once the first chunk is on the wire the HTTP status can no longer carry a failure, so withholding the done marker is what stops a client that does not parse the error event from reading a truncated answer as a finished one. + +--- + +## `GET /models` + +```bash +curl "https://demo.dotcms.com/api/inference/v1/models" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" +``` + +```json +{ "object": "list", "data": [ + { "id": "gpt-4o", "object": "model", "created": 1789000000, "owned_by": "dotcms" }, + { "id": "gpt-4o-mini", "object": "model", "created": 1789000000, "owned_by": "dotcms" } ] } +``` + +`created` is the time the listing was built, the same value on every entry: dotCMS serves a configuration, not a catalogue, and does not know when a vendor published a model. + +--- + +## `POST /embeddings` + +`input` accepts a single string or an array of strings; batching is how content is ordinarily embedded. + +```bash +curl -X POST "https://demo.dotcms.com/api/inference/v1/embeddings" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{ + "model": "text-embedding-3-small", + "input": ["The quick brown fox", "jumps over the lazy dog"] + }' +``` + +```json +{ "object": "list", "model": "text-embedding-3-small", + "data": [ { "object": "embedding", "index": 0, "embedding": [0.0023, -0.0091] }, + { "object": "embedding", "index": 1, "embedding": [0.0512, 0.0034] } ], + "usage": { "prompt_tokens": 12, "total_tokens": 12 } } +``` + +The response is always a list with one entry per input, each carrying the `index` of the input it embeds — the caller's only way to correlate a vector back to the text they sent. A batch with one bad element is refused whole rather than cleaned, because dropping an element would shift every index after it. + +Refused with a `400` naming `input`: an absent, null or empty `input`; an empty array; an element that is not a string (arrays of token ids are not supported); and a blank or whitespace-only string wherever it appears. Where one element of an array is at fault, the message says which index. There is no separate cap on element count — `DOT_INFERENCE_MAX_REQUEST_BYTES` already bounds what can arrive. + +Input text is never logged and never echoed back into an error message. + +--- + +## `POST /images/generations` + +```bash +curl -X POST "https://demo.dotcms.com/api/inference/v1/images/generations" \ + -H "Authorization: Bearer $DOTCMS_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{ + "model": "dall-e-3", + "prompt": "A cat in a hammock", + "n": 2, + "size": "1024x1024" + }' +``` + +```json +{ "created": 1789000000, "data": [ { "b64_json": "iVBORw0KGgo…" }, { "b64_json": "R0lGODlhAQAB…" } ] } +``` + +**Results are always base64 (`b64_json`), never a hosted URL.** The response shape has no `url` component at all. A URL would mean deciding storage, authentication and lifetime for an artifact generated from a prompt that may carry customer data, so this family declines to create a separately-addressable artifact. Where a provider offers the choice, dotCMS asks it for the inline form so nothing is minted upstream either; where a provider only returns a link, dotCMS fetches and re-encodes it. On those providers an artifact does exist upstream — the guarantee is that a caller never receives one. + +`n` is honored; omitting it means one image. It is refused with a `400` naming `n` — never clamped, and never surfaced as a retryable upstream error — when it is: + +- below `1`; +- above `DOT_INFERENCE_MAX_IMAGES_PER_REQUEST`; +- above `1` on a site whose configured image model can only produce one. Support is probed from the model implementation rather than a list of provider names, so it cannot rot on a library upgrade. In the current provider set, `GoogleAiGeminiImageModel` is the one that does not support it, while the OpenAI image models do. + +A model limitation that can never succeed is a `400` precisely because a `502` would tell a standard client's back-off to keep retrying a request that cannot succeed however long it waits. + +`prompt` is required, and is refused with a `400` naming `prompt` when blank. `size` is passed through to the provider as a `WIDTHxHEIGHT` string; where the site carries an `imageSize` setting, the caller's value wins and the site's is the default for a request that omits it, which is what the existing dotAI image endpoint already does. + +Prompts are never logged and never echoed back into an error message. + +--- + +## Errors + +Every refusal uses the standard OpenAI error envelope, so a client library deserializes a failure into its own types with no adapter. Nothing is wrapped in dotCMS's `ResponseEntityView`. + +```json +{ "error": { "message": "The model field is required; there is no implicit default model", "type": "invalid_request_error", "param": "model", "code": null } } +``` + +**Retryability is carried by the HTTP status, not by a body field** — that is what a standard client's back-off keys off, and the standard error shape has no retryable field. + +| Status | `type` | When | +|---|---|---| +| `400` | `invalid_request_error` | Caller mistakes: missing `model`, empty `messages`, a malformed or uncorrelated tool message, `n > 1` on chat, a bad `input`, a bad `prompt`, an `n` the site's image model or the configured ceiling will not allow, an unresolvable site override | +| `401` | `invalid_request_error` | Anonymous, or a credential that is not a bearer token | +| `403` | `invalid_request_error` | An explicit site override naming a site the caller cannot read | +| `404` | `invalid_request_error` | A model the resolved site has not configured for that capability | +| `413` | `invalid_request_error` | Request body over `DOT_INFERENCE_MAX_REQUEST_BYTES` | +| `429` | `rate_limit_error` | The node is already serving its ceiling of concurrent streamed completions, **or** the model provider is rate limiting this site | +| `502` | `api_error` | A genuine upstream provider failure | + +A `502` carries a fixed, safe message; the provider's own message is logged and never returned, because it can carry the provider's endpoint, account identifiers, or a fragment of the prompt. Retryability is carried by the status alone — the standard error shape has no retryable field, and adding one would break the no-adapter promise these endpoints exist to keep — so a provider rate limit is reported as `429` rather than folded into `502`: the two call for different back-off, and a client that reads `502` treats a busy provider as a broken one. **Known limitation**: when a provider sends a `Retry-After` header, dotCMS cannot currently relay it. The client library underneath surfaces a failed call as an exception carrying a status code and a message only, discarding response headers, so the value never reaches dotCMS. A client backs off on the status alone until that changes. A site with no usable configuration for the requested capability lands on the same `404` as an unknown model, so a caller cannot use the distinction to learn which sites have dotAI set up. + +Only exchange **metadata** is recorded — resolved site, model, token counts, status, duration. Request and response bodies are never written to a log or a durable store, and no conversation is retained after the response is delivered; the family is stateless. + +--- + +## Superseded endpoints + +These three `/api/v1/ai/*` operations are now `@Deprecated`. They remain fully functional and no removal date is committed. + +| Deprecated | Replaced by | +|---|---| +| `POST /api/v1/ai/text/generate` | `POST /api/inference/v1/chat/completions` | +| `POST /api/v1/ai/image/generate` | `POST /api/inference/v1/images/generations` | +| `POST /api/v1/ai/completions/rawPrompt` | `POST /api/inference/v1/chat/completions` | + +Every other `/api/v1/ai/*` endpoint is unaffected and is not superseded. + +--- + +## Related + +- [REST API Patterns](REST_API_PATTERNS.md) — JAX-RS, Swagger, `@Schema` rules +- [Configuration Patterns](CONFIGURATION_PATTERNS.md) — `Config.getProperty()` usage +- [Security Patterns](SECURITY_BACKEND.md) — auth, input validation, secure logging diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java index 1204d582c24e..89bbfdddb0c5 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java @@ -701,7 +701,9 @@ private static InferenceError toInferenceError(final Throwable throwable) { ? "The request could not be served with this site's configuration" : throwable.getMessage(), null); } - return InferenceError.upstream(UPSTREAM_FAILURE_MESSAGE); + // A streamed exchange is rate limited exactly as often as a buffered one, and a client + // reading an error event backs off on the same status, so the translation belongs here too. + return InferenceError.fromProviderFailure(throwable, UPSTREAM_FAILURE_MESSAGE); } /** diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java index a24e642a6556..19668c3fee8d 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java @@ -54,6 +54,52 @@ public static InferenceError upstream(final String message) { return new InferenceError("api_error", message, null, 502); } + /** + * Translates a provider failure into the status a standard client's back-off keys off. + * + *

FR-031. A provider refusing on rate limit and a provider genuinely broken are different + * events with different right answers, and collapsing both into 502 tells a client to retry a + * throttled request on the wrong schedule while telling it nothing about when to come back. + * Rate limiting is 429; anything else upstream stays 502.

+ * + *

The provider's own message is never carried out of here — it can hold its endpoint, its + * account identifiers, and occasionally a fragment of the prompt. Only the status is taken + * from it, which is why the caller supplies the wording.

+ * + *

The cause chain is walked rather than the top exception inspected, because the provider + * client wraps: a rate limit surfaces as its own type or as a generic HTTP failure carrying + * 429, and by the time a retrying fallback chain has rethrown it, either can be two or three + * causes down.

+ * + * @param failure the exception the provider client raised + * @param upstreamMessage the wording to return for a non-rate-limit failure + * @return the error to answer with + */ + public static InferenceError fromProviderFailure(final Throwable failure, + final String upstreamMessage) { + for (Throwable current = failure; current != null; current = current.getCause()) { + if (current instanceof dev.langchain4j.exception.RateLimitException) { + return rateLimited(); + } + if (current instanceof dev.langchain4j.exception.HttpException httpFailure + && httpFailure.statusCode() == 429) { + return rateLimited(); + } + if (current.getCause() == current) { + break; + } + } + return upstream(upstreamMessage); + } + + /** + * @return the standard rate-limit refusal, which a client reads as "retry later" + */ + public static InferenceError rateLimited() { + return new InferenceError("rate_limit_error", + "The model provider is rate limiting this site's requests", null, 429); + } + /** @return whether a standard client should retry, as implied by the status */ public boolean isRetryable() { return httpStatus == 429 || httpStatus >= 500; diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index f043b3fb0e96..dbdb09fe9470 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -289,7 +289,7 @@ private Response completeWhole(final ResolvedAiContext context, // occasionally a fragment of the prompt, so it is logged and never returned. Logger.error(this, "Chat completion failed for site " + AiHostResolver.sanitize(context.servingSiteId()), e); - return errorResponse(InferenceError.upstream(UPSTREAM_FAILURE_MESSAGE)); + return errorResponse(InferenceError.fromProviderFailure(e, UPSTREAM_FAILURE_MESSAGE)); } } diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java index 99e4539cb677..9b827a3e6203 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java @@ -234,6 +234,7 @@ private Response embed(final ResolvedAiContext context, Logger.error(this, "The embeddings provider returned " + batch.vectors().size() + " vectors for " + inputs.size() + " inputs on site " + AiHostResolver.sanitize(context.servingSiteId())); + // No exception to inspect here: the provider answered, it just answered short. return errorResponse(InferenceError.upstream(UPSTREAM_FAILURE_MESSAGE)); } @@ -245,7 +246,7 @@ private Response embed(final ResolvedAiContext context, // occasionally a fragment of the input, so it is logged and never returned. Logger.error(this, "Embeddings failed for site " + AiHostResolver.sanitize(context.servingSiteId()), e); - return errorResponse(InferenceError.upstream(UPSTREAM_FAILURE_MESSAGE)); + return errorResponse(InferenceError.fromProviderFailure(e, UPSTREAM_FAILURE_MESSAGE)); } } diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java index 6675a50eb44f..a4cd3cd1f524 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java @@ -300,7 +300,7 @@ private Response generate(final ResolvedAiContext context, // occasionally a fragment of the prompt, so it is logged and never returned. Logger.error(this, "Image generation failed for site " + AiHostResolver.sanitize(context.servingSiteId()), e); - return errorResponse(InferenceError.upstream(UPSTREAM_FAILURE_MESSAGE)); + return errorResponse(InferenceError.fromProviderFailure(e, UPSTREAM_FAILURE_MESSAGE)); } } diff --git a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java new file mode 100644 index 000000000000..3a0dafa06cfb --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java @@ -0,0 +1,130 @@ +package com.dotcms.inference; + +import com.dotcms.inference.model.InferenceError; +import dev.langchain4j.exception.HttpException; +import dev.langchain4j.exception.RateLimitException; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Covers FR-031's translation of a provider failure into the status a client backs off on. + * + *

The distinction this pins is not cosmetic. A standard client decides what to do next from the + * HTTP status alone — the error shape carries no retryable field, and inventing one would break the + * no-adapter promise of SC-008 — so answering 502 to a throttled request tells the client the + * provider is broken rather than busy, and it retries on the wrong schedule. Every assertion here + * is about that one decision.

+ */ +public class InferenceErrorTranslationTest { + + private static final String UPSTREAM = "The model provider failed to complete the request"; + + /** + * Given a provider that refused because the site is over its rate limit + * When the failure is translated + * Then the caller is told 429, in the standard rate-limit shape + */ + @Test + public void test_rateLimitFailure_becomesA429() { + final InferenceError error = InferenceError.fromProviderFailure( + new RateLimitException("slow down"), UPSTREAM); + + assertEquals("A throttled request is 429, which is what a client's back-off keys off", + 429, error.httpStatus()); + assertEquals("rate_limit_error", error.type()); + assertTrue(error.isRetryable()); + } + + /** + * Given a provider that answered with HTTP 429 rather than a typed exception + * When the failure is translated + * Then it is still a 429 + * + *

Both shapes occur: the provider client raises its own type for some providers and a plain + * HTTP failure carrying the status for others, and a translation that recognised only the typed + * one would be correct for whichever provider it was written against and wrong for the rest.

+ */ + @Test + public void test_http429_becomesA429() { + final InferenceError error = InferenceError.fromProviderFailure( + new HttpException(429, "Too Many Requests"), UPSTREAM); + + assertEquals(429, error.httpStatus()); + assertEquals("rate_limit_error", error.type()); + } + + /** + * Given a rate limit buried under the wrappers a retrying fallback chain adds + * When the failure is translated + * Then it is still a 429 + * + *

This is the case that actually reaches production. The fallback chain catches, logs and + * rethrows as it walks a site's configured models, so by the time the resource sees the failure + * the rate limit is several causes down. A translation that inspected only the top exception + * would pass every test above and still answer 502 to every real throttled request.

+ */ + @Test + public void test_rateLimitWrappedByTheFallbackChain_isStillA429() { + final InferenceError error = InferenceError.fromProviderFailure( + new RuntimeException("image model 'dall-e-3' failed", + new IllegalStateException("retrying", + new RateLimitException("slow down"))), + UPSTREAM); + + assertEquals("A rate limit the fallback chain wrapped is still a rate limit", + 429, error.httpStatus()); + } + + /** + * Given a provider that genuinely failed, and a self-referencing cause chain + * When the failure is translated + * Then it is 502, and the translation terminates + * + *

The 5xx half of FR-031, plus the loop guard: a cause chain that points at itself is rare + * but legal, and a walk without the guard hangs the request thread rather than failing it.

+ */ + @Test + public void test_ordinaryFailure_becomes502_andSelfReferencingCauseTerminates() { + final InferenceError plain = InferenceError.fromProviderFailure( + new HttpException(500, "Internal Server Error"), UPSTREAM); + assertEquals(502, plain.httpStatus()); + assertEquals("api_error", plain.type()); + assertEquals(UPSTREAM, plain.message()); + + final RuntimeException selfReferencing = new RuntimeException("broken") { + @Override + public synchronized Throwable getCause() { + return this; + } + }; + assertEquals(502, InferenceError.fromProviderFailure(selfReferencing, UPSTREAM).httpStatus()); + } + + /** + * Given a provider failure of any kind + * When it is translated + * Then the provider's own wording never reaches the caller + * + *

FR-031 and FR-036 together: a provider message can carry its endpoint, account + * identifiers, or a fragment of the prompt, so only the status is taken from it.

+ */ + @Test + public void test_providerWordingIsNeverReturned() { + final String leaky = "api.openai.com rejected key sk-secret for org acme: 'draft a memo'"; + + final InferenceError rateLimited = InferenceError.fromProviderFailure( + new RateLimitException(leaky), UPSTREAM); + final InferenceError failed = InferenceError.fromProviderFailure( + new HttpException(503, leaky), UPSTREAM); + + for (final InferenceError error : new InferenceError[]{rateLimited, failed}) { + assertFalse("The provider's own message must never be handed to a caller", + error.message().contains("sk-secret")); + assertFalse(error.message().contains("api.openai.com")); + assertFalse(error.message().contains("draft a memo")); + } + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index 8b39a1493beb..908a21ba415f 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -70,6 +70,7 @@ import com.dotcms.inference.rest.ChatCompletionsStreamingTest; import com.dotcms.inference.rest.ChatCompletionsTest; import com.dotcms.inference.rest.InferenceAuthorizationTest; +import com.dotcms.inference.rest.InferenceClientConformanceTest; import com.dotcms.inference.rest.InferenceEmbeddingsTest; import com.dotcms.inference.rest.InferenceFallbackTest; import com.dotcms.inference.rest.InferenceImagesTest; @@ -457,6 +458,7 @@ ChatCompletionsTest.class, ChatCompletionsStreamingTest.class, InferenceAuthorizationTest.class, + InferenceClientConformanceTest.class, InferenceEmbeddingsTest.class, InferenceFallbackTest.class, InferenceImagesTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java new file mode 100644 index 000000000000..13dee826088d --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java @@ -0,0 +1,531 @@ +package com.dotcms.inference.rest; + +import com.dotcms.ai.AiTest; +import com.dotcms.auth.providers.jwt.beans.ApiToken; +import com.dotcms.datagen.SiteDataGen; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.inference.rest.view.ChatCompletionRequestView; +import com.dotcms.inference.rest.view.EmbeddingsRequestView; +import com.dotcms.inference.rest.view.ImageGenerationRequestView; +import com.dotcms.rest.api.v1.DotObjectMapperProvider; +import com.dotcms.util.IntegrationTestInitService; +import com.dotcms.util.network.IPUtils; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.PropertyNamingStrategies; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.TextNode; +import com.github.tomakehurst.wiremock.WireMockServer; +import com.liferay.portal.model.User; +import dev.langchain4j.data.embedding.Embedding; +import dev.langchain4j.data.image.Image; +import dev.langchain4j.data.message.UserMessage; +import dev.langchain4j.model.chat.response.ChatResponse; +import dev.langchain4j.model.embedding.EmbeddingModel; +import dev.langchain4j.model.image.ImageModel; +import dev.langchain4j.model.openai.OpenAiChatModel; +import dev.langchain4j.model.openai.OpenAiEmbeddingModel; +import dev.langchain4j.model.openai.OpenAiImageModel; +import dev.langchain4j.model.openai.internal.models.ModelsListResponse; +import dev.langchain4j.model.output.Response; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.StreamingOutput; +import java.io.ByteArrayOutputStream; +import java.time.Duration; +import java.time.Instant; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.post; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Proves SC-008: every one of the four operations returns payloads that deserialize into a + * standard client library's own result types with no adapter. + * + *

Every other test in this family asserts the shape dotCMS intended to produce, by + * reading the view objects it returned. That is a different claim, and a weaker one: a payload can + * satisfy every assertion this codebase makes about it and still fail in the only place that + * matters, because a real client library is stricter than the test that wrote the field. A + * required property spelled correctly but typed as a string instead of a number, an enum value the + * library maps to an enum constant, a field the library refuses to ignore — none of those show up + * until something that was not written here tries to read the bytes.

+ * + *

So this test does not read the view objects at all. It serializes what the endpoint returned + * exactly as the REST layer would, serves those bytes from WireMock, and points an + * unmodified OpenAI client at them. The client is LangChain4j's, chosen because it + * is already a dependency and is a genuine third-party OpenAI-compatible implementation: nothing in + * it was written with dotCMS in mind, and it is the same client code a customer would point at + * these endpoints. Assertions are made on the library's own result types — {@link ChatResponse}, + * {@link Embedding}, {@link Image}, {@link ModelsListResponse} — never on ours.

+ * + *

"No adapter" is the load-bearing phrase, and it is what the absence of code here demonstrates: + * the payload goes from the endpoint to the client untouched except by {@code writeValueAsString}. + * If a field had to be renamed, re-typed, or removed to make the client accept it, that work would + * be visible in this file, and SC-008 would be false.

+ * + *

Two WireMock roles share one server, separated by path prefix. The provider stubs sit at the + * root ({@code /chat/completions}) and stand in for OpenAI as dotCMS's upstream. The client-facing + * stubs sit under {@value #DOTCMS_FACING_PREFIX} and replay dotCMS's own answer back to the + * library. Reusing one server is deliberate: a second would need a second fixed port, and the port + * these tests already share is the thing most likely to collide.

+ */ +public class InferenceClientConformanceTest { + + /** Where the client-facing stubs live, so they cannot be confused with the provider stubs. */ + private static final String DOTCMS_FACING_PREFIX = "/dotcms-facing"; + + private static final String BASE_URL = + "http://localhost:" + AiTest.PORT + DOTCMS_FACING_PREFIX; + + private static final String CHAT_MODEL = "gpt-4o-mini"; + private static final String EMBEDDINGS_MODEL = AiTest.EMBEDDINGS_MODEL; + private static final String IMAGE_MODEL = AiTest.IMAGE_MODEL; + + private static final String PROMPT = "A cat in a hammock"; + private static final String ANSWER = "A cat is asleep in a hammock."; + + /** A 1x1 PNG, so the image assertions are about transport rather than about pixels. */ + private static final String IMAGE_BASE64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQ=="; + + /** What the stubbed provider answers dotCMS for a completion. */ + private static final String PROVIDER_CHAT_RESPONSE = """ + { + "id": "chatcmpl-conformance", + "object": "chat.completion", + "created": 1789000000, + "model": "%s", + "choices": [ + { + "index": 0, + "message": {"role": "assistant", "content": "%s"}, + "finish_reason": "stop" + } + ], + "usage": {"prompt_tokens": 11, "completion_tokens": 7, "total_tokens": 18} + } + """.formatted(CHAT_MODEL, ANSWER); + + /** What the stubbed provider answers dotCMS for a streamed completion. */ + private static final String PROVIDER_STREAM_RESPONSE = + "data: {\"id\":\"chatcmpl-conformance\",\"object\":\"chat.completion.chunk\"," + + "\"created\":1789000000,\"model\":\"" + CHAT_MODEL + "\",\"choices\":" + + "[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"content\":\"A cat \"}," + + "\"finish_reason\":null}]}\n\n" + + "data: {\"id\":\"chatcmpl-conformance\",\"object\":\"chat.completion.chunk\"," + + "\"created\":1789000000,\"model\":\"" + CHAT_MODEL + "\",\"choices\":" + + "[{\"index\":0,\"delta\":{\"content\":\"is asleep.\"}," + + "\"finish_reason\":null}]}\n\n" + + "data: {\"id\":\"chatcmpl-conformance\",\"object\":\"chat.completion.chunk\"," + + "\"created\":1789000000,\"model\":\"" + CHAT_MODEL + "\",\"choices\":" + + "[{\"index\":0,\"delta\":{},\"finish_reason\":\"stop\"}]}\n\n" + + "data: [DONE]\n\n"; + + private static final String PROVIDER_EMBEDDINGS_RESPONSE = """ + { + "object": "list", + "model": "%s", + "data": [ + {"object": "embedding", "index": 0, "embedding": [0.1, 0.2, 0.3]} + ], + "usage": {"prompt_tokens": 4, "total_tokens": 4} + } + """.formatted(EMBEDDINGS_MODEL); + + private static final String PROVIDER_IMAGE_RESPONSE = """ + { + "created": 1789000000, + "data": [{"b64_json": "%s", "revised_prompt": "A cat asleep in a hammock."}] + } + """.formatted(IMAGE_BASE64); + + /** The mapper the REST layer serializes responses with, so the bytes match what a caller gets. */ + private static final ObjectMapper MAPPER = DotObjectMapperProvider.createDefaultMapper(); + + private static WireMockServer wireMockServer; + private static User user; + private static String bearerToken; + + private Host host; + + private final ChatCompletionsResource chatResource = new ChatCompletionsResource(); + private final ModelsResource modelsResource = new ModelsResource(); + private final EmbeddingsResource embeddingsResource = new EmbeddingsResource(); + private final ImagesResource imagesResource = new ImagesResource(); + + @BeforeClass + public static void beforeClass() throws Exception { + IntegrationTestInitService.getInstance().init(); + IPUtils.disabledIpPrivateSubnet(true); + wireMockServer = AiTest.prepareWireMock(); + stubProvider(); + + user = new UserDataGen() + .roles(APILocator.getRoleAPI().loadBackEndUserRole(), + APILocator.getRoleAPI().loadCMSAdminRole()) + .nextPersisted(); + final ApiToken apiToken = APILocator.getApiTokenAPI().persistApiToken( + user.getUserId(), + Date.from(Instant.now().plus(Duration.ofDays(1))), + APILocator.systemUser().getUserId(), + "127.0.0.1"); + bearerToken = "Bearer " + APILocator.getApiTokenAPI().getJWT(apiToken, user); + } + + @AfterClass + public static void afterClass() { + wireMockServer.stop(); + IPUtils.disabledIpPrivateSubnet(false); + } + + @Before + public void before() throws Exception { + host = new SiteDataGen() + .name("inference-conformance-" + UUID.randomUUID() + ".dotcms.com") + .nextPersisted(); + AiTest.aiAppSecretsWithProviderConfig( + host, AiTest.providerConfigJson(AiTest.PORT, CHAT_MODEL)); + } + + @After + public void after() throws Exception { + AiTest.removeAiAppSecrets(host); + } + + /** + * Given a completion produced by dotCMS + * When an unmodified OpenAI client library reads it + * Then it deserializes into the library's own {@link ChatResponse}, carrying the answer, the + * finish reason and the token usage + * + *

SC-008 for the operation every agent framework drives first. The finish reason and usage + * are asserted alongside the text because they are what a framework acts on rather than + * displays: a client decides whether to continue a tool loop from the finish reason, and bills + * or budgets from the usage, so a payload that carries readable text and an unreadable finish + * reason is not usable even though it looks right.

+ */ + @Test + public void test_chatCompletion_deserializesIntoTheClientLibrarysOwnType() throws Exception { + final javax.ws.rs.core.Response dotcmsAnswer = chatResource.completions( + mockRequest("POST", bearerToken), mockResponse(), host.getIdentifier(), + new ChatCompletionRequestView(CHAT_MODEL, + List.of(new ChatCompletionRequestView.MessageView("user", PROMPT, null, null, null)), + null, null, null, null, null, null, null, null, null, null)); + assertEquals(200, dotcmsAnswer.getStatus()); + + replayToClient("/chat/completions", MAPPER.writeValueAsString(dotcmsAnswer.getEntity())); + + final ChatResponse parsed = OpenAiChatModel.builder() + .apiKey("not-used-by-the-stub") + .baseUrl(BASE_URL) + .modelName(CHAT_MODEL) + .maxRetries(0) + .build() + .chat(dev.langchain4j.model.chat.request.ChatRequest.builder() + .messages(UserMessage.from(PROMPT)) + .build()); + + assertNotNull("The client library produced no response at all", parsed); + assertNotNull(parsed.aiMessage()); + assertEquals("The answer text must survive the round trip into the library's own type", + ANSWER, parsed.aiMessage().text()); + assertNotNull("A client decides whether to keep going from the finish reason, so an " + + "unreadable one breaks tool loops even when the text is fine", + parsed.finishReason()); + assertNotNull("A client budgets from usage; it has to deserialize", parsed.tokenUsage()); + assertEquals(Integer.valueOf(11), parsed.tokenUsage().inputTokenCount()); + assertEquals(Integer.valueOf(7), parsed.tokenUsage().outputTokenCount()); + } + + /** + * Given a streamed completion produced by dotCMS, as the bytes a caller receives + * When an unmodified OpenAI client library consumes the event stream + * Then it reassembles the answer from the library's own streaming callbacks + * + *

SC-003 and SC-008 together, and the case most likely to break in a way no other test in + * this file would catch. Streaming is not JSON — it is a framing format, and a client can parse + * every individual chunk correctly while still failing on the frame boundaries, the terminal + * {@code [DONE]} sentinel, or a usage event whose {@code choices} array is empty. The assertion + * is on the text the library reassembled, because that is the only evidence that every frame + * was read in order and none was dropped.

+ */ + @Test + public void test_streamedCompletion_isReadableByTheClientLibrarysOwnStreamReader() + throws Exception { + final javax.ws.rs.core.Response dotcmsAnswer = chatResource.completions( + mockRequest("POST", bearerToken), mockResponse(), host.getIdentifier(), + new ChatCompletionRequestView(CHAT_MODEL, + List.of(new ChatCompletionRequestView.MessageView("user", PROMPT, null, null, null)), + null, null, null, Boolean.TRUE, null, null, null, null, null, null)); + assertEquals(200, dotcmsAnswer.getStatus()); + assertTrue("A streamed completion must come back as a stream, not a buffered body", + dotcmsAnswer.getEntity() instanceof StreamingOutput); + + final ByteArrayOutputStream captured = new ByteArrayOutputStream(); + ((StreamingOutput) dotcmsAnswer.getEntity()).write(captured); + final String sse = captured.toString(java.nio.charset.StandardCharsets.UTF_8); + + assertTrue("dotCMS must terminate the stream with the sentinel the format defines, or a " + + "client waits forever", sse.contains("data: [DONE]")); + + wireMockServer.stubFor(post(urlPathEqualTo(DOTCMS_FACING_PREFIX + "/chat/completions")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "text/event-stream") + .withBody(sse))); + + final StringBuilder reassembled = new StringBuilder(); + final java.util.concurrent.CompletableFuture done = + new java.util.concurrent.CompletableFuture<>(); + + dev.langchain4j.model.openai.OpenAiStreamingChatModel.builder() + .apiKey("not-used-by-the-stub") + .baseUrl(BASE_URL) + .modelName(CHAT_MODEL) + .build() + .chat(dev.langchain4j.model.chat.request.ChatRequest.builder() + .messages(UserMessage.from(PROMPT)) + .build(), + new dev.langchain4j.model.chat.response.StreamingChatResponseHandler() { + @Override + public void onPartialResponse(final String token) { + reassembled.append(token); + } + + @Override + public void onCompleteResponse(final ChatResponse response) { + done.complete(response); + } + + @Override + public void onError(final Throwable error) { + done.completeExceptionally(error); + } + }); + + final ChatResponse parsed = done.get(30, java.util.concurrent.TimeUnit.SECONDS); + + assertNotNull(parsed); + assertFalse("The library read the stream but reassembled nothing, which means the frames " + + "were not where it expected them", reassembled.toString().isBlank()); + assertEquals("Every frame must arrive, in order, exactly once", + "A cat is asleep.", reassembled.toString()); + } + + /** + * Given the model listing produced by dotCMS + * When an unmodified OpenAI client library reads it + * Then it deserializes into the library's own {@link ModelsListResponse} + * + *

SC-008 for the listing. This is the one operation with no high-level client API to drive, + * so the library's response type is used directly rather than through a model class — which is + * the same thing the library's own client does with the bytes once they arrive.

+ */ + @Test + public void test_modelListing_deserializesIntoTheClientLibrarysOwnType() throws Exception { + final javax.ws.rs.core.Response dotcmsAnswer = modelsResource.models( + mockRequest("GET", bearerToken), mockResponse(), host.getIdentifier()); + assertEquals(200, dotcmsAnswer.getStatus()); + + final String wire = MAPPER.writeValueAsString(dotcmsAnswer.getEntity()); + + // Configured the way an OpenAI client configures its own reader — snake_case, because the + // format spells fields owned_by rather than ownedBy, and tolerant of properties this + // version's type does not model. Both are the client's settings rather than a concession + // made for dotCMS: a reader that rejected unknown fields could not survive OpenAI adding + // one, so no real client is written that way, and pretending otherwise would fail this + // test on payloads every deployed client accepts. + final ObjectMapper clientMapper = new ObjectMapper() + .setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE) + .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + final ModelsListResponse parsed = clientMapper.readValue(wire, ModelsListResponse.class); + + assertNotNull(parsed); + assertEquals("The listing must announce itself as a list, which is what a client keys on", + "list", parsed.getObject()); + assertNotNull(parsed.getData()); + assertFalse("A site with a configured chat model must list at least that model", + parsed.getData().isEmpty()); + assertTrue("The configured model has to appear in the listing a caller picks from", + parsed.getData().stream().anyMatch(m -> CHAT_MODEL.equals(m.id()))); + } + + /** + * Given embeddings produced by dotCMS + * When an unmodified OpenAI client library reads them + * Then they deserialize into the library's own {@link Embedding} vectors + * + *

SC-008 for the operation that carries the most data and the least tolerance for a wrong + * type: an embedding is a list of numbers, and a payload that renders them as strings parses + * as JSON and fails in the client. The vector's contents are asserted, not merely its presence, + * for that reason.

+ */ + @Test + public void test_embeddings_deserializeIntoTheClientLibrarysOwnType() throws Exception { + final ArrayNode input = MAPPER.createArrayNode(); + input.add(new TextNode("The quick brown fox")); + + final javax.ws.rs.core.Response dotcmsAnswer = embeddingsResource.embeddings( + mockRequest("POST", bearerToken), mockResponse(), host.getIdentifier(), + new EmbeddingsRequestView(EMBEDDINGS_MODEL, input)); + assertEquals(200, dotcmsAnswer.getStatus()); + + replayToClient("/embeddings", MAPPER.writeValueAsString(dotcmsAnswer.getEntity())); + + final EmbeddingModel client = OpenAiEmbeddingModel.builder() + .apiKey("not-used-by-the-stub") + .baseUrl(BASE_URL) + .modelName(EMBEDDINGS_MODEL) + .maxRetries(0) + .build(); + + final Response parsed = client.embed("The quick brown fox"); + + assertNotNull(parsed); + assertNotNull("The client library produced no vector", parsed.content()); + assertEquals("The vector must arrive with the dimensions dotCMS sent, not truncated or " + + "padded by the client's own parsing", 3, parsed.content().dimension()); + assertEquals(0.1f, parsed.content().vector()[0], 0.0001f); + assertEquals(0.3f, parsed.content().vector()[2], 0.0001f); + } + + /** + * Given a generated image produced by dotCMS + * When an unmodified OpenAI client library reads it + * Then it deserializes into the library's own {@link Image}, carrying the base64 payload + * + *

SC-008 for images, and the place FR-012's inline-only rule meets a real client: the + * library populates either the URL or the base64 field depending on what arrived, so asserting + * that it found base64 is simultaneously a conformance check and a check that dotCMS never + * handed out a hosted artifact.

+ */ + @Test + public void test_generatedImage_deserializesIntoTheClientLibrarysOwnType() throws Exception { + final javax.ws.rs.core.Response dotcmsAnswer = imagesResource.generations( + mockRequest("POST", bearerToken), mockResponse(), host.getIdentifier(), + new ImageGenerationRequestView(IMAGE_MODEL, PROMPT, 1, AiTest.IMAGE_SIZE)); + assertEquals(200, dotcmsAnswer.getStatus()); + + replayToClient("/images/generations", MAPPER.writeValueAsString(dotcmsAnswer.getEntity())); + + final ImageModel client = OpenAiImageModel.builder() + .apiKey("not-used-by-the-stub") + .baseUrl(BASE_URL) + .modelName(IMAGE_MODEL) + .maxRetries(0) + .build(); + + final Response parsed = client.generate(PROMPT); + + assertNotNull(parsed); + assertNotNull("The client library produced no image", parsed.content()); + assertNotNull("FR-012 returns the bytes inline, so this is where a client finds them", + parsed.content().base64Data()); + assertFalse(parsed.content().base64Data().isBlank()); + assertNull("A client that received a URL instead would be holding a hosted artifact " + + "this family promises never to mint", parsed.content().url()); + } + + /** + * Serves dotCMS's own answer back to the client library, on the client-facing prefix. + * + * @param operationPath the path the client library appends to its base URL + * @param dotcmsPayload the bytes dotCMS produced, replayed verbatim + */ + private static void replayToClient(final String operationPath, final String dotcmsPayload) { + wireMockServer.stubFor(post(urlPathEqualTo(DOTCMS_FACING_PREFIX + operationPath)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(dotcmsPayload))); + } + + /** + * Stubs the upstream provider dotCMS itself calls, at the server root. + */ + private static void stubProvider() { + wireMockServer.stubFor(post(urlPathEqualTo("/chat/completions")) + .withRequestBody(com.github.tomakehurst.wiremock.client.WireMock + .matchingJsonPath("$[?(@.stream == true)]")) + .atPriority(1) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "text/event-stream") + .withBody(PROVIDER_STREAM_RESPONSE))); + + wireMockServer.stubFor(post(urlPathEqualTo("/chat/completions")) + .atPriority(2) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(PROVIDER_CHAT_RESPONSE))); + + wireMockServer.stubFor(post(urlPathEqualTo("/embeddings")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(PROVIDER_EMBEDDINGS_RESPONSE))); + + wireMockServer.stubFor(post(urlPathEqualTo("/images/generations")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(PROVIDER_IMAGE_RESPONSE))); + + wireMockServer.stubFor(get(urlPathEqualTo("/models")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("{\"object\":\"list\",\"data\":[]}"))); + } + + private static HttpServletRequest mockRequest(final String method, final String credential) { + final HttpServletRequest request = mock(HttpServletRequest.class); + final Map attributes = new HashMap<>(); + final String uri = "/api/inference/v1/conformance"; + + when(request.getRequestURI()).thenReturn(uri); + when(request.getRequestURL()) + .thenReturn(new StringBuffer("http://localhost" + uri)); + when(request.getMethod()).thenReturn(method); + when(request.getRemoteAddr()).thenReturn("127.0.0.1"); + when(request.getServerName()).thenReturn("localhost"); + when(request.getHeader("Authorization")).thenReturn(credential); + + doAnswer(invocation -> attributes.put(invocation.getArgument(0), invocation.getArgument(1))) + .when(request).setAttribute(anyString(), any()); + when(request.getAttribute(anyString())) + .thenAnswer(invocation -> attributes.get(invocation.getArgument(0))); + + return request; + } + + private static HttpServletResponse mockResponse() { + return mock(HttpServletResponse.class); + } +} diff --git a/specs/37431-openai-compatible-inference/spec.md b/specs/37431-openai-compatible-inference/spec.md index b1c73299462c..3535eaa05e96 100644 --- a/specs/37431-openai-compatible-inference/spec.md +++ b/specs/37431-openai-compatible-inference/spec.md @@ -165,7 +165,7 @@ The same client credentials and base URL also serve text embeddings and image ge #### Operational and forward compatibility - **FR-030**: The system MUST NOT emit cross-origin headers on this family, and the endpoints MUST be documented as server-side only, because the credential is a long-lived token with the full authority of its owner and cross-origin support would invite placing it in browser JavaScript. -- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code** — 429 for rate limiting, 5xx for upstream failure — because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. The response body MUST stay conformant to the standard error shape. When the provider supplies a `Retry-After` header, the system MUST relay it, so a client backs off on the provider's own instruction rather than on a guess — the format's clients already honour that header, and discarding it makes every back-off worse than it needs to be. +- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code** — 429 for rate limiting, 5xx for upstream failure — because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. The response body MUST stay conformant to the standard error shape. When the provider supplies a `Retry-After` header, the system MUST relay it, so a client backs off on the provider's own instruction rather than on a guess — the format's clients already honour that header, and discarding it makes every back-off worse than it needs to be. **Unmet as of 2026-09-15, and recorded rather than quietly dropped.** The status half is implemented: a provider rate limit becomes a 429 in the standard rate-limit shape, anything else upstream stays 502, and the cause chain is walked because the fallback chain wraps. The header half is **not implementable through the current provider abstraction**: the client library surfaces a failed call as an exception carrying a status code and a message and nothing else — it exposes no response headers on the error path — so `Retry-After` is discarded before dotCMS can see it. Meeting this requirement needs either a custom HTTP client underneath the provider abstraction or an upstream change; until one exists, a client backs off on the status alone. This is the requirement to revisit first if throttling proves painful in practice. - **FR-032**: Each new path MUST register a request cost in the band used for operations that make a remote network round trip, so the existing instance-wide rate-limit backstop prices them correctly. A per-site or per-token AI spend quota is out of scope. - **FR-033**: No existing dotAI endpoint's behavior may change. Retrieval-augmented completions, semantic search, the vector corpus operations, and provider administration remain first-class, as no standard verb covers them. - **FR-034**: The three existing operations superseded by this family — text generation, image generation, and raw-prompt completion — MUST be documented as superseded while remaining fully functional. From 3355bd4f9179fa22024334d2a13b90ee3a327950 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Tue, 15 Sep 2026 12:14:36 -0600 Subject: [PATCH 04/15] feat(ai): send Retry-After on dotCMS's own 429, amend FR-031 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FR-031 required relaying the provider's Retry-After. It cannot be met as the code is wired: the client library surfaces a failed call as an exception carrying a status code and a message, discarding response headers on the error path, so the value never reaches dotCMS. Relaying it needs a custom HTTP client injected through the library's supported extension point — bounded work, but work that takes ownership of timeouts, proxying and multipart for every provider call, so it belongs in its own change with its own review. The requirement is split into two clauses that are each true. Where the provider throttles, relay its header where the provider abstraction exposes it — it does not today, and that is recorded rather than dropped. Where dotCMS refuses on its own capacity, send Retry-After, because that wait is one dotCMS actually knows. The second half is implemented here. The streaming ceiling's refusal said "retry shortly" in a prose message no program can act on; it now carries Retry-After: 5. Five seconds is a judgement, not a measurement, and the comment says so: long enough that refused callers do not all return at once and hold the node at the capacity it was shedding, short enough that freed capacity is not left idle. Deliberately not a config key — the ceiling is configurable, this only says how long to wait for it. The ceiling itself had no test at all. The new one drives the limit to zero through configuration rather than opening fifty real streams, which would be slow, machine-dependent, and would test the thread pool rather than the refusal. Tests: 67 unit, 78 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- docs/backend/INFERENCE_API.md | 4 +- .../rest/ChatCompletionsResource.java | 42 ++++++++++++++- .../rest/ChatCompletionsStreamingTest.java | 54 +++++++++++++++++++ .../37431-openai-compatible-inference/spec.md | 2 +- 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/docs/backend/INFERENCE_API.md b/docs/backend/INFERENCE_API.md index c1464f30b05e..56febb2fd31c 100644 --- a/docs/backend/INFERENCE_API.md +++ b/docs/backend/INFERENCE_API.md @@ -285,10 +285,10 @@ Every refusal uses the standard OpenAI error envelope, so a client library deser | `403` | `invalid_request_error` | An explicit site override naming a site the caller cannot read | | `404` | `invalid_request_error` | A model the resolved site has not configured for that capability | | `413` | `invalid_request_error` | Request body over `DOT_INFERENCE_MAX_REQUEST_BYTES` | -| `429` | `rate_limit_error` | The node is already serving its ceiling of concurrent streamed completions, **or** the model provider is rate limiting this site | +| `429` | `rate_limit_error` | The node is already serving its ceiling of concurrent streamed completions (carries `Retry-After`), **or** the model provider is rate limiting this site (no `Retry-After` — see below) | | `502` | `api_error` | A genuine upstream provider failure | -A `502` carries a fixed, safe message; the provider's own message is logged and never returned, because it can carry the provider's endpoint, account identifiers, or a fragment of the prompt. Retryability is carried by the status alone — the standard error shape has no retryable field, and adding one would break the no-adapter promise these endpoints exist to keep — so a provider rate limit is reported as `429` rather than folded into `502`: the two call for different back-off, and a client that reads `502` treats a busy provider as a broken one. **Known limitation**: when a provider sends a `Retry-After` header, dotCMS cannot currently relay it. The client library underneath surfaces a failed call as an exception carrying a status code and a message only, discarding response headers, so the value never reaches dotCMS. A client backs off on the status alone until that changes. A site with no usable configuration for the requested capability lands on the same `404` as an unknown model, so a caller cannot use the distinction to learn which sites have dotAI set up. +A `502` carries a fixed, safe message; the provider's own message is logged and never returned, because it can carry the provider's endpoint, account identifiers, or a fragment of the prompt. Retryability is carried by the status alone — the standard error shape has no retryable field, and adding one would break the no-adapter promise these endpoints exist to keep — so a provider rate limit is reported as `429` rather than folded into `502`: the two call for different back-off, and a client that reads `502` treats a busy provider as a broken one. The two `429`s differ in one way that matters to a client. When dotCMS refuses because **this node** is at its streaming ceiling, the response carries `Retry-After: 5` — that is a wait dotCMS knows, so it states it in the form every standard client already honours rather than only in prose. When the refusal comes from the **provider**, no `Retry-After` is sent: the provider knows how long it wants to be left alone and says so in a header the client library discards on its error path, so the value never reaches dotCMS, and inventing one would present a guess to the client as an instruction. Relaying the provider's value needs a custom HTTP client under the provider abstraction and is tracked as follow-up work; until then a client backs off on the status alone for upstream throttling. A site with no usable configuration for the requested capability lands on the same `404` as an unknown model, so a caller cannot use the distinction to learn which sites have dotAI set up. Only exchange **metadata** is recorded — resolved site, model, token counts, status, duration. Request and response bodies are never written to a log or a durable store, and no conversation is retained after the response is delivered; the family is stateless. diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index dbdb09fe9470..9a5338e7634e 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -111,6 +111,21 @@ public class ChatCompletionsResource { private static final String SITE_HEADER = "X-dotCMS-Site"; /** Prefix the completion id carries, as standard clients expect. */ + /** The header a standard client reads to learn how long to wait before retrying. */ + private static final String RETRY_AFTER_HEADER = "Retry-After"; + + /** + * How long to tell a caller to wait when this node is at its streaming ceiling. + * + *

Five seconds, which is a judgement rather than a measurement: a streamed completion runs + * for seconds to minutes, so no single number is right for every deployment. It is chosen to be + * long enough that a refused client does not immediately return and re-refuse — turning one + * ceiling into a retry storm that keeps the node at capacity — and short enough that capacity + * freed a moment later does not sit idle. It is not a configurable limit because it is not a + * limit: the ceiling itself is configurable, and this only says how long to wait for it.

+ */ + private static final int CAPACITY_RETRY_AFTER_SECONDS = 5; + private static final String COMPLETION_ID_PREFIX = "chatcmpl-"; /** What a caller is told when the provider failed; never the provider's own words. */ @@ -314,12 +329,17 @@ private Response streamCompletion(final ResolvedAiContext context, ACTIVE_STREAMS.decrementAndGet(); Logger.warn(this, "Refusing a streamed completion: the node is already serving its " + "ceiling of " + maxConcurrentStreams + " concurrent streams"); + // Carries Retry-After because this refusal is dotCMS's own: the wait is until a + // stream on this node finishes, not until some provider decides to let us back in. + // "Retry shortly" in the message is prose a client cannot act on; the header is the + // same instruction in the form every standard client already honours. return errorResponse(new InferenceError( "rate_limit_error", "This node is already serving its ceiling of " + maxConcurrentStreams + " concurrent streamed completions; retry shortly", null, - 429)); + 429), + CAPACITY_RETRY_AFTER_SECONDS); } // Fixed for the whole stream: a client correlates the chunks it stitches together by the @@ -452,6 +472,26 @@ private static Response errorResponse(final InferenceError error) { .build(); } + /** + * Renders a refusal that tells the caller when to come back. + * + *

Only for refusals whose duration dotCMS actually knows — its own capacity ceilings. An + * upstream rate limit is not one of those: the provider knows how long it wants to be left + * alone and says so in a header the provider abstraction discards before dotCMS sees it, and + * inventing a number there would be a guess presented to the client as an instruction.

+ * + * @param error the refusal + * @param retryAfterSeconds how long the caller should wait before trying again + * @return the refusal, carrying {@code Retry-After} + */ + private static Response errorResponse(final InferenceError error, final int retryAfterSeconds) { + return Response.status(error.httpStatus()) + .entity(InferenceErrorView.of(error)) + .type(MediaType.APPLICATION_JSON) + .header(RETRY_AFTER_HEADER, retryAfterSeconds) + .build(); + } + /** * Writes stream events out as server-sent event frames, and decides how the stream ends. * diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java index af77bf124bd6..559108c108ea 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java @@ -6,12 +6,15 @@ import com.dotcms.datagen.UserDataGen; import com.dotcms.inference.rest.mapper.SseSerializer; import com.dotcms.inference.rest.view.ChatCompletionRequestView; +import com.dotcms.inference.rest.view.InferenceErrorView; +import com.dotcms.inference.model.InferenceLimits; import com.dotcms.inference.rest.view.ChatCompletionRequestView.MessageView; import com.dotcms.inference.rest.view.ChatCompletionRequestView.StreamOptionsView; import com.dotcms.util.IntegrationTestInitService; import com.dotcms.util.network.IPUtils; import com.dotmarketing.beans.Host; import com.dotmarketing.business.APILocator; +import com.dotmarketing.util.Config; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.github.tomakehurst.wiremock.WireMockServer; @@ -512,6 +515,57 @@ private static void stubProviderStreamVolunteeringUsage() { * @param streamOptions the streaming options to send, or null to send none * @return a one-turn conversation asking for a streamed answer */ + /** + * Given a node already at its ceiling of concurrent streamed completions + * When another streamed completion is asked for + * Then it is refused with a 429 that carries {@code Retry-After}, and nothing reaches the + * provider + * + *

FR-037 for the ceiling itself, which had no coverage at all until now, and FR-031 for the + * header. The two belong in one test because the ceiling is only half an answer without it: a + * refusal that says "retry shortly" in a prose message tells a program nothing, and a client + * that cannot read a wait interval invents one — which under load means every refused caller + * returning at once and holding the node at capacity it was trying to shed.

+ * + *

The ceiling is driven to zero rather than fifty streams being opened. Fifty real streams + * would make the test slow, machine-dependent and flaky, and would test the thread pool rather + * than the refusal; the limit is read from configuration on every request precisely so it can + * be changed without a restart, and that is the seam used here.

+ */ + @Test + public void test_atTheStreamCeiling_refusesWith429AndRetryAfter() { + final String previous = Config.getStringProperty( + InferenceLimits.MAX_CONCURRENT_STREAMS_KEY, null); + Config.setProperty(InferenceLimits.MAX_CONCURRENT_STREAMS_KEY, "0"); + try { + final Response response = resource.completions( + mockRequest(), mockResponse(), host.getIdentifier(), streamingRequest(null)); + + assertNotNull(response); + assertEquals("A node at its ceiling refuses rather than queues, so the caller can go " + + "elsewhere instead of waiting on a thread that will not free up", + 429, response.getStatus()); + + assertEquals("The caller has to be told how long to wait in a form it can act on, not " + + "only in prose it cannot parse", + String.valueOf(5), response.getHeaderString("Retry-After")); + + assertTrue(response.getEntity() instanceof InferenceErrorView); + final InferenceErrorView.Body error = + ((InferenceErrorView) response.getEntity()).error(); + assertNotNull(error); + assertEquals("rate_limit_error", error.type()); + + wireMockServer.verify(0, postRequestedFor(urlPathEqualTo(COMPLETIONS_PATH))); + } finally { + if (previous == null) { + Config.setProperty(InferenceLimits.MAX_CONCURRENT_STREAMS_KEY, null); + } else { + Config.setProperty(InferenceLimits.MAX_CONCURRENT_STREAMS_KEY, previous); + } + } + } + private static ChatCompletionRequestView streamingRequest(final StreamOptionsView streamOptions) { return new ChatCompletionRequestView( CHAT_MODEL, diff --git a/specs/37431-openai-compatible-inference/spec.md b/specs/37431-openai-compatible-inference/spec.md index 3535eaa05e96..043ee3b8e314 100644 --- a/specs/37431-openai-compatible-inference/spec.md +++ b/specs/37431-openai-compatible-inference/spec.md @@ -165,7 +165,7 @@ The same client credentials and base URL also serve text embeddings and image ge #### Operational and forward compatibility - **FR-030**: The system MUST NOT emit cross-origin headers on this family, and the endpoints MUST be documented as server-side only, because the credential is a long-lived token with the full authority of its owner and cross-origin support would invite placing it in browser JavaScript. -- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code** — 429 for rate limiting, 5xx for upstream failure — because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. The response body MUST stay conformant to the standard error shape. When the provider supplies a `Retry-After` header, the system MUST relay it, so a client backs off on the provider's own instruction rather than on a guess — the format's clients already honour that header, and discarding it makes every back-off worse than it needs to be. **Unmet as of 2026-09-15, and recorded rather than quietly dropped.** The status half is implemented: a provider rate limit becomes a 429 in the standard rate-limit shape, anything else upstream stays 502, and the cause chain is walked because the fallback chain wraps. The header half is **not implementable through the current provider abstraction**: the client library surfaces a failed call as an exception carrying a status code and a message and nothing else — it exposes no response headers on the error path — so `Retry-After` is discarded before dotCMS can see it. Meeting this requirement needs either a custom HTTP client underneath the provider abstraction or an upstream change; until one exists, a client backs off on the status alone. This is the requirement to revisit first if throttling proves painful in practice. +- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code** — 429 for rate limiting, 5xx for upstream failure — because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. The response body MUST stay conformant to the standard error shape. Where the system refuses on its own capacity rather than the provider's, it MUST send a `Retry-After` header, because that refusal is one whose duration dotCMS actually knows — and a client told to "retry shortly" in prose it cannot parse is a client that guesses. Where the **provider** supplies a `Retry-After`, the system MUST relay it **where the provider abstraction exposes it**. **It currently does not, and that is recorded rather than quietly dropped** (amended 2026-09-15). The client library surfaces a failed call as an exception carrying a status code and a message and nothing else; response headers are read and discarded on the error path, so the value never reaches dotCMS. Relaying it needs a custom HTTP client injected through the library's supported extension point — bounded work, not a dead end, but work that takes ownership of timeouts, proxying and multipart for every provider call and so belongs in its own change with its own review, not at the tail of this one. Until then a client backs off on the status alone, which every standard client already implements as its default. Revisit this first if throttling proves painful in practice. - **FR-032**: Each new path MUST register a request cost in the band used for operations that make a remote network round trip, so the existing instance-wide rate-limit backstop prices them correctly. A per-site or per-token AI spend quota is out of scope. - **FR-033**: No existing dotAI endpoint's behavior may change. Retrieval-augmented completions, semantic search, the vector corpus operations, and provider administration remain first-class, as no standard verb covers them. - **FR-034**: The three existing operations superseded by this family — text generation, image generation, and raw-prompt completion — MUST be documented as superseded while remaining fully functional. From ce5201b55c09998c295cf706a85a3122aa89f391 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Tue, 15 Sep 2026 12:42:50 -0600 Subject: [PATCH 05/15] fix(ai): bound undeclared-length request bodies, anchor metadata-only logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the three gaps /speckit-converge found. T093 — FR-037's request-size ceiling could be skipped entirely. RequestSizeLimitFilter returned early whenever a request declared no Content-Length, commenting that "the resource enforces the ceiling as it reads". No resource did, and there was no size check anywhere outside that filter, so the single control on body size was defeated by omitting one header — which several HTTP clients do by default when they stream a body. The comment was the worse half of the defect: it told every later reader the case was handled. The entity stream is now replaced with one that counts bytes and refuses past the ceiling, raising the same typed 413 the declared path returns, so a caller cannot tell which route refused it. Counting on the way through rather than buffering to measure is the point: buffering the body to size it hands over exactly the memory the ceiling exists to bound. Four tests, including that a valid undeclared-length body still arrives byte for byte, since every streamed request now passes through this stream. T094 — FR-036 was satisfied but unenforced. No request or response body was logged, and nothing held that line; a debug line echoing a request would have broken it without failing any test. A sentinel is planted in the prompt and the embeddings input, every record emitted anywhere in the JVM is captured through a root-logger appender, and the test fails if it appears — on the failure paths too, which is where the request that was rejected is the natural thing to log. The captured throwable chain is searched as well, since a safe message with an unsafe cause leaks just as well. The negative assertion was verified to fail: a Logger.info echoing the request was injected, all three chat cases failed with the prompt quoted back, and it was reverted. The test also fails when nothing is logged at all, so it cannot quietly stop testing anything. T092 — the inverted Test-First gates — is untouched. It needs a developer's sign-off or an explicitly accepted deviation, and cannot be closed by running anything. Tests: 71 unit, 83 integration across twelve classes, all green. Co-Authored-By: Claude Opus 5 (1M context) --- .../rest/RequestSizeLimitFilter.java | 86 ++++- .../inference/RequestSizeLimitFilterTest.java | 165 +++++++++ .../src/test/java/com/dotcms/MainSuite2b.java | 2 + .../inference/rest/InferenceLoggingTest.java | 347 ++++++++++++++++++ 4 files changed, 598 insertions(+), 2 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/RequestSizeLimitFilter.java b/dotCMS/src/main/java/com/dotcms/inference/rest/RequestSizeLimitFilter.java index 19f7f97dafa4..993cfcffee60 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/RequestSizeLimitFilter.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/RequestSizeLimitFilter.java @@ -9,7 +9,10 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.ext.Provider; +import javax.ws.rs.WebApplicationException; +import java.io.FilterInputStream; import java.io.IOException; +import java.io.InputStream; /** * Refuses a request body larger than the configured ceiling, before it is read. @@ -31,11 +34,22 @@ public class RequestSizeLimitFilter implements ContainerRequestFilter { @Override public void filter(final ContainerRequestContext requestContext) throws IOException { final int declaredLength = requestContext.getLength(); + final InferenceLimits limits = InferenceLimits.current(); + if (declaredLength < 0) { - // Chunked or unknown length; the resource enforces the ceiling as it reads. + // Chunked, or a client that simply did not declare a length — which several HTTP + // clients do by default when they stream a body. There is no number to check here, + // so the ceiling is enforced on the way through instead: the entity stream is + // replaced with one that counts bytes and refuses past the limit. An earlier version + // returned at this point, with a comment claiming the resource enforced the ceiling + // as it read. No resource did, which left the single control on body size skippable + // by omitting one header. + requestContext.setEntityStream( + new CeilingEnforcingStream(requestContext.getEntityStream(), + limits.maxRequestBytes())); return; } - final InferenceLimits limits = InferenceLimits.current(); + if (limits.exceedsMaxRequestBytes(declaredLength)) { final InferenceError error = new InferenceError("invalid_request_error", "Request body of " + declaredLength + " bytes exceeds the maximum of " @@ -48,4 +62,72 @@ public void filter(final ContainerRequestContext requestContext) throws IOExcept .build()); } } + + /** + * Builds the refusal sent when a body exceeds the ceiling. + * + * @param maxRequestBytes the ceiling in force + * @return the 413, in the standard error shape + */ + static Response tooLarge(final int maxRequestBytes) { + final InferenceError error = new InferenceError("invalid_request_error", + "Request body exceeds the maximum of " + maxRequestBytes + " bytes", null, + Response.Status.REQUEST_ENTITY_TOO_LARGE.getStatusCode()); + return Response.status(error.httpStatus()) + .entity(InferenceErrorView.of(error)) + .type(MediaType.APPLICATION_JSON) + .build(); + } + + /** + * An entity stream that refuses to yield more than the ceiling allows. + * + *

Used when the request declares no length. Counting on the way through is the only way to + * bound a body whose size is not known until it ends, and refusing mid-read is the point: the + * alternative is to buffer the whole thing to measure it, which hands an attacker exactly the + * memory cost the ceiling exists to prevent.

+ * + *

The refusal is a {@link WebApplicationException} carrying the same typed 413 the declared + * path returns, so a caller cannot tell which route refused it — and neither can a client + * library, which is the point of a contract.

+ */ + static final class CeilingEnforcingStream extends FilterInputStream { + + private final int maxRequestBytes; + private long seen; + + CeilingEnforcingStream(final InputStream delegate, final int maxRequestBytes) { + super(delegate); + this.maxRequestBytes = maxRequestBytes; + } + + @Override + public int read() throws IOException { + final int value = super.read(); + if (value != -1) { + count(1); + } + return value; + } + + @Override + public int read(final byte[] buffer, final int offset, final int length) + throws IOException { + final int read = super.read(buffer, offset, length); + if (read > 0) { + count(read); + } + return read; + } + + /** + * @param justRead how many bytes the last read produced + */ + private void count(final int justRead) { + seen += justRead; + if (seen > maxRequestBytes) { + throw new WebApplicationException(tooLarge(maxRequestBytes)); + } + } + } } diff --git a/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java b/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java new file mode 100644 index 000000000000..27dfd0106e02 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java @@ -0,0 +1,165 @@ +package com.dotcms.inference; + +import com.dotcms.inference.model.InferenceLimits; +import com.dotcms.inference.rest.RequestSizeLimitFilter; +import com.dotcms.inference.rest.view.InferenceErrorView; +import com.dotmarketing.util.Config; +import org.junit.After; +import org.junit.Test; + +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.Response; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Covers FR-037's request-size ceiling, including the case that used to skip it entirely. + * + *

A body that declares its length is easy: the filter compares one number. A body that does + * not — chunked, or simply streamed by a client that omits {@code Content-Length}, which several + * HTTP clients do by default — has no number to compare, and an earlier version of the filter + * returned at that point with a comment saying the resource enforced the ceiling as it read. No + * resource did. The single control on body size could be skipped by leaving out one header, and + * the comment is what kept anyone from noticing.

+ * + *

These tests exist to keep that from coming back: the undeclared path is exercised here in + * exactly the way the declared one is, so the two cannot drift apart again.

+ */ +public class RequestSizeLimitFilterTest { + + private final RequestSizeLimitFilter filter = new RequestSizeLimitFilter(); + + @After + public void after() { + Config.setProperty(InferenceLimits.MAX_REQUEST_BYTES_KEY, null); + } + + /** + * Given a request that declares a length over the ceiling + * When the filter runs + * Then it is refused with a typed 413 before the body is read at all + */ + @Test + public void test_declaredOverSizeBody_isRefusedBeforeItIsRead() throws IOException { + Config.setProperty(InferenceLimits.MAX_REQUEST_BYTES_KEY, "10"); + + final ContainerRequestContext context = mock(ContainerRequestContext.class); + when(context.getLength()).thenReturn(64); + final AtomicReference aborted = new AtomicReference<>(); + doAnswer(invocation -> { + aborted.set(invocation.getArgument(0)); + return null; + }).when(context).abortWith(any()); + + filter.filter(context); + + assertNotNull("A body that announces it is too large must be refused on the announcement, " + + "not after it has been read into memory", aborted.get()); + assertEquals(413, aborted.get().getStatus()); + assertTrue(aborted.get().getEntity() instanceof InferenceErrorView); + } + + /** + * Given a request that declares no length and stays under the ceiling + * When its body is read + * Then it is delivered untouched + * + *

The ordinary case, and the one a wrapper most easily breaks: every streamed request with + * no {@code Content-Length} now passes through this stream, so a bug here would refuse valid + * traffic rather than merely fail to refuse invalid traffic.

+ */ + @Test + public void test_undeclaredLengthUnderTheCeiling_passesThroughIntact() throws IOException { + Config.setProperty(InferenceLimits.MAX_REQUEST_BYTES_KEY, "1024"); + + final byte[] body = "{\"model\":\"gpt-4o\"}".getBytes(); + final InputStream wrapped = wrapUndeclared(new ByteArrayInputStream(body)); + + assertEquals("The body must arrive byte for byte", new String(body), + new String(wrapped.readAllBytes())); + } + + /** + * Given a request that declares no length and runs past the ceiling + * When its body is read + * Then it is refused mid-read with the same typed 413 the declared path returns + * + *

This is the hole. Before the fix the same request was accepted in full, because the + * filter had no length to check and nothing downstream checked either. Refusing mid-read + * rather than measuring first is deliberate: buffering the body to size it would hand over + * exactly the memory the ceiling exists to bound.

+ */ + @Test + public void test_undeclaredLengthOverTheCeiling_isRefusedMidRead() throws IOException { + Config.setProperty(InferenceLimits.MAX_REQUEST_BYTES_KEY, "16"); + + final InputStream wrapped = wrapUndeclared(new ByteArrayInputStream(new byte[4096])); + + final WebApplicationException refused = + assertThrows("A body with no declared length must still be bounded", + WebApplicationException.class, wrapped::readAllBytes); + + assertEquals("The refusal must be the same 413 a declared over-size body gets, so a " + + "client cannot tell which route refused it", + 413, refused.getResponse().getStatus()); + assertTrue(refused.getResponse().getEntity() instanceof InferenceErrorView); + } + + /** + * Given a request that declares no length + * When the filter runs + * Then the request is not aborted outright — it is allowed to proceed under a counted stream + * + *

Refusing every undeclared-length body would also close the hole, and would break every + * client that streams its request. The fix has to bound the body without rejecting the shape.

+ */ + @Test + public void test_undeclaredLength_isNotRefusedOutright() throws IOException { + Config.setProperty(InferenceLimits.MAX_REQUEST_BYTES_KEY, "1024"); + + final ContainerRequestContext context = mock(ContainerRequestContext.class); + when(context.getLength()).thenReturn(-1); + when(context.getEntityStream()).thenReturn(new ByteArrayInputStream(new byte[8])); + + filter.filter(context); + + verify(context, never()).abortWith(any()); + verify(context).setEntityStream(any()); + } + + /** + * @param body the raw body an undeclared-length request would carry + * @return the stream the filter installs in place of it + */ + private InputStream wrapUndeclared(final InputStream body) throws IOException { + final ContainerRequestContext context = mock(ContainerRequestContext.class); + when(context.getLength()).thenReturn(-1); + when(context.getEntityStream()).thenReturn(body); + + final AtomicReference installed = new AtomicReference<>(); + doAnswer(invocation -> { + installed.set(invocation.getArgument(0)); + return null; + }).when(context).setEntityStream(any()); + + filter.filter(context); + + assertNotNull("The filter must install a counting stream when no length is declared", + installed.get()); + return installed.get(); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index 908a21ba415f..7c08609ea6d1 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -74,6 +74,7 @@ import com.dotcms.inference.rest.InferenceEmbeddingsTest; import com.dotcms.inference.rest.InferenceFallbackTest; import com.dotcms.inference.rest.InferenceImagesTest; +import com.dotcms.inference.rest.InferenceLoggingTest; import com.dotcms.inference.rest.InferenceModelsTest; import com.dotcms.inference.rest.InferenceModelValidationTest; import com.dotcms.inference.rest.InferenceSiteIsolationTest; @@ -462,6 +463,7 @@ InferenceEmbeddingsTest.class, InferenceFallbackTest.class, InferenceImagesTest.class, + InferenceLoggingTest.class, InferenceModelsTest.class, InferenceModelValidationTest.class, InferenceSiteIsolationTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java new file mode 100644 index 000000000000..f465d91685ef --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java @@ -0,0 +1,347 @@ +package com.dotcms.inference.rest; + +import com.dotcms.ai.AiTest; +import com.dotcms.auth.providers.jwt.beans.ApiToken; +import com.dotcms.datagen.SiteDataGen; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.inference.rest.view.ChatCompletionRequestView; +import com.dotcms.inference.rest.view.EmbeddingsRequestView; +import com.dotcms.inference.rest.view.ImageGenerationRequestView; +import com.dotcms.util.IntegrationTestInitService; +import com.dotcms.util.network.IPUtils; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.TextNode; +import com.github.tomakehurst.wiremock.WireMockServer; +import com.liferay.portal.model.User; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; +import java.io.ByteArrayOutputStream; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.post; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Holds FR-036's line: metadata may be logged, the caller's content may not. + * + *

No request or response body is written to a log today. That was true before this test + * existed and is exactly why the test is worth having — nothing enforced it. Every log line in + * this family carries identifiers, sizes, model names and timings, and a contributor adding + * {@code Logger.debug(this, "request: " + requestView)} while chasing a bug would break the + * requirement without failing a single other test in the suite. The leak would then sit in the + * logs of every deployment until someone read one.

+ * + *

The method is a sentinel: a string that could only have come from the caller's content is + * planted in the prompt and the embeddings input, every log record emitted anywhere in the JVM + * during the call is captured, and the test fails if the sentinel appears in any of them. Because + * the appender is attached to the root logger rather than to a named one, a future log statement + * in a class nobody thought of is covered too.

+ * + *

The failure paths are exercised alongside the happy ones, because that is where content + * leaks into logs in practice: the natural thing to write when a provider rejects a request is + * the request that was rejected.

+ */ +public class InferenceLoggingTest { + + /** + * Planted in the caller's content. Nonsense on purpose — a realistic prompt could plausibly + * appear in a log for some unrelated reason, and the assertion has to mean one thing. + */ + private static final String SENTINEL = "zqx-carrier-pigeon-7741-confidential"; + + private static final String CHAT_MODEL = "gpt-4o-mini"; + private static final String COMPLETIONS_PATH = "/chat/completions"; + private static final String EMBEDDINGS_PATH = "/embeddings"; + private static final String IMAGES_PATH = "/images/generations"; + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static WireMockServer wireMockServer; + private static User user; + private static String bearerToken; + + private Host host; + private CapturingAppender appender; + private Logger rootLogger; + + private final ChatCompletionsResource chatResource = new ChatCompletionsResource(); + private final EmbeddingsResource embeddingsResource = new EmbeddingsResource(); + private final ImagesResource imagesResource = new ImagesResource(); + + @BeforeClass + public static void beforeClass() throws Exception { + IntegrationTestInitService.getInstance().init(); + IPUtils.disabledIpPrivateSubnet(true); + wireMockServer = AiTest.prepareWireMock(); + + user = new UserDataGen() + .roles(APILocator.getRoleAPI().loadBackEndUserRole(), + APILocator.getRoleAPI().loadCMSAdminRole()) + .nextPersisted(); + final ApiToken apiToken = APILocator.getApiTokenAPI().persistApiToken( + user.getUserId(), + Date.from(Instant.now().plus(Duration.ofDays(1))), + APILocator.systemUser().getUserId(), + "127.0.0.1"); + bearerToken = "Bearer " + APILocator.getApiTokenAPI().getJWT(apiToken, user); + } + + @AfterClass + public static void afterClass() { + wireMockServer.stop(); + IPUtils.disabledIpPrivateSubnet(false); + } + + @Before + public void before() throws Exception { + host = new SiteDataGen() + .name("inference-logging-" + UUID.randomUUID() + ".dotcms.com") + .nextPersisted(); + AiTest.aiAppSecretsWithProviderConfig( + host, AiTest.providerConfigJson(AiTest.PORT, CHAT_MODEL)); + + appender = new CapturingAppender(); + appender.start(); + rootLogger = (Logger) LogManager.getRootLogger(); + rootLogger.addAppender(appender); + } + + @After + public void after() throws Exception { + if (rootLogger != null && appender != null) { + rootLogger.removeAppender(appender); + appender.stop(); + } + AiTest.removeAiAppSecrets(host); + } + + /** + * Given a completion whose prompt carries a sentinel, served successfully + * When the exchange finishes + * Then the sentinel appears in no log record + */ + @Test + public void test_completion_doesNotLogThePrompt() { + stubProvider(COMPLETIONS_PATH, 200, providerChatResponse()); + + chatResource.completions(mockRequest(), mockResponse(), host.getIdentifier(), + chatRequest(false)); + + assertSentinelNeverLogged("a completion that succeeded"); + } + + /** + * Given a streamed completion whose prompt carries a sentinel + * When the stream is consumed to its end + * Then the sentinel appears in no log record + * + *

Separated from the buffered case because the streaming path logs on its own threads and + * has its own failure and timeout reporting, none of which the buffered path exercises.

+ */ + @Test + public void test_streamedCompletion_doesNotLogThePrompt() throws Exception { + stubProvider(COMPLETIONS_PATH, 200, providerChatResponse()); + + final Response response = chatResource.completions( + mockRequest(), mockResponse(), host.getIdentifier(), chatRequest(true)); + if (response.getEntity() instanceof StreamingOutput streaming) { + streaming.write(new ByteArrayOutputStream()); + } + + assertSentinelNeverLogged("a streamed completion"); + } + + /** + * Given a completion the provider refuses + * When the failure is reported + * Then the sentinel appears in no log record + * + *

The case that matters most. When a provider rejects a request the obvious thing to log + * is the request that was rejected, so this is where a prompt reaches a log file first.

+ */ + @Test + public void test_failedCompletion_doesNotLogThePrompt() { + stubProvider(COMPLETIONS_PATH, 500, "{\"error\":{\"message\":\"upstream exploded\"}}"); + + chatResource.completions(mockRequest(), mockResponse(), host.getIdentifier(), + chatRequest(false)); + + assertSentinelNeverLogged("a completion the provider refused"); + } + + /** + * Given embeddings whose input carries a sentinel, and a provider that refuses + * When the failure is reported + * Then the sentinel appears in no log record + * + *

Embeddings carry the most caller content of any operation here — whole documents, in + * batches — so they are the costliest thing to leak and are checked on the failure path.

+ */ + @Test + public void test_failedEmbeddings_doesNotLogTheInput() { + stubProvider(EMBEDDINGS_PATH, 500, "{\"error\":{\"message\":\"upstream exploded\"}}"); + + final ArrayNode input = MAPPER.createArrayNode(); + input.add(new TextNode("Please embed " + SENTINEL)); + + embeddingsResource.embeddings(mockRequest(), mockResponse(), host.getIdentifier(), + new EmbeddingsRequestView(AiTest.EMBEDDINGS_MODEL, input)); + + assertSentinelNeverLogged("embeddings the provider refused"); + } + + /** + * Given an image request whose prompt carries a sentinel, and a provider that refuses + * When the failure is reported + * Then the sentinel appears in no log record + */ + @Test + public void test_failedImageGeneration_doesNotLogThePrompt() { + stubProvider(IMAGES_PATH, 500, "{\"error\":{\"message\":\"upstream exploded\"}}"); + + imagesResource.generations(mockRequest(), mockResponse(), host.getIdentifier(), + new ImageGenerationRequestView(AiTest.IMAGE_MODEL, + "Draw " + SENTINEL, 1, AiTest.IMAGE_SIZE)); + + assertSentinelNeverLogged("an image request the provider refused"); + } + + /** + * Fails when the caller's content reached a log, naming what was being exercised. + * + * @param what a description of the exchange, so a failure says which path leaked + */ + private void assertSentinelNeverLogged(final String what) { + final List captured = appender.messages(); + + assertFalse("Nothing was logged at all during " + what + ", so this assertion proves " + + "nothing — the appender is not attached, or the code stopped logging " + + "and this test has quietly stopped testing anything", + captured.isEmpty()); + + final List leaking = new ArrayList<>(); + for (final String message : captured) { + if (message != null && message.contains(SENTINEL)) { + leaking.add(message); + } + } + + assertTrue("FR-036 allows metadata in the logs and never the caller's content, but " + + what + " wrote it: " + leaking, + leaking.isEmpty()); + } + + private ChatCompletionRequestView chatRequest(final boolean stream) { + return new ChatCompletionRequestView( + CHAT_MODEL, + Collections.singletonList(new ChatCompletionRequestView.MessageView( + "user", "Summarise " + SENTINEL, null, null, null)), + null, null, null, stream ? Boolean.TRUE : null, + null, null, null, null, null, null); + } + + private static void stubProvider(final String path, final int status, final String body) { + wireMockServer.resetAll(); + wireMockServer.stubFor(post(urlPathEqualTo(path)) + .willReturn(aResponse() + .withStatus(status) + .withHeader("Content-Type", "application/json") + .withBody(body))); + } + + private static String providerChatResponse() { + return "{\"id\":\"chatcmpl-log\",\"object\":\"chat.completion\",\"created\":1789000000," + + "\"model\":\"" + CHAT_MODEL + "\",\"choices\":[{\"index\":0,\"message\":" + + "{\"role\":\"assistant\",\"content\":\"A summary.\"},\"finish_reason\":\"stop\"}]," + + "\"usage\":{\"prompt_tokens\":9,\"completion_tokens\":3,\"total_tokens\":12}}"; + } + + private HttpServletRequest mockRequest() { + final HttpServletRequest request = mock(HttpServletRequest.class); + final Map attributes = new HashMap<>(); + final String uri = "/api/inference/v1/logging"; + + when(request.getRequestURI()).thenReturn(uri); + when(request.getRequestURL()).thenReturn(new StringBuffer("http://localhost" + uri)); + when(request.getMethod()).thenReturn("POST"); + when(request.getRemoteAddr()).thenReturn("127.0.0.1"); + when(request.getServerName()).thenReturn(host.getHostname()); + when(request.getHeader("Authorization")).thenReturn(bearerToken); + + doAnswer(invocation -> attributes.put(invocation.getArgument(0), invocation.getArgument(1))) + .when(request).setAttribute(anyString(), any()); + when(request.getAttribute(anyString())) + .thenAnswer(invocation -> attributes.get(invocation.getArgument(0))); + + return request; + } + + private static HttpServletResponse mockResponse() { + return mock(HttpServletResponse.class); + } + + /** + * Collects every log record emitted while it is attached, including the rendered throwable. + * + *

The throwable matters as much as the message: a provider client's exception can quote the + * request back, so logging a safe message with an unsafe cause leaks just as effectively.

+ */ + private static final class CapturingAppender extends AbstractAppender { + + private final List captured = Collections.synchronizedList(new ArrayList<>()); + + private CapturingAppender() { + super("InferenceLoggingCapture", null, null, true, null); + } + + @Override + public void append(final LogEvent event) { + captured.add(event.getMessage() == null + ? null : event.getMessage().getFormattedMessage()); + if (event.getThrown() != null) { + for (Throwable current = event.getThrown(); + current != null && current != current.getCause(); + current = current.getCause()) { + captured.add(current.getMessage()); + } + } + } + + private List messages() { + synchronized (captured) { + return new ArrayList<>(captured); + } + } + } +} From 8fa5a85ce61fdca2cd9a1e7aa47cd415d72c95be Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Tue, 15 Sep 2026 15:24:17 -0600 Subject: [PATCH 06/15] fix(ai): stop a provider's message reaching the caller on the stream path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found reviewing this PR. When every model in a site's chain failed to initialise, the caller received the provider client's own message in the SSE error frame — endpoint, account identifiers, and on some clients a fragment of the prompt. The unit test added here shows what the old code returned: Failed to initialize chat model 'gpt-4o-mini': api.openai.com rejected key sk-live-9f3c for org acme-corp while completing 'draft the Q3 board memo' The buffered paths never did this and ImagesResource documents that they must not, so the streaming path was a discrepancy rather than a policy. FR-031 also requires translating upstream failures without leaking the provider's envelope. The cause was using a type as a proxy for provenance. Nearly everything reaching the translator is an IllegalArgumentException, and they arrive from two places: sentences dotCMS composed about the site's configuration, and the fallback chain's wrapper around a failed initialisation, which appends whatever the provider client said. Matching on the type returned both. Provenance is now recorded where the text is written. CallerSafeException marks a message dotCMS authored and may return; every other IllegalArgumentException gets a generic sentence and is logged in full. It extends IllegalArgumentException so the shipped endpoints that catch the broader type are unaffected — the throw sites are shared and FR-033 puts their behaviour out of bounds. The pass-through is kept rather than suppressed outright: "No model configured in providerConfig.chat — set 'model'" is the one message that tells an operator what to fix, and answering every configuration problem generically would be safe and useless. Covered at both levels: four unit tests on the translator, including that FR-031's 429 still survives the new branch sitting in front of it, and an integration test asserting what the caller can actually read. The integration test proved itself by failing for the right reason while the fix was briefly absent. Tests: 75 unit, 84 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- .../client/langchain4j/InferenceAIClient.java | 18 ++- .../langchain4j/LangChain4jAIClient.java | 6 +- .../inference/model/CallerSafeException.java | 31 +++++ .../langchain4j/InferenceErrorLeakTest.java | 120 ++++++++++++++++++ .../rest/ChatCompletionsStreamingTest.java | 54 ++++++++ 5 files changed, 224 insertions(+), 5 deletions(-) create mode 100644 dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java create mode 100644 dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java index 89bbfdddb0c5..52a822ff7cbc 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java @@ -1,6 +1,7 @@ package com.dotcms.ai.client.langchain4j; import com.dotcms.ai.app.AppConfig; +import com.dotcms.inference.model.CallerSafeException; import com.dotcms.inference.model.InferenceError; import com.dotcms.inference.model.InferenceLimits; import com.dotcms.inference.model.MultipleImagesUnsupportedException; @@ -695,11 +696,20 @@ private static InferenceUsage toInferenceUsage(final TokenUsage tokenUsage) { * @param throwable what went wrong * @return the error to report */ - private static InferenceError toInferenceError(final Throwable throwable) { + static InferenceError toInferenceError(final Throwable throwable) { + if (throwable instanceof CallerSafeException + && !nullToEmpty(throwable.getMessage()).isBlank()) { + // dotCMS wrote this sentence, so it is returned: it names something about the site's + // configuration that the caller or an operator can actually act on. + return InferenceError.invalidRequest(throwable.getMessage(), null); + } if (throwable instanceof IllegalArgumentException) { - return InferenceError.invalidRequest(nullToEmpty(throwable.getMessage()).isBlank() - ? "The request could not be served with this site's configuration" - : throwable.getMessage(), null); + // dotCMS did not write this one. The fallback chain reports a failed model + // initialisation as an IllegalArgumentException with the provider client's own message + // appended, and that message can carry the provider's endpoint, its account + // identifiers, or a fragment of the prompt. Logged in full, never returned. + return InferenceError.invalidRequest( + "The request could not be served with this site's configuration", null); } // A streamed exchange is rate limited exactly as often as a buffered one, and a client // reading an error event backs off on the same status, so the translation belongs here too. diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java index 6b0d33b46ed8..2508b88de5f5 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java @@ -4,6 +4,7 @@ import com.dotcms.ai.AiKeys; import com.dotcms.ai.app.AIModelType; import com.dotcms.ai.app.AppConfig; +import com.dotcms.inference.model.CallerSafeException; import com.dotcms.ai.client.AIClient; import com.dotcms.ai.client.AIRequest; import com.dotcms.ai.client.JSONObjectAIRequest; @@ -554,7 +555,10 @@ R executeWithFallbackTyped( final BiFunction executor) { final List models = effectiveModels(baseConfig); if (models.isEmpty()) { - throw new IllegalArgumentException( + // CallerSafeException, not a bare IllegalArgumentException: this sentence was written + // here, names the site's own configuration, and is the one thing that tells a caller + // what to fix. The type is what marks it returnable — see CallerSafeException. + throw new CallerSafeException( "No model configured in providerConfig." + section + " — set 'model'"); } // Each failure is logged immediately. The last exception is rethrown only after diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java b/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java new file mode 100644 index 000000000000..786a9cd60b02 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java @@ -0,0 +1,31 @@ +package com.dotcms.inference.model; + +/** + * An invalid-request failure whose message dotCMS wrote and is therefore safe to return. + * + *

Exists because "is this text safe to show a caller?" is not a question that can be answered + * by looking at an exception's type. Most of what reaches the error translator is an + * {@link IllegalArgumentException}, and they arrive from two very different places: some carry a + * sentence dotCMS composed about the site's own configuration, which is exactly what a caller + * needs to fix the problem; others carry a provider client's message, which can hold the + * provider's endpoint, its account identifiers, or a fragment of the prompt. Passing the message + * through whenever the type matched meant the second kind reached the caller too.

+ * + *

So the decision is made where the text is written rather than where it is rendered. Throwing + * this type is a statement that the message was authored here and may be returned; everything + * else is answered with a generic sentence and logged in full. Extending + * {@link IllegalArgumentException} keeps existing callers that catch the broader type working + * unchanged, which matters because the throw sites are shared with the endpoints FR-033 puts out + * of bounds.

+ */ +public class CallerSafeException extends IllegalArgumentException { + + private static final long serialVersionUID = 1L; + + /** + * @param message a message written by dotCMS, safe to return to a caller + */ + public CallerSafeException(final String message) { + super(message); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java b/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java new file mode 100644 index 000000000000..56fe3e5c3edf --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java @@ -0,0 +1,120 @@ +package com.dotcms.ai.client.langchain4j; + +import com.dotcms.inference.model.CallerSafeException; +import com.dotcms.inference.model.InferenceError; +import dev.langchain4j.exception.HttpException; +import dev.langchain4j.exception.RateLimitException; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Covers the one decision that separates a message a caller may read from one they may not. + * + *

Most failures reaching the stream's error translator are {@link IllegalArgumentException}s, + * and they come from two places that look identical at the catch site. Some carry a sentence + * dotCMS composed about the site's configuration — the caller needs that to fix the problem. + * Others are the fallback chain's wrapper around a failed model initialisation, which appends the + * provider client's own message, and that can hold the provider's endpoint, its account + * identifiers, or a fragment of the prompt.

+ * + *

Passing the message through on type alone returned both. These tests pin the distinction by + * intent — {@link CallerSafeException} is returned, everything else is not — so a future + * contributor who throws a bare {@code IllegalArgumentException} carrying provider text gets a + * generic sentence rather than a leak.

+ */ +public class InferenceErrorLeakTest { + + /** What a provider client's message can look like, and must never reach a caller. */ + private static final String LEAKY = + "api.openai.com rejected key sk-live-9f3c for org acme-corp while completing " + + "'draft the Q3 board memo'"; + + /** + * Given a model initialisation that failed with the provider's own message attached + * When the failure is translated for the caller + * Then the caller is told the request could not be served, and none of the provider's text + * reaches them + * + *

This is the shape the fallback chain actually produces: it catches whatever the provider + * client threw and rethrows an {@code IllegalArgumentException} whose message is dotCMS's + * prefix followed by the provider's. Before this test, that whole string was returned.

+ */ + @Test + public void test_wrappedProviderMessage_isNotReturnedToTheCaller() { + final IllegalArgumentException asTheFallbackChainThrowsIt = new IllegalArgumentException( + "Failed to initialize chat model 'gpt-4o-mini': " + LEAKY, + new RuntimeException(LEAKY)); + + final InferenceError error = + InferenceAIClient.toInferenceError(asTheFallbackChainThrowsIt); + + assertEquals(400, error.httpStatus()); + assertEquals("The request could not be served with this site's configuration", + error.message()); + assertFalse("The provider's endpoint must not reach the caller", + error.message().contains("api.openai.com")); + assertFalse("An API key must never reach the caller", + error.message().contains("sk-live-9f3c")); + assertFalse("The caller's own prompt must not be quoted back through an error", + error.message().contains("board memo")); + assertFalse("Even the internal wrapper text is withheld; it names nothing actionable", + error.message().contains("Failed to initialize")); + } + + /** + * Given a configuration problem dotCMS itself described + * When the failure is translated + * Then the caller gets that sentence, because it is the one thing that says what to fix + * + *

The other half of the rule, and the reason this is a type distinction rather than a + * blanket suppression: answering every configuration problem with "could not be served" would + * be safe and useless, and a site administrator would have nothing to act on.

+ */ + @Test + public void test_messageDotcmsWrote_isReturned() { + final InferenceError error = InferenceAIClient.toInferenceError( + new CallerSafeException("No model configured in providerConfig.chat — set 'model'")); + + assertEquals(400, error.httpStatus()); + assertEquals("No model configured in providerConfig.chat — set 'model'", error.message()); + } + + /** + * Given a provider failure that is not an argument problem at all + * When it is translated + * Then it stays on the upstream path, rate limits included + * + *

Guards against the fix above swallowing the FR-031 translation: the new branch sits in + * front of it, and a version that matched too eagerly would turn every 429 into a 400 and + * stop clients backing off.

+ */ + @Test + public void test_upstreamFailures_stillTranslateByStatus() { + assertEquals("A rate limit must still be a 429", + 429, InferenceAIClient.toInferenceError(new RateLimitException(LEAKY)).httpStatus()); + + final InferenceError upstream = + InferenceAIClient.toInferenceError(new HttpException(503, LEAKY)); + assertEquals(502, upstream.httpStatus()); + assertTrue("An upstream failure keeps the api_error type", + "api_error".equals(upstream.type())); + assertFalse(upstream.message().contains("sk-live-9f3c")); + } + + /** + * Given a caller-safe exception carrying no message + * When it is translated + * Then the caller still gets a usable sentence rather than a blank one + */ + @Test + public void test_blankSafeMessage_fallsBackToTheGenericSentence() { + final InferenceError error = InferenceAIClient.toInferenceError(new CallerSafeException(" ")); + + assertEquals(400, error.httpStatus()); + assertEquals("The request could not be served with this site's configuration", + error.message()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java index 559108c108ea..465275df6f2d 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java @@ -578,6 +578,60 @@ private static ChatCompletionRequestView streamingRequest(final StreamOptionsVie null, null, null, null, null); } + /** + * Given a site whose provider configuration makes every chat model fail to initialise + * When a streamed completion is requested + * Then what the caller receives names nothing about the provider or the site's credentials + * + *

FR-031 and FR-036. The fallback chain reports a failed initialisation by rethrowing an + * {@code IllegalArgumentException} whose message is dotCMS's prefix followed by whatever the + * provider client said — and a provider client's message can carry its endpoint, its account + * identifiers, or a fragment of the prompt. The stream's error translator used to return that + * message on type alone, so it reached the caller in the error frame. The buffered paths never + * did, and {@code ImagesResource} documents that they must not, which is what made the + * streaming path's behaviour a discrepancy rather than a policy.

+ * + *

Asserted on what the caller can read rather than on the translator, because that is the + * boundary the requirement is about: the full detail is still logged, and is still expected to + * be there for an operator.

+ */ + @Test + public void test_whenEveryModelFailsToStart_theCallerLearnsNothingAboutTheProvider() + throws Exception { + AiTest.aiAppSecretsWithProviderConfig(host, providerConfigThatCannotInitialise()); + + final Response response = resource.completions( + mockRequest(), mockResponse(), host.getIdentifier(), streamingRequest(null)); + + final String visible = clientVisibleBody(response); + + assertFalse("The internal wrapper text names nothing a caller can act on and must not " + + "be returned", visible.contains("Failed to initialize")); + assertFalse("A site's credential must never appear in a response", + visible.contains(AiTest.API_KEY)); + assertFalse("The provider endpoint dotCMS calls is infrastructure detail", + visible.contains(String.valueOf(AiTest.PORT))); + assertFalse(visible.contains("localhost")); + } + + /** + * @return a provider configuration whose chat section cannot produce a model, so the failure + * happens during initialisation rather than at the provider + */ + private static String providerConfigThatCannotInitialise() { + final String endpoint = String.format("http://localhost:%d/", AiTest.PORT); + return String.format( + "{" + + "\"chat\":{\"provider\":\"openai\",\"apiKey\":\"\",\"model\":\"%1$s\"," + + "\"endpoint\":\"%2$s\",\"maxRetries\":0}," + + "\"embeddings\":{\"provider\":\"openai\",\"apiKey\":\"%3$s\"," + + "\"model\":\"%4$s\",\"endpoint\":\"%2$s\",\"maxRetries\":0}," + + "\"image\":{\"provider\":\"openai\",\"apiKey\":\"%3$s\",\"model\":\"%5$s\"," + + "\"endpoint\":\"%2$s\",\"maxRetries\":0}" + + "}", + CHAT_MODEL, endpoint, AiTest.API_KEY, AiTest.EMBEDDINGS_MODEL, AiTest.IMAGE_MODEL); + } + /** * Writes the streamed entity out in full, the way the container would. * From b9ce94bb475189459717c44c5aed4f196e536a4b Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Wed, 16 Sep 2026 11:11:19 -0600 Subject: [PATCH 07/15] fix(ai): stop reporting unfixable provider refusals as retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the live agent-loop test, after four curl calls and the whole suite passed. An OpenRouter account out of credit answers 402 — "requires more credits, or fewer max_tokens" — and this family reported it as 502. That is the one status it must not be: 502 is what a standard client's back-off reads as "try again", so a request that cannot succeed until somebody adds credit gets retried on a schedule. The live log showed three attempts before the caller saw an answer. The translation now has three outcomes rather than two. A rate limit stays 429, temporary and worth retrying. A refusal the provider will repeat — an exhausted account, a rejected key, a model it does not serve — becomes a 400, terminal. Everything left stays 502, which is what a genuine upstream fault is for. Permanence is read from the provider library's own classification rather than from a list of exception types or status codes: it already sorts failures into retriable and non-retriable, so a type added in a later release is handled without touching this method. 400 is an imperfect fit and the comment says so — the caller has usually done nothing wrong, and what needs fixing is the site's provider account or configuration. But the status is the only thing carrying retryability, a 5xx keeps the client coming back, and the message points at where to look. The provider's own wording, which names the account and sometimes the prompt, stays in the log. Tests: 79 unit, all green. Co-Authored-By: Claude Opus 5 (1M context) --- .../inference/model/InferenceError.java | 32 ++++++++ .../InferenceErrorTranslationTest.java | 81 +++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java index 19668c3fee8d..175a2596c0ea 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java @@ -66,6 +66,18 @@ public static InferenceError upstream(final String message) { * account identifiers, and occasionally a fragment of the prompt. Only the status is taken * from it, which is why the caller supplies the wording.

* + *

Three outcomes, not two, because "the provider said no" splits along a line a client + * acts on. A rate limit is temporary: 429, come back later. A refusal the provider will + * repeat — an exhausted account, a rejected key, a model the provider does not serve — is + * permanent until somebody changes something, and answering 502 there tells a standard + * client's back-off to retry a request that cannot ever succeed. Everything left is a + * genuine upstream fault, which is what 502 is for.

+ * + *

Permanence is read from the provider library's own classification rather than from a + * list of exception types or status codes: it already sorts its failures into retriable and + * non-retriable, and keying off that means a type added in a later release is classified + * correctly without this method being touched.

+ * *

The cause chain is walked rather than the top exception inspected, because the provider * client wraps: a rate limit surfaces as its own type or as a generic HTTP failure carrying * 429, and by the time a retrying fallback chain has rethrown it, either can be two or three @@ -85,6 +97,9 @@ public static InferenceError fromProviderFailure(final Throwable failure, && httpFailure.statusCode() == 429) { return rateLimited(); } + if (current instanceof dev.langchain4j.exception.NonRetriableException) { + return providerRefused(); + } if (current.getCause() == current) { break; } @@ -95,6 +110,23 @@ public static InferenceError fromProviderFailure(final Throwable failure, /** * @return the standard rate-limit refusal, which a client reads as "retry later" */ + /** + * @return the refusal for a provider failure that a retry cannot fix + */ + public static InferenceError providerRefused() { + // 400 rather than 502, because the status is the only thing carrying retryability and a + // 5xx would keep a client coming back. It is an imperfect fit — the caller has usually + // done nothing wrong, and the thing to fix is the site's provider account or + // configuration — so the message says where to look rather than implying the request was + // malformed. The provider's own wording, which names the account and sometimes the + // prompt, stays in the log. + return new InferenceError("invalid_request_error", + "The model provider refused this request, and will refuse it again on retry. " + + "This is a provider account or site configuration problem rather than " + + "a problem with the request; the details are in the server log.", + null, 400); + } + public static InferenceError rateLimited() { return new InferenceError("rate_limit_error", "The model provider is rate limiting this site's requests", null, 429); diff --git a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java index 3a0dafa06cfb..e1ee57cc2589 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java @@ -1,7 +1,11 @@ package com.dotcms.inference; import com.dotcms.inference.model.InferenceError; +import dev.langchain4j.exception.AuthenticationException; import dev.langchain4j.exception.HttpException; +import dev.langchain4j.exception.InternalServerException; +import dev.langchain4j.exception.InvalidRequestException; +import dev.langchain4j.exception.ModelNotFoundException; import dev.langchain4j.exception.RateLimitException; import org.junit.Test; @@ -22,6 +26,11 @@ public class InferenceErrorTranslationTest { private static final String UPSTREAM = "The model provider failed to complete the request"; + /** Stands in for a provider message: endpoint, key and a fragment of the caller's prompt. */ + private static final String LEAKY = + "api.openai.com rejected key sk-live-9f3c for org acme-corp while completing " + + "'draft the Q3 board memo'"; + /** * Given a provider that refused because the site is over its rate limit * When the failure is translated @@ -103,6 +112,78 @@ public synchronized Throwable getCause() { assertEquals(502, InferenceError.fromProviderFailure(selfReferencing, UPSTREAM).httpStatus()); } + /** + * Given a provider that refused on an exhausted account + * When the failure is translated + * Then the caller gets a 400, not a retryable 502 + * + *

The case that found this. An OpenRouter account out of credit answers 402 with "requires + * more credits, or fewer max_tokens"; the provider library maps that to a non-retriable + * exception, and this family used to answer 502. A 502 is the one thing that must not happen + * there: it is the status a client's back-off reads as "try again", so a request that cannot + * succeed until somebody adds credit gets retried on a schedule, burning a round trip each + * time. The live log showed three attempts before the caller ever saw an answer.

+ */ + @Test + public void test_exhaustedProviderAccount_isNotReportedAsRetryable() { + final InferenceError error = InferenceError.fromProviderFailure( + new InvalidRequestException( + "This request requires more credits, or fewer max_tokens. You requested " + + "up to 65536 tokens, but can only afford 30546"), + UPSTREAM); + + assertEquals("A refusal a retry cannot fix must not carry a retryable status", + 400, error.httpStatus()); + assertFalse("The whole point is that a client stops retrying", error.isRetryable()); + assertFalse("The provider's wording names the account and the limits; it stays in the log", + error.message().contains("65536")); + assertFalse(error.message().contains("credits")); + } + + /** + * Given each kind of refusal the provider library classifies as non-retriable + * When they are translated + * Then none of them is reported as retryable + * + *

Asserted across the family rather than on the one that was reported, because they share + * a parent in the library and the translation keys off that parent. A bad key and a model the + * provider does not serve are as permanent as an empty account, and all three used to be + * 502.

+ */ + @Test + public void test_everyNonRetriableRefusal_isTerminal() { + for (final RuntimeException refusal : new RuntimeException[]{ + new InvalidRequestException(LEAKY), + new AuthenticationException(LEAKY), + new ModelNotFoundException(LEAKY)}) { + + final InferenceError error = InferenceError.fromProviderFailure(refusal, UPSTREAM); + assertFalse(refusal.getClass().getSimpleName() + + " cannot be fixed by retrying, so it must not be reported as retryable", + error.isRetryable()); + assertFalse(error.message().contains("sk-live-9f3c")); + } + } + + /** + * Given a genuine upstream fault + * When it is translated + * Then it stays a retryable 502 + * + *

The boundary in the other direction: the library calls this one retriable, and it is — + * a provider having a bad minute is exactly what 502 and a client back-off are for. A fix + * that turned every provider failure into a terminal 4xx would be as wrong as the bug.

+ */ + @Test + public void test_genuineUpstreamFault_staysRetryable() { + final InferenceError error = + InferenceError.fromProviderFailure(new InternalServerException(LEAKY), UPSTREAM); + + assertEquals(502, error.httpStatus()); + assertTrue("A provider having a bad minute is worth retrying", error.isRetryable()); + assertEquals(UPSTREAM, error.message()); + } + /** * Given a provider failure of any kind * When it is translated From 15420176d6cd6e0a21bf6dc9a100172c3a88d56b Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Wed, 16 Sep 2026 11:52:31 -0600 Subject: [PATCH 08/15] fix(ai): log the provider's message when a stream fails, not just its type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The streaming path logged only the exception class name: WARN InferenceAIClient - Inference stream failed: InvalidRequestException which leaves an operator unable to tell an exhausted account from a rejected key from a malformed request. It also made the caller-facing message untrue: providerRefused() says the detail is in the server log, and on this path it was not. The buffered path has always logged the full exception — that is how the OpenRouter 402 was identified earlier today — so the two paths now match. Found while debugging a real client against a real provider. The failure was undiagnosable precisely because the one place the detail was supposed to be had only a class name. InferenceLoggingTest still passes: the caller's prompt does not reach a log. Worth stating what that does and does not prove — the stubbed provider in that test returns a generic error body, so it demonstrates dotCMS does not log the prompt itself, not that a provider which echoes a prompt back in its error text would be filtered. That residual is the same one the buffered path already carries. Co-Authored-By: Claude Opus 5 (1M context) --- .../ai/client/langchain4j/InferenceAIClient.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java index 52a822ff7cbc..7a8796fecf2b 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/InferenceAIClient.java @@ -181,8 +181,12 @@ public void stream(final AppConfig appConfig, } catch (final RuntimeException e) { // Nothing reached the sink — every model in the chain failed to start — so the sink // still has to learn that this stream will not finish. - Logger.warn(InferenceAIClient.class, - "Inference stream could not be started: " + e.getClass().getSimpleName()); + // Logged with the exception rather than just its type. What the caller receives is a + // safe sentence that says the detail is in the log, so the detail has to actually be + // here — a bare class name leaves an operator with "InvalidRequestException" and no + // way to tell an exhausted account from a rejected key from a malformed request. The + // buffered path has always logged the full exception; this matches it. + Logger.warn(InferenceAIClient.class, "Inference stream could not be started", e); state.fail(toInferenceError(e)); } } @@ -344,8 +348,7 @@ public void onCompleteResponse(final ChatResponse response) { @Override public void onError(final Throwable throwable) { - Logger.warn(InferenceAIClient.class, - "Inference stream failed: " + throwable.getClass().getSimpleName()); + Logger.warn(InferenceAIClient.class, "Inference stream failed", throwable); state.fail(toInferenceError(throwable)); } }); From b00c7c2662a4909b1c565088195037b384525bd4 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Wed, 16 Sep 2026 18:10:33 -0600 Subject: [PATCH 09/15] fix(ai): list every configured model, not just the chat section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FR-010 says the listing returns "the models the resolved site has configured". It returned only the chat section, which left the embeddings and image operations with no discovery at all: both require an exact model name, there is no implicit default, and FR-024 names this listing as the mechanism for learning one. Two of the four operations could only be used by someone who had been told their model names out of band. Found by using it, not by a test. Every test here asserted the chat-only behaviour, so the suite agreed with the bug. Lists all three sections, chat first, deduplicated. Chat-first is load-bearing rather than cosmetic and is now asserted: the documented way to ask for "whatever this site runs" is to take the first entry, so an embeddings model at the head would silently break callers following that advice. Capability is deliberately NOT signalled on the entries. The adopted format has nowhere to put it — its model object is id, object, created, owned_by, with no type, mode or modality — and no established implementation invents one. OpenRouter publishes an entirely different, richer object; LiteLLM keeps this endpoint's shape and exposes mode (chat, embedding, image_generation) on a separate endpoint. Adding a field inside the standard shape is the one thing neither does, and is what SC-008's no-adapter promise exists to prevent. A caller that picks an entry an operation does not serve is refused by that operation with a 404 naming the field; that refusal is the mechanism. Four existing tests were expectation updates rather than fixes — their intent is unchanged, only the expected set grew, and each now says why. A fifth asserted the bug outright and was inverted. Adds a dedup test for one model configured across several sections. Tests: 79 unit, 85 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- docs/backend/INFERENCE_API.md | 4 +- .../dotcms/inference/rest/ModelsResource.java | 53 ++++++++++++-- .../inference/rest/InferenceModelsTest.java | 72 +++++++++++++++---- 3 files changed, 108 insertions(+), 21 deletions(-) diff --git a/docs/backend/INFERENCE_API.md b/docs/backend/INFERENCE_API.md index 56febb2fd31c..3392d503d981 100644 --- a/docs/backend/INFERENCE_API.md +++ b/docs/backend/INFERENCE_API.md @@ -15,7 +15,7 @@ Source: `dotCMS/src/main/java/com/dotcms/inference/`. Design intent lives in `sp | Operation | Path | Notes | |---|---|---| | Chat completion | `POST /api/inference/v1/chat/completions` | Multi-turn messages, tool calling, optional SSE streaming | -| Model listing | `GET /api/inference/v1/models` | The models the resolved site configured for chat | +| Model listing | `GET /api/inference/v1/models` | Every model the resolved site configured — chat, embeddings and images — chat first | | Embeddings | `POST /api/inference/v1/embeddings` | One string or an array of strings | | Image generation | `POST /api/inference/v1/images/generations` | Always base64, never a URL | @@ -47,7 +47,7 @@ The value is validated against what the resolved site has configured **for that A model the site has not configured for that capability is refused with a `404`, so a chat model sent to the images endpoint is refused even though the same site configured it perfectly well for chat. Nothing is passed through to the provider. -`GET /api/inference/v1/models` is the discovery mechanism: it lists exactly the names `chat/completions` accepts, in configured order, the first being the site's primary model. No provider is contacted and nothing synthetic is added. A caller that wants "whatever this site runs" reads the list and takes the first entry. +`GET /api/inference/v1/models` is the discovery mechanism: it lists every model the resolved site has configured, in configured order, chat section first — so a caller who wants "whatever this site runs" reads the list and takes the first entry. Embeddings and image models are listed alongside chat models because every operation here requires an exact model name and this is the only place to learn one; listing chat alone would leave two of the four operations undiscoverable. The format has nowhere to record what a model is for — its model object carries no type, mode or modality field, which is why OpenAI's own listing mixes chat, embedding and image models the same way — so picking an entry the operation does not serve is refused by that operation with a `404` naming `model`. Nothing is added to the entries to signal capability: adding a field the format does not define is what the no-adapter promise of this family exists to avoid. Because the model is required and validated, swapping a site's provider is **not** invisible to callers: a caller pinning the old vendor's model gets an explicit `404` and has to re-read the model list. That is deliberate. diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java index 579d1f2bc605..daf87c88ee90 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ModelsResource.java @@ -35,6 +35,7 @@ import javax.ws.rs.core.Response; import java.time.Instant; import java.util.ArrayList; +import java.util.LinkedHashSet; import java.util.List; import java.util.Optional; @@ -43,7 +44,7 @@ * *

This endpoint is the discovery mechanism the rest of the family depends on. There is no * implicit default model and no reserved alias, so the names listed here are exactly the names - * {@link ChatCompletionsResource} accepts — a caller who wants "whatever this site runs" reads the + * this family accepts — a caller who wants "whatever this site runs" reads the * list and takes the first entry. Two consequences follow, and both are deliberate.

* *
    @@ -74,6 +75,19 @@ public class ModelsResource { /** Section of the site's {@code providerConfig} JSON the listed models come from. */ private static final String CHAT_SECTION = "chat"; + private static final String EMBEDDINGS_SECTION = "embeddings"; + private static final String IMAGE_SECTION = "image"; + + /** + * The sections this listing draws from, chat first. + * + *

    Order is load-bearing rather than cosmetic. There is no implicit default model, and the + * documented way to ask for "whatever this site runs" is to read this list and take the first + * entry — so the first entry has to remain the site's primary chat model, as it was when this + * listed nothing else.

    + */ + private static final List LISTED_SECTIONS = + List.of(CHAT_SECTION, EMBEDDINGS_SECTION, IMAGE_SECTION); /** * Header form of the site override. Server-side callers routinely sit behind a proxy that @@ -84,7 +98,7 @@ public class ModelsResource { private static final String SITE_HEADER = "X-dotCMS-Site"; /** - * Lists the models the resolved site has configured for chat. + * Lists the models the resolved site has configured, across every capability it serves. * * @param request the inbound request * @param response the outbound response, used only by the authentication handshake @@ -171,12 +185,43 @@ public Response models(@Context final HttpServletRequest request, // on refusals as well as on the happy path. request.setAttribute(InferenceRequestAttributes.RESOLVED_SITE_ID, context.servingSiteId()); - return Response.ok(toView( - LangChain4jAIClient.get().configuredModels(context.config(), CHAT_SECTION))) + return Response.ok(toView(configuredModels(context))) .type(MediaType.APPLICATION_JSON) .build(); } + /** + * Collects the model names this site has configured, across every capability it serves. + * + *

    All of them, not just chat. The adopted format has one flat model list and its model + * object carries no field for what a model is for — no type, no mode, no modality — so the + * reference implementation returns chat, embedding and image models together and leaves a + * client to know which is which. Listing only chat would be the narrower, tidier answer and + * was what shipped first, but it leaves the embeddings and image operations with no discovery + * at all: both require an exact model name, and this is the only place a caller could learn + * one.

    + * + *

    The alternative — adding a capability field to each entry — was rejected after looking at + * what other gateways do. Nobody extends the standard model object that way: implementations + * either keep this endpoint's shape and expose capability on a separate, non-standard one, or + * publish an entirely different object of their own. Inventing a field inside the standard + * shape is the one thing none of them does, and it is exactly what the no-adapter promise of + * this family exists to avoid.

    + * + *

    A name configured in more than one section is listed once. Duplicates would be harmless + * to a client but would misrepresent the site as running two things.

    + * + * @param context the resolved site and its configuration + * @return the configured names, chat first, in configured order, without repeats + */ + private static List configuredModels(final ResolvedAiContext context) { + final LinkedHashSet names = new LinkedHashSet<>(); + for (final String section : LISTED_SECTIONS) { + names.addAll(LangChain4jAIClient.get().configuredModels(context.config(), section)); + } + return List.copyOf(names); + } + /** * Wraps the configured model names in the listing shape. * diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java index eac65d340071..b6e4c8200d90 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java @@ -168,7 +168,10 @@ public void test_models_withSingleConfiguredModel_listsThatModel() { final ModelListView view = (ModelListView) response.getEntity(); assertEquals(OBJECT_LIST, view.object()); assertNotNull(view.data()); - assertEquals(1, view.data().size()); + // The site also configures an embeddings and an image model, and both are listed — the + // chat model is simply first, which is the ordering a caller taking entry zero relies on. + assertEquals(List.of(CHAT_MODEL, AiTest.EMBEDDINGS_MODEL, AiTest.IMAGE_MODEL), + modelIds(view)); final ModelListView.ModelView model = view.data().get(0); assertEquals(CHAT_MODEL, model.id()); @@ -198,7 +201,11 @@ public void test_models_withFallbackChain_listsEveryEntryInOrder() throws Except final ModelListView view = (ModelListView) response.getEntity(); assertEquals(OBJECT_LIST, view.object()); - assertEquals(List.of(CHAIN_PRIMARY, CHAIN_FALLBACK), modelIds(view)); + assertEquals("Every entry of the chat chain is listed, in configured order, ahead of the " + + "other capabilities' models", + List.of(CHAIN_PRIMARY, CHAIN_FALLBACK, + AiTest.EMBEDDINGS_MODEL, AiTest.IMAGE_MODEL), + modelIds(view)); assertEquals("The primary model is the first entry", CHAIN_PRIMARY, view.data().get(0).id()); view.data().forEach( @@ -227,9 +234,13 @@ public void test_models_withConfiguredChain_addsNothingSynthetic() throws Except assertEquals(200, response.getStatus()); final ModelListView view = (ModelListView) response.getEntity(); + final List expected = new ArrayList<>(configured); + expected.add(AiTest.EMBEDDINGS_MODEL); + expected.add(AiTest.IMAGE_MODEL); + assertEquals("The list must hold one entry per configured model, and no more", - configured.size(), view.data().size()); - assertEquals(configured, modelIds(view)); + expected.size(), view.data().size()); + assertEquals(expected, modelIds(view)); } /** @@ -282,7 +293,8 @@ public void test_models_withSiteOverride_listsTheResolvedSitesModels() throws Ex assertEquals(200, response.getStatus()); final ModelListView view = (ModelListView) response.getEntity(); - assertEquals(List.of(OTHER_CHAT_MODEL), modelIds(view)); + assertEquals(List.of(OTHER_CHAT_MODEL, AiTest.EMBEDDINGS_MODEL, AiTest.IMAGE_MODEL), + modelIds(view)); assertFalse("The other site's model must not appear", modelIds(view).contains(CHAT_MODEL)); } @@ -337,14 +349,25 @@ public void test_models_withoutBearerToken_isUnauthorized() { * Given a site configured with a chat model, a different embeddings model and a different * image model * When the models are listed - * Then only the chat model is listed + * Then all three are listed, with the chat model first + * + *

    Every operation in this family requires an exact model name and there is no implicit + * default, so this listing is the only way a caller can learn one. Listing chat alone — which + * is what shipped first — left the embeddings and image operations with no discovery at all: + * a caller had to be told their model names out of band, which is precisely what a discovery + * endpoint exists to avoid.

    * - *

    The chat endpoint validates {@code model} against the chat section, so a list drawn from - * any other section would advertise names that endpoint refuses. Embeddings and images have - * their own operations and their own model sets.

    + *

    Chat first is asserted rather than incidental. The documented way to ask for "whatever + * this site runs" is to read this list and take the first entry, so an ordering that put an + * embeddings model at the head would silently break every caller following that advice.

    + * + *

    The format offers nowhere to say which entry serves which operation — its model object + * has no type, mode or modality field — so a caller that picks the wrong one is refused by + * the operation itself, naming the field. That refusal is the mechanism; this list is + * deliberately not the place to duplicate it.

    */ @Test - public void test_models_withDistinctSectionModels_listsOnlyTheChatSection() throws Exception { + public void test_models_withDistinctSectionModels_listsEveryCapability() throws Exception { final Host site = siteConfiguredWith(CHAT_MODEL, EMBEDDINGS_MODEL, IMAGE_MODEL); final Response response = resource.models( @@ -354,11 +377,30 @@ public void test_models_withDistinctSectionModels_listsOnlyTheChatSection() thro assertEquals(200, response.getStatus()); final ModelListView view = (ModelListView) response.getEntity(); - assertEquals(List.of(CHAT_MODEL), modelIds(view)); - assertFalse("The embeddings model belongs to the embeddings operation, not here", - modelIds(view).contains(EMBEDDINGS_MODEL)); - assertFalse("The image model belongs to the image operation, not here", - modelIds(view).contains(IMAGE_MODEL)); + assertEquals("Every configured model is discoverable, chat first", + List.of(CHAT_MODEL, EMBEDDINGS_MODEL, IMAGE_MODEL), modelIds(view)); + assertEquals("A caller taking the first entry must still get the site's primary chat " + + "model", CHAT_MODEL, modelIds(view).get(0)); + } + + /** + * Given a site whose sections name the same model + * When the models are listed + * Then it appears once + * + *

    A repeat would be harmless to a client but would misrepresent the site as running two + * things, and a caller counting entries to decide what is available would be wrong.

    + */ + @Test + public void test_models_withOneModelServingEverySection_listsItOnce() throws Exception { + final Host site = siteConfiguredWith(CHAT_MODEL, CHAT_MODEL, CHAT_MODEL); + + final Response response = resource.models( + mockRequest(), mockResponse(), site.getIdentifier()); + + assertNotNull(response); + assertEquals(200, response.getStatus()); + assertEquals(List.of(CHAT_MODEL), modelIds((ModelListView) response.getEntity())); } /** From af9180beb3218cec6f56853d14af7ce055b2d7e6 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Wed, 16 Sep 2026 19:09:45 -0600 Subject: [PATCH 10/15] docs(ai): distinguish retryable from terminal provider failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The status table told a reader that an exhausted provider account was a 502. That is the opposite of what the code now does, and it is the reading that causes harm: someone writing a client against these docs would wrap a back-off around a failure that can never succeed. The 400 row now names both kinds of thing it covers — caller mistakes, and a provider refusal a retry cannot fix — and says why the second is a 4xx despite not being the caller's fault. The 502 row is narrowed to faults actually worth retrying. Adds a short paragraph stating that retryability is carried entirely by the status, since the standard error shape has no field for it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/backend/INFERENCE_API.md | 8 +++++--- specs/37431-openai-compatible-inference/spec.md | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/backend/INFERENCE_API.md b/docs/backend/INFERENCE_API.md index 3392d503d981..9091d4ca36dd 100644 --- a/docs/backend/INFERENCE_API.md +++ b/docs/backend/INFERENCE_API.md @@ -280,15 +280,17 @@ Every refusal uses the standard OpenAI error envelope, so a client library deser | Status | `type` | When | |---|---|---| -| `400` | `invalid_request_error` | Caller mistakes: missing `model`, empty `messages`, a malformed or uncorrelated tool message, `n > 1` on chat, a bad `input`, a bad `prompt`, an `n` the site's image model or the configured ceiling will not allow, an unresolvable site override | +| `400` | `invalid_request_error` | Two different things. **Caller mistakes**: missing `model`, empty `messages`, a malformed or uncorrelated tool message, `n > 1` on chat, a bad `input`, a bad `prompt`, an `n` the site's image model or the configured ceiling will not allow, an unresolvable site override. **And a provider refusal a retry cannot fix**: an exhausted provider account, a rejected credential, a model the provider does not serve. The second kind is not the caller's fault and says so — it points at the site's provider account or configuration — but it is a `4xx` because that is the only way to tell a client's back-off to stop | | `401` | `invalid_request_error` | Anonymous, or a credential that is not a bearer token | | `403` | `invalid_request_error` | An explicit site override naming a site the caller cannot read | | `404` | `invalid_request_error` | A model the resolved site has not configured for that capability | | `413` | `invalid_request_error` | Request body over `DOT_INFERENCE_MAX_REQUEST_BYTES` | | `429` | `rate_limit_error` | The node is already serving its ceiling of concurrent streamed completions (carries `Retry-After`), **or** the model provider is rate limiting this site (no `Retry-After` — see below) | -| `502` | `api_error` | A genuine upstream provider failure | +| `502` | `api_error` | A genuine upstream fault — the provider had a bad minute. Retryable, and the only provider failure that is | -A `502` carries a fixed, safe message; the provider's own message is logged and never returned, because it can carry the provider's endpoint, account identifiers, or a fragment of the prompt. Retryability is carried by the status alone — the standard error shape has no retryable field, and adding one would break the no-adapter promise these endpoints exist to keep — so a provider rate limit is reported as `429` rather than folded into `502`: the two call for different back-off, and a client that reads `502` treats a busy provider as a broken one. The two `429`s differ in one way that matters to a client. When dotCMS refuses because **this node** is at its streaming ceiling, the response carries `Retry-After: 5` — that is a wait dotCMS knows, so it states it in the form every standard client already honours rather than only in prose. When the refusal comes from the **provider**, no `Retry-After` is sent: the provider knows how long it wants to be left alone and says so in a header the client library discards on its error path, so the value never reaches dotCMS, and inventing one would present a guess to the client as an instruction. Relaying the provider's value needs a custom HTTP client under the provider abstraction and is tracked as follow-up work; until then a client backs off on the status alone for upstream throttling. A site with no usable configuration for the requested capability lands on the same `404` as an unknown model, so a caller cannot use the distinction to learn which sites have dotAI set up. +A `502` carries a fixed, safe message; the provider's own message is logged and never returned, because it can carry the provider's endpoint, account identifiers, or a fragment of the prompt. Retryability is carried by the status alone — the standard error shape has no retryable field, and adding one would break the no-adapter promise these endpoints exist to keep — so a provider rate limit is reported as `429` rather than folded into `502`: the two call for different back-off, and a client that reads `502` treats a busy provider as a broken one. **Which failures are worth retrying is carried entirely by the status.** The standard error shape has no retryable field and adding one would break the no-adapter promise, so a client reads the number: `429` and `502` mean come back, every other `4xx` means do not. That is why a provider account out of credit answers `400` rather than `502` — it will refuse the next attempt identically, and a client that retried it would burn a provider round trip each time for as long as the account stayed empty. Whether a refusal is permanent is read from the provider library's own classification rather than a list of status codes dotCMS maintains, so a new failure kind in a later release is classified without a code change here. + +The two `429`s differ in one way that matters to a client. When dotCMS refuses because **this node** is at its streaming ceiling, the response carries `Retry-After: 5` — that is a wait dotCMS knows, so it states it in the form every standard client already honours rather than only in prose. When the refusal comes from the **provider**, no `Retry-After` is sent: the provider knows how long it wants to be left alone and says so in a header the client library discards on its error path, so the value never reaches dotCMS, and inventing one would present a guess to the client as an instruction. Relaying the provider's value needs a custom HTTP client under the provider abstraction and is tracked as follow-up work; until then a client backs off on the status alone for upstream throttling. A site with no usable configuration for the requested capability lands on the same `404` as an unknown model, so a caller cannot use the distinction to learn which sites have dotAI set up. Only exchange **metadata** is recorded — resolved site, model, token counts, status, duration. Request and response bodies are never written to a log or a durable store, and no conversation is retained after the response is delivered; the family is stateless. diff --git a/specs/37431-openai-compatible-inference/spec.md b/specs/37431-openai-compatible-inference/spec.md index 043ee3b8e314..a60c1e50cc97 100644 --- a/specs/37431-openai-compatible-inference/spec.md +++ b/specs/37431-openai-compatible-inference/spec.md @@ -165,7 +165,7 @@ The same client credentials and base URL also serve text embeddings and image ge #### Operational and forward compatibility - **FR-030**: The system MUST NOT emit cross-origin headers on this family, and the endpoints MUST be documented as server-side only, because the credential is a long-lived token with the full authority of its owner and cross-origin support would invite placing it in browser JavaScript. -- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code** — 429 for rate limiting, 5xx for upstream failure — because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. The response body MUST stay conformant to the standard error shape. Where the system refuses on its own capacity rather than the provider's, it MUST send a `Retry-After` header, because that refusal is one whose duration dotCMS actually knows — and a client told to "retry shortly" in prose it cannot parse is a client that guesses. Where the **provider** supplies a `Retry-After`, the system MUST relay it **where the provider abstraction exposes it**. **It currently does not, and that is recorded rather than quietly dropped** (amended 2026-09-15). The client library surfaces a failed call as an exception carrying a status code and a message and nothing else; response headers are read and discarded on the error path, so the value never reaches dotCMS. Relaying it needs a custom HTTP client injected through the library's supported extension point — bounded work, not a dead end, but work that takes ownership of timeouts, proxying and multipart for every provider call and so belongs in its own change with its own review, not at the tail of this one. Until then a client backs off on the status alone, which every standard client already implements as its default. Revisit this first if throttling proves painful in practice. +- **FR-031**: The system MUST translate upstream provider rate-limit and server errors into the standard error shape without leaking the provider's raw envelope. Retryability MUST be conveyed by the **HTTP status code**, because that is what a standard client's back-off keys off; the standard error shape carries no retryable field, and inventing one would put this requirement in tension with SC-008. Three outcomes, because "the provider said no" splits along a line a client acts on. **Rate limiting is 429**: temporary, and the client should come back. **A refusal the provider will repeat MUST NOT carry a retryable status** — an exhausted account, a rejected credential, a model the provider does not serve are permanent until a human changes something, and answering 5xx there tells a client to retry a request that cannot ever succeed. **A genuine upstream fault is 5xx**, which is what that range is for. Permanence MUST be determined from the provider abstraction's own classification of its failures rather than from a list of status codes or exception types the system maintains, so that a failure kind added by a later release of that library is classified without changing this system. **Amended 2026-09-16** — the original text named only rate limiting and upstream failure. A provider account out of credit answers 402, which is neither, and was being reported as a retryable 502: the client's back-off then retried a request that could not succeed until somebody added credit. The third outcome is that case. The response body MUST stay conformant to the standard error shape. Where the system refuses on its own capacity rather than the provider's, it MUST send a `Retry-After` header, because that refusal is one whose duration dotCMS actually knows — and a client told to "retry shortly" in prose it cannot parse is a client that guesses. Where the **provider** supplies a `Retry-After`, the system MUST relay it **where the provider abstraction exposes it**. **It currently does not, and that is recorded rather than quietly dropped** (amended 2026-09-15). The client library surfaces a failed call as an exception carrying a status code and a message and nothing else; response headers are read and discarded on the error path, so the value never reaches dotCMS. Relaying it needs a custom HTTP client injected through the library's supported extension point — bounded work, not a dead end, but work that takes ownership of timeouts, proxying and multipart for every provider call and so belongs in its own change with its own review, not at the tail of this one. Until then a client backs off on the status alone, which every standard client already implements as its default. Revisit this first if throttling proves painful in practice. - **FR-032**: Each new path MUST register a request cost in the band used for operations that make a remote network round trip, so the existing instance-wide rate-limit backstop prices them correctly. A per-site or per-token AI spend quota is out of scope. - **FR-033**: No existing dotAI endpoint's behavior may change. Retrieval-augmented completions, semantic search, the vector corpus operations, and provider administration remain first-class, as no standard verb covers them. - **FR-034**: The three existing operations superseded by this family — text generation, image generation, and raw-prompt completion — MUST be documented as superseded while remaining fully functional. From 040a25e953520dc7322943307d9e7ae3697a2f04 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Thu, 17 Sep 2026 07:29:03 -0600 Subject: [PATCH 11/15] fix(ai): stop sending an output format the current image models reject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Image generation was broken against every current OpenAI image model. We sent response_format=b64_json on each request; OpenAI's own spec says that parameter is for dall-e-2 and dall-e-3 and "isn't supported for the GPT image models, which always return base64-encoded images". Those models answer 400 unknown_parameter, so every request to gpt-image-1, -2 and their variants failed. Only the legacy models worked — which is exactly what the twelve tests in this file stubbed. The parameter is no longer sent at all, rather than sent conditionally. An allowlist of models that accept it was the first instinct and is the worse option: that set is closed and legacy, while the set that rejects it grows with each release, so the list would rot. FR-012's purpose is unaffected — those models return base64 by themselves, and a model that answers with a URL is still fetched and re-encoded before a caller sees anything. Second defect, independent of the first and found by the same request. The provider's 400 came back as a retryable 502: it arrives as a bare HttpException rather than one of the library's classified non-retriable types, so it fell through to the retryable default. Any provider 4xx is now terminal and 5xx stays retryable, with tests on both sides of that boundary. This is the fourth case today of a permanent failure wearing a retryable status. The regression test asserts on what leaves dotCMS rather than on what comes back. A stub cannot fail the way a real provider does unless it is told to, which is why twelve green tests sat on top of a broken endpoint. Tests: 81 unit, 86 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- docs/backend/INFERENCE_API.md | 2 +- .../langchain4j/LangChain4jAIClient.java | 18 +++++--- .../inference/model/InferenceError.java | 16 +++++-- .../InferenceErrorTranslationTest.java | 46 +++++++++++++++++++ .../inference/rest/InferenceImagesTest.java | 38 +++++++++++++++ .../37431-openai-compatible-inference/spec.md | 2 +- 6 files changed, 111 insertions(+), 11 deletions(-) diff --git a/docs/backend/INFERENCE_API.md b/docs/backend/INFERENCE_API.md index 9091d4ca36dd..83202949449d 100644 --- a/docs/backend/INFERENCE_API.md +++ b/docs/backend/INFERENCE_API.md @@ -252,7 +252,7 @@ curl -X POST "https://demo.dotcms.com/api/inference/v1/images/generations" \ { "created": 1789000000, "data": [ { "b64_json": "iVBORw0KGgo…" }, { "b64_json": "R0lGODlhAQAB…" } ] } ``` -**Results are always base64 (`b64_json`), never a hosted URL.** The response shape has no `url` component at all. A URL would mean deciding storage, authentication and lifetime for an artifact generated from a prompt that may carry customer data, so this family declines to create a separately-addressable artifact. Where a provider offers the choice, dotCMS asks it for the inline form so nothing is minted upstream either; where a provider only returns a link, dotCMS fetches and re-encodes it. On those providers an artifact does exist upstream — the guarantee is that a caller never receives one. +**Results are always base64 (`b64_json`), never a hosted URL.** The response shape has no `url` component at all. A URL would mean deciding storage, authentication and lifetime for an artifact generated from a prompt that may carry customer data, so this family declines to create a separately-addressable artifact. dotCMS does not send an output-format parameter: the current image models reject it — they only ever return base64, so there is nothing to ask for — and sending it broke every request to them. Where a model answers with a link instead, dotCMS fetches and re-encodes it before the caller sees anything. On those providers an artifact does exist upstream — the guarantee is that a caller never receives one. `n` is honored; omitting it means one image. It is refused with a `400` naming `n` — never clamped, and never surfaced as a retryable upstream error — when it is: diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java index 2508b88de5f5..b85cf59303b0 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/LangChain4jAIClient.java @@ -83,7 +83,6 @@ public class LangChain4jAIClient implements AIClient { * Response format asked of an image provider by the {@code /api/inference/v1} family, so the * bytes arrive inline and no separately-addressable artifact is minted upstream. */ - private static final String INLINE_IMAGE_FORMAT = "b64_json"; /** * Cache-key discriminator for models the inference family borrows. Those differ from the ones @@ -215,9 +214,17 @@ public R withEmbeddingModel(final AppConfig appConfig, *

    The image counterpart of {@link #withChatModel}, with two differences that are the * caller's request rather than the site's configuration. The requested {@code size} is applied * over the configured one, because a caller who named a size changed both what they receive - * and what the site pays. And {@code responseFormat} is set to {@value #INLINE_IMAGE_FORMAT} - * so a provider that offers the choice never mints a hosted artifact in the first place; - * providers that ignore it still answer, and the caller re-encodes what comes back.

    + * and what the site pays.

    + * + *

    The output-format parameter is deliberately not sent. Asking for the + * inline form was the obvious way to stop a provider minting a hosted artifact, and it is what + * this did first — but the current generation of image models rejects the parameter outright + * ("Unknown parameter: 'response_format'"), because they only ever return base64 and there is + * nothing to choose. Sending it broke image generation against every one of them while working + * against the older models this family's tests stubbed. Not sending it is both simpler and + * safer than naming the models that accept it: the set that rejects it grows with every + * release, so any list would rot. Where a legacy model returns a URL instead, the caller still + * receives base64 — the answer is fetched and re-encoded, which this family already does.

    * *

    Both are folded into the cache key, so a model asked for one size is never handed to a * request that asked for another, and the legacy image endpoint — which wants the provider's @@ -235,8 +242,7 @@ public R withImageModel(final AppConfig appConfig, final ProviderConfig baseConfig = parseSection(appConfig.getProviderConfig(), IMAGE_SECTION); final boolean sized = size != null && !size.isBlank(); final ProviderConfig requestConfig = ImmutableProviderConfig.copyOf(baseConfig) - .withSize(sized ? size : baseConfig.size()) - .withResponseFormat(INLINE_IMAGE_FORMAT); + .withSize(sized ? size : baseConfig.size()); return executeWithFallbackTyped( cacheKeyPrefix(appConfig) + INFERENCE_KEY_SEGMENT diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java index 175a2596c0ea..aa396eb020a6 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java @@ -93,9 +93,19 @@ public static InferenceError fromProviderFailure(final Throwable failure, if (current instanceof dev.langchain4j.exception.RateLimitException) { return rateLimited(); } - if (current instanceof dev.langchain4j.exception.HttpException httpFailure - && httpFailure.statusCode() == 429) { - return rateLimited(); + if (current instanceof dev.langchain4j.exception.HttpException httpFailure) { + if (httpFailure.statusCode() == 429) { + return rateLimited(); + } + // Any other 4xx is the provider saying the request itself is wrong — a parameter + // it does not accept, a model it does not serve, an account it will not bill. It + // will say the same thing to an identical retry, so it must not leave here with a + // retryable status. This arrives as a bare HttpException rather than one of the + // library's classified types, which is how a permanently malformed request used + // to be reported as a temporary upstream fault. + if (httpFailure.statusCode() >= 400 && httpFailure.statusCode() < 500) { + return providerRefused(); + } } if (current instanceof dev.langchain4j.exception.NonRetriableException) { return providerRefused(); diff --git a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java index e1ee57cc2589..3b7453e65c5b 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java @@ -165,6 +165,52 @@ public void test_everyNonRetriableRefusal_isTerminal() { } } + /** + * Given a provider that rejected the request itself, delivered as a plain HTTP failure + * When it is translated + * Then the caller is told it is terminal, not asked to retry + * + *

    The case that broke image generation. OpenAI answers {@code 400 unknown_parameter} when + * sent a field the model does not accept, and that arrives as a bare {@link HttpException} + * rather than one of the library's classified exception types — so it fell past the + * non-retriable check and was reported as a retryable 502. A malformed request is as permanent + * as an empty account: the same request will be rejected the same way forever.

    + * + *

    Checked across the 4xx range rather than on 400 alone, since the same reasoning covers a + * model the provider does not serve and an account it will not bill.

    + */ + @Test + public void test_providerRejectedTheRequest_isTerminal() { + for (final int status : new int[]{400, 403, 404, 422}) { + final InferenceError error = InferenceError.fromProviderFailure( + new HttpException(status, "{\"error\":{\"message\":\"Unknown parameter: " + + "'response_format'.\",\"code\":\"unknown_parameter\"}}"), + UPSTREAM); + + assertFalse("A provider " + status + " will be a " + status + " on retry too, so it " + + "must not carry a retryable status", error.isRetryable()); + assertFalse("The provider's own wording stays in the log", + error.message().contains("response_format")); + } + } + + /** + * Given a provider 5xx delivered as a plain HTTP failure + * When it is translated + * Then it stays retryable + * + *

    The boundary. Narrowing 4xx to terminal must not drag the 5xx range with it — a provider + * having a bad minute is the case 502 and a client back-off exist for.

    + */ + @Test + public void test_providerServerError_staysRetryable() { + final InferenceError error = + InferenceError.fromProviderFailure(new HttpException(503, LEAKY), UPSTREAM); + + assertEquals(502, error.httpStatus()); + assertTrue(error.isRetryable()); + } + /** * Given a genuine upstream fault * When it is translated diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java index aedf5803ab29..1261524fc7d3 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java @@ -15,6 +15,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.verification.LoggedRequest; import com.liferay.portal.model.User; import org.junit.After; import org.junit.AfterClass; @@ -31,6 +32,7 @@ import java.util.Base64; import java.util.Date; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.UUID; @@ -789,6 +791,42 @@ private static String geminiImageProviderConfigJson() { EMBEDDINGS_MODEL, GEMINI_IMAGE_MODEL); } + /** + * Given an ordinary image request + * When the provider is called + * Then the outbound request carries no output-format parameter + * + *

    Every other test in this file stubs a provider that accepts whatever it is sent, so all + * of them passed while image generation was broken against every current OpenAI image model. + * Those models reject the format parameter outright — "Unknown parameter: 'response_format'" — + * because they only ever return base64 and there is nothing to choose. Sending it was a + * deliberate attempt to stop a provider minting a hosted artifact, and it worked against the + * older models these stubs imitate; against the current ones it turned every request into a + * provider-side 400.

    + * + *

    Asserted on what leaves dotCMS rather than on what comes back, because a stub cannot fail + * the way a real provider does unless it is told to. The requirement this protects is still + * met: those models return base64 by themselves, and a legacy model that answers with a URL is + * fetched and re-encoded before the caller sees anything.

    + */ + @Test + public void test_generations_sendsNoOutputFormatParameter() throws Exception { + final Response response = resource.generations( + mockRequest(host.getHostname(), bearerToken), mockResponse(), + host.getIdentifier(), generationFor(IMAGE_MODEL, PROMPT)); + + assertEquals(200, response.getStatus()); + + final List sent = + wireMockServer.findAll(postRequestedFor(urlPathEqualTo(IMAGES_PATH))); + assertEquals("Exactly one provider call is expected", 1, sent.size()); + + final JsonNode body = OBJECT_MAPPER.readTree(sent.get(0).getBodyAsString()); + assertFalse("The current image models reject this field, and asking for a format they " + + "already produce breaks every request to them: " + body, + body.has("response_format")); + } + /** * Asserts that an {@code n} below 1 is refused here — as a 400 in the standard error shape, * naming the field, with nothing reaching the provider. diff --git a/specs/37431-openai-compatible-inference/spec.md b/specs/37431-openai-compatible-inference/spec.md index a60c1e50cc97..b08dfa3e033b 100644 --- a/specs/37431-openai-compatible-inference/spec.md +++ b/specs/37431-openai-compatible-inference/spec.md @@ -137,7 +137,7 @@ The same client credentials and base URL also serve text embeddings and image ge - **FR-009**: The system MUST report token usage for streamed exchanges **when the client requests it through the standard streaming option**, and MUST NOT emit it otherwise. In the adopted format a usage event carries an empty choices array, which some stream readers do not tolerate — emitting it unconditionally would break clients SC-003 and SC-008 promise to support. - **FR-010**: The system MUST expose a model-listing operation returning the models the resolved site has configured, including every entry of its fallback chains, in the standard model-list shape. - **FR-011**: The system MUST expose an embeddings operation returning vectors in the standard embeddings shape, accepting input as **either a single string or an array of strings** — batching is how content is ordinarily embedded, so refusing the array form would fail the common case and make SC-008 false for it. The response is always a list, one entry per input, each carrying the index of the input it corresponds to so a caller can correlate results back to what they sent. **Every** element of an array MUST be a string — validating only the first would accept a mixed array and fail deeper in the provider with a message naming nothing useful. An absent, null or empty input MUST be refused with a validation error naming the field, rather than answered with an empty list that reads as success. A blank or whitespace-only string is refused on the same grounds, whether it arrives alone or as an element of an array — there is no meaningful embedding of nothing, and providers differ in whether they error or return a zero vector, which is precisely the inconsistency this family exists to hide. The error names the field as `input`; where one element of an array is at fault the **message** MUST identify which, since a caller who sent five hundred should not have to find it themselves. No separate cap on element count is imposed: the request-size limit of FR-037 already bounds what can arrive, and a second count-based limit would overlap it while needing a default of its own to get wrong. Because a site's embeddings model is configured separately from its chat models, the model validation of FR-023 MUST apply to this operation against the site's configured **embeddings** models rather than its chat models. -- **FR-012**: The system MUST expose an image-generation operation returning results in the standard image-generation shape, delivering the image as a **base64 payload** rather than a hosted URL. A URL would require deciding storage, authentication and lifetime for content generated from a possibly sensitive prompt; returning the bytes inline avoids creating a durable, separately-addressable artifact at all. Should a URL form be offered later, it MUST be authenticated and time-limited. Where a provider offers the choice, the system MUST **ask it for the inline form**, so that no hosted artifact is minted at all — returning base64 to the caller constrains only this family's own output, while a provider-minted URL is a real object on the provider's infrastructure, frequently public and long-lived, and that is the exposure this requirement reaches for. Where a provider offers no such choice and returns a URL regardless, the system MUST fetch and re-encode it rather than refuse the provider: the upstream artifact exists either way at that point, and declining to serve would break image generation on part of a multi-provider gateway to avoid an exposure already incurred. **The residual is deliberate and worth stating**: on those providers an addressable artifact does exist upstream, and this family's guarantee is only that it never hands one to a caller. `n` MUST be honored where the resolved site's model can honor it: the adopted format supports generating several images in one request and the provider abstraction exposes a multi-image call, so refusing it outright would be this family declining something the standard supports. A value below `1` MUST be refused with a validation error naming the field; omitting it means one. A request for several images against a model that can only produce one MUST be refused with a validation error naming `n`, **not** surfaced as an upstream failure. The capability is declared on the provider abstraction but only some implementations honour it, and the unsupported case throws from inside the client library; translating that into FR-031's upstream error would return a retryable status for a request that can never succeed, sending a standard client's back-off into retrying forever. Refusing per-model is also what the adopted format itself does — it documents `n` on the operation while restricting it for models that cannot honour it — so this is conformance, not a dotCMS deviation. Support MUST be determined from the model rather than from a hard-coded list of provider names, which would rot on the first library upgrade. `n` MUST also be bounded above by a configurable maximum (FR-037), because each additional image is provider spend on one priced request and FR-032 puts per-site quotas out of scope — without a ceiling, one accepted request is an unbounded bill. The default ceiling MUST be the one the adopted format documents for this field, so a client written against the standard meets the same limit here that it already handles there. Values below the minimum and above the maximum are both refused naming the field rather than clamped: a clamp answers a request for four hundred images with a handful and a bill, and the caller never learns that what they asked for is not what they got. **Corrected 2026-09-14** — an earlier draft refused any `n` other than 1, justified by the claim that the provider abstraction returns a single image per call. That claim was false and was not checked before being written; the refusal reached the tests before the error was found. The correction then introduced two further gaps, both recorded above: it assumed interface support implied implementation support, and it removed an accidental spend ceiling without replacing it. `size` MUST be passed through to the provider, for the same reason FR-013 passes the sampling parameters through: it changes both what the caller receives and what the site pays. Where the site also carries an `imageSize` App setting, the caller's value wins and the site setting is the default for a request that omits it — which is what the existing dotAI image endpoint already does via `applyRequestSize`, so FR-027's "behaves as the existing endpoints do" is satisfied rather than strained. Values are the format's `WIDTHxHEIGHT` strings; a value a given provider will not accept is the provider's refusal to make, translated per FR-031, not something this family maintains a per-vendor table to pre-empt. +- **FR-012**: The system MUST expose an image-generation operation returning results in the standard image-generation shape, delivering the image as a **base64 payload** rather than a hosted URL. A URL would require deciding storage, authentication and lifetime for content generated from a possibly sensitive prompt; returning the bytes inline avoids creating a durable, separately-addressable artifact at all. Should a URL form be offered later, it MUST be authenticated and time-limited. The system MUST NOT send an output-format parameter to the provider. **Amended 2026-09-17**: an earlier version required asking for the inline form, so that no hosted artifact was minted at all. That reasoning still holds, but the parameter is rejected outright by the current generation of image models — they only ever return base64, so there is nothing to choose, and asking turned every request to them into a provider-side 400 while working against the older models this family's tests stubbed. Not sending it is also more durable than naming the models that accept it: the set that accepts the parameter is closed and legacy, while the set that rejects it grows with each release. The requirement's purpose survives unchanged — those models return base64 by themselves, and the fetch-and-re-encode path below still guarantees a caller never receives a URL from a model that answers with one. Where a provider offers no such choice and returns a URL regardless, the system MUST fetch and re-encode it rather than refuse the provider: the upstream artifact exists either way at that point, and declining to serve would break image generation on part of a multi-provider gateway to avoid an exposure already incurred. **The residual is deliberate and worth stating**: on those providers an addressable artifact does exist upstream, and this family's guarantee is only that it never hands one to a caller. `n` MUST be honored where the resolved site's model can honor it: the adopted format supports generating several images in one request and the provider abstraction exposes a multi-image call, so refusing it outright would be this family declining something the standard supports. A value below `1` MUST be refused with a validation error naming the field; omitting it means one. A request for several images against a model that can only produce one MUST be refused with a validation error naming `n`, **not** surfaced as an upstream failure. The capability is declared on the provider abstraction but only some implementations honour it, and the unsupported case throws from inside the client library; translating that into FR-031's upstream error would return a retryable status for a request that can never succeed, sending a standard client's back-off into retrying forever. Refusing per-model is also what the adopted format itself does — it documents `n` on the operation while restricting it for models that cannot honour it — so this is conformance, not a dotCMS deviation. Support MUST be determined from the model rather than from a hard-coded list of provider names, which would rot on the first library upgrade. `n` MUST also be bounded above by a configurable maximum (FR-037), because each additional image is provider spend on one priced request and FR-032 puts per-site quotas out of scope — without a ceiling, one accepted request is an unbounded bill. The default ceiling MUST be the one the adopted format documents for this field, so a client written against the standard meets the same limit here that it already handles there. Values below the minimum and above the maximum are both refused naming the field rather than clamped: a clamp answers a request for four hundred images with a handful and a bill, and the caller never learns that what they asked for is not what they got. **Corrected 2026-09-14** — an earlier draft refused any `n` other than 1, justified by the claim that the provider abstraction returns a single image per call. That claim was false and was not checked before being written; the refusal reached the tests before the error was found. The correction then introduced two further gaps, both recorded above: it assumed interface support implied implementation support, and it removed an accidental spend ceiling without replacing it. `size` MUST be passed through to the provider, for the same reason FR-013 passes the sampling parameters through: it changes both what the caller receives and what the site pays. Where the site also carries an `imageSize` App setting, the caller's value wins and the site setting is the default for a request that omits it — which is what the existing dotAI image endpoint already does via `applyRequestSize`, so FR-027's "behaves as the existing endpoints do" is satisfied rather than strained. Values are the format's `WIDTHxHEIGHT` strings; a value a given provider will not accept is the provider's refusal to make, translated per FR-031, not something this family maintains a per-vendor table to pre-empt. - **FR-013**: The system MUST ignore request fields that do not change output semantics — the incidental fields a standard client sends by default — so a client's default payload succeeds. It MUST NOT silently ignore a field that changes what the caller gets or pays for. Specifically, the common sampling parameters (`temperature`, `max_tokens`, `top_p`, `stop`) MUST be passed through to the provider, and a field that changes output semantics but cannot be honored (for example a request for several choices, which this family does not support) MUST be rejected with a standard-shaped validation error naming it, rather than accepted and disregarded. Silently dropping a caller's token ceiling is a cost and correctness failure, not a compatibility courtesy. - **FR-014**: The legacy non-chat completion operation is explicitly out of scope and MUST NOT be added; it is deprecated in the format being adopted. From 6502a32700f8b363b703b81774fc96e5393278fb Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Thu, 17 Sep 2026 15:15:35 -0600 Subject: [PATCH 12/15] test(ai): guard the two defects that had no test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven of the nine defects found by using this family against a real provider got a guard as they were fixed. These are the other two. An integration test class that no MainSuite lists compiles, passes under -Dit.test=, and is never executed by CI — no error, no warning, no coverage, and a green build. Three classes here were in that state and it was caught by eye; at least one class elsewhere in the repo still is. The failure mode is invisible by construction, because the only thing that can notice an unregistered test is another test. The guard reads MainSuite2b's own @SuiteClasses by reflection rather than a copy, and discovers test classes from the compiled output rather than a hand-maintained list, so a class added later is covered without anyone remembering this file exists. Verified by removing InferenceImagesTest from the suite: it failed naming that class. The second guards the opposite mistake from everything else in InferenceLoggingTest. Callers are told a failure's detail is in the server log; for a while that was false, because the streaming path logged only an exception class name and discarded the provider's explanation. A live failure then took three rounds of guessing to diagnose, since the evidence was being destroyed at the moment it was produced. Verified by reverting that log line. The two assertions are mirrors and the file says so: the caller's content must never be logged, the provider's explanation must always be. Each is an easy way to break the other. Not attempted here, and worth its own change: every provider stub in these tests accepts whatever it is sent, which is what let a parameter OpenAI rejects ship past twelve green tests. Making the stubs strict would catch that whole class, but it touches fixtures across several files and risks brittleness against harmless additions. Tests: 81 unit, 88 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/test/java/com/dotcms/MainSuite2b.java | 2 + .../inference/rest/InferenceLoggingTest.java | 38 +++++++++ .../rest/InferenceTestsAreRegisteredTest.java | 85 +++++++++++++++++++ 3 files changed, 125 insertions(+) create mode 100644 dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceTestsAreRegisteredTest.java diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index 7c08609ea6d1..10ebcaf90591 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -78,6 +78,7 @@ import com.dotcms.inference.rest.InferenceModelsTest; import com.dotcms.inference.rest.InferenceModelValidationTest; import com.dotcms.inference.rest.InferenceSiteIsolationTest; +import com.dotcms.inference.rest.InferenceTestsAreRegisteredTest; import com.dotcms.inference.rest.InferenceSiteResolutionTest; import com.dotcms.integritycheckers.ContentFileAssetIntegrityCheckerTest; import com.dotcms.integritycheckers.ContentPageIntegrityCheckerTest; @@ -467,6 +468,7 @@ InferenceModelsTest.class, InferenceModelValidationTest.class, InferenceSiteIsolationTest.class, + InferenceTestsAreRegisteredTest.class, InferenceSiteResolutionTest.class, TimeMachineAPITest.class, Task240513UpdateContentTypesSystemFieldTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java index f465d91685ef..405ca5ea3c04 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java @@ -237,6 +237,44 @@ public void test_failedImageGeneration_doesNotLogThePrompt() { assertSentinelNeverLogged("an image request the provider refused"); } + /** + * Given a streamed completion the provider refuses + * When the failure is reported + * Then the provider's own explanation reaches the log + * + *

    The mirror of every other assertion in this file, and it exists because the opposite + * mistake is just as damaging. The caller gets a safe sentence saying the detail is in the + * server log; if the log holds only an exception class name, that sentence is a lie and an + * operator has no way to tell an exhausted account from a rejected key from a malformed + * request. That is exactly what happened: the streaming path logged + * {@code "Inference stream failed: InvalidRequestException"} and nothing else, and a live + * failure took three rounds of guessing to diagnose because the evidence was being discarded + * at the moment it was produced.

    + * + *

    Note this does not contradict the sentinel assertions above. What must never be logged is + * the caller's content; what must always be logged is the provider's + * explanation of why it refused.

    + */ + @Test + public void test_failedStream_logsTheProvidersExplanation() throws Exception { + final String providerReason = "zqx-provider-said-no-9f3c"; + stubProvider(COMPLETIONS_PATH, 400, + "{\"error\":{\"message\":\"" + providerReason + "\",\"code\":\"bad_request\"}}"); + + final Response response = chatResource.completions( + mockRequest(), mockResponse(), host.getIdentifier(), chatRequest(true)); + if (response.getEntity() instanceof StreamingOutput streaming) { + streaming.write(new ByteArrayOutputStream()); + } + + final boolean logged = appender.messages().stream() + .anyMatch(message -> message != null && message.contains(providerReason)); + + assertTrue("The caller is told the detail is in the server log, so it has to be there: a " + + "bare exception type leaves an operator with nothing to act on", + logged); + } + /** * Fails when the caller's content reached a log, naming what was being exercised. * diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceTestsAreRegisteredTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceTestsAreRegisteredTest.java new file mode 100644 index 000000000000..51b86649af7c --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceTestsAreRegisteredTest.java @@ -0,0 +1,85 @@ +package com.dotcms.inference.rest; + +import com.dotcms.MainSuite2b; +import org.junit.Test; +import org.junit.runners.Suite; + +import java.io.File; +import java.net.URL; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +/** + * Fails if an integration test in this family is not registered in a suite CI actually runs. + * + *

    A test class that no {@code MainSuite} lists compiles, passes locally under + * {@code -Dit.test=}, and is never executed in CI. There is no error, no warning + * and no coverage — the build stays green and the class silently protects nothing. Three classes + * in this family were in exactly that state and it was caught by eye, not by tooling; at least one + * class elsewhere in the repo still is.

    + * + *

    This guard exists because the failure mode is invisible by construction: the only thing that + * would notice an unregistered test is another test. It reads the suite's own registration list + * rather than a copy, so it cannot drift from what CI runs, and it discovers test classes from the + * compiled output rather than a hand-maintained list, so a class added tomorrow is covered without + * anyone remembering this file exists.

    + */ +public class InferenceTestsAreRegisteredTest { + + /** + * Given every compiled test class in this package + * When the suite CI runs is inspected + * Then each one is registered in it + */ + @Test + public void test_everyInferenceTestClassIsRegisteredInTheSuite() throws Exception { + final Set registered = + Arrays.stream(MainSuite2b.class.getAnnotation(Suite.SuiteClasses.class).value()) + .map(Class::getName) + .collect(Collectors.toSet()); + + final List unregistered = new ArrayList<>(); + for (final Class testClass : compiledTestClassesInThisPackage()) { + final boolean hasTests = Arrays.stream(testClass.getDeclaredMethods()) + .anyMatch(method -> method.isAnnotationPresent(Test.class)); + if (hasTests && !registered.contains(testClass.getName())) { + unregistered.add(testClass.getSimpleName()); + } + } + + assertTrue("These test classes are never run by CI — they compile, they pass when named " + + "explicitly, and the build stays green while they protect nothing. Add " + + "them to MainSuite2b's @SuiteClasses: " + unregistered, + unregistered.isEmpty()); + } + + /** + * @return every compiled class in this package, read from the build output rather than a list + * someone has to remember to update + */ + private List> compiledTestClassesInThisPackage() throws Exception { + final String packagePath = getClass().getPackageName().replace('.', '/'); + final URL location = getClass().getClassLoader().getResource(packagePath); + assertNotNull("Could not locate the compiled test classes; this guard would silently " + + "pass without checking anything", location); + + final File[] files = new File(location.toURI()).listFiles( + (dir, name) -> name.endsWith("Test.class") && !name.contains("$")); + assertNotNull(files); + assertTrue("No compiled test classes found — the guard is not actually looking at " + + "anything", files.length > 0); + + final List> classes = new ArrayList<>(files.length); + for (final File file : files) { + final String simpleName = file.getName().replace(".class", ""); + classes.add(Class.forName(getClass().getPackageName() + "." + simpleName)); + } + return classes; + } +} From 079facd9218491445ddfd867d8d3f0a5f1ecbd6a Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Thu, 17 Sep 2026 18:02:26 -0600 Subject: [PATCH 13/15] fix(ai): name the capability when refusing a model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refusal read "The model 'x' is not configured for this site" regardless of which endpoint refused it. That is a contradiction a caller cannot resolve from the API alone: GET /models lists every model the site has configured across chat, embeddings and images, so they can see an id in the listing and then be told it is not configured. Both halves were true and the message told neither. The model is configured — for a different capability. It now says which: "...is not configured for chat on this site". This follows directly from listing all capabilities in one flat list, which the adopted format requires because its model object has no field for what a model is for. With nowhere in the listing to record capability, the refusal is the only place that information can live, so it has to carry it. Noticed by reading every message the family produces in one run rather than by a failing test — all three refusals assert the status and the param name, and both stayed correct. Tests: 81 unit, 88 integration, all green. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/dotcms/inference/model/InferenceError.java | 12 ++++++++++-- .../inference/rest/ChatCompletionsResource.java | 2 +- .../dotcms/inference/rest/EmbeddingsResource.java | 2 +- .../com/dotcms/inference/rest/ImagesResource.java | 2 +- .../inference/rest/view/InferenceErrorView.java | 2 +- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java index aa396eb020a6..2834481995c1 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java @@ -41,9 +41,17 @@ public static InferenceError invalidRequest(final String message, final String p * @param model the model the caller asked for * @return a 404 error in the shape clients recognise as an unknown model */ - public static InferenceError noSuchModel(final String model) { + public static InferenceError noSuchModel(final String model, final String capability) { + // Naming the capability rather than saying "not configured for this site", which is true + // of the site's chat models only in the sense that matters and false in the sense a caller + // reads. The model listing returns every model the site has configured across all three + // capabilities, because the adopted format has nowhere to record what a model is for — so + // a caller can see an id in the list and then be told it is not configured, with no way to + // resolve the contradiction from the API alone. This refusal is the only place that + // information can live. return new InferenceError("invalid_request_error", - "The model '" + model + "' is not configured for this site", "model", 404); + "The model '" + model + "' is not configured for " + capability + " on this site", + "model", 404); } /** diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index 9a5338e7634e..cbcc9ad9b33d 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -391,7 +391,7 @@ private Response refuseUnconfiguredModel(final ResolvedAiContext context, if (!configuredModels.contains(requestedModel.trim())) { Logger.warn(this, "Site " + AiHostResolver.sanitize(context.servingSiteId()) + " has no chat model matching the requested one"); - return errorResponse(InferenceError.noSuchModel(echoable(requestedModel))); + return errorResponse(InferenceError.noSuchModel(echoable(requestedModel), "chat")); } return null; diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java index 9b827a3e6203..cc5608f621fd 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/EmbeddingsResource.java @@ -303,7 +303,7 @@ private Response refuseUnconfiguredModel(final ResolvedAiContext context, if (!configuredModels.contains(requestedModel.trim())) { Logger.warn(this, "Site " + AiHostResolver.sanitize(context.servingSiteId()) + " has no embeddings model matching the requested one"); - return errorResponse(InferenceError.noSuchModel(echoable(requestedModel))); + return errorResponse(InferenceError.noSuchModel(echoable(requestedModel), "embeddings")); } return null; diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java index a4cd3cd1f524..aede5ab1f2c7 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ImagesResource.java @@ -330,7 +330,7 @@ private Response refuseUnconfiguredModel(final ResolvedAiContext context, if (!configuredModels.contains(requestedModel.trim())) { Logger.warn(this, "Site " + AiHostResolver.sanitize(context.servingSiteId()) + " has no image model matching the requested one"); - return errorResponse(InferenceError.noSuchModel(echoable(requestedModel))); + return errorResponse(InferenceError.noSuchModel(echoable(requestedModel), "images")); } return null; diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/view/InferenceErrorView.java b/dotCMS/src/main/java/com/dotcms/inference/rest/view/InferenceErrorView.java index de43475314c5..17c9e360a704 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/view/InferenceErrorView.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/view/InferenceErrorView.java @@ -32,7 +32,7 @@ public record InferenceErrorView(@JsonProperty("error") @Schema(description = "T @Schema(description = "Error detail") public record Body( @JsonProperty("message") @Schema(description = "Human-readable description of the failure", - example = "The model 'gpt-4o' is not configured for this site") String message, + example = "The model 'gpt-4o' is not configured for chat on this site") String message, @JsonProperty("type") @Schema(description = "Error family", example = "invalid_request_error") String type, @JsonProperty("param") @Schema(description = "Offending request field, when applicable", From 3c57d15d079f9e9aebba93cedb03d6929a1624dc Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Thu, 17 Sep 2026 21:08:44 -0600 Subject: [PATCH 14/15] revert(ai): drop the deprecation of the existing /v1/ai endpoints Marking text/generate, image/generate and completions/rawPrompt deprecated was a statement about their future, and that needs a wider discussion than this change. Removes the five annotations and their javadoc notes, and the generated contract no longer marks those operations deprecated. The new endpoints now stand alongside the existing ones with no claim about what happens to the old ones. Nothing else changes: the shipped endpoints were already untouched behaviourally, and those three files are now byte-identical to the feature branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../main/java/com/dotcms/ai/rest/CompletionsResource.java | 4 ---- .../src/main/java/com/dotcms/ai/rest/ImageResource.java | 8 -------- dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java | 8 -------- dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml | 5 ----- 4 files changed, 25 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java index cdec0ae4be9e..f36da6942d0e 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java @@ -111,11 +111,7 @@ public final Response summarizeFromContent(@Context final HttpServletRequest req * @param response the HTTP response * @param formIn the form data containing the prompt * @return a Response object containing the generated completions - * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the - * OpenAI-compatible shape any standard client and agent framework already - * speaks. Not marked {@code forRemoval}: no removal date is committed. */ - @Deprecated(since = "Sep 14th, 26") @Path("/rawPrompt") @POST @JSONP diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java index ed1347ee9810..b3aa0cc54fde 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/ImageResource.java @@ -61,11 +61,7 @@ public final Response indexByInode(@Context final HttpServletRequest request, * @param prompt the prompt to generate images from * @return a Response object containing the generated images * @throws IOException if an I/O error occurs - * @deprecated Superseded by {@code POST /api/inference/v1/images/generations}, which answers in the - * OpenAI-compatible shape any standard client and agent framework already - * speaks. Not marked {@code forRemoval}: no removal date is committed. */ - @Deprecated(since = "Sep 14th, 26") @GET @JSONP @Path("/generate") @@ -86,11 +82,7 @@ public final Response indexByInode(@Context final HttpServletRequest request, * @param aiImageRequestDTO the AIImageRequestDTO containing the prompt and other parameters * @return a Response object containing the generated images * @throws IOException if an I/O error occurs - * @deprecated Superseded by {@code POST /api/inference/v1/images/generations}, which answers in the - * OpenAI-compatible shape any standard client and agent framework already - * speaks. Not marked {@code forRemoval}: no removal date is committed. */ - @Deprecated(since = "Sep 14th, 26") @POST @Path("/generate") @Produces(MediaType.APPLICATION_JSON) diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java b/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java index e27945f95f95..c49ce73adecb 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/TextResource.java @@ -43,11 +43,7 @@ public class TextResource { * @param prompt the prompt to generate text from * @return a Response object containing the generated text * @throws IOException if an I/O error occurs - * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the - * OpenAI-compatible shape any standard client and agent framework already - * speaks. Not marked {@code forRemoval}: no removal date is committed. */ - @Deprecated(since = "Sep 14th, 26") @Path("/generate") @GET @Produces(MediaType.APPLICATION_JSON) @@ -66,11 +62,7 @@ public Response doGet(@Context final HttpServletRequest request, * @param form the form data containing the prompt * @return a Response object containing the generated text * @throws IOException if an I/O error occurs - * @deprecated Superseded by {@code POST /api/inference/v1/chat/completions}, which answers in the - * OpenAI-compatible shape any standard client and agent framework already - * speaks. Not marked {@code forRemoval}: no removal date is committed. */ - @Deprecated(since = "Sep 14th, 26") @Path("/generate") @POST @Produces(MediaType.APPLICATION_JSON) diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index 15ab85dbed6f..f23cc9f6b583 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -3053,7 +3053,6 @@ paths: - AI /v1/ai/completions/rawPrompt: post: - deprecated: true description: Processes raw prompts directly through the AI service without content preprocessing. Supports both streaming and non-streaming responses. operationId: rawPrompt @@ -3216,7 +3215,6 @@ paths: - AI /v1/ai/image/generate: get: - deprecated: true operationId: indexByInode_1 parameters: - in: query @@ -3231,7 +3229,6 @@ paths: tags: - AI post: - deprecated: true operationId: handleImageRequest requestBody: content: @@ -3471,7 +3468,6 @@ paths: - AI /v1/ai/text/generate: get: - deprecated: true operationId: doGet parameters: - in: query @@ -3486,7 +3482,6 @@ paths: tags: - AI post: - deprecated: true operationId: doPost requestBody: content: From 01aa0d440a18f421ed4fd1bf7515d4799ece7071 Mon Sep 17 00:00:00 2001 From: Freddy Montes Date: Fri, 18 Sep 2026 07:30:23 -0600 Subject: [PATCH 15/15] docs(ai): state the rules directly instead of citing spec identifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments across the inference code and its tests referred to requirements by identifier — "FR-012 wants no hosted artifact", "SC-008 for the listing" — which only means something with the spec open alongside. Each one now states the rule it was pointing at, so the comment stands on its own for a reader in the code. No behaviour change: every file is byte-identical once comments are stripped, apart from five JUnit failure-message strings that named identifiers the same way. Those are the first argument to assertTrue/assertNotNull/assertFalse and render only on a failing assertion; the compared expressions are untouched. Co-Authored-By: Claude Opus 5 (1M context) --- .../ai/client/langchain4j/ProviderConfig.java | 2 +- .../com/dotcms/ai/rest/AiHostResolver.java | 10 +- .../inference/model/CallerSafeException.java | 4 +- .../inference/model/InferenceError.java | 2 +- .../rest/ChatCompletionsResource.java | 5 +- .../rest/InferenceRequestAttributes.java | 2 +- .../langchain4j/InferenceErrorLeakTest.java | 6 +- .../ChatCompletionRequestMapperTest.java | 10 +- .../ChatCompletionResponseMapperTest.java | 8 +- .../InferenceErrorTranslationTest.java | 13 +- .../InferenceRequestValidationTest.java | 16 +-- .../inference/RequestSizeLimitFilterTest.java | 2 +- .../com/dotcms/inference/SseFailureTest.java | 10 +- .../dotcms/inference/SseSerializerTest.java | 6 +- .../inference/SseToolCallStreamTest.java | 4 +- .../dotcms/inference/SseUsageGatingTest.java | 10 +- .../com/dotcms/rest/api/CorsFilterTest.java | 4 +- .../rest/ChatCompletionsStreamingTest.java | 25 ++-- .../inference/rest/ChatCompletionsTest.java | 10 +- .../rest/InferenceAuthorizationTest.java | 20 +-- .../rest/InferenceClientConformanceTest.java | 16 +-- .../rest/InferenceEmbeddingsTest.java | 92 +++++++------- .../inference/rest/InferenceFallbackTest.java | 8 +- .../inference/rest/InferenceImagesTest.java | 117 +++++++++--------- .../inference/rest/InferenceLoggingTest.java | 5 +- .../rest/InferenceModelValidationTest.java | 23 ++-- .../inference/rest/InferenceModelsTest.java | 22 ++-- .../rest/InferenceSiteIsolationTest.java | 10 +- .../rest/InferenceSiteResolutionTest.java | 22 ++-- 29 files changed, 245 insertions(+), 239 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/ProviderConfig.java b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/ProviderConfig.java index ce389c738af4..ed732cf9ac71 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/ProviderConfig.java +++ b/dotCMS/src/main/java/com/dotcms/ai/client/langchain4j/ProviderConfig.java @@ -135,7 +135,7 @@ default List allModels() { * made by the caller that builds the model, which is why it is left null by default and the * strategies pass it on only when somebody asked for one. * - *

    {@code /api/inference/v1/images/generations} sets {@code b64_json} because FR-012 wants + *

    {@code /api/inference/v1/images/generations} sets {@code b64_json} because it returns * no hosted artifact minted upstream at all, not merely none returned to the caller. The * legacy {@code /api/v1/ai/image} endpoint leaves it unset and keeps the provider's default, * because its own response contract is a URL.

    diff --git a/dotCMS/src/main/java/com/dotcms/ai/rest/AiHostResolver.java b/dotCMS/src/main/java/com/dotcms/ai/rest/AiHostResolver.java index 0ec0cf64f443..273e4d5318fd 100644 --- a/dotCMS/src/main/java/com/dotcms/ai/rest/AiHostResolver.java +++ b/dotCMS/src/main/java/com/dotcms/ai/rest/AiHostResolver.java @@ -117,11 +117,11 @@ private static Host resolveFromRequest(final HttpServletRequest request) { * *

    Deliberately not {@code getCurrentHostNoThrow}, which is the obvious call and the wrong * one: it reads the {@code host_id} and {@code Host} request parameters before it ever looks - * at the server name (`HostWebAPIImpl.getCurrentHostFromRequest`), and FR-025 requires those - * be ignored on this family. They were already ignored whenever the server name matched a - * site, because that path returns before reaching here — so the effect was that a legacy - * override was honoured in precisely the case where it could redirect which site's - * credentials get spent, and ignored everywhere it was harmless.

    + * at the server name (`HostWebAPIImpl.getCurrentHostFromRequest`), and those parameters must + * not decide which site's credentials get spent here. They were already ignored whenever the + * server name matched a site, because that path returns before reaching here — so the effect + * was that a legacy override was honoured in precisely the case where it could redirect + * which site's credentials get spent, and ignored everywhere it was harmless.

    * * @return the default site */ diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java b/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java index 786a9cd60b02..bfd5c90c5d07 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/CallerSafeException.java @@ -15,8 +15,8 @@ * this type is a statement that the message was authored here and may be returned; everything * else is answered with a generic sentence and logged in full. Extending * {@link IllegalArgumentException} keeps existing callers that catch the broader type working - * unchanged, which matters because the throw sites are shared with the endpoints FR-033 puts out - * of bounds.

    + * unchanged, which matters because the throw sites are shared with the older dotAI endpoints, + * whose behaviour must not shift.

    */ public class CallerSafeException extends IllegalArgumentException { diff --git a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java index 2834481995c1..e80ace571d53 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java +++ b/dotCMS/src/main/java/com/dotcms/inference/model/InferenceError.java @@ -65,7 +65,7 @@ public static InferenceError upstream(final String message) { /** * Translates a provider failure into the status a standard client's back-off keys off. * - *

    FR-031. A provider refusing on rate limit and a provider genuinely broken are different + *

    A provider refusing on rate limit and a provider genuinely broken are different * events with different right answers, and collapsing both into 502 tells a client to retry a * throttled request on the wrong schedule while telling it nothing about when to come back. * Rate limiting is 429; anything else upstream stays 502.

    diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java index cbcc9ad9b33d..65617e18c5d3 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/ChatCompletionsResource.java @@ -211,8 +211,9 @@ public class ChatCompletionsResource { // Pricing this honestly means charging on what a request actually consumed, token counts being // the unit that matches what the provider bills, which is a design this endpoint cannot settle // on its own: it needs a decision about where usage is metered, what happens when a stream - // fails halfway, and how that reconciles with the per-site spend attribution FR-032 puts out - // of scope. Until that exists, the flat price stands and is known to be wrong for streams. + // fails halfway, and how it reconciles with per-site spend attribution, which is not + // something this endpoint family answers. Until that exists, the flat price stands and is + // known to be wrong for streams. @RequestCost(Price.HTTP_FETCH) @Path("/completions") @Consumes(MediaType.APPLICATION_JSON) diff --git a/dotCMS/src/main/java/com/dotcms/inference/rest/InferenceRequestAttributes.java b/dotCMS/src/main/java/com/dotcms/inference/rest/InferenceRequestAttributes.java index 319b5facc87b..3fae3e4a697b 100644 --- a/dotCMS/src/main/java/com/dotcms/inference/rest/InferenceRequestAttributes.java +++ b/dotCMS/src/main/java/com/dotcms/inference/rest/InferenceRequestAttributes.java @@ -5,7 +5,7 @@ * *

    The resolved site is published here rather than returned, so the response filter can report * it even on a response the resource never produced — an exception mapped to a 4xx, or a stream - * that failed after it began. FR-020 asks for the serving site on every response, and a + * that failed after it began. The serving site is reported on every response, and a * value only the happy path can set would not deliver that.

    */ public final class InferenceRequestAttributes { diff --git a/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java b/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java index 56fe3e5c3edf..040a5670483d 100644 --- a/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java +++ b/dotCMS/src/test/java/com/dotcms/ai/client/langchain4j/InferenceErrorLeakTest.java @@ -87,9 +87,9 @@ public void test_messageDotcmsWrote_isReturned() { * When it is translated * Then it stays on the upstream path, rate limits included * - *

    Guards against the fix above swallowing the FR-031 translation: the new branch sits in - * front of it, and a version that matched too eagerly would turn every 429 into a 400 and - * stop clients backing off.

    + *

    Guards against the fix above swallowing the translation of upstream status codes: the new + * branch sits in front of it, and a version that matched too eagerly would turn every 429 into + * a 400 and stop clients backing off.

    */ @Test public void test_upstreamFailures_stillTranslateByStatus() { diff --git a/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionRequestMapperTest.java b/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionRequestMapperTest.java index f1f405e15e7a..8bb6591adae8 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionRequestMapperTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionRequestMapperTest.java @@ -286,7 +286,7 @@ public void test_toInferenceRequest_responseFormatJsonObject_mapsToJsonObjectTyp /** * Given a request carrying temperature, max_tokens, top_p and stop, * When mapped to the internal request, - * Then every sampling parameter reaches the internal field it belongs to (FR-013) — a dropped + * Then every sampling parameter reaches the internal field it belongs to — a dropped * {@code max_tokens} is both a cost and a correctness failure, so it is asserted by value. */ @Test @@ -311,7 +311,7 @@ public void test_toInferenceRequest_samplingParameters_passThroughUnchanged() th * Given a request that says nothing about streaming, * When mapped to the internal request, * Then both {@code stream} and {@code includeUsageInStream} default to false, and the optional - * sampling parameters stay null rather than being invented (FR-009). + * sampling parameters stay null rather than being invented. */ @Test public void test_toInferenceRequest_streamAbsent_defaultsToFalse() throws Exception { @@ -332,7 +332,7 @@ public void test_toInferenceRequest_streamAbsent_defaultsToFalse() throws Except /** * Given a streaming request asking for usage through the standard streaming option, * When mapped to the internal request, - * Then {@code stream} is true and {@code includeUsageInStream} is true (FR-009). + * Then {@code stream} is true and {@code includeUsageInStream} is true. */ @Test public void test_toInferenceRequest_streamOptionsIncludeUsage_mapsToIncludeUsageInStream() @@ -352,8 +352,8 @@ public void test_toInferenceRequest_streamOptionsIncludeUsage_mapsToIncludeUsage /** * Given a streaming request with no stream options at all, * When mapped to the internal request, - * Then streaming is on but usage is withheld — {@code includeUsageInStream} defaults to false - * (FR-009). The view is built through its canonical constructor here, so the default does not + * Then streaming is on but usage is withheld — {@code includeUsageInStream} defaults to false. + * The view is built through its canonical constructor here, so the default does not * depend on Jackson leaving the field null. */ @Test diff --git a/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionResponseMapperTest.java b/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionResponseMapperTest.java index a57d4593dfaf..e1553ab878e5 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionResponseMapperTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/ChatCompletionResponseMapperTest.java @@ -27,13 +27,13 @@ * *

    Coverage:

    *
      - *
    • Envelope fields — {@code id}, {@code object}, {@code created}, {@code model} (FR-003).
      • + *
    • Envelope fields — {@code id}, {@code object}, {@code created}, {@code model}.
      • *
    • {@code model} reporting the model that actually served the request, which is how a caller * detects that a site's fallback chain moved on to a later entry.
      • *
    • A text answer rendering as exactly one choice, index {@code 0}, role {@code assistant}, * finish reason {@code stop}.
      • *
    • A tool-calling answer rendering {@code tool_calls}, with the provider-assigned id carried - * verbatim rather than synthesised (FR-004).
      • + * verbatim rather than synthesised. *
    • Every serializable {@link FinishReason} mapping to its wire string.
      • *
    • Usage mapping to {@code prompt_tokens} / {@code completion_tokens} / {@code total_tokens}, * and {@link InferenceUsage#UNREPORTED} leaving usage absent rather than fabricating zeros — @@ -49,7 +49,7 @@ public class ChatCompletionResponseMapperTest { /** * Given a completed response with an id, a model and a creation time. * When it is rendered to the wire shape. - * Then every envelope field is populated and {@code object} is {@code chat.completion}. (FR-003) + * Then every envelope field is populated and {@code object} is {@code chat.completion}. */ @Test public void test_toView_textResponse_populatesEnvelopeFields() { @@ -140,7 +140,7 @@ public void test_toView_textAnswer_mapsToSingleAssistantChoice() { * Given an assistant turn asking for two tools to be executed. * When it is rendered to the wire shape. * Then the finish reason is {@code tool_calls} and each entry carries its provider-assigned id - * verbatim, type {@code function}, and the function name and raw JSON arguments. (FR-004) + * verbatim, type {@code function}, and the function name and raw JSON arguments. */ @Test public void test_toView_toolCallAnswer_mapsToolCallsCarryingIdVerbatim() { diff --git a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java index 3b7453e65c5b..46d4e159093a 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/InferenceErrorTranslationTest.java @@ -14,11 +14,11 @@ import static org.junit.Assert.assertTrue; /** - * Covers FR-031's translation of a provider failure into the status a client backs off on. + * Covers the translation of a provider failure into the status a client backs off on. * *

      The distinction this pins is not cosmetic. A standard client decides what to do next from the * HTTP status alone — the error shape carries no retryable field, and inventing one would break the - * no-adapter promise of SC-008 — so answering 502 to a throttled request tells the client the + * no-adapter promise — so answering 502 to a throttled request tells the client the * provider is broken rather than busy, and it retries on the wrong schedule. Every assertion here * is about that one decision.

      */ @@ -92,8 +92,9 @@ public void test_rateLimitWrappedByTheFallbackChain_isStillA429() { * When the failure is translated * Then it is 502, and the translation terminates * - *

      The 5xx half of FR-031, plus the loop guard: a cause chain that points at itself is rare - * but legal, and a walk without the guard hangs the request thread rather than failing it.

      + *

      The 5xx half of the translation rule, plus the loop guard: a cause chain that points at + * itself is rare but legal, and a walk without the guard hangs the request thread rather than + * failing it.

      */ @Test public void test_ordinaryFailure_becomes502_andSelfReferencingCauseTerminates() { @@ -235,8 +236,8 @@ public void test_genuineUpstreamFault_staysRetryable() { * When it is translated * Then the provider's own wording never reaches the caller * - *

      FR-031 and FR-036 together: a provider message can carry its endpoint, account - * identifiers, or a fragment of the prompt, so only the status is taken from it.

      + *

      A provider message can carry its endpoint, account identifiers, or a fragment of the + * prompt, so only the status is taken from it.

      */ @Test public void test_providerWordingIsNeverReturned() { diff --git a/dotCMS/src/test/java/com/dotcms/inference/InferenceRequestValidationTest.java b/dotCMS/src/test/java/com/dotcms/inference/InferenceRequestValidationTest.java index 879e5169b919..b3cbfbfa6095 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/InferenceRequestValidationTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/InferenceRequestValidationTest.java @@ -30,7 +30,7 @@ * {@link ChatCompletionMapper#toInferenceRequest(ChatCompletionRequestView)}, which translates the * wire shape into the internal one and is where wire-level validation belongs. That method is a * skeleton at the time of writing, so these tests fail — deliberately. They are the specification - * the implementation is written against, in particular the FR-013 line: a field this family has no + * the implementation is written against, in particular this rule: a field this family has no * opinion about is ignored, but a field that changes what the caller gets or pays for — {@code n} * being the example — must fail loudly rather than be silently dropped.

      * @@ -53,7 +53,7 @@ public class InferenceRequestValidationTest { /** * Given a request built with a blank model, * When the record is constructed, - * Then it is rejected naming {@code model} — there is no implicit default (FR-024). + * Then it is rejected naming {@code model} — there is no implicit default. */ @Test public void test_inferenceRequest_blankModel_throwsNamingModel() { @@ -83,7 +83,7 @@ public void test_inferenceRequest_nullModel_throwsNamingModel() { /** * Given a request carrying an empty conversation, * When the record is constructed, - * Then it is rejected naming {@code message} — there is nothing to infer from (FR-002). + * Then it is rejected naming {@code message} — there is nothing to infer from. */ @Test public void test_inferenceRequest_emptyMessages_throwsNamingMessages() { @@ -97,7 +97,7 @@ public void test_inferenceRequest_emptyMessages_throwsNamingMessages() { * Given a TOOL turn whose {@code toolCallId} answers a call no earlier assistant turn made, * When the record is constructed, * Then it is rejected and the error names the orphaned identity, so the caller can find it - * without diffing the conversation by hand (FR-005). + * without diffing the conversation by hand. */ @Test public void test_inferenceRequest_uncorrelatedToolResult_throwsNamingOffendingId() { @@ -116,7 +116,7 @@ public void test_inferenceRequest_uncorrelatedToolResult_throwsNamingOffendingId * Given a TOOL turn whose {@code toolCallId} matches a call a preceding assistant turn made, * When the record is constructed, * Then it is accepted and the conversation survives intact — correlation is by identity, not - * by position (FR-005). + * by position. */ @Test public void test_inferenceRequest_correlatedToolResult_isAccepted() { @@ -192,7 +192,7 @@ public void test_inferenceMessage_noContentAndNoToolCalls_throwsNamingContent() * When it is mapped to the internal representation, * Then it is rejected with a validation error naming {@code n}, rather than silently served as * a single choice. Unknown fields are tolerated; a field that changes what the caller gets or - * pays for is not (FR-013). + * pays for is not. */ @Test public void test_toInferenceRequest_nGreaterThanOne_throwsNamingN() { @@ -209,7 +209,7 @@ public void test_toInferenceRequest_nGreaterThanOne_throwsNamingN() { * Given a request view with no model, * When it is mapped to the internal representation, * Then it is rejected with a validation error naming {@code model}, at the wire layer, before - * any provider is chosen (FR-024). + * any provider is chosen. */ @Test public void test_toInferenceRequest_absentModel_throwsNamingModel() { @@ -225,7 +225,7 @@ public void test_toInferenceRequest_absentModel_throwsNamingModel() { /** * Given a request view carrying an empty conversation, * When it is mapped to the internal representation, - * Then it is rejected with a validation error naming {@code messages} (FR-002). + * Then it is rejected with a validation error naming {@code messages}. */ @Test public void test_toInferenceRequest_emptyMessages_throwsNamingMessages() { diff --git a/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java b/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java index 27dfd0106e02..65bf0a0ca5d9 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/RequestSizeLimitFilterTest.java @@ -27,7 +27,7 @@ import static org.mockito.Mockito.when; /** - * Covers FR-037's request-size ceiling, including the case that used to skip it entirely. + * Covers the request-size ceiling, including the case that used to skip it entirely. * *

      A body that declares its length is easy: the filter compares one number. A body that does * not — chunked, or simply streamed by a client that omits {@code Content-Length}, which several diff --git a/dotCMS/src/test/java/com/dotcms/inference/SseFailureTest.java b/dotCMS/src/test/java/com/dotcms/inference/SseFailureTest.java index 76915b9ed073..6869d0570995 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/SseFailureTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/SseFailureTest.java @@ -16,7 +16,7 @@ import static org.junit.Assert.assertTrue; /** - * Unit tests for how {@link SseSerializer} ends a stream that failed after it had begun (FR-039). + * Unit tests for how {@link SseSerializer} ends a stream that failed after it had begun. * *

      Once the first chunk is written the HTTP status is already on the wire and can no longer * carry the failure. The error event is the only way a caller learns the answer is incomplete, @@ -61,7 +61,6 @@ public class SseFailureTest { * When the error event is rendered. * Then the frame's JSON carries a top-level {@code error} object with {@code message} and * {@code type}, which is the shape a standard client deserializes into its own error type. - * (FR-039) * * @throws Exception when the rendered frame cannot be parsed */ @@ -124,7 +123,7 @@ public void test_toFrame_errorEvent_omitsHttpStatusFromBody() throws Exception { * *

      This is the single most important assertion in the streaming work: withholding * {@code [DONE]} is the only thing that stops a client which does not parse the error event - * from treating a truncated answer as a complete one. (FR-039)

      + * from treating a truncated answer as a complete one.

      */ @Test public void test_shouldWriteDoneMarker_errorEvent_returnsFalse() { @@ -143,7 +142,7 @@ public void test_shouldWriteDoneMarker_errorEvent_returnsFalse() { * has started. * When the serializer is asked whether the terminal marker follows. * Then it says no, identically to the provider-failure path — the caller must not be able to - * tell a timed-out answer from a complete one only by luck. (FR-039) + * tell a timed-out answer from a complete one only by luck. */ @Test public void test_shouldWriteDoneMarker_completionTimeoutErrorEvent_returnsFalse() { @@ -190,7 +189,6 @@ public void test_shouldWriteDoneMarker_finishAndUsageEvents_returnTrue() { * When it is rendered to a frame. * Then the frame is neither equal to nor contains {@link SseSerializer#DONE_MARKER}, so a * client scanning the byte stream for the terminator never finds one on a failed stream. - * (FR-039) */ @Test public void test_toFrame_errorEvent_frameNeitherIsNorContainsDoneMarker() { @@ -222,7 +220,7 @@ public void test_toFrame_errorEvent_frameNeitherIsNorContainsDoneMarker() { * When the error event is rendered. * Then it renders the same standard error shape as the provider-failure path — a top-level * {@code error} object with {@code message} and {@code type}, and no status in the body — so - * the second way to fail mid-stream is indistinguishable in handling from the first. (FR-039) + * the second way to fail mid-stream is indistinguishable in handling from the first. * * @throws Exception when the rendered frame cannot be parsed */ diff --git a/dotCMS/src/test/java/com/dotcms/inference/SseSerializerTest.java b/dotCMS/src/test/java/com/dotcms/inference/SseSerializerTest.java index 1fe73e172aed..cb7b116f4f2e 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/SseSerializerTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/SseSerializerTest.java @@ -17,7 +17,7 @@ * server-sent events a standard chat-completions client reads from * {@code /api/inference/v1/chat/completions} when {@code "stream": true}. * - *

      This class covers the chunk envelope (FR-007); incremental tool calls (FR-008) are covered by + *

      This class covers the chunk envelope; incremental tool calls are covered by * {@code SseToolCallStreamTest}.

      * *

      Coverage:

      @@ -26,7 +26,7 @@ * tells an SSE reader the event is complete. Without the terminator a client blocks holding * a chunk it already has.
      • *
    • Envelope fields on every chunk — {@code id}, {@code object} equal to - * {@code chat.completion.chunk}, {@code created} and {@code model} (FR-007).
      • + * {@code chat.completion.chunk}, {@code created} and {@code model}. *
    • The id, model and creation time being the ones passed in, and staying identical across * successive chunks of the same completion — a client correlates chunks by that id.
      • *
    • A content fragment landing at {@code choices[0].delta.content} with {@code index} 0.
      • @@ -74,7 +74,7 @@ public void test_toFrame_contentDelta_isDataPrefixedAndBlankLineTerminated() { * Given a content fragment of an answer. * When it is rendered to a frame. * Then the parsed chunk carries every envelope field, with {@code object} equal to - * {@code chat.completion.chunk}. (FR-007) + * {@code chat.completion.chunk}. * * @throws Exception if the frame does not carry parseable JSON */ diff --git a/dotCMS/src/test/java/com/dotcms/inference/SseToolCallStreamTest.java b/dotCMS/src/test/java/com/dotcms/inference/SseToolCallStreamTest.java index 63bf515e142d..dfaabc375f4a 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/SseToolCallStreamTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/SseToolCallStreamTest.java @@ -13,7 +13,7 @@ import static org.junit.Assert.assertTrue; /** - * Unit tests for {@link SseSerializer} rendering incremental tool calls (FR-008) on a streamed + * Unit tests for {@link SseSerializer} rendering incremental tool calls on a streamed * completion served at {@code /api/inference/v1/chat/completions}. * *

      A tool call does not arrive whole. Its identity and name come on the first fragment, its @@ -50,7 +50,7 @@ public class SseToolCallStreamTest { * Given the first fragment of a tool call, carrying its id and the tool's name. * When it is rendered to a frame. * Then {@code choices[0].delta.tool_calls[0]} carries the index, the id, a {@code type} of - * {@code function} and the function name. (FR-008) + * {@code function} and the function name. * * @throws Exception if the frame does not carry parseable JSON */ diff --git a/dotCMS/src/test/java/com/dotcms/inference/SseUsageGatingTest.java b/dotCMS/src/test/java/com/dotcms/inference/SseUsageGatingTest.java index f2051bf58072..f3441adac9af 100644 --- a/dotCMS/src/test/java/com/dotcms/inference/SseUsageGatingTest.java +++ b/dotCMS/src/test/java/com/dotcms/inference/SseUsageGatingTest.java @@ -14,7 +14,7 @@ /** * Unit tests for the streamed usage chunk rendered by - * {@link SseSerializer#toFrame(InferenceStreamEvent, String, String, long)} (FR-009, research R5). + * {@link SseSerializer#toFrame(InferenceStreamEvent, String, String, long)}. * *

      In the chat-completions wire format the usage event is a chunk whose {@code choices} array is * empty. Some stream readers assume every chunk has a non-empty {@code choices} @@ -27,7 +27,7 @@ *

    • A {@link InferenceStreamEvent.Usage} event rendering a chunk with a present but empty * {@code choices} array.
      • *
    • That chunk still carrying the per-chunk envelope — {@code id}, {@code object} - * ({@code chat.completion.chunk}), {@code created}, {@code model} (FR-007).
      • + * ({@code chat.completion.chunk}), {@code created}, {@code model}. *
    • The counts landing on the wire as {@code usage.prompt_tokens}, * {@code usage.completion_tokens} and {@code usage.total_tokens}.
      • *
    • {@link InferenceUsage#UNREPORTED} rendering with the token fields absent or null rather @@ -52,7 +52,7 @@ public class SseUsageGatingTest { * Given a usage event closing a stream. * When it is rendered to a frame. * Then the chunk carries a {@code choices} array that is present and empty, which is the shape - * standard clients recognise as a usage-only chunk. (FR-009) + * standard clients recognise as a usage-only chunk. * * @throws Exception when the rendered frame cannot be parsed */ @@ -76,7 +76,7 @@ public void test_toFrame_usageEvent_rendersEmptyChoicesArray() throws Exception * When it is rendered to a frame. * Then the chunk carries the same envelope as every other chunk — {@code id}, {@code object}, * {@code created}, {@code model} — because a client stitching a stream together keys off those - * on every frame, usage-only ones included. (FR-007) + * on every frame, usage-only ones included. * * @throws Exception when the rendered frame cannot be parsed */ @@ -176,7 +176,7 @@ public void test_toFrame_unreportedUsage_omitsTokenCountsRatherThanFabricatingZe * Given a usage event, which on an asking stream is the last event before the terminator. * When the serializer is asked whether the terminal marker follows. * Then it says yes, so a stream that ends with usage still closes cleanly and a client is not - * left waiting on a stream that will never terminate. (FR-009) + * left waiting on a stream that will never terminate. */ @Test public void test_shouldWriteDoneMarker_usageEvent_returnsTrue() { diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java index 6c47608aac83..85f3be7826ec 100644 --- a/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java +++ b/dotCMS/src/test/java/com/dotcms/rest/api/CorsFilterTest.java @@ -36,7 +36,7 @@ private static class OptedOutResource { * When the CORS filter decides whether to add its headers * Then every one of them is exempt * - *

      FR-030. Asserted per class rather than on one of them: the annotation is applied by hand + *

      Asserted per class rather than on one of them: the annotation is applied by hand * to each resource, so a fifth operation added later without it is exactly the mistake this * catches, and testing a single representative would not.

      */ @@ -62,7 +62,7 @@ public void test_everyInferenceResource_optsOutOfCors() { * When the CORS filter decides * Then the opt-out does not apply, so dotCMS's default behaviour is untouched * - *

      The other half of FR-030's guarantee, and the one that keeps this change additive: the + *

      The other half of that guarantee, and the one that keeps this change additive: the * exemption has to be something a resource asks for, not something the filter now does to * everyone. A null stands in for a request that matched no resource, which the filter already * tolerates.

      diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java index 465275df6f2d..72ab2b9b838d 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsStreamingTest.java @@ -61,15 +61,15 @@ * ends, which is the only part a client cannot recover from on its own.

      * *
        - *
      • FR-007 — every chunk carries {@code id}, {@code object} = {@code chat.completion.chunk}, + *
      • Every chunk carries {@code id}, {@code object} = {@code chat.completion.chunk}, * {@code created} and {@code model}; the last content chunk carries a {@code finish_reason}; * the stream closes with {@code data: [DONE]}.
        • - *
      • FR-008 — tool-call arguments arrive as fragments that reassemble into the complete + *
      • Tool-call arguments arrive as fragments that reassemble into the complete * argument JSON, with the call's id carried verbatim rather than synthesised.
        • - *
      • FR-009 — a usage chunk, with an empty {@code choices} array, appears immediately before + *
      • A usage chunk, with an empty {@code choices} array, appears immediately before * the terminal marker only when the caller asked for it through * {@code stream_options.include_usage}, and never otherwise.
        • - *
      • FR-039 — a provider that fails part-way through produces an error frame and a stream + *
      • A provider that fails part-way through produces an error frame and a stream * that closes without the terminal marker. This is the assertion that * matters most in this file: withholding {@code [DONE]} is what stops a client which does not * parse the error frame from reading a truncated answer as a finished one.
        • @@ -161,7 +161,7 @@ public class ChatCompletionsStreamingTest { /** * A stream that starts well and then breaks: the third frame is truncated JSON and the * provider never sends a terminal marker. The generation is already in flight by then, which - * is exactly the window FR-039 is about. + * is exactly the window that matters here. */ private static final String PROVIDER_MALFORMED_STREAM = """ data: {"id":"chatcmpl-stream-3","object":"chat.completion.chunk","created":1789000000,"model":"gpt-4o-mini","choices":[{"index":0,"delta":{"role":"assistant","content":""}}]} @@ -188,8 +188,8 @@ public static void beforeClass() throws Exception { wireMockServer = AiTest.prepareWireMock(); // A bare UserDataGen user has no roles at all, so it is neither a backend nor a - // frontend user and FR-016 rejects it; it also cannot read the site these tests pass - // as an explicit override, which FR-019 checks. Two roles are needed, not one: the check + // frontend user, and is rejected; it also cannot read the site these tests pass + // as an explicit override, which is also checked. Two roles are needed, not one: the check // in WebResource.checkRolePermissions is doesUserHaveRole(user, "DOTCMS_BACK_END_USER") // by key and does not walk inheritance, so being an admin does not imply it. Admin is // what grants read on the site. Role-specific behaviour is US3's tests, not these. @@ -521,10 +521,11 @@ private static void stubProviderStreamVolunteeringUsage() { * Then it is refused with a 429 that carries {@code Retry-After}, and nothing reaches the * provider * - *

        FR-037 for the ceiling itself, which had no coverage at all until now, and FR-031 for the - * header. The two belong in one test because the ceiling is only half an answer without it: a - * refusal that says "retry shortly" in a prose message tells a program nothing, and a client - * that cannot read a wait interval invents one — which under load means every refused caller + *

        Covers the ceiling itself, which had no coverage at all until now, together with the + * {@code Retry-After} header. The two belong in one test because the ceiling is only half an + * answer without it: a refusal that says "retry shortly" in a prose message tells a program + * nothing, and a client that cannot read a wait interval invents one — which under load means + * every refused caller * returning at once and holding the node at capacity it was trying to shed.

        * *

        The ceiling is driven to zero rather than fifty streams being opened. Fifty real streams @@ -583,7 +584,7 @@ private static ChatCompletionRequestView streamingRequest(final StreamOptionsVie * When a streamed completion is requested * Then what the caller receives names nothing about the provider or the site's credentials * - *

        FR-031 and FR-036. The fallback chain reports a failed initialisation by rethrowing an + *

        The fallback chain reports a failed initialisation by rethrowing an * {@code IllegalArgumentException} whose message is dotCMS's prefix followed by whatever the * provider client said — and a provider client's message can carry its endpoint, its account * identifiers, or a fragment of the prompt. The stream's error translator used to return that diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsTest.java index 945178aa4590..3d4a8db40481 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/ChatCompletionsTest.java @@ -57,13 +57,13 @@ * The four tests here cover exactly that arc.

        * *
          - *
        • FR-004 — tools declared in the request produce {@code tool_calls} in the response, with + *
        • Tools declared in the request produce {@code tool_calls} in the response, with * an id, a function name and its arguments.
          • - *
        • FR-005 — a follow-up turn replaying the assistant tool call plus a {@code role:"tool"} + *
        • A follow-up turn replaying the assistant tool call plus a {@code role:"tool"} * result carrying that id is accepted and answered.
          • *
        • A {@code role:"tool"} turn whose {@code tool_call_id} correlates with nothing is a * client error, refused before the provider is ever contacted.
          • - *
        • FR-024 — {@code model} is required; there is no implicit default.
          • + *
        • {@code model} is required; there is no implicit default.
          • *
        * *

        The provider is a WireMock server standing in for an OpenAI-compatible endpoint, wired in @@ -158,8 +158,8 @@ public static void beforeClass() throws Exception { stubProvider(); // A bare UserDataGen user has no roles at all, so it is neither a backend nor a - // frontend user and FR-016 rejects it; it also cannot read the site these tests pass - // as an explicit override, which FR-019 checks. Two roles are needed, not one: the check + // frontend user, and is rejected; it also cannot read the site these tests pass + // as an explicit override, which is also checked. Two roles are needed, not one: the check // in WebResource.checkRolePermissions is doesUserHaveRole(user, "DOTCMS_BACK_END_USER") // by key and does not walk inheritance, so being an admin does not imply it. Admin is // what grants read on the site. Role-specific behaviour is US3's tests, not these. diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceAuthorizationTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceAuthorizationTest.java index ddf12b6b08fd..119594ce7dfc 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceAuthorizationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceAuthorizationTest.java @@ -54,15 +54,15 @@ * next to the others:

        * *
          - *
        • FR-015 — the credential must be a dotCMS API token presented as a bearer. Anonymous + *
        • The credential must be a dotCMS API token presented as a bearer. Anonymous * callers are refused, and so are the session cookies that surrounding dotCMS filters * otherwise accept: this family is server-side only by design, and admitting ambient browser * credentials would make the absence of cross-origin headers a formality rather than a * control.
          • - *
        • FR-016 — any authenticated user is admitted, backend or frontend alike, because + *
        • Any authenticated user is admitted, backend or frontend alike, because * a site calling AI on behalf of a visitor is a supported use case. The bar is authentication, * not privilege.
          • - *
        • FR-019 — but the moment a caller names a site explicitly, READ on that site is enforced, + *
        • But the moment a caller names a site explicitly, READ on that site is enforced, * and a caller who lacks it is refused. This is the one check that stands between an * authenticated caller and another site's credentials.
          • *
        @@ -109,7 +109,7 @@ public class InferenceAuthorizationTest { private static User backendUser; private static String backendBearerToken; - /** A frontend-only user: authenticated, unprivileged, and admitted by FR-016 all the same. */ + /** A frontend-only user: authenticated, unprivileged, and admitted all the same. */ private static User frontendUser; private static String frontendBearerToken; @@ -143,7 +143,7 @@ public static void beforeClass() throws Exception { frontendBearerToken = bearerTokenFor(frontendUser); // Deliberately no administrator role: this user passes authentication and the role gate, - // and then fails on site READ, which is exactly the boundary FR-019 draws. + // and then fails on site READ, which is exactly where the boundary lies. unprivilegedUser = new UserDataGen() .roles(APILocator.getRoleAPI().loadBackEndUserRole()) .nextPersisted(); @@ -176,7 +176,7 @@ public void after() throws Exception { * When a completion is requested * Then it is refused as unauthorized * - *

        FR-015. The refusal carries the standard error shape, as the contract's status table + *

        The refusal carries the standard error shape, as the contract's status table * requires of a 401 — a caller's client library should be able to deserialize a refusal into * its own error type exactly as it would a success. Accepts a thrown * {@link WebApplicationException} too, since the surrounding authentication handshake may @@ -205,7 +205,7 @@ public void test_completions_withNoCredential_isUnauthorized() { * When a completion is requested * Then it is refused as unauthorized * - *

        FR-015 accepts a dotCMS API token presented as a bearer credential and nothing else. The + *

        Only a dotCMS API token presented as a bearer credential is accepted here. The * session cookies the surrounding dotCMS filters generally honour must not be honoured here: * this family is server-side only by design, and a browser that can be made to carry an * ambient session would otherwise be able to spend a site's AI credentials.

        @@ -233,7 +233,7 @@ public void test_completions_withSessionCredentialOnly_isUnauthorized() { * When a completion is requested with that site as the override * Then it is refused as forbidden, in the standard error shape, naming the offending field * - *

        FR-019. The caller authenticates and clears the role gate; what stops them is READ on the + *

        The caller authenticates and clears the role gate; what stops them is READ on the * site whose credentials they asked to spend.

        */ @Test @@ -259,7 +259,7 @@ public void test_completions_withOverrideForUnreadableSite_isForbidden() { * When a completion is requested * Then it is served * - *

        FR-016, one half. Stated alongside the frontend case because the requirement is that + *

        Stated alongside the frontend case because the rule is that * both are admitted; either one alone would be satisfied by a rule that excluded the * other.

        */ @@ -280,7 +280,7 @@ public void test_completions_withBackendUser_isAccepted() { * When a completion is requested * Then it is served * - *

        FR-016, the other half. A site calling AI on behalf of a visitor is a supported use case, + *

        A site calling AI on behalf of a visitor is a supported use case, * so the bar on this family is authentication, not privilege.

        */ @Test diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java index 13dee826088d..3578b89f8699 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceClientConformanceTest.java @@ -64,7 +64,7 @@ import static org.mockito.Mockito.when; /** - * Proves SC-008: every one of the four operations returns payloads that deserialize into a + * Proves that every one of the four operations returns payloads that deserialize into a * standard client library's own result types with no adapter. * *

        Every other test in this family asserts the shape dotCMS intended to produce, by @@ -86,7 +86,7 @@ *

        "No adapter" is the load-bearing phrase, and it is what the absence of code here demonstrates: * the payload goes from the endpoint to the client untouched except by {@code writeValueAsString}. * If a field had to be renamed, re-typed, or removed to make the client accept it, that work would - * be visible in this file, and SC-008 would be false.

        + * be visible in this file, and the no-adapter promise would be false.

        * *

        Two WireMock roles share one server, separated by path prefix. The provider stubs sit at the * root ({@code /chat/completions}) and stand in for OpenAI as dotCMS's upstream. The client-facing @@ -223,7 +223,7 @@ public void after() throws Exception { * Then it deserializes into the library's own {@link ChatResponse}, carrying the answer, the * finish reason and the token usage * - *

        SC-008 for the operation every agent framework drives first. The finish reason and usage + *

        This is the operation every agent framework drives first. The finish reason and usage * are asserted alongside the text because they are what a framework acts on rather than * displays: a client decides whether to continue a tool loop from the finish reason, and bills * or budgets from the usage, so a payload that carries readable text and an unreadable finish @@ -267,7 +267,7 @@ public void test_chatCompletion_deserializesIntoTheClientLibrarysOwnType() throw * When an unmodified OpenAI client library consumes the event stream * Then it reassembles the answer from the library's own streaming callbacks * - *

        SC-003 and SC-008 together, and the case most likely to break in a way no other test in + *

        This is the case most likely to break in a way no other test in * this file would catch. Streaming is not JSON — it is a framing format, and a client can parse * every individual chunk correctly while still failing on the frame boundaries, the terminal * {@code [DONE]} sentinel, or a usage event whose {@code choices} array is empty. The assertion @@ -342,7 +342,7 @@ public void onError(final Throwable error) { * When an unmodified OpenAI client library reads it * Then it deserializes into the library's own {@link ModelsListResponse} * - *

        SC-008 for the listing. This is the one operation with no high-level client API to drive, + *

        This is the one operation with no high-level client API to drive, * so the library's response type is used directly rather than through a model class — which is * the same thing the library's own client does with the bytes once they arrive.

        */ @@ -380,7 +380,7 @@ public void test_modelListing_deserializesIntoTheClientLibrarysOwnType() throws * When an unmodified OpenAI client library reads them * Then they deserialize into the library's own {@link Embedding} vectors * - *

        SC-008 for the operation that carries the most data and the least tolerance for a wrong + *

        This is the operation that carries the most data and the least tolerance for a wrong * type: an embedding is a list of numbers, and a payload that renders them as strings parses * as JSON and fails in the client. The vector's contents are asserted, not merely its presence, * for that reason.

        @@ -419,7 +419,7 @@ public void test_embeddings_deserializeIntoTheClientLibrarysOwnType() throws Exc * When an unmodified OpenAI client library reads it * Then it deserializes into the library's own {@link Image}, carrying the base64 payload * - *

        SC-008 for images, and the place FR-012's inline-only rule meets a real client: the + *

        This is the place the inline-only rule for images meets a real client: the * library populates either the URL or the base64 field depending on what arrived, so asserting * that it found base64 is simultaneously a conformance check and a check that dotCMS never * handed out a hosted artifact.

        @@ -444,7 +444,7 @@ public void test_generatedImage_deserializesIntoTheClientLibrarysOwnType() throw assertNotNull(parsed); assertNotNull("The client library produced no image", parsed.content()); - assertNotNull("FR-012 returns the bytes inline, so this is where a client finds them", + assertNotNull("The bytes are returned inline, so this is where a client finds them", parsed.content().base64Data()); assertFalse(parsed.content().base64Data().isBlank()); assertNull("A client that received a URL instead would be holding a hosted artifact " diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceEmbeddingsTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceEmbeddingsTest.java index e0fe984e071d..43ce53309b88 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceEmbeddingsTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceEmbeddingsTest.java @@ -55,8 +55,8 @@ * Specifies {@code POST /api/inference/v1/embeddings} through * {@link EmbeddingsResource#embeddings(HttpServletRequest, HttpServletResponse, String, EmbeddingsRequestView)}. * - *

        FR-011 is not "expose an embeddings endpoint" — the vector round trip is the easy half. What - * the requirement actually adds is that the model gate of FR-023 has to look at a + *

        This endpoint is not "expose an embeddings endpoint" — the vector round trip is the easy half. + * What actually matters is that the model gate has to look at a * different section of the site's configuration than the chat endpoint does. A * site configures its chat models and its embeddings model separately, so an implementation that * reuses the chat section here would accept a chat model for an embeddings call and refuse the @@ -65,17 +65,18 @@ * the same site in one go: the embeddings model succeeds and the chat model is refused. * Either assertion alone is satisfied by the bug.

        * - *

        The second thing FR-011 adds is that {@code input} is not a string. It is either a - * string or an array of strings, because batching is how content is ordinarily embedded — - * anyone indexing a site sends an array — and a scalar-only endpoint would fail that common case - * while still looking correct against a one-off example. The array form is what makes the response - * a list of more than one entry, and therefore what makes {@code index} load-bearing: it is the - * only thing that lets a caller correlate a vector back to the text it sent. The tests below - * assert that index explicitly, looking each entry up by it rather than by its position in + *

        The second thing this endpoint gets right is that {@code input} is not a string. It is + * either a string or an array of strings, because batching is how content is + * ordinarily embedded — anyone indexing a site sends an array — and a scalar-only endpoint + * would fail that common case while still looking correct against a one-off example. The array + * form is what makes the response a list of more than one entry, and therefore what makes + * {@code index} load-bearing: it is the only thing that lets a caller correlate a vector back to + * the text it sent. The tests below assert that index explicitly, looking each entry up by it + * rather than by its position in * {@code data}, and they pin every refusal the array form introduces — nothing to embed, and * elements that are not text.

        * - *

        FR-011 now spells out what "an array of strings" costs an implementation, and each clause is + *

        Accepting "an array of strings" costs an implementation several things, and each clause is * a test below. Every element has to be a string, so the all-numeric array is * joined by a mixed one: an implementation that inspects only the first element passes the * all-numeric case and still accepts {@code ["some text", 5]}, which is the shape that actually @@ -84,7 +85,7 @@ * "null" has two wire forms — the JSON null and the field left out — which are separate code paths * and so are asserted separately.

        * - *

        FR-011 also settles two things this file used to leave open. A blank or + *

        This file also settles two things that used to be left open. A blank or * whitespace-only string is refused on the same grounds as an absent, null or empty one, * whether it arrives alone or as one element of an array: there is no meaningful embedding of * nothing, and providers differ in whether they error on it or hand back a zero vector — which is @@ -102,22 +103,22 @@ * so a message that happens to contain a digit cannot pass for one that locates the fault.

        * *
          - *
        • FR-011 — input text comes back as a vector in the standard embeddings shape, with the + *
        • Input text comes back as a vector in the standard embeddings shape, with the * serving model and the token counts.
          • - *
        • FR-011 — {@code input} accepts a single string or an array of strings; an array yields + *
        • {@code input} accepts a single string or an array of strings; an array yields * one entry per input, each carrying the index of the input it corresponds to, and the * reported usage covers the whole batch. An empty array, a null or absent input, a blank or * whitespace-only string, and an array any of whose elements is not a string or is blank, are * all refused naming {@code input}.
          • - *
        • FR-011 — where one element of an array is at fault, the message identifies which + *
        • Where one element of an array is at fault, the message identifies which * one.
          • - *
        • FR-011 / R7 — {@code model} is validated against the site's embeddings + *
        • {@code model} is validated against the site's embeddings * section; the site's chat model is refused here, even though the same site has it * configured.
          • - *
        • FR-023 — a model the site configured for nothing at all is refused the same way.
          • - *
        • FR-024 — {@code model} is required; there is no implicit default.
          • - *
        • FR-015 — an anonymous caller is refused.
          • - *
        • FR-020 — the serving site is published for the response filter to report.
          • + *
        • A model the site configured for nothing at all is refused the same way.
          • + *
        • {@code model} is required; there is no implicit default.
          • + *
        • An anonymous caller is refused.
          • + *
        • The serving site is published for the response filter to report.
          • *
        * *

        The provider is a WireMock server standing in for an OpenAI-compatible endpoint, wired in @@ -156,7 +157,7 @@ public class InferenceEmbeddingsTest { private static final String EMPTY_INPUT = ""; /** - * A string with nothing in it but whitespace. FR-011 refuses it on the same grounds as the + * A string with nothing in it but whitespace. It is refused on the same grounds as the * empty string, and it is asserted separately because it is a separate guard: an * {@code isEmpty()} check accepts it and passes three spaces to a provider that will either * error or bill for a zero vector. @@ -261,8 +262,8 @@ public static void beforeClass() throws Exception { stubProvider(); // A bare UserDataGen user has no roles at all, so it is neither a backend nor a frontend - // user and FR-016 rejects it; it also cannot read the site these tests pass as an explicit - // override, which FR-019 checks. Two roles are needed, not one: the check in + // user and is rejected on that ground; it also cannot read the site these tests pass as an + // explicit override, which is checked separately. Two roles are needed, not one: the check in // WebResource.checkRolePermissions is doesUserHaveRole(user, "DOTCMS_BACK_END_USER") by key // and does not walk inheritance, so being an admin does not imply it. Admin is what grants // read on the site. Role-specific behaviour is US3's tests, not these. @@ -347,7 +348,7 @@ public void test_embeddings_withInputText_returnsVectorInStandardShape() { * Then three entries come back, one per input, each carrying the index of the input it * corresponds to and a vector of its own * - *

        FR-011's array form. The assertions look each entry up by its index rather than + *

        The array form. The assertions look each entry up by its index rather than * reading positions out of {@code data}, because the index is the whole mechanism by which a * caller correlates a vector back to the text it sent; a test that trusted array position * would pass against an implementation that stamped indexes on positionally and never @@ -396,11 +397,12 @@ public void test_embeddings_withArrayInput_returnsOneEntryPerInputInOrder() { * When the embedding is requested * Then exactly one entry comes back, at index 0 * - *

        The scalar form is the half of FR-011 that already worked, and widening {@code input} to - * accept an array is exactly the kind of change that quietly breaks it — by normalising the - * scalar into a one-element array and then losing it, or by rejecting anything that is not an - * array. Both halves of "a string or an array of strings" have to hold at once, so the scalar - * form gets its own guard rather than living only inside the standard-shape test above.

        + *

        The scalar form is the half of this behavior that already worked, and widening + * {@code input} to accept an array is exactly the kind of change that quietly breaks it — by + * normalising the scalar into a one-element array and then losing it, or by rejecting anything + * that is not an array. Both halves of "a string or an array of strings" have to hold at once, + * so the scalar form gets its own guard rather than living only inside the standard-shape test + * above.

        */ @Test public void test_embeddings_withScalarInput_returnsSingleEntryAtIndexZero() { @@ -434,7 +436,7 @@ public void test_embeddings_withScalarInput_returnsSingleEntryAtIndexZero() { * *

        Embedding nothing is a caller mistake — a batch built from a query that matched no * content, most likely. Answering it with an empty {@code data} list and a 200 would look like - * success, and the caller would index nothing and never find out why. FR-011 now says this in + * success, and the caller would index nothing and never find out why. This is now stated in * as many words — an absent, null or empty input is refused naming the field — where before it * had to be inferred from the response being "one entry per input"; the test is unchanged, * which is the point of recording it.

        @@ -488,7 +490,7 @@ public void test_embeddings_withNonStringArrayElements_isRejectedNamingTheField( * Then it is refused as a client error naming {@code input}, and the message identifies the * element at fault, and the provider is never contacted * - *

        FR-011 requires every element of the array to be a string, and this is + *

        Every element of the array is required to be a string, and this is * the test that makes that word load-bearing. The all-numeric array above is refused by an * implementation that looks only at {@code input.get(0)}; a mixed array is not, and a mixed * array is the one a caller actually sends — a list assembled from two sources where one @@ -498,7 +500,7 @@ public void test_embeddings_withNonStringArrayElements_isRejectedNamingTheField( * that the strings ahead of it are not decoration: an implementation that checks the first two * elements and stops passes a two-element mixed array and fails this one.

        * - *

        Naming the field is no longer enough on its own. FR-011 requires the message to identify + *

        Naming the field is no longer enough on its own. The message is required to identify * which element is at fault, because {@code param} can only ever say {@code input} and * a caller who sent a batch is otherwise left to find the bad one themselves. The assertion * looks for the offending position in the message; the position is one no other number in the @@ -527,7 +529,7 @@ public void test_embeddings_withMixedArrayElements_isRejectedIdentifyingTheEleme * Then both are refused as a client error naming {@code input}, and the provider is never * contacted * - *

        FR-011 refuses a blank or whitespace-only string on the same grounds as an absent, null or + *

        A blank or whitespace-only string is refused on the same grounds as an absent, null or * empty input: there is nothing there to embed. Leaving it to the provider is the failure this * family exists to prevent — some error, some return a zero vector, and a caller indexing a * site would silently store a meaningless vector against a document and retrieve it forever @@ -551,7 +553,7 @@ public void test_embeddings_withBlankStringInput_isRejectedNamingTheField() { * actually happens — a column that was empty for one row of five hundred — and an * implementation that validates the array as a whole rather than element by element sends it * upstream without noticing. Refusing the batch rather than skipping the blank is the point: - * dropping it would shift the index of every entry after it, and FR-011 makes those indexes the + * dropping it would shift the index of every entry after it, and those indexes are the * caller's only means of correlating vectors back to what they sent.

        * *

        As with the mixed array, the message must say which element — and the blank sits at a @@ -574,7 +576,7 @@ public void test_embeddings_withBlankArrayElement_isRejectedIdentifyingTheElemen * Then both are refused as a client error naming {@code input}, and the provider is never * contacted * - *

        FR-011 lists "absent" and "null" alongside "empty", and the two are asserted together + *

        "Absent" and "null" are refused alongside "empty", and the two are asserted together * because they are not the same code path. A JSON {@code "input": null} deserializes to a * {@link NullNode} — an object, present, and non-null as far as a {@code != null} guard is * concerned — while an omitted field leaves the component null outright. An implementation @@ -638,7 +640,8 @@ public void test_embeddings_withArrayInput_reportsUsageForWholeBatch() { * Then the embeddings model is served and the chat model is refused as a model this site has * not configured — because it is not configured for embeddings * - *

        FR-011 and R7, stated as one assertion rather than two files apart. An implementation + *

        Model validation and section separation, stated as one assertion rather than two files + * apart. An implementation * that validated against the chat section would fail exactly one of these two halves, and an * implementation that validated against nothing at all would fail only the second — so both * belong in the same test, against the same site, in the same configuration.

        @@ -713,9 +716,10 @@ public void test_embeddings_withModelConfiguredForNothing_isRefusedAsNoSuchModel * When the embedding is requested * Then it is refused as a client error naming {@code model}, with no implicit default applied * - *

        FR-024 holds across the family, not only on chat. A site has exactly one embeddings model - * configured, which makes defaulting to it look harmless — and is precisely why it has to be - * refused here: a caller who never named a model cannot tell when the site's changes.

        + *

        The requirement that {@code model} be explicit holds across the family, not only on chat. + * A site has exactly one embeddings model configured, which makes defaulting to it look + * harmless — and is precisely why it has to be refused here: a caller who never named a model + * cannot tell when the site's changes.

        */ @Test public void test_embeddings_withoutModel_isRejectedNamingTheField() { @@ -740,7 +744,7 @@ public void test_embeddings_withoutModel_isRejectedNamingTheField() { * When the embedding is requested * Then it is refused as unauthorized and the provider is never contacted * - *

        FR-015. Accepts a thrown {@link WebApplicationException} as well as a returned 401, since + *

        Accepts a thrown {@link WebApplicationException} as well as a returned 401, since * the surrounding authentication handshake may refuse before the resource body runs and either * is a valid refusal.

        */ @@ -769,7 +773,7 @@ request, mockResponse(), host.getIdentifier(), * When the embedding is requested * Then the resolved site is published on the request for the response filter to report * - *

        FR-020 asks for the serving site on every response, and it reaches a real caller as the + *

        The serving site is published on every response, and it reaches a real caller as the * {@code X-dotCMS-Resolved-Site} header that {@link ResolvedSiteHeaderFilter} writes from * {@link InferenceRequestAttributes#RESOLVED_SITE_ID}. This test calls the resource method * directly, so no JAX-RS response filter runs and no header exists to read; the attribute is @@ -831,7 +835,7 @@ private static EmbeddingListView.EmbeddingView embeddingAtIndex(final EmbeddingL } /** - * Asserts that an {@code input} FR-011 refuses is refused here — as a 400 in the standard + * Asserts that an invalid {@code input} is refused here — as a 400 in the standard * error shape, naming the field, with nothing reaching the provider. * *

        Shared by the refusals that differ only in what was sent, so that a new one is a line @@ -867,7 +871,7 @@ private InferenceErrorView.Body assertInputRefusedNamingTheField(final JsonNode * Asserts everything {@link #assertInputRefusedNamingTheField(JsonNode)} does, and additionally * that the message identifies which element of the array is at fault. * - *

        FR-011 keeps {@code param} at {@code input} — that is the field the caller sent — so the + *

        {@code param} stays at {@code input} — that is the field the caller sent — so the * message is the only place the offending position can appear, and a caller who batched five * hundred strings should not have to bisect their own payload to find it. The index is the * 0-based one the response's own {@code index} correlates on, so that the number in the error @@ -906,7 +910,7 @@ private static EmbeddingsRequestView embeddingFor(final String model, final Stri /** * The array form of {@code input}, as * {@code {"model": "…", "input": ["The quick brown fox", "…"]}}. Called with no inputs it - * builds the empty array, which FR-011 refuses. + * builds the empty array, which is refused. * * @param model the model to ask for, or null to omit the field * @param inputs the texts to embed, in the order the caller sent them @@ -926,7 +930,7 @@ private static EmbeddingsRequestView embeddingForAll(final String model, * Builds a request arriving at a given host name, with or without a bearer credential. * *

        Request attributes are backed by a real map rather than left as mock no-ops, because both - * the authentication handshake and FR-020's site attribution publish through them, and a mock + * the authentication handshake and the site-attribution logic publish through them, and a mock * that forgot what was set on it would not behave like a servlet container.

        * * @param serverName the host name the request arrives on diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceFallbackTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceFallbackTest.java index 472d74bc62e8..747abb6ae55b 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceFallbackTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceFallbackTest.java @@ -56,11 +56,11 @@ * system level rather than per site.

        * *
          - *
        • FR-020 — a host name matching no site or alias is served by the default site, not + *
        • A host name matching no site or alias is served by the default site, not * refused, and the site that served is the one reported as having resolved.
          • - *
        • FR-021 — a site with no dotAI configuration of its own inherits the system-level one, + *
        • A site with no dotAI configuration of its own inherits the system-level one, * exactly as every other dotAI endpoint does, and the request succeeds.
          • - *
        • FR-021 — when neither the resolved site nor the system level has any configuration, the + *
        • When neither the resolved site nor the system level has any configuration, the * request is refused rather than served from some unrelated site's credentials.
          • *
        * @@ -193,7 +193,7 @@ public void test_completions_withUnmatchedHostName_isServedByDefaultSite() throw * When the completion is requested with no explicit site override * Then the site reported as having served is the default site * - *

        FR-020 asks for the fallback to be logged as well. Asserting on the log would mean + *

        The fallback is expected to be logged as well. Asserting on the log would mean * attaching an appender to dotCMS's Log4j configuration from an integration test, which is * both brittle and outside what this family owns, so what is asserted here is the observable * half of the same requirement: the serving site published for the response header. The log diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java index 1261524fc7d3..f096f1c43729 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceImagesTest.java @@ -57,8 +57,8 @@ * Specifies {@code POST /api/inference/v1/images/generations} through * {@link ImagesResource#generations(HttpServletRequest, HttpServletResponse, String, ImageGenerationRequestView)}. * - *

        Two decisions in FR-012 are the reason this file exists rather than a copy of the chat tests - * with a different noun.

        + *

        Two decisions about how a generated image is returned are the reason this file exists rather + * than a copy of the chat tests with a different noun.

        * *

        The image comes back inline, as base64, and never as a URL. That is a * deliberate choice and not an incidental one: a hosted URL would mean deciding storage, @@ -70,13 +70,13 @@ * why it must not.

        * *

        Deliberately not asserted here: that dotCMS asks the provider for base64 upstream. - * FR-012 does require it — a provider-minted URL is a real artifact on someone else's + * Requesting base64 is required — a provider-minted URL is a real artifact on someone else's * infrastructure, frequently public and long-lived, which is a larger exposure than anything this - * family's own response shape can create — but it requires it only "where a provider offers the + * family's own response shape can create — but only "where a provider offers the * choice", and the seven configured vendors do not agree on whether that choice exists or what it - * is called. Where no such choice exists the requirement says the opposite thing: dotCMS fetches - * the provider's URL and re-encodes it rather than refusing the provider, and FR-012 now states - * the residual openly — on those providers an addressable artifact really does exist upstream, and + * is called. Where no such choice exists the rule says the opposite thing: dotCMS fetches + * the provider's URL and re-encodes it rather than refusing the provider, and the residual is + * stated openly — on those providers an addressable artifact really does exist upstream, and * this family's guarantee is only that a caller never receives one. An assertion on the outbound * request body would therefore pin one vendor's spelling of a field the requirement itself makes * conditional, and would fail the moment the provider abstraction changed how it phrases a request @@ -90,9 +90,9 @@ *

        {@code size}, by contrast, is asserted on the outbound request, and the * difference is not inconsistency. Asking for the inline form is a provider capability — whether * the ask exists at all varies by vendor, so a test of it is a test of the vendor. Passing a - * caller's {@code size} through is dotCMS's own behaviour: the caller named a size, FR-012 says it + * caller's {@code size} through is dotCMS's own behaviour: the caller named a size, and it * reaches the provider "where the provider accepts it", and the stub here is a provider that - * accepts it. It is the same logic FR-013 applies to the sampling parameters — {@code size} + * accepts it. It is the same logic applied to the sampling parameters — {@code size} * changes both what the caller receives and what the site pays for, so dropping it silently is a * cost and correctness failure rather than a compatibility courtesy, and a dropped {@code size} is * invisible in the response: a 1024x1024 image is a perfectly plausible answer to a request for @@ -104,19 +104,19 @@ * 200/404 in the right shapes.

        * *
          - *
        • FR-012 — a prompt returns the standard image-generation shape: a {@code created} + *
        • A prompt returns the standard image-generation shape: a {@code created} * timestamp and one entry in {@code data}.
          • - *
        • FR-012 — that entry carries {@code b64_json} and carries no hosted {@code url}.
          • - *
        • FR-012 — {@code n} is honored: a request for two images answers with two distinct + *
        • That entry carries {@code b64_json} and carries no hosted {@code url}.
          • + *
        • {@code n} is honored: a request for two images answers with two distinct * entries in {@code data}, while {@code n} of exactly 1 and {@code n} omitted are each one * image.
          • - *
        • FR-012 — an {@code n} below 1 is refused naming the field — zero and negative alike.
          • - *
        • FR-012 / FR-013 — {@code size} reaches the provider rather than being dropped.
          • - *
        • FR-023 / R7 — {@code model} is validated against the site's image + *
        • An {@code n} below 1 is refused naming the field — zero and negative alike.
          • + *
        • {@code size} reaches the provider rather than being dropped.
          • + *
        • {@code model} is validated against the site's image * section; the site's chat and embeddings models are both refused here.
          • - *
        • FR-024 — {@code model} is required, and so is {@code prompt}; there is nothing to + *
        • {@code model} is required, and so is {@code prompt}; there is nothing to * generate without one.
          • - *
        • FR-015 — an anonymous caller is refused.
          • + *
        • An anonymous caller is refused.
          • *
        * *

        The provider is a WireMock server standing in for an OpenAI-compatible endpoint, wired in @@ -216,7 +216,7 @@ public class InferenceImagesTest { * {@code src/test/resources/mappings} — {@code created}, and a {@code data} entry carrying the * provider's rewritten prompt — except that the payload is the image itself rather than a * hosted link, which is what an OpenAI-compatible provider returns for a base64 request and - * what FR-012 requires this family to deal in. + * what this family deals in. */ private static final String PROVIDER_RESPONSE = """ { @@ -288,8 +288,8 @@ public static void beforeClass() throws Exception { stubProvider(); // A bare UserDataGen user has no roles at all, so it is neither a backend nor a frontend - // user and FR-016 rejects it; it also cannot read the site these tests pass as an explicit - // override, which FR-019 checks. Two roles are needed, not one: the check in + // user and is rejected on that ground; it also cannot read the site these tests pass as an + // explicit override, which is checked separately. Two roles are needed, not one: the check in // WebResource.checkRolePermissions is doesUserHaveRole(user, "DOTCMS_BACK_END_USER") by key // and does not walk inheritance, so being an admin does not imply it. Admin is what grants // read on the site. Role-specific behaviour is US3's tests, not these. @@ -360,7 +360,7 @@ public void test_generations_withPrompt_returnsImageInStandardShape() { * Then the entry carries a non-blank, decodable {@code b64_json} payload and no hosted * {@code url} * - *

        FR-012. The assertion is made against the serialized wire shape rather than against the + *

        The assertion is made against the serialized wire shape rather than against the * view's accessors, because that is where the decision can be undone: a {@code url} property * added to the payload later — by widening the view, or by letting the provider's own entry * through unmapped — would restore exactly the separately-addressable artifact the requirement @@ -380,7 +380,7 @@ public void test_generations_withGeneratedImage_returnsBase64AndNoHostedUrl() { final ImageGenerationView view = (ImageGenerationView) response.getEntity(); final String b64Json = view.data().get(0).b64Json(); - assertNotNull("FR-012 returns the image inline, so b64_json is not optional", b64Json); + assertNotNull("The image is returned inline, so b64_json is not optional", b64Json); assertFalse(b64Json.isBlank()); try { @@ -395,7 +395,7 @@ public void test_generations_withGeneratedImage_returnsBase64AndNoHostedUrl() { assertTrue("The wire payload names the image b64_json, as standard clients read it", entry.hasNonNull("b64_json")); assertEquals(b64Json, entry.get("b64_json").asText()); - assertFalse("FR-012 creates no hosted, separately-addressable artifact, so no url may " + assertFalse("No hosted, separately-addressable artifact is created, so no url may " + "reach the caller", entry.hasNonNull("url")); } @@ -404,23 +404,23 @@ public void test_generations_withGeneratedImage_returnsBase64AndNoHostedUrl() { * When each generation is requested * Then each is refused as a client error naming {@code n}, and the provider is never contacted * - *

        FR-012 draws its line below 1, not around it: zero and a negative are not quantities of - * images a caller can mean, and they are the two ways of expressing that. They are also what a - * clamp swallows — {@code Math.max(1, n)} answers a request for no images with an image and a - * request for minus one with a bill, while every other assertion in this file keeps passing. - * Zero is not an empty success either: a caller who sends it has made a mistake, and answering - * it with an empty {@code data} list would be the same false success FR-011 refuses for an - * empty batch of embeddings.

        + *

        The rule for {@code n} draws its line below 1, not around it: zero and a negative are not + * quantities of images a caller can mean, and they are the two ways of expressing that. They are + * also what a clamp swallows — {@code Math.max(1, n)} answers a request for no images with an + * image and a request for minus one with a bill, while every other assertion in this file keeps + * passing. Zero is not an empty success either: a caller who sends it has made a mistake, and + * answering it with an empty {@code data} list would be the same false success that is refused + * for an empty batch of embeddings.

        * *

        That is why the provider must not be contacted at all. Clamping is the plausible * implementation of this field — the request reaches the provider once, a perfectly valid * single-image 200 comes back, and every other assertion in this file still passes. A refusal * that never leaves dotCMS is the only observable difference.

        * - *

        An {@code n} of 2 is deliberately not among these values. An earlier draft of FR-012 - * refused any {@code n} other than 1, on the claim that the provider abstraction returns a - * single image per call; the claim was false — {@code ImageModel.generate(prompt, n)} returns a - * list, and the adopted format documents several images per request — and the requirement was + *

        An {@code n} of 2 is deliberately not among these values. An earlier version of this + * endpoint refused any {@code n} other than 1, on the claim that the provider abstraction returns + * a single image per call; the claim was false — {@code ImageModel.generate(prompt, n)} returns a + * list, and the adopted format documents several images per request — and the behavior was * corrected. Two images are served, by * {@link #test_generations_withNOfTwo_returnsTwoDistinctImages()}.

        */ @@ -436,9 +436,9 @@ public void test_generations_withNBelowOne_isRejectedNamingTheField() { * Then two entries come back in {@code data}, each carrying its own non-blank {@code b64_json}, * and the two are not the same image * - *

        FR-012 as corrected: {@code n} is honored, because the adopted format supports several - * images in one request and this site's provider implements the multi-image call. Refusing would be - * this family declining something both the standard and this provider support, so the count a + *

        As corrected: {@code n} is honored, because the adopted format supports several + * images in one request and this site's provider implements the multi-image call. Refusing would + * be this family declining something both the standard and this provider support, so the count a * caller asks for is the count they receive. A provider that cannot is a different case, with * its own test below.

        * @@ -476,7 +476,7 @@ public void test_generations_withNOfTwo_returnsTwoDistinctImages() { for (int index = 0; index < TWO_IMAGES; index++) { final ImageGenerationView.ImageView entry = view.data().get(index); assertNotNull("Entry " + index + " is missing entirely", entry); - assertNotNull("FR-012 returns every image inline, so entry " + index + assertNotNull("Every image is returned inline, so entry " + index + " carries a b64_json", entry.b64Json()); assertFalse("A blank payload is not an image, and entry " + index + " carries one", entry.b64Json().isBlank()); @@ -500,14 +500,13 @@ public void test_generations_withNOfTwo_returnsTwoDistinctImages() { * Then the size reaches the provider, and the answer is the one only a provider that was told * that size returns * - *

        FR-012 requires {@code size} to be passed through where the provider accepts it, on the - * same reasoning FR-013 applies to the sampling parameters: it changes both what the caller - * receives and what the site pays, so dropping it is a cost and correctness failure rather than - * the harmless compatibility courtesy that ignoring an incidental field is. Unlike the base64 - * ask this file deliberately does not assert, passing a caller's own parameter through is - * dotCMS's behaviour rather than a provider capability that varies by vendor — the stub here is - * a provider that accepts {@code size}, and that is all FR-012 conditions the requirement - * on.

        + *

        {@code size} is passed through where the provider accepts it, on the same reasoning applied + * to the sampling parameters: it changes both what the caller receives and what the site pays, so + * dropping it is a cost and correctness failure rather than the harmless compatibility courtesy + * that ignoring an incidental field is. Unlike the base64 ask this file deliberately does not + * assert, passing a caller's own parameter through is dotCMS's behaviour rather than a provider + * capability that varies by vendor — the stub here is a provider that accepts {@code size}, and + * that is the only condition on passing it through.

        * *

        It has to be asserted on the outbound request because it is invisible in the response: a * provider asked for nothing in particular returns a perfectly well-formed image, and every @@ -546,7 +545,7 @@ public void test_generations_withSize_passesTheSizeToTheProvider() { * entry * *

        The boundary from the other side, and the reason it is a separate test rather than - * another assertion inside one: what FR-012 refuses is an {@code n} below 1, not the presence + * another assertion inside one: what is refused is an {@code n} below 1, not the presence * of the field, and not the absence of it either. An implementation that rejected any request * carrying {@code n} would satisfy the refusal test on its own while breaking every standard * client that sends the format's own default of 1, and an implementation that rejected a @@ -667,7 +666,7 @@ public void test_generations_withoutPrompt_isRejectedNamingTheField() { * When the image is requested * Then it is refused as unauthorized and the provider is never contacted * - *

        FR-015. Accepts a thrown {@link WebApplicationException} as well as a returned 401, since + *

        Accepts a thrown {@link WebApplicationException} as well as a returned 401, since * the surrounding authentication handshake may refuse before the resource body runs and either * is a valid refusal.

        */ @@ -696,11 +695,11 @@ request, mockResponse(), host.getIdentifier(), * When the generation is requested * Then it is refused with a 400 naming {@code n}, and nothing reaches the provider * - *

        FR-012 and FR-037. Images are priced per image, so without a ceiling one accepted request - * multiplies a site's provider spend by whatever number it carried, and FR-032 puts per-site - * spend quotas out of scope — there is no second line of defence behind this one. The earlier - * draft that refused every {@code n} above 1 capped the spend by accident; correcting it to - * honor {@code n} removed that cap, and this is the deliberate replacement.

        + *

        Images are priced per image, so without a ceiling one accepted request multiplies a site's + * provider spend by whatever number it carried, and per-site spend quotas are out of scope here + * — there is no second line of defence behind this one. The earlier draft that refused every + * {@code n} above 1 capped the spend by accident; correcting it to honor {@code n} removed that + * cap, and this is the deliberate replacement.

        * *

        Refused rather than clamped, and asserted as such: an implementation that quietly served * {@link InferenceLimits#DEFAULT_MAX_IMAGES_PER_REQUEST} images here would return a 200 and a @@ -737,9 +736,9 @@ public void test_generations_aboveTheImageCeiling_isRejectedNamingTheField() { * When several images are requested * Then it is refused with a 400 naming {@code n} — not a retryable upstream failure * - *

        FR-012. The multi-image call is a default method on the provider abstraction that throws + *

        The multi-image call is a default method on the provider abstraction that throws * unless the implementation overrides it: the OpenAI-backed models override it, the Gemini one - * does not. Left to the ordinary upstream translation of FR-031 that throw becomes a 502, which + * does not. Left to the ordinary upstream-failure translation, that throw becomes a 502, which * is the wrong answer in a way that costs the caller real time — 502 is retryable, so a * standard client's back-off keeps re-sending a request that cannot succeed however long it * waits. The status is the assertion that matters here; the param name is what lets the caller @@ -943,9 +942,9 @@ private static ImageGenerationRequestView generationFor(final String model, * @param model the model to ask for, or null to omit the field * @param prompt the prompt to generate from, or null to omit the field * @param count how many images to ask for, or null to omit {@code n} entirely — which is - * why it is boxed: "not sent" is a value a caller can express and FR-012 has to - * answer with one image, and a primitive would silently turn it into the 0 that - * FR-012 refuses + * why it is boxed: "not sent" is a value a caller can express and must be + * answered with one image, and a primitive would silently turn it into the 0 that + * is refused * @return an image-generation request asking for that many images */ private static ImageGenerationRequestView generationFor(final String model, @@ -972,7 +971,7 @@ private static ImageGenerationRequestView generationFor(final String model, * Builds a request arriving at a given host name, with or without a bearer credential. * *

        Request attributes are backed by a real map rather than left as mock no-ops, because both - * the authentication handshake and FR-020's site attribution publish through them, and a mock + * the authentication handshake and the site-attribution logic publish through them, and a mock * that forgot what was set on it would not behave like a servlet container.

        * * @param serverName the host name the request arrives on diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java index 405ca5ea3c04..7562c6d817b9 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceLoggingTest.java @@ -53,7 +53,8 @@ import static org.mockito.Mockito.when; /** - * Holds FR-036's line: metadata may be logged, the caller's content may not. + * Holds the line between what may be logged and what may not: metadata may be logged, the + * caller's content may not. * *

        No request or response body is written to a log today. That was true before this test * existed and is exactly why the test is worth having — nothing enforced it. Every log line in @@ -295,7 +296,7 @@ private void assertSentinelNeverLogged(final String what) { } } - assertTrue("FR-036 allows metadata in the logs and never the caller's content, but " + assertTrue("Metadata is allowed in the logs and never the caller's content, but " + what + " wrote it: " + leaking, leaking.isEmpty()); } diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelValidationTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelValidationTest.java index 27ba6e2ba4c7..d56b9b5835c0 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelValidationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelValidationTest.java @@ -51,19 +51,19 @@ * *

        The interesting assertion in this file is the negative one: there is no administrator * exemption. A role-conditional passthrough is exactly the branching that let two shipped dotAI - * endpoints drift to opposite model policies, and it would make SC-007 unstatable as a blanket - * invariant. So an administrator naming an unconfigured model is refused with the same status and - * the same body as a caller with no privileges at all, and that equality is asserted directly + * endpoints drift to opposite model policies, and it would make that invariant unstatable as a + * blanket rule. So an administrator naming an unconfigured model is refused with the same status + * and the same body as a caller with no privileges at all, and that equality is asserted directly * rather than inferred from two separate tests.

        * *
          - *
        • FR-023 — a model the site configured is accepted; one it did not is refused with the + *
        • A model the site configured is accepted; one it did not is refused with the * standard "no such model" shape, naming nothing the caller did not already send.
          • - *
        • FR-023 / SC-007 — the refusal is identical for an administrator and for a + *
        • The refusal is identical for an administrator and for a * non-administrator, so no caller of any role can cause an unconfigured model to be invoked.
          • - *
        • FR-024 — a request omitting {@code model} is refused naming the field; there is no + *
        • A request omitting {@code model} is refused naming the field; there is no * implicit default.
          • - *
        • FR-023 — a site configured with a fallback chain accepts any entry of the chain, not + *
        • A site configured with a fallback chain accepts any entry of the chain, not * only the first.
          • *
        * @@ -137,7 +137,7 @@ public static void beforeClass() throws Exception { .nextPersisted(); adminToken = bearerTokenFor(adminUser); - // The counterpart: admitted by FR-016 as an authenticated backend user, and nothing more. + // The counterpart: admitted as an authenticated backend user, and nothing more. // Read on the site is granted per test, since the site is created per test. limitedUser = new UserDataGen() .roles(APILocator.getRoleAPI().loadBackEndUserRole()) @@ -226,8 +226,9 @@ public void test_completions_withUnconfiguredModel_isRefusedAsNoSuchModel() { * When each names a model the site has not configured * Then both are refused, with the same status and the same error body * - *

        This is the SC-007 invariant stated as one assertion rather than two: no caller, of any - * role, can cause a model outside the site's configuration to be invoked. Asserting the two + *

        This is the no-administrator-exemption invariant stated as one assertion rather than + * two: no caller, of any role, can cause a model outside the site's configuration to be + * invoked. Asserting the two * refusals are equal is what would catch an exemption added later, which two * independent single-role tests would not.

        */ @@ -336,7 +337,7 @@ private static void configureChain(final Host site, final List chain) th /** * Grants read on a site to a user's own role, so a caller with no administrative role can - * still name the site as an explicit override. FR-019 refuses the override otherwise, which + * still name the site as an explicit override. The override is otherwise refused, which * would make the non-administrator's refusal a 403 about the site rather than the 404 about * the model this file is comparing. * diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java index b6e4c8200d90..d2f3ca9aab7f 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceModelsTest.java @@ -41,7 +41,7 @@ * Specifies model discovery through * {@link ModelsResource#models(HttpServletRequest, HttpServletResponse, String)}. * - *

        FR-024 removed every reserved alias and every implicit default from this family, which makes + *

        Every reserved alias and every implicit default was removed from this family, which makes * this endpoint load-bearing rather than decorative: the list it returns is the only way * a caller can learn which {@code model} values the chat endpoint will accept. That is what the * tests here pin down — that the list is complete, that it is exactly the configured set with @@ -49,10 +49,10 @@ * unconfigured instance says so with an empty list rather than with somebody else's models.

        * *
          - *
        • FR-010 — every entry of a fallback chain is listed, in configured order, because each + *
        • Every entry of a fallback chain is listed, in configured order, because each * one is a real choice a caller may name.
          • - *
        • FR-024 — nothing synthetic is added; the count equals the configured count.
          • - *
        • FR-022 — no configuration at either the site or the system level yields an empty + *
        • Nothing synthetic is added; the count equals the configured count.
          • + *
        • No configuration at either the site or the system level yields an empty * {@code data} array, never another site's models.
          • *
        • The models come from the chat section, not the embeddings or image * ones, matching what the chat endpoint validates against.
          • @@ -115,8 +115,8 @@ public static void beforeClass() throws Exception { wireMockServer = AiTest.prepareWireMock(); // A bare UserDataGen user has no roles at all, so it is neither a backend nor a frontend - // user and FR-016 rejects it; it also cannot read the sites these tests pass as an - // explicit override, which FR-019 checks. Two roles are needed, not one: the check in + // user, and is rejected; it also cannot read the sites these tests pass as an + // explicit override, which is also checked. Two roles are needed, not one: the check in // WebResource.checkRolePermissions is doesUserHaveRole(user, "DOTCMS_BACK_END_USER") by // key and does not walk inheritance, so being an admin does not imply it. Admin is what // grants read on the site. @@ -185,7 +185,7 @@ public void test_models_withSingleConfiguredModel_listsThatModel() { * When the models are listed * Then every entry of the chain is listed, in configured order, the primary first * - *

          FR-010. Each entry of the chain is a name the completions endpoint accepts, so a list + *

          Each entry of the chain is a name the completions endpoint accepts, so a list * that showed only the primary would hide choices the caller is entitled to make.

          */ @Test @@ -217,7 +217,7 @@ public void test_models_withFallbackChain_listsEveryEntryInOrder() throws Except * When the models are listed * Then the list is exactly those three — no alias, no placeholder, no invented entry * - *

          FR-024 removed the reserved alias precisely so the list equals the set of acceptable + *

          The reserved alias was removed precisely so the list equals the set of acceptable * {@code model} values. An extra entry here would be a name the completions endpoint refuses, * which is worse than no list at all.

          */ @@ -249,7 +249,7 @@ public void test_models_withConfiguredChain_addsNothingSynthetic() throws Except * When the models are listed * Then the answer is a successful, empty list * - *

          FR-022. The failure this guards against is not an error but a wrong success: falling + *

          The failure this guards against is not an error but a wrong success: falling * back to whichever site happens to be configured would hand a caller model names their own * site will refuse, and would disclose that some other site has dotAI set up.

          */ @@ -304,7 +304,7 @@ public void test_models_withSiteOverride_listsTheResolvedSitesModels() throws Ex * When the models are listed * Then the resolved site is published on the request, so every response can report it * - *

          FR-020. Asserted on the request attribute rather than on the header because invoking the + *

          Asserted on the request attribute rather than on the header because invoking the * resource method directly never runs the JAX-RS response filter that turns the attribute into * {@code X-dotCMS-Resolved-Site} — the attribute is the part this endpoint is responsible * for.

          @@ -327,7 +327,7 @@ public void test_models_withSiteOverride_publishesTheResolvedSiteId() { * When the models are listed * Then it is refused as unauthorized, in the standard error shape * - *

          FR-015. The model list names the site's configured vendors and models, which is exactly + *

          The model list names the site's configured vendors and models, which is exactly * the kind of reconnaissance an anonymous caller should not get for free.

          */ @Test diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteIsolationTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteIsolationTest.java index 18e24cdf57e0..364812674751 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteIsolationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteIsolationTest.java @@ -46,7 +46,7 @@ /** * Specifies cross-site isolation for * {@link ChatCompletionsResource#completions(HttpServletRequest, HttpServletResponse, String, ChatCompletionRequestView)} - * — FR-027 and FR-028. + * — cross-site isolation of provider configuration and credentials. * *

          Isolation here is not enforced by a check anywhere; it is a property of one line, the cache * key {@code host + ":" + providerConfigHash} in @@ -63,11 +63,11 @@ * made it.

          * *
            - *
          • FR-028 — two sites with different provider configurations, driven with interleaved + *
          • Two sites with different provider configurations, driven with interleaved * requests, are each served by their own provider.
            • - *
          • FR-028 — rotating one site's configuration takes effect on its next request and leaves + *
          • Rotating one site's configuration takes effect on its next request and leaves * the other site untouched.
            • - *
          • FR-020 / FR-027 — a completion never reports another site's resolved site id.
            • + *
          • A completion never reports another site's resolved site id.
            • *
          */ public class InferenceSiteIsolationTest { @@ -201,7 +201,7 @@ public void test_completions_afterRotatingOneSitesConfiguration_usesTheNewOneAnd * When a completion is requested for each * Then each publishes its own site as the one that served, and never the other's * - *

          The serving site is what FR-020 puts on every response and what a reconciliation + *

          The serving site is what is published on every response and what a reconciliation * pipeline would bill against, so reporting a neighbour's id would be a billing error rather * than a cosmetic one.

          */ diff --git a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteResolutionTest.java b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteResolutionTest.java index bf2fca40bb28..0f655e6d487e 100644 --- a/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteResolutionTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/inference/rest/InferenceSiteResolutionTest.java @@ -56,15 +56,15 @@ * 404 for site B's model on the same host name is proof that site B's was not.

          * *
            - *
          • FR-017 — with no override, the site comes from the request's host name.
            • - *
          • FR-018 — an explicit override wins, given either as the {@code siteId} query parameter + *
          • With no override, the site comes from the request's host name.
            • + *
          • An explicit override wins, given either as the {@code siteId} query parameter * or as the {@code X-dotCMS-Site} header; the header wins when the two disagree.
            • - *
          • FR-025 — the legacy {@code host_id} and {@code Host} request parameters, which elsewhere + *
          • The legacy {@code host_id} and {@code Host} request parameters, which elsewhere * in dotCMS act as a de-facto site override, are ignored here.
            • - *
          • FR-020 — every response identifies the site whose configuration served it.
            • + *
          • Every response identifies the site whose configuration served it.
            • *
          * - *

          What is asserted for FR-020. The serving site reaches a real caller as the + *

          What is asserted for the serving site. It reaches a real caller as the * {@code X-dotCMS-Resolved-Site} response header, which {@link ResolvedSiteHeaderFilter} writes * from the request attribute {@link InferenceRequestAttributes#RESOLVED_SITE_ID}. These tests call * the resource method directly, so no JAX-RS response filter runs and no header exists to read. @@ -82,8 +82,8 @@ public class InferenceSiteResolutionTest { private static final String SITE_HEADER = "X-dotCMS-Site"; /** - * Legacy request parameter that elsewhere in dotCMS selects the current site. FR-025 requires - * this family to ignore it. + * Legacy request parameter that elsewhere in dotCMS selects the current site. This family + * is required to ignore it. */ private static final String LEGACY_HOST_ID_PARAM = "host_id"; @@ -272,7 +272,7 @@ public void test_completions_withHeaderAndQueryParameterDisagreeing_prefersTheHe * Then the legacy parameter is ignored: site A serves the request and is reported as the * serving site * - *

          FR-025. The parameter is a pre-existing de-facto site override on the rest of the + *

          The parameter is a pre-existing de-facto site override on the rest of the * product, and a token-authenticated API must not inherit it silently — a caller who never * asked for another site must not be able to spend its credentials by copying a query string. * The model is site A's, so honouring the parameter cannot merely look like success: it would @@ -323,8 +323,8 @@ public void test_completions_withLegacyHostParameter_ignoresIt() { * Then the legacy parameter is still ignored: the default site serves the request, exactly as * it would have without the parameter * - *

          FR-025 has no exception for the fallback path, and it is the path that matters most. The - * unmatched host name is not an edge case here — FR-020 keeps the default-site fallback + *

          There is no exception for the fallback path, and it is the path that matters most. The + * unmatched host name is not an edge case here — the default-site fallback is kept * precisely because the server-side callers this family exists for routinely arrive on * internal DNS, container service names or {@code localhost}, none of which are site aliases. * A legacy override that is ignored only while the host name happens to match is not ignored; @@ -372,7 +372,7 @@ public void test_completions_withLegacyHostParameterAndUnmatchedHostName_ignores * When the completion is requested * Then the serving site is still reported * - *

          FR-020 asks for the serving site on every response, not only the ones that + *

          The serving site is reported on every response, not only the ones that * worked. A refusal is precisely the case an operator reconciling spend needs attributed, and * a value only the happy path published would not deliver it.

          */