Conversation
User Stories 1 and 2 of #37431: an unmodified OpenAI-compatible client can run a multi-turn, tool-calling conversation against dotCMS with only a base URL and an API token, streamed or not. 71 tests green: 60 unit, 4 integration (tool round trip), 7 integration (streaming), all against a live instance. Structure - New top-level package com.dotcms.inference, sibling to com.dotcms.ai rather than nested inside what it supersedes. - com.dotcms.inference.model holds the internal representation and carries NO Jackson annotations: FR-038 asks that it not be a binding of the wire JSON, and making serialization structurally impossible there is the only way to guarantee it. InferenceStreamEvent is a sealed interface, so the SSE serializer is a total function and a sixth variant breaks the build rather than falling through a default branch. Provider access - New InferenceAIClient owns the standard-bound semantics, which are driven by an external standard and will change when it does, separately from the dotAI endpoints which evolve on dotCMS's terms. - It does NOT own model construction, caching or eviction. AIAppListener flushes a site's cached providers on credential rotation through LangChain4jAIClient alone; a second cache would keep serving a revoked key until the TTL expired, with no symptom. So the new client borrows models through two additive accessors, withChatModel/withStreamingChatModel. - executeWithFallback generalised to a typed variant the original delegates to, so the fallback chain, cache keying and logging have one implementation and shipped behaviour is bit-identical. Notable behaviour - A failed stream withholds the [DONE] marker, so it can never be mistaken for a finished one. Verified against both provider error and connection fault. - Tool-call identity is announced once, on the first fragment. langchain4j repeats it on every fragment; passing that through made one call read as two. Caught by an integration test. - Streamed usage is emitted only when the client asks, and suppressed when a provider volunteers it unasked -- its empty choices array is what breaks readers assuming every chunk carries one. - stream_options is never forwarded: four of seven providers do not understand it. dotCMS builds the chunk from the counts the unified provider abstraction returns, which works for all of them. Also - com.dotcms.inference.rest registered in BOTH DotRestApplication (Jersey) and swagger-maven-plugin resourcePackages. Missing the second is silent: the endpoint works, the contract omits it, CI still passes. - openapi.yaml regenerated and committed. - Integration fixtures grant DOTCMS_BACK_END_USER explicitly; the role check matches by key and does not walk inheritance, so admin does not imply it. Refs #37431 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds User Story 3's test coverage -- per-site credential governance -- and fixes the two real defects it found. 26 tests across five suites, all green. The tests were written after the implementation, because US3's code was built as part of the foundation. That inverts the TDD gate and is recorded as such in tasks.md rather than papered over. It was worth doing anyway: 11 of the 14 initial tests merely verified working code, and the other 3 found defects that review had missed and that the chat-completion tests passed straight over. FR-025 -- legacy request parameters honoured where they did damage AiHostResolver.resolveFromRequest fell through to getCurrentHostNoThrow, which reads the host_id and Host request parameters before it ever looks at the server name. The parameters were already ignored whenever the host name matched a site, because that path returns earlier -- so the effect was that a legacy override was ignored everywhere it was harmless and honoured in precisely the case where it could redirect which site's credentials get spent. Now resolves the default site explicitly. resolveHost/resolveHostStrict keep the old call: they serve the shipped endpoints and FR-033 puts them out of bounds. That duplication is #37491's to resolve. FR-015 -- bearer-only was never implemented A request with no Authorization header but a live session was served normally; basic auth would have been too. That undercuts the reasoning for emitting no CORS headers, which rests on the credential being a token someone deliberately issued and placed on a server rather than one a browser attaches by itself. The rule now lives in one method that both a name-bound filter and the resource call -- the filter so it covers the three resources not yet written, the resource because a guarantee that only exists inside the JAX-RS chain is invisible to tests that invoke resource methods directly, which is how every integration test here reaches one. Two authorization tests were changed. They asserted the refusal arrives as a thrown WebApplicationException, which encoded the behaviour from before this family refused for itself. A 401 now carries InferenceErrorView, as the contract's status table requires, so a client library can deserialize a refusal into its own error type. The assertions were strengthened to check the body shape as well as the status, not relaxed. Refs #37431 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes US4 and US5: the three remaining operations, each resolving the site and validating the model through the same shared component the chat endpoint uses, and each returning the standard wire shape. Images: `n` is honored, with both bounds refused rather than clamped. An earlier draft of FR-012 refused every `n` other than 1, justified by the claim that the provider abstraction returns a single image per call. That claim was false — `ImageModel.generate(prompt, n)` returns a list, and the adopted format documents up to 10 images per request. Correcting it surfaced two further problems, both fixed here: - `generate(prompt, n)` is a default method that throws unless overridden. OpenAiImageModel and OpenAiOfficialImageModel override it; GoogleAiGeminiImageModel does not. So on a Gemini-configured site a request for several images threw inside the client library and reached the caller as a 502 — a retryable status for a request that can never succeed, which sends a standard client's back-off into an unwinnable loop. It is now a 400 naming the field. Support is probed from the model's declaring class rather than a provider-name list, so it cannot rot on a library upgrade. - Honoring `n` removed the spend ceiling the old rule had imposed by accident, and FR-032 puts per-site quotas out of scope. Adds DOT_INFERENCE_MAX_IMAGES_PER_REQUEST, defaulting to 10 — the ceiling the OpenAI images API documents for this field, so a client written against the standard meets the same limit here it already handles there. Also registers all three new test classes in MainSuite2b. Unregistered integration tests compile and pass locally but are silently never run in CI. Tests: 60 unit, 72 integration across the ten inference classes, all green. Run several dotAI classes at once with -Dit.test.forkcount=1; they share the fixed WireMock port 50505 and forkCount defaults to 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
22 tasks
github-actions
Bot
added
Area : Backend
Contributor
|
Claude finished @fmontes's task in 3m 8s —— View job Code Review —
|
fmontes
force-pushed
the
fmontes/37431-openai-compatible-inference
branch
from
September 15, 2026 21:27
1081938 to
9d95eb0
Compare
github-actions
Bot
removed
the
Area : Documentation
Contributor
|
Pull Request Unsafe to Rollback!!!
|
The four /api/inference/v1 resource methods were declared final while carrying
@RequestCost, which is a CDI interceptor binding. Weld intercepts by
subclassing, so a final method cannot be proxied, and it refuses the deployment:
WELD-001504: Intercepted bean method ... public final
ChatCompletionsResource.completions(...) cannot be declared final
That fails DotRestApplication's servlet init, which does not break these four
endpoints — it takes down every REST endpoint in dotCMS. Each subsequent
request then retries the init and logs a secondary "resource configuration is
not modifiable" error, which reads like an unrelated Jersey problem and is
where an investigation naturally starts. No other @RequestCost method in the
codebase is final.
Nothing in the test suite could have caught this: every integration test in
this family invokes the resource methods directly, so none of them passes
through Weld or Jersey. The suite was green while the application could not
start. InterceptedMethodsAreNotFinalTest closes that specific gap by
reflection, over every declared method rather than a list of today's four, so a
fifth operation added later is covered too. It was verified to fail by
restoring final on one method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Pull Request Unsafe to Rollback!!!
No database migrations, Elasticsearch mapping changes, structural storage changes, or other C/H-level categories were found in this diff — the rest of the changes (new model/DTO classes, provider-client refactor in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OpenAI-wire-format endpoints at
/api/inference/v1, so any standard client or agent framework can point at dotCMS and use the customer's own provider credentials, resolved per site.Hardening follow-up is #37561, stacked on this branch. Land them together.
Architecture
New package
com.dotcms.inference.rest, sitting besidecom.dotcms.ai.restrather than inside it.Nothing new for configuration, credentials, provider selection or model caching — it all comes from the existing dotAI plumbing.
Where to look
AiHostResolver— widened to public, gainsresolve(request, siteOverride, user)returningResolvedAiContext(User, Host, AppConfig). Additive:resolveHost/resolveHostStrictuntouched, so shipped endpoints are unaffected.LangChain4jAIClient—executeWithFallbackgeneralised to a typed variant the original delegates to; newwithChatModel/withStreamingChatModel/withEmbeddingModel/withImageModelaccessors. Models are still built and cached here, which is whyAIAppListener's cache flush keeps working on key rotation.InferenceAIClient— everything wire-format-specific. Tool specs, tool-call identity, stream assembly, usage.SseSerializer— chunk framing and the[DONE]rules.Decisions
A separate client, not an extension of
LangChain4jAIClient.AIProxyClientregisters exactly one client withcurrentProviderhardcoded, and the shippedtoSseChunkemits a shape the legacy endpoint depends on. Changing either would move behaviour under existing callers. The new client borrows models but owns semantics.A separate cache would have been a security bug.
AIAppListenerflushesLangChain4jAIClient's caches by host on secret change. A second cache would keep serving revoked keys — so the new client deliberately takes models from the existing one.Model is required. No default, no alias. Every comparable API requires it, and it keeps the accepted set exactly equal to what the site configured, with no reserved word a provider's naming could collide with. Consequence, accepted knowingly: a provider swap is not invisible — a caller pinning an old vendor model gets a 404 and must re-read
/models.Site resolution stays standard, fallbacks included. These callers are servers presenting internal DNS, container names,
localhost— none of which is a site alias. Refusing them would break the primary deployment. The risk (nobody can tell which site paid) is answered by logging the fallback and naming the serving site inX-dotCMS-Resolved-Siteon every response, refusals included.Bearer tokens only. The credential is long-lived with its owner's full authority. Session and basic are refused. The check is a static method both the filter and the resource call — a filter alone only exists inside the JAX-RS chain, and every integration test here invokes resource methods directly, so a filter-only control passes CI while proving nothing.
The internal model has no Jackson annotations. Structural, not stylistic: it stops the chat-completions shape leaking into the core when a second wire format arrives.
Two defects this found in shipped code
host_id/Hostrequest parameters were honoured on the unmatched-host fallback path — a caller could append?host_id=<other site>and spend that site's credentials. Ignored where the server name matched, live exactly where it did damage. Fixed for this family only; the shipped endpoints still have it (see #37491).Bearer-only had never been implemented — a session cookie authenticated.
Notes for running it
A new REST package must be registered twice:
DotRestApplication's scanned packages, andswagger-maven-plugin'sresourcePackages. Miss the second and the endpoint works while never appearing inopenapi.yaml, with CI green.Running several of these test classes together needs
-Dit.test.forkcount=1— they share the fixed WireMock port and the module defaults to four forks. Without it you get ten bogus bind failures that look like a broad regression.Test-first gates did not hold
US3 was implemented before its tests existed; US4/US5 were written test-first but their Red run was the agent's, with no developer approval. Not ticked as if they had. Eleven mutations were applied afterwards — each deleting one behaviour, building, running the guarding test, reverting — and all eleven were detected. That covers the outcome, not the process. Decide for yourself how much extra scrutiny US3–US5 warrant.
🤖 Generated with Claude Code