Conversation
22 tasks
Contributor
|
Claude finished @fmontes's task in 3m 55s —— View job Review — inference hardening (#37561)
I reviewed the diff against
FR-031 chain-walking (rate-limit → 429, non-retriable → 400, else 502), the dotCMS-own New Issues
Notes (non-blocking)
Nothing here is blocking — both Medium items are Javadoc-binding defects, not behaviour. The logic, security posture, and test coverage are solid. |
…d ops 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.<name>.*` 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
… logging 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) <noreply@anthropic.com>
…path
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) <noreply@anthropic.com>
fmontes
force-pushed
the
fmontes/37431-inference-hardening
branch
from
September 16, 2026 14:12
1081938 to
8fa5a85
Compare
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) <noreply@anthropic.com>
… type
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) <noreply@anthropic.com>
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.
Stacked on #37559 — review that one first. Base is its branch, so this PR's diff is only the hardening.
Split out deliberately. #37559 is the feature; this is the set of changes where a missed detail has a consequence, and all of it was buried under 11k lines of endpoints and tests. Three of the six commits fix defects that were not in the original scope and that no test would have caught.
The defects
A provider's message reached the caller on the stream path. 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. What the old code returned, from the test that now pins it:
Endpoint, key, and a fragment of the prompt. The buffered paths never did this and
ImagesResourcedocuments that they must not, so the streaming path was a discrepancy rather than a policy. The cause was using an exception type as a proxy for provenance: nearly everything reaching the translator is anIllegalArgumentException, and they arrive both from sentences dotCMS composed and from the fallback chain's wrapper around a failed initialisation, which appends whatever the provider said.CallerSafeExceptionnow records provenance where the text is written.CORS headers were being emitted on an API whose credential is a long-lived bearer token. dotCMS ships
api.cors.default.Access-Control-Allow-Origin=*, andCorsFilteris a global@Providerapplying the default mapping to every resource without one of its own. FR-030 requires the opposite. A@NoCorsmarker now opts a resource out; a resource that does not ask for it keeps today's behaviour exactly, pinned from both sides byCorsFilterTest.The request-size ceiling could be skipped by omitting one header.
RequestSizeLimitFilterreturned early whenever a request declared noContent-Length— which several HTTP clients do by default when streaming a body — behind a comment claiming "the resource enforces the ceiling as it reads". No resource did. The entity stream is now replaced with one that counts and refuses mid-read, raising the same typed 413 as the declared path. Counting rather than buffering-to-measure is the point: buffering to size it hands over exactly the memory the ceiling exists to bound.The rest
502, rate limits included, so a throttled client was told the provider was broken rather than busy. A rate limit is now429, walking the cause chain because the fallback chain wraps. dotCMS's own capacity refusal now carriesRetry-After; the provider's cannot be relayed, and that limitation is recorded in the spec and the docs rather than dropped quietly.@Deprecatedwith noforRemovaland no removal date.InferenceClientConformanceTestserializes what each endpoint returned, serves those bytes from WireMock, and points an unmodified LangChain4j OpenAI client at them, asserting only on the library's own result types. "No adapter" is demonstrated by the absence of code between the two. Streaming included, by replaying real SSE bytes through the library's stream reader.Shared code
Two files outside the feature change, both additively.
CorsFilterhonours@NoCors; anything that does not ask for it is unaffected.executeWithFallbackwas generalised to a typed variant the original delegates to. The shipped/api/v1/ai/*endpoints keep their behaviour exactly (FR-033) —CallerSafeExceptionextendsIllegalArgumentExceptionprecisely so callers catching the broader type are untouched.Verification
75 unit, 84 integration, all green. Every negative assertion here was proven capable of failing: the logging test by injecting a leak and watching three tests fail with the prompt quoted back; the error-leak test by restoring the old pass-through; the size ceiling, CORS opt-out and 429 mapping each by a mutation that removed the behaviour. Eleven mutations total, all detected — the table is in the tasks file and summarised on #37559.
Run several of these classes together with
-Dit.test.forkcount=1; they share the fixed WireMock port and the module defaults to four forks.Rollback
Additive; no schema change. Rollback removes the hardening along with the endpoints it protects, which is coherent only if #37559 is rolled back too — these should land and revert together.
🤖 Generated with Claude Code
This PR fixes: #37431