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>
github-actions
Bot
added
Area : Backend
|
Claude finished @fmontes's task in 5m 44s —— View job Claude PR ReviewReviewed the production surface of the new New Issues
Verified clean (checked, not issues)
Blocking on the 🟠 High; the Mediums are worth fixing but don't block. Since #37561 is the hardening PR stacked on this branch, the streaming-fallback and size-limit gaps may be natural fits there if you'd rather not expand this one. · branch |
1081938 to
9d95eb0
Compare
github-actions
Bot
removed
the
Area : Documentation
|
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>
1 participant
Implementation for #37431. The spec was #37493, approved and merged.
Split in two. This PR is the feature. The hardening — three defects found while reviewing it, plus the conformance test — is #37561, stacked on this branch. Land them together; the hardening protects the endpoints added here.
Adds an OpenAI-wire-format endpoint family at
/api/inference/v1so any standard client or agentic framework can use dotCMS as its model provider, with the customer's own provider credentials, resolved per site.POST /api/inference/v1/chat/completionsGET /api/inference/v1/modelsPOST /api/inference/v1/embeddingsPOST /api/inference/v1/images/generationsBearer token only. The model is required on every request and validated against the site's configuration for that capability, so a chat model is refused on the images endpoint. Every response names the serving site in
X-dotCMS-Resolved-Site.Reading order
Three commits, each a complete slice — reviewing them in order is far easier than reading the diff:
bb86ffd0chat completions and streaming — the internal representation, the mappers, the SSE serializer, and the two chat endpoints. The largest commit and the one that sets every pattern the others follow.d94dc463bearer-only auth and the legacy site-param leak — US3. Two real defects: the legacyhost_id/Hostrequest parameters were honoured on the unmatched-host fallback path, where they could redirect which site's credentials were spent; and bearer-only had never been implemented, so a session cookie authenticated.9d95eb0emodels, embeddings and images — the remaining three operations, on the patterns established in 1.Roughly 40% of the insertions are tests, most of it explanatory javadoc. The production surface is much smaller than the line count suggests.
Two things to read before the code
1. The Test-First gates did not hold, and I have not ticked them as though they did.
Constitution Principle V requires tests → developer approval → Red → implementation. US3's implementation was built before its tests existed, so they could not gate it. US4 and US5 were written test-first and did fail first, but the Red run was the agent's own and no developer approved the sets beforehand. That process fact cannot be made true retroactively.
What was recoverable is the outcome the gate exists to produce — proof these tests fail for the right reason when the behaviour they guard is removed. Mutations were applied one at a time, each deleting one load-bearing behaviour, each followed by a build, a targeted test run, and a revert. All were detected. The ones covering this PR:
host_idhonoured on fallbacknbounds removedUS3 carried the real risk — tests written against existing code can encode what the code does rather than what the requirement says — and all three US3 mutations killed their tests, on top of the two genuine defects those tests found when first written. Decide for yourself how much extra scrutiny US3–US5 warrant; the gate did not do that job for you. Six further mutations covering #37561 are listed there.
The prevention is process, not code: run
/speckit-implementin per-story slices that stop at each gate, rather than executing the whole task list in one pass. That habit also produced a PR this size.2. Four requirements were amended after the spec was approved, and need re-approval.
inputaccepts a single string or an array; every element validated, offending index namednis honored (an earlier draft refused it on a false premise), bounded above, and refused per-model where the provider cannot honor itRetry-Afterwhere the abstraction exposes it — it does not — and always send one on dotCMS's own capacity refusalsFR-031 and FR-037's changes land in #37561; FR-011 and FR-012 are here. The diff is five lines in
spec.md, and the reasoning is recorded inline in each requirement — including the false premise that produced the original FR-012 and how it was caught.Scope and blast radius
Additive. The shipped
/api/v1/ai/*endpoints keep their behaviour exactly (FR-033) —AiHostResolver.resolveHost/resolveHostStrictandLangChain4jAIClient.toSseChunkare untouched, and the new family got its own paths alongside them.executeWithFallbackwas generalised to a typed variant the original delegates to, so the fallback chain, cache keying and logging have one implementation.One trap worth knowing for any future endpoint here: a new REST package must be registered twice —
DotRestApplication's scanned packages for Jersey, andswagger-maven-plugin'sresourcePackagesfor the contract. Miss the second and the endpoint works while never appearing inopenapi.yaml, with CI green.Tests
All ten integration classes are registered in
MainSuite2b. An unregistered class compiles, passes locally, and never runs in CI — three of these were in that state until it was caught.Run several of these classes together with
-Dit.test.forkcount=1; they share the fixed WireMock port 50505 and the module defaults to four forks. Without it you get ten bogus "Failed to bind" failures that look like a broad regression.Rollback
M-3, MEDIUM — this adds a public REST API contract. Nothing is renamed or removed and there is no schema change, so a rollback is clean for existing behaviour; the risk is that clients which integrated against the new endpoints in this release get 404s if it is reverted. Labelled
AI: Not Safe To Rollbackon that basis — downgrade it if you read M-3 differently for a purely additive surface.Follow-ups, deliberately not here
HttpClientso a provider'sRetry-Aftercan be relayed (see fix(ai): harden /api/inference/v1 — stop three information and capacity leaks (#37431) #37561)BulkEmbeddingsRunnerTestis in noMainSuiteand has never run in CI — pre-existing, unrelatedStill outstanding before merge: the live-provider demo (quickstart step 8) and the Postman collection run.
🤖 Generated with Claude Code