fix(reliability): readiness-gated deploys, stable streamed enrichment text, and secure operational telemetry - #72
Merged
Merged
Conversation
Make frontend/package.json the canonical Node runtime owner and bind every environment selector and generated projection to it with an executable parity check. - Build the frontend with Node 24.15.0 in Docker - Synchronize nvm, lockfile, and setup documentation - Fail frontend validation when a projection drifts
Render the current body of an unresolved enrichment as ordinary markdown while withholding a lone terminal brace that represents an incomplete stream close marker. - Preserve streamed prose instead of leaking directive syntax - Retain nested valid enrichments and completed content braces
Record recognized provider unavailability in health state while allowing unexpected runtime defects to propagate without overwriting the last completed probe observation. - Preserve deferred-probe behavior - Align health documentation and regression coverage - Remove the broad RuntimeException rethrow finding
Make the immutable renderer final and narrow its PNG regression test to the checked image-read failure it can actually raise, eliminating the SpotBugs subclassing warning without suppressions.
Replace the broad checked-exception declaration in concurrent embedding tests with the interruption, execution, and timeout failures their futures can raise.
Remove low-trust request headers from failure diagnostics, preserve sanitized request context as SLF4J key-value fields, and configure every active Logback encoder to render those fields. Regression coverage verifies levels, causes, field sanitization, bounded URI logging, and the deployed console pattern.
Narrow the concurrent provider-circuit regression test to the interruption, execution, and timeout failures exposed by Future.get, removing the broad checked-exception declaration.
Remove request-derived resource names from documentation read failures and add MVC coverage for direct, forwarded, invalid, missing, and unreadable error pages. The read-failure regression verifies the error log carries only its typed cause and no formatting arguments.
Describe the private classloader fixture so the error-documentation regression suite satisfies the repository documentation gate without suppressing PMD.
Expose Prometheus through the Boot-managed registry while separating process liveness from dependency readiness and retaining the aggregate health contract. - Restrict Actuator access to health probes and Prometheus - Use the liveness group for container restart decisions - Preserve readiness checks for Qdrant and the embedding model - Run Java as PID 1 for reliable container signal handling
Global lazy initialization must not defer required credential failures until the first provider request. - Mark credential validation as explicitly eager - Preserve the existing typed startup validation behavior
The AOP logger duplicated pipeline lifecycle events already owned by ChatController and retained disabled and low-value advice. - Delete the obsolete ProcessingLogger aspect - Keep the canonical PIPELINE appenders and ChatController events - Confine verbose application and Spring AI logging to the dev profile
Provider failures were logged repeatedly while propagating through service layers, producing duplicate error events for one request. - Remove log-and-rethrow handling from ChatService - Log only terminal completion failures after provider routing is exhausted - Keep unavailable providers at warning severity until the request boundary - Cover the unavailable-provider severity contract
Keep the existing container serving traffic until the replacement has completed dependency initialization and can accept application requests. - Probe Spring Boot readiness instead of process liveness - Allow 120 seconds for cold embedding and Qdrant startup - Check readiness every five seconds during startup
Unexposed actuator paths could fall through to the SPA security chain instead of being rejected, obscuring the real management boundary. - Deny every unmatched /actuator path in the application chain - Keep aggregate health, probes, and Prometheus available - Exercise real Boot observability endpoints and denied metrics/info routes
Credential validation must remain excluded from global lazy initialization so missing secrets fail during startup. - Assert the configuration carries an explicit non-lazy contract
Keep aggregate and readiness health assertions independent from the scheduled embedding warm-up timing. - Replace the embedding health contributor with a deterministic DOWN state - Preserve the operational actuator surface checks
Contributor
Warning Review limit reached
Next review available in:27 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 (4)
📝 WalkthroughWalkthroughThe PR updates container readiness and Prometheus metrics, synchronizes frontend Node.js to 24.15.0, improves streaming markdown enrichment handling, refactors backend logging with structured fields and bounded values, introduces terminal streaming failure reporting to prevent duplicate alerts, and adds comprehensive test coverage for the new error handling patterns. ChangesRuntime observability and deployment
Frontend toolchain, markdown streaming, and component refactoring
Backend streaming failure reporting and error diagnostics
Test infrastructure and assertions
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Coolify
participant Docker
participant Actuator
participant HealthChecks
participant Prometheus
Coolify->>Docker: evaluate container health
Docker->>Actuator: GET /actuator/health/readiness
Actuator->>HealthChecks: evaluate readinessState<br/>qdrant + embeddingModelKeepAlive
HealthChecks-->>Actuator: readiness UP/DOWN
Actuator-->>Docker: readiness response
Docker-->>Coolify: pass/fail healthcheck
Prometheus->>Actuator: GET /actuator/prometheus
Actuator-->>Prometheus: jvm/custom metrics
Possibly related PRs
Suggested labels: 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.
Pull request overview
This PR strengthens deployment reliability and operational safety by gating container cutover on dependency-aware readiness, tightening public actuator exposure, reducing duplicate provider-failure noise, and making streamed enrichment output more resilient during partial renders. It also improves observability via structured key/value logging and ensures reproducible frontend builds by pinning and validating a single Node.js version across repo touchpoints.
Changes:
- Gate rolling deployments on
/actuator/health/readiness, expose only operational actuator endpoints (health probes + Prometheus), and add Prometheus registry runtime support. - Stabilize partial streamed enrichment rendering and reduce noisy/duplicated provider-failure logging while preserving root causes.
- Enforce Node.js 24.15.0 consistently via
package.jsonengines plus a repo-wide validation script.
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/williamcallahan/javachat/web/OpenGraphImageRendererTest.java | Narrows test exception signature to IOException. |
| src/test/java/com/williamcallahan/javachat/web/ErrorDocumentationControllerTest.java | Adds coverage for forwarded-status semantics and safe failure logging. |
| src/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.java | Updates assertions for structured logging fields and bounded/safe metadata. |
| src/test/java/com/williamcallahan/javachat/service/ProviderCircuitStateTest.java | Refines declared checked exceptions for concurrent test behavior. |
| src/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.java | Adds validation that “providers unavailable” is logged at request boundary severity. |
| src/test/java/com/williamcallahan/javachat/service/OpenAiCompatibleEmbeddingClientTest.java | Refines declared checked exceptions for concurrency tests. |
| src/test/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAliveTest.java | Updates expectations so unexpected probe failures propagate without flipping health state. |
| src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java | Adds MockMvc actuator surface assertions (allowed probes/prometheus; forbid metrics/info). |
| src/test/java/com/williamcallahan/javachat/config/RequiredCredentialValidationTest.java | Asserts credential validation remains eager even with global lazy init. |
| src/main/resources/logback-spring.xml | Adds %kvp to patterns and reduces Spring AI logging noise outside dev. |
| src/main/resources/application.properties | Restricts exposed actuator endpoints; enables liveness/readiness probe groups. |
| src/main/resources/application-dev.properties | Restores Spring AI debug logging for dev profile only. |
| src/main/java/com/williamcallahan/javachat/web/OpenGraphImageRenderer.java | Makes renderer final (deterministic, cached startup rendering). |
| src/main/java/com/williamcallahan/javachat/web/ErrorDocumentationController.java | Avoids logging request/resource-derived interpolation while preserving HTTP status on forwards. |
| src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java | Switches request-failure logging to structured key/value fields with safe bounding. |
| src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java | Demotes “providers unavailable” to WARN and removes duplicate streaming-failure error logs. |
| src/main/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAlive.java | Treats recognized provider unavailability as readiness-impacting; propagates unexpected defects. |
| src/main/java/com/williamcallahan/javachat/service/ChatService.java | Removes redundant error log + rethrow on streaming errors to avoid duplicate signals. |
| src/main/java/com/williamcallahan/javachat/logging/ProcessingLogger.java | Removes high-noise pipeline logging aspect. |
| src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java | Denies non-operational actuator routes and whitelists only probes + Prometheus. |
| src/main/java/com/williamcallahan/javachat/config/RequiredCredentialValidation.java | Forces eager startup validation via @Lazy(false). |
| gradle/libs.versions.toml | Adds Micrometer Prometheus registry coordinate. |
| build.gradle.kts | Adds Prometheus registry as a runtime dependency. |
| frontend/src/lib/services/markdown.ts | Preserves streamed prose for incomplete enrichments while hiding unfinished directive syntax. |
| frontend/src/lib/services/markdown.test.ts | Adds regression test for partial enrichment rendering. |
| frontend/scripts/verify-node-version.mjs | Adds repo-wide Node version projection verification (package.json/lockfile/.nvmrc/Dockerfile/docs). |
| frontend/package.json | Pins Node engine to 24.15.0 and wires node-version validation into npm run validate. |
| frontend/package-lock.json | Updates lockfile engine projection to 24.15.0. |
| frontend/.nvmrc | Pins dev Node version to v24.15.0. |
| docs/getting-started.md | Documents Node.js 24.15.0 requirement. |
| Dockerfile | Updates builder image to Node 24.15.0; readiness-gated healthcheck; execs Java as PID 1. |
Files not reviewed (1)
- frontend/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
frontend/src/lib/services/markdown.ts (1)
350-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract shared
marked.parseoptions into a constant.The options object
{ async: false, gfm: true, breaks: false }is duplicated between the unresolved path (line 350) and the resolved path (line 381). A shared constant would keep them in lockstep if they ever need to change together.♻️ Optional refactor
+const ENRICHMENT_PARSE_OPTIONS = { async: false, gfm: true, breaks: false } as const;+ return marked.parse(normalizeMarkdownForStreaming(unresolvedContent), { - async: false,- gfm: true,- breaks: false,- });+ ...ENRICHMENT_PARSE_OPTIONS,+ });And similarly at line 381:
const innerHtml = marked.parse(normalizedContent, { - async: false,- gfm: true,- breaks: false, // Preserve fence detection accuracy- });+ ...ENRICHMENT_PARSE_OPTIONS, // Preserve fence detection accuracy+ });🤖 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 `@frontend/src/lib/services/markdown.ts` around lines 350 - 354, In the markdown parsing flow, extract the duplicated { async: false, gfm: true, breaks: false } options from both the unresolved and resolved paths into one shared constant, then pass that constant to each marked.parse call so the configurations remain synchronized.src/main/resources/application.properties (1)
147-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClean up now-dead
/actuator/infoconfiguration (lines 151–154).Line 147 narrowed exposure to
health,prometheus, removinginfoandmetrics. The remainingmanagement.info.*andinfo.*properties on lines 151–154 are now dead configuration — they configure an endpoint that is no longer web-exposed.Per the Delete Unused guideline, remove these lines. Additionally,
management.info.env.enabled=true(line 152) is a latent security risk: ifinfois ever re-added to the exposure list, environment properties (potentially including sensitive values) would be exposed without review.🧹 Proposed cleanup
management.endpoint.health.group.readiness.include=readinessState,qdrant,embeddingModelKeepAlive -management.info.build.enabled=true-management.info.env.enabled=true-info.application.name=${spring.application.name}-info.deployment.commit=${SOURCE_COMMIT:unknown} # Reduce Qdrant client warning verbosity via logging🤖 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/resources/application.properties` around lines 147 - 150, Remove the unused management.info.* and info.* properties from application.properties, including management.info.env.enabled, while preserving the health, probe, and prometheus exposure settings.Source: Coding guidelines
Dockerfile (1)
97-101: 🩺 Stability & Availability | 🔵 TrivialReadiness probe now gates on external dependencies — confirm cascading-failure risk is acceptable.
The HEALTHCHANGE probes
/actuator/health/readiness, which includesqdrantandembeddingModelKeepAliveperapplication.propertiesline 150. If either external service is temporarily unavailable, the container is marked unhealthy and may be restarted by the orchestrator, potentially causing cascading failures during transient outages.The
--start-period=120sgives a reasonable warmup window, and--retries=3at 30s intervals means ~90s of sustained failure before restart. Confirm this behavior aligns with your Coolify rolling-cutover expectations.The
exec javaentrypoint correctly makes Java PID 1 for proper signal handling — LGTM on that change.🤖 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 `@Dockerfile` around lines 97 - 101, Confirm whether Coolify should restart containers when external dependencies reported by /actuator/health/readiness, including qdrant and embeddingModelKeepAlive, are unavailable for roughly 90 seconds after the 120-second startup period. If cascading restarts during transient outages are unacceptable, update the Docker HEALTHCHECK to use an application-liveness endpoint or otherwise exclude dependency readiness while preserving the existing exec java entrypoint.src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java (1)
154-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent log level for "LLM providers unavailable" between streaming and completion paths.
streamResponselogs at WARN (line 157) whilecompletelogs at ERROR (line 242) for the same "no providers available" condition. The streaming path test explicitly defers error severity to the request boundary, but the completion path logs ERROR immediately — which may cause duplicate logging if the subscriber also handles the error. Consider aligningcompleteto WARN for consistency with the "logged once" principle.♻️ Suggested alignment for
completeString unavailableReason = "LLM providers unavailable - active provider is rate limited or misconfigured"; - log.error("[LLM] {}", unavailableReason);+ log.warn("[LLM] {}", unavailableReason); return Mono.error(new IllegalStateException(unavailableReason));🤖 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/service/OpenAIStreamingService.java` around lines 154 - 158, Update the no-provider handling in complete to log the same unavailable-provider condition at WARN rather than ERROR, matching streamResponse and preserving the existing error propagation behavior.src/main/java/com/williamcallahan/javachat/web/ErrorDocumentationController.java (1)
71-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog message no longer identifies which documentation file failed to read.
log.error("Failed to read error documentation page", exception)dropsdocumentationFilenameentirely. If reading one of several error pages fails in production, the log gives no way to tell which resource (not-found.html,validation-failed.html, etc.) was affected without inspecting the stack trace. The companion test only forbids a formatting argument (getArgumentArray()is null) — concatenating the filename directly into the message string keeps that contract while preserving diagnosability.🔍 Proposed fix to restore filename context without adding a format argument
} catch (IOException exception) { - log.error("Failed to read error documentation page", exception);+ log.error("Failed to read error documentation page: " + documentationFilename, exception); return ResponseEntity.internalServerError().build(); }🤖 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/ErrorDocumentationController.java` around lines 71 - 82, Update the IOException handler in serveHtmlFile to include documentationFilename directly in the log message while continuing to pass exception as the throwable and without adding formatting arguments. Preserve the existing error context and response behavior.src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
isApiRequest(requestUri)call.
handleErrorcomputesisApiRequestat line 93, thenlogRequestFailurerecomputes the same value at line 114 viaresolveFailureLogLevel(statusCode, isApiRequest(requestUri)). Pass the already-computed boolean through instead.♻️ Proposed fix to avoid recomputing isApiRequest
- boolean isApiRequest = isApiRequest(requestUri);- logRequestFailure(request, statusCode, requestUri, errorExceptionAttribute);+ boolean isApiRequest = isApiRequest(requestUri);+ logRequestFailure(request, statusCode, requestUri, errorExceptionAttribute, isApiRequest);- private void logRequestFailure(HttpServletRequest request, int statusCode, String requestUri, Object exception) {+ private void logRequestFailure(+ HttpServletRequest request, int statusCode, String requestUri, Object exception, boolean apiRequest) { String method = safeLogField(request.getMethod()); String canonicalUri = safeLogField(requestUri.split("[?#]", 2)[0]); String safeRequestId = safeLogField(request.getRequestId()); String source = safeLogField(request.getAttribute(RequestDispatcher.ERROR_SERVLET_NAME)); - LoggingEventBuilder requestFailureLog = log.atLevel(- resolveFailureLogLevel(statusCode, isApiRequest(requestUri)))+ LoggingEventBuilder requestFailureLog = log.atLevel(resolveFailureLogLevel(statusCode, apiRequest)) .setMessage("Request failed")Also applies to: 107-126
🤖 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 93 - 94, Update handleError and logRequestFailure to pass the already-computed isApiRequest boolean through the call chain, and change resolveFailureLogLevel invocation to use that parameter instead of calling isApiRequest(requestUri) again. Preserve the existing status-code and logging behavior.src/test/java/com/williamcallahan/javachat/web/ErrorDocumentationControllerTest.java (1)
44-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog-capture boilerplate is duplicated (and slightly inconsistent) across three test files.
This file,
OpenAIStreamingServiceTest.java, andCustomErrorControllerTest.javaeach hand-roll the sameLogger/ListAppender@BeforeEach/@AfterEachwiring. This file additionally saves/restoresisAdditive(), which the other two don't — a real behavioral divergence (whether the captured logger's events also flow to the root/console appender during tests). Worth extracting a small shared JUnit extension (e.g.,LogCaptureExtension) to keep this consistent and DRY.As per coding guidelines, "Avoid redundant code. Reuse code where appropriate and consistent with clean code principles (DRY principle)" and "New abstractions must earn reuse—extend existing code first; only add new type/helper when it removes real duplication (Earn Reuse)."
🤖 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/ErrorDocumentationControllerTest.java` around lines 44 - 62, Extract the duplicated logger/ListAppender lifecycle from captureControllerLogs and stopCapturingControllerLogs, OpenAIStreamingServiceTest, and CustomErrorControllerTest into a shared JUnit LogCaptureExtension. Ensure the extension consistently saves and restores logger additivity, attaches and detaches the appender, and starts/stops and clears it, then update all three tests to use the shared extension and remove their local wiring.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@Dockerfile`:
- Around line 97-101: Confirm whether Coolify should restart containers when
external dependencies reported by /actuator/health/readiness, including qdrant
and embeddingModelKeepAlive, are unavailable for roughly 90 seconds after the
120-second startup period. If cascading restarts during transient outages are
unacceptable, update the Docker HEALTHCHECK to use an application-liveness
endpoint or otherwise exclude dependency readiness while preserving the existing
exec java entrypoint.
In `@frontend/src/lib/services/markdown.ts`:
- Around line 350-354: In the markdown parsing flow, extract the duplicated {
async: false, gfm: true, breaks: false } options from both the unresolved and
resolved paths into one shared constant, then pass that constant to each
marked.parse call so the configurations remain synchronized.
In
`@src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java`:
- Around line 154-158: Update the no-provider handling in complete to log the
same unavailable-provider condition at WARN rather than ERROR, matching
streamResponse and preserving the existing error propagation behavior.
In `@src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java`:
- Around line 93-94: Update handleError and logRequestFailure to pass the
already-computed isApiRequest boolean through the call chain, and change
resolveFailureLogLevel invocation to use that parameter instead of calling
isApiRequest(requestUri) again. Preserve the existing status-code and logging
behavior.
In
`@src/main/java/com/williamcallahan/javachat/web/ErrorDocumentationController.java`:
- Around line 71-82: Update the IOException handler in serveHtmlFile to include
documentationFilename directly in the log message while continuing to pass
exception as the throwable and without adding formatting arguments. Preserve the
existing error context and response behavior.
In `@src/main/resources/application.properties`:
- Around line 147-150: Remove the unused management.info.* and info.* properties
from application.properties, including management.info.env.enabled, while
preserving the health, probe, and prometheus exposure settings.
In
`@src/test/java/com/williamcallahan/javachat/web/ErrorDocumentationControllerTest.java`:
- Around line 44-62: Extract the duplicated logger/ListAppender lifecycle from
captureControllerLogs and stopCapturingControllerLogs,
OpenAIStreamingServiceTest, and CustomErrorControllerTest into a shared JUnit
LogCaptureExtension. Ensure the extension consistently saves and restores logger
additivity, attaches and detaches the appender, and starts/stops and clears it,
then update all three tests to use the shared extension and remove their local
wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abf52f5a-a8c5-4921-9969-2e8e6e303895
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
Dockerfilebuild.gradle.ktsdocs/getting-started.mdfrontend/.nvmrcfrontend/package.jsonfrontend/scripts/verify-node-version.mjsfrontend/src/lib/services/markdown.test.tsfrontend/src/lib/services/markdown.tsgradle/libs.versions.tomlsrc/main/java/com/williamcallahan/javachat/config/RequiredCredentialValidation.javasrc/main/java/com/williamcallahan/javachat/config/SecurityConfig.javasrc/main/java/com/williamcallahan/javachat/logging/ProcessingLogger.javasrc/main/java/com/williamcallahan/javachat/service/ChatService.javasrc/main/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAlive.javasrc/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.javasrc/main/java/com/williamcallahan/javachat/web/CustomErrorController.javasrc/main/java/com/williamcallahan/javachat/web/ErrorDocumentationController.javasrc/main/java/com/williamcallahan/javachat/web/OpenGraphImageRenderer.javasrc/main/resources/application-dev.propertiessrc/main/resources/application.propertiessrc/main/resources/logback-spring.xmlsrc/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.javasrc/test/java/com/williamcallahan/javachat/config/RequiredCredentialValidationTest.javasrc/test/java/com/williamcallahan/javachat/service/EmbeddingModelKeepAliveTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAiCompatibleEmbeddingClientTest.javasrc/test/java/com/williamcallahan/javachat/service/ProviderCircuitStateTest.javasrc/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.javasrc/test/java/com/williamcallahan/javachat/web/ErrorDocumentationControllerTest.javasrc/test/java/com/williamcallahan/javachat/web/OpenGraphImageRendererTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/williamcallahan/javachat/logging/ProcessingLogger.java
An ambiguous two-brace suffix could consume a Java closing brace while assistant content was still streaming. Make enrichment closure state-aware and isolate assistant markdown rendering from message chrome. - use separate complete and streaming Marked instances - cover partial delimiters and literal-brace completion - extract assistant rendering below the component size limit
Tests should load their defaults only through the test profile without placing fake credentials in property resources. - activate the test profile for Gradle test tasks - supply the fake token only to the unit-test process - move test defaults into application-test.properties
The existing dependency ranges now resolve to newer compatible frontend test and build tooling. - record the current npm dependency graph - keep declared package ranges unchanged
The bundled display font lacked its license and no longer matched the intended current distribution. - update the canonical frontend font asset - include the SIL Open Font License - keep the generated Spring static projection byte-aligned
The client accepted missing event types and malformed payloads as display text, hiding server protocol defects and potentially exposing raw error content. - name the shared text chunk identically at producer and consumer - reject unsupported or invalid SSE events at the parser boundary - cover valid and malformed stream payloads end to end
Provider lifecycle output can include empty text deltas before a terminal upstream failure, but no user-visible text has been emitted at that point. - mark first output only for non-empty text chunks - retain the pre-text retry path after empty deltas - cover the terminal second-attempt context
Host and User-Agent values are client-controlled, high-cardinality fields that obscure request-failure alerts and trigger static-analysis warnings. - retain method, canonical URI, source, and request ID - omit host and User-Agent from structured request failures - lock the reduced field inventory in controller tests
The test profile is a task-wide execution invariant, so repeating it across every Spring test class created duplicate configuration ownership. - restore the single Gradle test-profile owner - remove repeated class-level profile declarations
Numeric heading detection duplicated ordered-list marker parsing and could drift from the canonical CommonMark boundary. - delegate numeric marker recognition to OrderedMarkerScanner - expose the delimiter position required for whitespace validation - cover three-digit headings and four-digit non-headings
The semantic-quality changes introduced named boundary rules that need behavior-level regression coverage. - reject non-rate-limit OpenAI statuses - reject incomplete GitHub repository paths - preserve four-digit text while stripping supported citation markers
Semantic naming rules must cover the authored Vite and Vitest configuration without scanning dependency trees. - include both frontend config entrypoints - retain the source-only node_modules exclusion
Generic response and payload names obscured which values belong to the CSRF retry protocol. - name request, response, parse, and validation values by boundary role - preserve the single ingress validation path and retry behavior
The canonical parser used generic names that obscured wire text, decoded events, and validated stream contracts. - use one StreamText name across schema and parser - name buffering and validation state by SSE role - cover malformed provider and JSON-shaped text events
CSS font-face rules are top-level at-rules and must not be nested inside the root selector. - move the Fraunces declaration before :root - preserve the variable font axes and fallback stack
The first rate-limit test retained a stale generic call-site name after the fixture was made intent-revealing. - call the domain-specific rateLimitService fixture consistently
The backend names each emitted text payload TextChunk, while the frontend had introduced a second name for the same governed shape. - use TextChunk for the canonical frontend schema and inferred type - update the sole SSE parser consumer without adding an alias
Flexmark parses bracketed citation numbers as reference nodes, so text-only cleanup left unresolved numeric markers in rendered output. - unlink unresolved numeric citation references at the AST boundary - preserve defined numeric reference links - traverse safely while unlinking nodes
Keep both the raw drain and Vector-derived typed events active for Java Chat until direct application telemetry reaches parity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java (1)
148-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider including the exception cause in structured error logs for debuggability.
Both error handlers use
log.atError()with structured key-value fields but don't attach the exception itself via.setCause(error). Without the cause, production logs will show the exception type but not the stack trace or message, making root-cause diagnosis harder.💡 Proposed improvement: add setCause to both log statements
// In streamLesson error handler (lines 149-156): log.atError() .setMessage("Guided lesson content stream error") .addKeyValue( "lessonSlug", StructuredLogValue.bounded(slug, MAX_GUIDED_LOG_FIELD_LENGTH) .text()) .addKeyValue("exceptionType", error.getClass().getSimpleName()) + .setCause(error) .log(); // In streamGuidedResponse error handler (lines 312-323): log.atError() .setMessage("Guided streaming error") .addKeyValue( "sessionId", StructuredLogValue.bounded(sessionId, MAX_GUIDED_LOG_FIELD_LENGTH) .text()) .addKeyValue( "lessonSlug", StructuredLogValue.bounded(lessonSlug, MAX_GUIDED_LOG_FIELD_LENGTH) .text()) .addKeyValue("exceptionType", error.getClass().getSimpleName()) + .setCause(error) .log();Also applies to: 309-328
🤖 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/GuidedLearningController.java` around lines 148 - 157, Update both GuidedLearningController error handlers, including the block using “Guided lesson content stream error” and the corresponding handler around the second referenced block, to attach the caught exception with setCause(error) before log(). Preserve the existing structured fields and filtering behavior.frontend/src/lib/components/MessageBubble.svelte (1)
174-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
.user-textrule blocks.Now that the wrap rule is scoped solely to
.user-text, it can be merged with the adjacent.user-texttypography block instead of existing as two separate rule sets for the same selector.🧹 Proposed consolidation
- /* Wrap long unbroken user strings such as URLs. */- .user-text {- overflow-wrap: break-word;- word-break: break-word;- }-- /* User text */- .user-text {- font-size: var(--text-base);- line-height: var(--leading-relaxed);- margin: 0;- }+ /* User text: wraps long unbroken strings such as URLs. */+ .user-text {+ overflow-wrap: break-word;+ word-break: break-word;+ font-size: var(--text-base);+ line-height: var(--leading-relaxed);+ margin: 0;+ }🤖 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 `@frontend/src/lib/components/MessageBubble.svelte` around lines 174 - 185, Merge the adjacent `.user-text` CSS blocks in MessageBubble.svelte into one rule, preserving all existing wrapping and typography declarations and their current behavior.src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java (1)
319-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the
ReportedStreamingFailureunwrap into a shared helper.This same
findInCauseChain(...).map(upstreamFailure).orElse(...)unwrap sequence is duplicated verbatim inChatController.onErrorResume. Since this behavior belongs to theReportedStreamingFailuretype itself, consider adding a static helper there (e.g.,ReportedStreamingFailure.unwrapUpstream(Throwable)) and having both call sites use it, keeping the invariant in one place.// In ReportedStreamingFailure.javastaticThrowableunwrapUpstream(Throwablefailure) { returnfindInCauseChain(failure).map(ReportedStreamingFailure::upstreamFailure).orElse(failure); }🤖 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/service/OpenAIStreamingService.java` around lines 319 - 324, Move the duplicated cause-chain unwrapping logic into a shared static helper on ReportedStreamingFailure, such as unwrapUpstream(Throwable), preserving the original fallback behavior. Update OpenAIStreamingService.isRecoverableStreamingFailure and ChatController.onErrorResume to call this helper instead of repeating findInCauseChain(...).map(...).orElse(...).
🤖 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 @.github/workflows/build.yml:
- Around line 14-19: Add an explicit least-privilege permissions block to the
frontend job, alongside runs-on and defaults, granting only the permissions
required by its workflow steps and setting all other token scopes to none.
- Around line 21-22: Update the actions/checkout step in the build workflow to
set persist-credentials to false, ensuring checkout credentials are not retained
during the job.
In `@frontend/src/lib/services/javaLanguageDetection.ts`:
- Around line 32-36: Update the keyword detection in the codeBlocks iteration to
match JAVA_KEYWORDS as whole words rather than substrings, replacing the current
codeText.includes check with word-boundary-aware matching. Preserve the existing
behavior of assigning JAVA_LANGUAGE_CLASS when any keyword matches.
In `@frontend/src/styles/global.css`:
- Around line 7-19: Update the font-family declaration in the `@font-face` rule by
removing the quotes around the Fraunces name, preserving the existing font
source and variation settings.
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 309-332: Update the stream error response in the guided streaming
handler to use a fixed user-friendly message, matching the existing “Lesson
content stream failed” behavior in streamLesson. Remove the upstreamFailure
class name from the userFacingMessage passed to sseSupport.streamErrorEvent;
keep retryable handling unchanged, and only include exception details through
diagnosticDetails if that parameter is already supported.
---
Nitpick comments:
In `@frontend/src/lib/components/MessageBubble.svelte`:
- Around line 174-185: Merge the adjacent `.user-text` CSS blocks in
MessageBubble.svelte into one rule, preserving all existing wrapping and
typography declarations and their current behavior.
In
`@src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java`:
- Around line 319-324: Move the duplicated cause-chain unwrapping logic into a
shared static helper on ReportedStreamingFailure, such as
unwrapUpstream(Throwable), preserving the original fallback behavior. Update
OpenAIStreamingService.isRecoverableStreamingFailure and
ChatController.onErrorResume to call this helper instead of repeating
findInCauseChain(...).map(...).orElse(...).
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 148-157: Update both GuidedLearningController error handlers,
including the block using “Guided lesson content stream error” and the
corresponding handler around the second referenced block, to attach the caught
exception with setCause(error) before log(). Preserve the existing structured
fields and filtering behavior.
🪄 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: 850cfda7-1858-40bd-abaa-563e8c251b84
⛔ Files ignored due to path filters (3)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/fonts/Fraunces-Variable.ttfis excluded by!**/*.ttfsrc/main/resources/static/fonts/Fraunces-Variable.ttfis excluded by!**/*.ttf
📒 Files selected for processing (53)
.dockerignore.github/workflows/build.ymlDockerfilebuild.gradle.ktsconfig/make/common.mkdocs/configuration.mdfrontend/package.jsonfrontend/public/fonts/Fraunces-OFL.txtfrontend/src/lib/components/AssistantMarkdownBody.sveltefrontend/src/lib/components/LearnView.sveltefrontend/src/lib/components/MessageBubble.sveltefrontend/src/lib/services/csrf.tsfrontend/src/lib/services/javaLanguageDetection.tsfrontend/src/lib/services/markdown.test.tsfrontend/src/lib/services/markdown.tsfrontend/src/lib/services/sse.test.tsfrontend/src/lib/services/sse.tsfrontend/src/lib/validation/schemas.tsfrontend/src/styles/global.cssinfra/docker-compose-qdrant.ymlsrc/main/java/com/williamcallahan/javachat/adapters/out/llm/openai/OpenAiStreamingFailureException.javasrc/main/java/com/williamcallahan/javachat/adapters/out/llm/openai/OpenAiStreamingFailureReporter.javasrc/main/java/com/williamcallahan/javachat/application/completion/CompletionRequestConfiguration.javasrc/main/java/com/williamcallahan/javachat/application/streaming/ReportedStreamingFailure.javasrc/main/java/com/williamcallahan/javachat/application/streaming/StreamingFailureReporter.javasrc/main/java/com/williamcallahan/javachat/config/QdrantIndexInitializer.javasrc/main/java/com/williamcallahan/javachat/service/HtmlContentExtractor.javasrc/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.javasrc/main/java/com/williamcallahan/javachat/service/RateLimitService.javasrc/main/java/com/williamcallahan/javachat/service/ingestion/GitHubRepositoryIdentityResolver.javasrc/main/java/com/williamcallahan/javachat/service/markdown/MarkdownAstUtils.javasrc/main/java/com/williamcallahan/javachat/service/markdown/MarkdownNormalizer.javasrc/main/java/com/williamcallahan/javachat/service/markdown/OrderedMarkerScanner.javasrc/main/java/com/williamcallahan/javachat/support/StructuredLogValue.javasrc/main/java/com/williamcallahan/javachat/web/ChatController.javasrc/main/java/com/williamcallahan/javachat/web/CustomErrorController.javasrc/main/java/com/williamcallahan/javachat/web/GuidedLearningController.javasrc/main/java/com/williamcallahan/javachat/web/MarkdownController.javasrc/main/java/com/williamcallahan/javachat/web/SseSupport.javasrc/main/resources/static/fonts/Fraunces-OFL.txtsrc/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.javasrc/test/java/com/williamcallahan/javachat/adapters/out/llm/openai/OpenAiStreamingFailureExceptionTest.javasrc/test/java/com/williamcallahan/javachat/application/completion/CompletionRequestConfigurationTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/RateLimitServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/ingestion/GitHubRepositoryIdentityResolverTest.javasrc/test/java/com/williamcallahan/javachat/service/markdown/MarkdownAstUtilsTest.javasrc/test/java/com/williamcallahan/javachat/service/markdown/MarkdownNormalizerTest.javasrc/test/java/com/williamcallahan/javachat/support/StructuredLogValueTest.javasrc/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.javasrc/test/java/com/williamcallahan/javachat/web/CustomErrorControllerTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerStreamingFailureTest.javasrc/test/resources/application-test.properties
💤 Files with no reviewable changes (3)
- infra/docker-compose-qdrant.yml
- src/test/java/com/williamcallahan/javachat/JavaChatApplicationTests.java
- src/test/resources/application-test.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- src/main/java/com/williamcallahan/javachat/web/CustomErrorController.java
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 13, 2026
This was referenced Jul 31, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Coolify now keeps the serving container active until its replacement passes dependency-aware readiness, preventing deployment windows from surfacing gateway failures. This release also preserves partial streamed enrichment prose, makes credential failures startup-blocking, removes duplicate provider-failure noise, narrows operational telemetry, and locks frontend builds to Node 24.15.0.
Changes
Bug Fixes
Dockerfile;src/main/resources/application.properties).frontend/src/lib/services/markdown.ts;frontend/src/lib/services/markdown.test.ts).EmbeddingModelKeepAlive.probeEmbeddingModel;EmbeddingModelKeepAliveTest).OpenAIStreamingService;ChatService;OpenAIStreamingServiceTest).RequiredCredentialValidation;RequiredCredentialValidationTest).CustomErrorController;ErrorDocumentationController; associated controller tests).Security and Observability
SecurityConfig.managementSecurityFilterChain;application.properties)./actuator/prometheusfor existing monitoring infrastructure (build.gradle.kts;gradle/libs.versions.toml).status,source,method,uri, andrequestIdfields; duplicate AOP pipeline events are removed; verbose Spring AI diagnostics remain development-only (CustomErrorController.logRequestFailure;logback-spring.xml; deletedProcessingLogger;application-dev.properties).Tooling
package.jsonowns Node 24.15.0, and validation fails when.nvmrc, the lockfile, Docker build image, or setup guide drifts from that version (frontend/package.json;frontend/scripts/verify-node-version.mjs;frontend/.nvmrc;Dockerfile;docs/getting-started.md).Validation
Breaking Changes
/actuator/infoand/actuator/metricsare no longer public; monitoring must use/actuator/prometheusand the health probe endpoints.Related Issues
None.