fix(runtime): shared Gemma routing, bounded structured reranking, embedding health, and attributed errors - #68
Conversation
Log bounded request metadata and request IDs for API 404s and server errors while keeping browser 404 noise at info.
Document the queued gateway alias and its configured provider failover without changing the application fallback.
Probe latency alone cannot prove a remote model reload, and reporting every slow success as a warning obscured genuine provider failures. Make the lifecycle state explicit so health and logs reflect observed outcomes. - classify ready, slow, unavailable, repeated, and recovered probes - expose embedding availability through Actuator health with prompt recovery retries - attribute events to the configured model and cover state transitions deterministically
Keep lifecycle escalation policy explicit and satisfy the repository magic-literal contract. - share one domain-qualified threshold across slow and failed probe loops
Warning Review limit reached
Next review available in:30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe changes add embedding health indicators, enforce JSON-object completion for reranking, structure and sanitize error logs, document gateway failover configuration, and adjust application test mocking. ChangesEmbedding health lifecycle
JSON completion requests
Structured error logging
Gateway configuration documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RerankerService
participant OpenAIStreamingService
participant OpenAiRequestFactory
participant LLMProvider
RerankerService->>OpenAIStreamingService: request JSON-object completion
OpenAIStreamingService->>OpenAiRequestFactory: build JSON response request
OpenAiRequestFactory->>LLMProvider: submit provider completion request
LLMProvider-->>OpenAIStreamingService: return completion
OpenAIStreamingService-->>RerankerService: return ranked JSON
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:44668cadaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
This pull request hardens Java Chat’s runtime behavior for a gateway-based chat configuration rollout by enforcing structured reranker outputs, improving embedding provider lifecycle/health reporting, and adding request-attributed error diagnostics, alongside updated gateway documentation and examples.
Changes:
- Enforce JSON-object response contracts for reranker completions and add focused request factory / reranker tests.
- Add embedding keep-alive lifecycle tracking (ready/slow/unavailable), expose it via Actuator health, and add shorter recovery retries with tests.
- Add bounded, sanitized request attribution to error logs and document the shared Gemma gateway configuration in docs and
.env.example.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java | Adds MVC tests verifying sanitized, attributed error logging behavior. |
| src/test/java/com/williamcallahan/javachat/service/RerankerServiceTest.java | Updates reranker tests to require JSON-object completions. |
| src/test/java/com/williamcallahan/javachat/service/OpenAiRequestFactoryTest.java | Adds coverage asserting JSON-object response format is declared. |
| src/test/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAliveTest.java | Expands tests for embedding probe lifecycle, logging, health, and recovery. |
| src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java | Adjusts context-load test mocking for the embedding client. |
| src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java | Adds bounded sanitization and request-attributed logging for failures. |
| src/main/java/com/williamcallahan/javachat/service/RerankerService.java | Switches reranking to a JSON-object completion path. |
| src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java | Introduces completeJsonObject and plumbs JSON requirement into completion requests. |
| src/main/java/com/williamcallahan/javachat/service/OpenAiRequestFactory.java | Adds JSON-object completion request builder and response-format declaration. |
| src/main/java/com/williamcallahan/javachat/service/OpenAiCompatibleEmbeddingClient.java | Implements EmbeddingClient.modelName() for health/lifecycle reporting. |
| src/main/java/com/williamcallahan/javachat/service/LocalEmbeddingClient.java | Implements EmbeddingClient.modelName() for health/lifecycle reporting. |
| src/main/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAlive.java | Adds lifecycle tracking, health projection, and recovery retry scheduling. |
| src/main/java/com/williamcallahan/javachat/service/EmbeddingClient.java | Extends the embedding port with modelName() for diagnostics. |
| docs/configuration.md | Documents shared Gemma gateway configuration and provider ordering behavior. |
| .env.example | Updates gateway notes and example values for the shared gateway setup. |
Comments suppressed due to low confidence (1)
src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java:31
EmbeddingModelKeepAlivenow requires a non-nullEmbeddingClient.modelName()during Spring context startup.@MockitoBean(answers = Answers.RETURNS_MOCKS)does not reliably provide a non-nullStringreturn (especially without an inline mock maker), so this context-load test can still fail with an NPE. Explicitly stubmodelName()on the mock to guarantee startup.
@MockitoBean(answers = Answers.RETURNS_MOCKS)
EmbeddingClient embeddingClient;
@MockitoBean
QdrantClient qdrantClient;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java (1)
27-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid broad
RETURNS_MOCKSbehavior in the context smoke test.This can make unstubbed embedding interactions appear valid by returning mocks/defaults, weakening the test’s ability to catch broken startup contracts. Prefer the default mock plus explicit stubbing for the exact embedding calls required by context initialization. Mockito documents
RETURNS_MOCKSas returning mocks for unstubbed invocations. (javadoc.io)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java` around lines 27 - 28, Replace the broad Answers.RETURNS_MOCKS configuration on the embeddingClient test mock with the default mock behavior, then explicitly stub the exact embedding interactions required during context initialization. Keep the context smoke test focused on those known calls so unexpected unstubbed interactions still fail.src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java (1)
98-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
logRequestFailureparameter count and align boolean namingThe helper takes 5 positional parameters (
request,statusCode,uri,apiRequest,exception), exceeding the >4 parameter limit. SincestatusCode,uri,isApiRequest, andexceptionare all derivable fromHttpServletRequestattributes, consider reducing the signature to justrequestand computing the rest internally — the derivation is a few one-liners and keeps the method self-contained.Additionally, the
apiRequestparameter diverges from theisApiRequestlocal variable inhandleErrorfor the same boolean concept. UseisApiRequestconsistently across the call chain.As per coding guidelines: ">4 parameters use parameter object or builder; never add 5th positional argument (Long Params)" and "The same concept uses the same name across method signatures, variable assignments, log messages, and documentation; do not alias the same thing with different names in the same scope or call chain (Alias Consistency)."
♻️ Proposed refactor
- private void logRequestFailure(- HttpServletRequest request, int statusCode, String uri, boolean apiRequest, Object exception) {+ private void logRequestFailure(HttpServletRequest request) {+ Object statusAttribute = request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);+ int statusCode = statusAttribute instanceof Integer integerStatus ? integerStatus : 500;+ Object requestUriAttribute = request.getAttribute(RequestDispatcher.ERROR_REQUEST_URI);+ String uri = requestUriAttribute != null ? requestUriAttribute.toString() : request.getRequestURI();+ boolean isApiRequest = uri.equals("/api") || uri.startsWith("/api/");+ Object exception = request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);+ String method = safeLogField(request.getMethod()); String canonicalUri = safeLogField(uri.split("[?#]", 2)[0]); String serverHost = safeLogField(request.getServerName()); String userAgent = safeLogField(request.getHeader("User-Agent")); String requestId = safeLogField(request.getRequestId()); String source = safeLogField(request.getAttribute(RequestDispatcher.ERROR_SERVLET_NAME)); String diagnostic = "Request failed status={} source={} method={} uri={} host={} userAgent={} requestId={}";And update the call site:
- logRequestFailure(request, statusCode, uri, isApiRequest, exception);+ logRequestFailure(request);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java` around lines 98 - 99, Refactor CustomErrorController.logRequestFailure to accept only HttpServletRequest, deriving statusCode, uri, isApiRequest, and exception from the request attributes inside the helper. Update handleError and all call sites to use the consistent isApiRequest name throughout the call chain while preserving the existing logging behavior.Source: Coding guidelines
src/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java (1)
59-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for 5xx-without-exception and
safeLogFieldedge casesThe three tests cover the main log paths well — INFO 404 non-API, WARN 404 API, and ERROR 500 with exception. Three gaps remain:
- 5xx without exception (controller lines 120–122): This is a distinct branch that logs at ERROR without a throwable. A 500 where
ERROR_EXCEPTIONis absent exercises it.safeLogFieldtruncation: No test sends a field exceeding 512 characters to verifyMAX_LOG_FIELD_LENGTHis enforced. A longUser-Agentheader would confirm the bound.safeLogFieldnull →"unknown": No test omits theUser-Agentheader to verify the"unknown"default appears in the log.All three are low-effort additions that close meaningful coverage gaps.
🧪 Suggested additional tests
`@Test` voidlogs_server_error_without_exception_at_error() throwsException { mvc.perform(errorRequest(HttpStatus.INTERNAL_SERVER_ERROR, "/page") .requestAttr(RequestDispatcher.ERROR_SERVLET_NAME, "dispatcherServlet")) .andExpect(status().isInternalServerError()); ILoggingEventevent = onlyLogEvent(); assertEquals(Level.ERROR, event.getLevel()); assertNull(event.getThrowableProxy()); } `@Test` voidtruncates_long_user_agent_to_max_log_field_length() throwsException { StringlongUserAgent = "A".repeat(1024); mvc.perform(errorRequest(HttpStatus.NOT_FOUND, "/missing") .header("User-Agent", longUserAgent)) .andExpect(status().isNotFound()); ILoggingEventevent = onlyLogEvent(); Stringmessage = event.getFormattedMessage(); intuserAgentStart = message.indexOf("userAgent=") + "userAgent=".length(); intuserAgentEnd = message.indexOf(" requestId=", userAgentStart); StringloggedUserAgent = message.substring(userAgentStart, userAgentEnd); assertEquals(512, loggedUserAgent.length()); } `@Test` voidsubstitutes_unknown_for_missing_user_agent() throwsException { mvc.perform(errorRequest(HttpStatus.NOT_FOUND, "/missing")) .andExpect(status().isNotFound()); ILoggingEventevent = onlyLogEvent(); assertTrue(event.getFormattedMessage().contains("userAgent=unknown")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java` around lines 59 - 117, Add three tests to CustomErrorControllerTest covering the missing branches: verify a 5xx request without ERROR_EXCEPTION logs at ERROR with no throwable, verify a 1024-character User-Agent is truncated to 512 characters in the message, and verify an omitted User-Agent logs as "unknown". Reuse errorRequest and onlyLogEvent, and preserve the existing log-level and response assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 30-33: Update the Researchly shared LLM gateway example in
.env.example to include LLM_PRIMARY_PROVIDER=openai alongside the gateway URL,
model, and key settings, matching the explicit provider selection documented in
configuration guidance.
---
Nitpick comments:
In `@src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java`:
- Around line 98-99: Refactor CustomErrorController.logRequestFailure to accept
only HttpServletRequest, deriving statusCode, uri, isApiRequest, and exception
from the request attributes inside the helper. Update handleError and all call
sites to use the consistent isApiRequest name throughout the call chain while
preserving the existing logging behavior.
In `@src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java`:
- Around line 27-28: Replace the broad Answers.RETURNS_MOCKS configuration on
the embeddingClient test mock with the default mock behavior, then explicitly
stub the exact embedding interactions required during context initialization.
Keep the context smoke test focused on those known calls so unexpected unstubbed
interactions still fail.
In
`@src/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java`:
- Around line 59-117: Add three tests to CustomErrorControllerTest covering the
missing branches: verify a 5xx request without ERROR_EXCEPTION logs at ERROR
with no throwable, verify a 1024-character User-Agent is truncated to 512
characters in the message, and verify an omitted User-Agent logs as "unknown".
Reuse errorRequest and onlyLogEvent, and preserve the existing log-level and
response assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 161585e5-fd0a-49cc-bd6d-da3b6d351c61
📒 Files selected for processing (15)
.env.exampledocs/configuration.mdsrc/main/java/com/williamcallahan/javachat/service/EmbeddingClient.javasrc/main/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAlive.javasrc/main/java/com/williamcallahan/javachat/service/LocalEmbeddingClient.javasrc/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.javasrc/main/java/com/williamcallahan/javachat/service/OpenAiCompatibleEmbeddingClient.javasrc/main/java/com/williamcallahan/javachat/service/OpenAiRequestFactory.javasrc/main/java/com/williamcallahan/javachat/service/RerankerService.javasrc/main/java/com/williamcallahan/javachat/web/CustomErrorController.javasrc/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.javasrc/test/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAliveTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAiRequestFactoryTest.javasrc/test/java/com/williamcallahan/javachat/service/RerankerServiceTest.javasrc/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java
Uh oh!
There was an error while loading. Please reload this page.
The keep-alive health bean reads the embedding model name while Spring creates the application context. Give the integration mock a non-null String default so enabled live tests reach their assertions without weakening production validation. - align the chat SSE embedding mock with the application context fixture
The guided live-test context creates the embedding keep-alive bean before test methods can stub the provider. Supply a non-null String default at mock creation so the context honors the embedding port contract. - align the guided SSE embedding mock with the application context fixture
The shared-gateway instructions named its URL, model, and key but did not repeat the provider-selection setting required when GitHub credentials are also present. Keep the general defaults unchanged while making the gateway setup self-contained. - require LLM_PRIMARY_PROVIDER=openai in the gateway instruction block
Co-authored-by: detail-app[bot] <180357370+detail-app[bot]@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Summary
Java Chat dev routes user-facing chat through the shared gateway's regular Gemma alias while preserving its separate Qwen embedding provider path. The branch also hardens reranking output, embedding-provider health and recovery, request-failure attribution, and enabled live-test startup.
Changes
gemma-4-26b-a4balias, which may fail over across configured gateway providers (.env.example,docs/configuration.md).EmbeddingClient,LocalEmbeddingClient,OpenAiCompatibleEmbeddingClient).OpenAiRequestFactory,OpenAIStreamingService,RerankerService).EmbeddingModelKeepAlive).CustomErrorController).ChatSseIntegrationTest,GuidedSseIntegrationTest).Verification
origin/dev.Deployment notes
Pushing
devtriggers the Java Chat dev deployment. User-facing chat usesgemma-4-26b-a4bthrough the shared gateway; embeddings remain on their existing explicit provider and model configuration.Related investigation: aventurevc/back-end#1118