fix(chat): make streaming, citations, and ingestion deterministic#77
Merged
Conversation
Make the resource manifest the sole owner of complete Java API mirror records. Project it into Java and Bash consumers, reject malformed or incomplete catalogs, and document the ownership rules for future agents.
Deduplicate candidates by the strongest available identity so reranking keeps distinct chunks from one page. Collapse final citations by canonical source URL.
Persist completed exchanges atomically and emit provider fallback state from one canonical notice stream. Remove client-side POST replay so failed stateful requests cannot duplicate chat or lesson work.
Use Java-compatible identifier continuation categories when detecting language keywords. Cover characters accepted by Unicode identifier rules but rejected by Java, plus Java-specific dollar and control characters.
Give lesson navigation and title presentation a focused component while preserving the canonical learning state in LearnView. Cover the back-to-lessons interaction through the rendered view.
Record the user and assistant turns only after chat or guided streaming completes. This keeps the controller call sites aligned with the atomic memory API and leaves failed streams out of session history.
Oracle Javadoc mirrors can legitimately land below their expected HTML minimum on an interrupted or throttled fetch, yet the fixed threshold treated any short mirror as a hard failure and quarantined the partial result, discarding work that incremental reruns could resume. Thread the manifest's seventh projection field (ALLOW_PARTIAL) through fetch_docs and its Oracle/generic helpers so sources flagged partial keep a validated short mirror with a warning instead of failing, and skip the pre-fetch quarantine that would erase it. Extract fetch_documentation_source to project one seven-field row into the fetch boundary and guard the script so it can be sourced by tests without executing. - Add partial_mirror_allowed parameter to validate_fetch_result, fetch_oracle_javadoc_seed, fetch_docs_mirror, and fetch_docs - Add fetch_documentation_source to parse and dispatch a seven-field row - Add scripts/test_java_api_fetch_projection.sh asserting manifest rows retain seven distinct fetch fields in order
The projection parity test proved the seventh field survived manifest projection but never exercised what the flag does, leaving the partial-mirror branch of validate_fetch_result and the pre-fetch quarantine guard in fetch_docs without behavioral coverage. Drive both policies directly: assert allowPartial=true keeps a validated below-minimum mirror while allowPartial=false rejects it, and that fetch_docs quarantines an incomplete mirror only when partial mirrors are disallowed. Use a mktemp docs root with an EXIT trap so the test leaves no artifacts behind. - Add validate_fetch_result true/false assertions for below-minimum counts - Add fetch_docs quarantine assertions gated on the partial-mirror policy - Replace the hardcoded docs root with a self-cleaning mktemp directory
WilliamAGH
added
bug
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change centralizes Java API documentation sources, revises streaming retries and persistence, adds a reusable lesson header, improves SSE and citation handling, updates CSRF and timeout behavior, and expands Java keyword detection coverage. ChangesJava API source catalog
Streaming and conversation flow
Frontend lesson and language behavior
Retrieval, citation, security, and configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatController
participant OpenAIStreamingService
participant SseSupport
participant ChatMemoryService
Client->>ChatController: Start streaming request
ChatController->>OpenAIStreamingService: streamResponse(...)
OpenAIStreamingService-->>SseSupport: Streaming notices and text chunks
SseSupport-->>Client: Ordered provider/status SSE events
OpenAIStreamingService-->>ChatController: Stream completion
ChatController->>ChatMemoryService: addExchange(sessionId, userQuery, fullResponse)
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Replace the session-coupled CSRF repository with Spring Security's cookie repository so valid cookie/header tokens survive stateless requests. Keep the browser-visible token lifetime and consistent JSON denial response on the verified framework boundary. - Configure SameSite and max-age through the cookie customizer - Remove server-session token metadata and expiry branching - Cover stateless success and missing-token failure through MockMvc
coderabbitai
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR tightens end-to-end streaming correctness (single execution per stateful request, accurate provider fallback signaling, and atomic conversation persistence), improves retrieval behavior for same-page evidence chunks, and centralizes complete Java API documentation sources into a canonical manifest consumed by Java + shell + CLI.
Changes:
- Emit fallback-provider SSE events from a single notice stream (provider event precedes fallback status/text) and record chat history only after successful stream completion.
- Preserve distinct same-page retrieval chunks for reranking while deduplicating citation pills by canonical (fragmentless) source URL.
- Add a canonical
java-api-documentation-sources.manifestand project it consistently into citation normalization, CLI doc sets, and doc-fetch scripts (plus parity/validation tests).
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/williamcallahan/javachat/web/SseSupport.java | Emits ordered provider/status SSE sequences from runtime streaming notices. |
| src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java | Avoids pre-stream history mutation; commits completed exchanges only. |
| src/main/java/com/williamcallahan/javachat/web/ChatController.java | Avoids pre-stream history mutation; commits completed exchanges only. |
| src/main/java/com/williamcallahan/javachat/service/StreamingResult.java | Simplifies streaming result contract to provider + text + notices. |
| src/main/java/com/williamcallahan/javachat/service/StreamingAttemptContext.java | Removes provider-change sink plumbing from attempt context. |
| src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java | Publishes replayable fallback notices without separate provider-change stream. |
| src/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.java | Refines retryability rules for streaming failures (auth/rate-limit/not-found no longer retryable). |
| src/main/java/com/williamcallahan/javachat/service/ChatMemoryService.java | Adds atomic addExchange to keep user/assistant turns aligned. |
| src/main/java/com/williamcallahan/javachat/service/RetrievalService.java | Dedupes candidates by content hash then canonical URL; dedupes citations by fragmentless URL. |
| src/main/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifest.java | Loads and validates the canonical Java API source manifest. |
| src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java | Projects Java API sources from the manifest; keeps non-Java sources in properties. |
| src/main/java/com/williamcallahan/javachat/cli/DocumentationSetCatalog.java | Builds CLI base doc sets by projecting manifest-driven Java API sources. |
| src/main/resources/java-api-documentation-sources.manifest | Canonical inventory of complete Java API mirrors (21/24/25). |
| src/main/resources/docs-sources.properties | Removes Java API base entries; keeps only non-complete-mirror sources. |
| scripts/lib/documentation_sources.sh | Parses/validates manifest for shell and provides projection helpers. |
| scripts/fetch_all_docs.sh | Adds manifest-driven Java API listing and fetch integration; improves failure reporting. |
| frontend/src/lib/services/streamRecovery.ts | Removes client-side automatic stream retry/replay logic. |
| frontend/src/lib/services/streamRecovery.test.ts | Removes tests for the deleted retry/replay behavior. |
| frontend/src/lib/services/chat.ts | Uses single-pass streamSse (no POST replay) for chat streaming. |
| frontend/src/lib/services/chat.test.ts | Updates expectations to “no replay” behavior on stream failures. |
| frontend/src/lib/services/guided.ts | Uses single-pass streamSse (no POST replay) for guided streaming. |
| frontend/src/lib/services/guided.test.ts | Updates expectations to “no replay” behavior on guided stream failures. |
| frontend/src/lib/composables/createStreamingState.svelte.ts | Includes active provider in visible stream status details. |
| frontend/src/lib/services/javaLanguageDetection.ts | Updates Java identifier-part detection to better match Java keyword boundaries. |
| frontend/src/lib/services/markdown.test.ts | Extends keyword-boundary tests (Unicode + $ + control chars). |
| frontend/src/lib/components/LearnView.svelte | Extracts lesson header UI into a focused component. |
| frontend/src/lib/components/GuidedLessonHeader.svelte | New component for lesson header navigation/title with accessibility label. |
| frontend/src/lib/components/LearnView.test.ts | Adds coverage for returning to lesson list via the header button. |
| frontend/src/lib/components/ChatView.test.ts | Adds coverage for displaying the active fallback provider in status UI. |
| src/test/java/com/williamcallahan/javachat/web/StreamingProviderFallbackSseEventTest.java | Verifies fallback provider/status ordering and single subscription to replayed notice stream. |
| src/test/java/com/williamcallahan/javachat/web/GuidedSseCitationEventTest.java | Updates test construction for new StreamingResult signature. |
| src/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerStreamingFailureTest.java | Updates test construction for new StreamingResult signature. |
| src/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.java | Updates test construction for new StreamingResult signature. |
| src/test/java/com/williamcallahan/javachat/web/ChatControllerSessionValidationTest.java | Updates session seeding to use addExchange. |
| src/test/java/com/williamcallahan/javachat/service/ChatMemoryServiceTest.java | Refactors tests to validate exchange-based persistence semantics. |
| src/test/java/com/williamcallahan/javachat/service/StreamingAttemptContextTest.java | Updates tests for removed provider-change sink argument. |
| src/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.java | Expands recoverability tests and updates provider naming to enum names. |
| src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java | Adds regression test for same-page chunk preservation and citation deduping. |
| src/test/java/com/williamcallahan/javachat/util/JavadocTypeCanonicalizerTest.java | Uses manifest-driven Java API base URL instead of hardcoded release URL. |
| src/test/java/com/williamcallahan/javachat/StandaloneExtractionTest.java | Projects Java API extraction sources from the manifest instead of hardcoding releases. |
| src/test/java/com/williamcallahan/javachat/ExtractorQualityTest.java | Projects Java API extraction sources from the manifest instead of hardcoding releases. |
| src/test/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifestTest.java | Cross-validates manifest grammar enforcement in Java and bash. |
| src/test/java/com/williamcallahan/javachat/config/DocsSourceRegistryTest.java | Ensures local-mirror → remote-base mapping is complete and unique per release/path. |
| src/test/java/com/williamcallahan/javachat/cli/JavaApiDocumentationSourceParityTest.java | Enforces parity across manifest, CLI catalog projection, and fetch script listing. |
| docs/pipeline-commands.md | Documents canonical Java API manifest ownership and listing command usage. |
| docs/ingestion.md | Updates ingestion guidance to use canonical manifest listing for doc-set paths. |
| docs/domains/all-parsing-and-markdown-logic.md | Updates referenced persistence method name to addExchange. |
| docs/contracts/code-change.md | Adds explicit governed-catalog “single semantic owner” guidance. |
| AGENTS.md | Adds explicit Java API catalog ownership/projection rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a97a5e398a
ℹ️ 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".
The projection test exercised the generic mirror path but left the Oracle Javadoc branch unverified, so a regression that dropped the partial-mirror policy while dispatching Oracle seeds or validating their fetch result would have gone unnoticed. Assert that fetch_docs forwards the partial-mirror policy to fetch_oracle_javadoc_seed, and that fetch_oracle_javadoc_seed in turn hands it to validate_fetch_result, by stubbing each boundary and capturing the propagated flag. - Capture the policy forwarded into fetch_oracle_javadoc_seed via fetch_docs - Capture the policy fetch_oracle_javadoc_seed forwards to validate_fetch_result
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.java (1)
234-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract a
createStreamingService(RateLimitService)overload to cut setup duplication.The
new OpenAiProviderRoutingService(rateLimitService, 600, ApiProvider.GITHUB_MODELS.getName())+new OpenAIStreamingService(...)pairing repeats verbatim across four tests, only because each needs a custom pre-stubbedRateLimitServicemock. An overload taking the mock as a parameter would remove the copy-paste without losing flexibility.♻️ Suggested helper
+ private OpenAIStreamingService createStreamingService(RateLimitService rateLimitService) { + OpenAiRequestFactory requestFactory = + new OpenAiRequestFactory(new Chunker(), new PromptTruncator(), "gpt-5.2", "gpt-5", ""); + OpenAiProviderRoutingService providerRoutingService = new OpenAiProviderRoutingService( + rateLimitService, 600, RateLimitService.ApiProvider.GITHUB_MODELS.getName()); + return new OpenAIStreamingService( + rateLimitService, requestFactory, providerRoutingService, new OpenAiStreamingFailureReporter()); + }Also applies to: 284-285, 331-332, 389-390
🤖 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/service/OpenAIStreamingServiceTest.java` around lines 234 - 235, Optionally add a createStreamingService(RateLimitService) test helper overload in OpenAIStreamingServiceTest that constructs OpenAiProviderRoutingService with the supplied mock, the existing 600 limit, and GITHUB_MODELS, then creates the streaming service. Replace the repeated provider-routing and OpenAIStreamingService setup at the referenced test locations with this helper while preserving each test’s custom mock configuration.src/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.java (1)
176-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid recoverability tree, but its message-matching tail duplicates
isStreamingFallbackEligible.The classification logic itself (auth/rate-limit/not-found → non-recoverable, IO/SSE/overflow → recoverable, status-code bucket, message-substring fallback) is well justified and matches the new test coverage (
recoverableStreamingFailureTreatsNotFoundAsNonRetryable,recoverableStreamingFailureRejectsAuthenticationAuthorizationAndRateLimitFailures). However, the final 7-line message-substring block (Lines 201-208) is byte-for-byte identical to the tail ofisStreamingFallbackEligible(Lines 161-167). Two independently-maintained copies of the same substring list will drift the next time one is updated but not the other.♻️ Extract the shared substring check into one helper
- String exceptionMessage = throwable.getMessage(); - if (exceptionMessage == null) { - return false; - } - String normalizedMessage = AsciiTextNormalizer.toLowerAscii(exceptionMessage); - return normalizedMessage.contains("invalid stream") - || normalizedMessage.contains("malformed") - || normalizedMessage.contains("unexpected end of json input") - || normalizedMessage.contains("timeout") - || normalizedMessage.contains("temporarily unavailable") - || normalizedMessage.contains("connection reset") - || normalizedMessage.contains("connection closed"); + return matchesTransientStreamingMessage(throwable.getMessage()); } + private boolean matchesTransientStreamingMessage(String exceptionMessage) { + if (exceptionMessage == null) { + return false; + } + String normalizedMessage = AsciiTextNormalizer.toLowerAscii(exceptionMessage); + return normalizedMessage.contains("invalid stream") + || normalizedMessage.contains("malformed") + || normalizedMessage.contains("unexpected end of json input") + || normalizedMessage.contains("timeout") + || normalizedMessage.contains("temporarily unavailable") + || normalizedMessage.contains("connection reset") + || normalizedMessage.contains("connection closed"); + }Apply the same replacement to the tail of
isStreamingFallbackEligible(Lines 161-167).As per coding guidelines,
**/*.{java,kt}requires applying "Clean Code, DDD, DRY, YAGNI, KISS, and Clean Architecture."🤖 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/OpenAiProviderRoutingService.java` around lines 176 - 209, Extract the duplicated normalized-message substring matching from isStreamingFallbackEligible and isRecoverableStreamingFailure into one private helper, reusing the existing AsciiTextNormalizer logic and recognized phrases. Replace both method tails with calls to that helper while preserving their current classification behavior.Source: Coding guidelines
frontend/src/lib/services/guided.test.ts (1)
94-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing assertion for the newly added
onErrorcall.Line 102 adds
callbacks.onError?.({ message: "Provider stream unavailable" })to the mock, but the test only asserts ononStatus(lines 120-126). The sibling test inchat.test.ts(forwarding non-retryable status) does assertonError's payload — this test should do the same so a regression inonErrorforwarding for the guided-chat path is caught.✅ Suggested assertion addition
expect(streamSseMock).toHaveBeenCalledTimes(1); expect(onStatus).toHaveBeenCalledWith( expect.objectContaining({ code: "stream.provider.fatal-error", retryable: false, }), ); + expect(onError).toHaveBeenCalledWith({ message: "Provider stream unavailable" }); });🤖 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/guided.test.ts` around lines 94 - 127, Add an assertion in the guided-chat test for the onError mock, verifying it receives the provider stream failure message after streamGuidedChat rejects. Keep the existing retry-count and onStatus assertions unchanged.scripts/fetch_all_docs.sh (1)
370-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--list-java-api-sourcesoutput format doesn't match the docs' "copy relativeMirrorPath" instruction. The script prints full raw manifest rows; the docs assume users can lift a bare path from that output.
scripts/fetch_all_docs.sh#L370-L377: change the listing loop to print justrelativeMirrorPathper line (see diff in the per-file comment above), or otherwise make the printed format match whatDOCS_SETSexpects.docs/pipeline-commands.md#L123-L134: once the script prints bare paths, this instruction becomes accurate as-is; until then, add acut -d'|' -f3step to show users how to extract the path from a raw row.🤖 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 `@scripts/fetch_all_docs.sh` around lines 370 - 377, Update the --list-java-api-sources listing loop in scripts/fetch_all_docs.sh to print only each source’s relativeMirrorPath, matching the paths expected by DOCS_SETS. No direct change is needed in docs/pipeline-commands.md lines 123-134 because the documentation becomes accurate once the script emits bare paths.
🤖 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 `@scripts/lib/documentation_sources.sh`:
- Around line 162-177: Update append_java_api_fetch_sources to emit DOC_SOURCES
entries with exactly six fields, matching fetch_all_docs.sh’s expected url,
target directory, name, cut directories, minimum files, and reject regex
structure. Remove the allowPartial value from the DOC_SOURCES append while
leaving the projection parsing unchanged.
In `@src/main/java/com/williamcallahan/javachat/service/RetrievalService.java`:
- Around line 187-223: Update deduplicateByContentHashThenHashlessCanonicalUrl
so hashless file:// document URLs always use
DocsSourceRegistry.normalizeDocUrl(documentUrl) as the deduplication key,
removing the separate resolveLocalPath/canonicalizeHttpDocUrl fallback path.
Preserve the existing handling for non-file URLs, blank URLs, and content-hashed
documents.
---
Nitpick comments:
In `@frontend/src/lib/services/guided.test.ts`:
- Around line 94-127: Add an assertion in the guided-chat test for the onError
mock, verifying it receives the provider stream failure message after
streamGuidedChat rejects. Keep the existing retry-count and onStatus assertions
unchanged.
In `@scripts/fetch_all_docs.sh`:
- Around line 370-377: Update the --list-java-api-sources listing loop in
scripts/fetch_all_docs.sh to print only each source’s relativeMirrorPath,
matching the paths expected by DOCS_SETS. No direct change is needed in
docs/pipeline-commands.md lines 123-134 because the documentation becomes
accurate once the script emits bare paths.
In
`@src/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.java`:
- Around line 176-209: Extract the duplicated normalized-message substring
matching from isStreamingFallbackEligible and isRecoverableStreamingFailure into
one private helper, reusing the existing AsciiTextNormalizer logic and
recognized phrases. Replace both method tails with calls to that helper while
preserving their current classification behavior.
In
`@src/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.java`:
- Around line 234-235: Optionally add a createStreamingService(RateLimitService)
test helper overload in OpenAIStreamingServiceTest that constructs
OpenAiProviderRoutingService with the supplied mock, the existing 600 limit, and
GITHUB_MODELS, then creates the streaming service. Replace the repeated
provider-routing and OpenAIStreamingService setup at the referenced test
locations with this helper while preserving each test’s custom mock
configuration.
🪄 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: 794201be-3214-4a74-8b70-836544d73486
📒 Files selected for processing (49)
AGENTS.mddocs/contracts/code-change.mddocs/domains/all-parsing-and-markdown-logic.mddocs/ingestion.mddocs/pipeline-commands.mdfrontend/src/lib/components/ChatView.test.tsfrontend/src/lib/components/GuidedLessonHeader.sveltefrontend/src/lib/components/LearnView.sveltefrontend/src/lib/components/LearnView.test.tsfrontend/src/lib/composables/createStreamingState.svelte.tsfrontend/src/lib/services/chat.test.tsfrontend/src/lib/services/chat.tsfrontend/src/lib/services/guided.test.tsfrontend/src/lib/services/guided.tsfrontend/src/lib/services/javaLanguageDetection.tsfrontend/src/lib/services/markdown.test.tsfrontend/src/lib/services/streamRecovery.test.tsfrontend/src/lib/services/streamRecovery.tsscripts/fetch_all_docs.shscripts/lib/documentation_sources.shsrc/main/java/com/williamcallahan/javachat/cli/DocumentationSetCatalog.javasrc/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.javasrc/main/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifest.javasrc/main/java/com/williamcallahan/javachat/service/ChatMemoryService.javasrc/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.javasrc/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.javasrc/main/java/com/williamcallahan/javachat/service/RetrievalService.javasrc/main/java/com/williamcallahan/javachat/service/StreamingAttemptContext.javasrc/main/java/com/williamcallahan/javachat/service/StreamingResult.javasrc/main/java/com/williamcallahan/javachat/web/ChatController.javasrc/main/java/com/williamcallahan/javachat/web/GuidedLearningController.javasrc/main/java/com/williamcallahan/javachat/web/SseSupport.javasrc/main/resources/docs-sources.propertiessrc/main/resources/java-api-documentation-sources.manifestsrc/test/java/com/williamcallahan/javachat/ExtractorQualityTest.javasrc/test/java/com/williamcallahan/javachat/StandaloneExtractionTest.javasrc/test/java/com/williamcallahan/javachat/cli/JavaApiDocumentationSourceParityTest.javasrc/test/java/com/williamcallahan/javachat/config/DocsSourceRegistryTest.javasrc/test/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifestTest.javasrc/test/java/com/williamcallahan/javachat/service/ChatMemoryServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/StreamingAttemptContextTest.javasrc/test/java/com/williamcallahan/javachat/util/JavadocTypeCanonicalizerTest.javasrc/test/java/com/williamcallahan/javachat/web/ChatControllerSessionValidationTest.javasrc/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerStreamingFailureTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedSseCitationEventTest.javasrc/test/java/com/williamcallahan/javachat/web/StreamingProviderFallbackSseEventTest.java
💤 Files with no reviewable changes (2)
- frontend/src/lib/services/streamRecovery.ts
- frontend/src/lib/services/streamRecovery.test.ts
Preserve nonempty partial mirrors for incremental retries without reporting them as completed fetches. Track partial sources separately and fail the fetch command before Qdrant ingestion until every configured minimum is satisfied. Extract metadata emission from the fetch orchestrator so the touched script retains one orchestration responsibility, and document the resulting pipeline contract.
Remove the redundant SDK read deadline so provider gateways remain the sole owner of first-output and inter-output timing while the whole request keeps its configured bound.
Use the framework's stateless session-cookie lifetime so logout can emit an immediate deletion cookie, and cover mismatched and single-sided token rejection.
Preserve document-to-citation alignment through PDF page estimation, then collapse only citations that resolve to the same final page anchor.
Invoke the canonical error callback exactly once for non-abort fetch and reader failures while preserving rejection identity and silent cancellation.
Keep every selectable Java documentation set manifest-backed and reject legacy quick tokens that no longer have fetch or citation mappings.
Prove that unresolved file URLs retain their raw identity because normalizing them to the shared redacted sentinel would collapse unrelated documents.
Move routing and failure-classification tests into their focused owner so the streaming service suite remains below the repository monolith threshold.
WilliamAGH
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java (1)
45-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice coverage of the happy path — consider also covering the partial-failure path.
This test nails the core regression scenario (distinct-page chunks preserved, same-page chunks collapsed). One gap: nothing exercises
buildPageAnchoredCitationswhen one document's conversion throws insideretrievalService.toCitations— worth a companion test asserting that a failing document is skipped from pairing (no size-mismatch toenhanceWithPageAnchors) while the remaining citations still get anchored correctly.🤖 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/service/GuidedLearningServiceCitationTest.java` around lines 45 - 75, Add a companion test for GuidedLearningService.citationsForBookDocuments that makes retrievalService.toCitations throw for one document while returning citations for another. Assert the failing document is skipped, enhanceWithPageAnchors receives only the remaining citations without a size mismatch, and the successful citation retains its expected page URL and anchor.src/test/java/com/williamcallahan/javachat/config/SecurityConfigTest.java (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference
CSRF_INVALID_MESSAGEfrom its canonical owner instead of duplicating.The message string is duplicated verbatim from
CsrfAccessDeniedHandler.CSRF_INVALID_MESSAGE(private). If the handler's message changes, this test constant will silently drift. Per coding guidelines, tests must bind governed concepts from their canonical owner without duplicated inventories. Consider making the handler's constantpublicso the test can import it directly.♻️ Proposed refactor
In
CsrfAccessDeniedHandler.java:- private static final String CSRF_INVALID_MESSAGE = + public static final String CSRF_INVALID_MESSAGE = "CSRF token missing or invalid. Refresh the page and retry the request.";In
SecurityConfigTest.java:+import com.williamcallahan.javachat.adapters.in.web.security.CsrfAccessDeniedHandler; + private static final String CSRF_COOKIE_SAME_SITE_POLICY = "Lax"; - private static final String CSRF_INVALID_MESSAGE = - "CSRF token missing or invalid. Refresh the page and retry the request."; + private static final String CSRF_INVALID_MESSAGE = CsrfAccessDeniedHandler.CSRF_INVALID_MESSAGE;🤖 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/config/SecurityConfigTest.java` around lines 38 - 39, Remove the duplicated CSRF_INVALID_MESSAGE from SecurityConfigTest and expose the existing constant from CsrfAccessDeniedHandler with appropriate public visibility. Update the test to reference CsrfAccessDeniedHandler.CSRF_INVALID_MESSAGE directly, preserving the canonical message value and avoiding a second inventory.Source: Coding guidelines
scripts/lib/documentation_fetch_metadata.sh (1)
20-67: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReuse
count_html_filesinstead of duplicating the find/wc/tr pattern six times.Lines 62-67 hand-roll
find <dir> -name "*.html" 2>/dev/null | wc -l | tr -d ' 'for each Spring source, duplicating logic that already exists ascount_html_files(used throughoutfetch_all_docs.sh, e.g. invalidate_fetch_result). Reusing it keeps the counting logic in one place and guarantees these reported numbers stay consistent with whatever counting rulescount_html_filesapplies (including any future exclusions).Separately, the aggregate
total_html_files/total_files(Lines 22, 24) scan the full$DOCS_ROOTtree, which also sweeps up anything currently sitting in$DOCS_ROOT/.quarantine(populated byquarantine_incomplete_dir/quarantine_path). Since quarantine snapshots accumulate across runs, these totals can silently drift away from the actual "live" documentation corpus size over time.♻️ Example fix for the duplicated counts
- "spring_boot_complete": "$(find "$DOCS_ROOT/spring-boot-complete" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')", - "spring_framework_complete": "$(find "$DOCS_ROOT/spring-framework-complete" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')", - "spring_ai_reference_stable": "$(find "$DOCS_ROOT/spring-ai-reference" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')", - "spring_ai_reference_2": "$(find "$DOCS_ROOT/spring-ai-reference-2" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')", - "spring_ai_api_stable": "$(find "$DOCS_ROOT/spring-ai-api-stable" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')", - "spring_ai_api_2": "$(find "$DOCS_ROOT/spring-ai-api-2" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')" + "spring_boot_complete": "$(count_html_files "$DOCS_ROOT/spring-boot-complete")", + "spring_framework_complete": "$(count_html_files "$DOCS_ROOT/spring-framework-complete")", + "spring_ai_reference_stable": "$(count_html_files "$DOCS_ROOT/spring-ai-reference")", + "spring_ai_reference_2": "$(count_html_files "$DOCS_ROOT/spring-ai-reference-2")", + "spring_ai_api_stable": "$(count_html_files "$DOCS_ROOT/spring-ai-api-stable")", + "spring_ai_api_2": "$(count_html_files "$DOCS_ROOT/spring-ai-api-2")"🤖 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 `@scripts/lib/documentation_fetch_metadata.sh` around lines 20 - 67, Update write_documentation_fetch_metadata to use count_html_files for each Spring directory count instead of repeating find/wc/tr pipelines. Also adjust total_html_files and total_files calculations so DOCS_ROOT/.quarantine and its contents are excluded, keeping aggregate statistics limited to the live documentation corpus.
🤖 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 `@scripts/lib/documentation_fetch_metadata.sh`:
- Around line 20-67: Update write_documentation_fetch_metadata to use
count_html_files for each Spring directory count instead of repeating find/wc/tr
pipelines. Also adjust total_html_files and total_files calculations so
DOCS_ROOT/.quarantine and its contents are excluded, keeping aggregate
statistics limited to the live documentation corpus.
In `@src/test/java/com/williamcallahan/javachat/config/SecurityConfigTest.java`:
- Around line 38-39: Remove the duplicated CSRF_INVALID_MESSAGE from
SecurityConfigTest and expose the existing constant from CsrfAccessDeniedHandler
with appropriate public visibility. Update the test to reference
CsrfAccessDeniedHandler.CSRF_INVALID_MESSAGE directly, preserving the canonical
message value and avoiding a second inventory.
In
`@src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java`:
- Around line 45-75: Add a companion test for
GuidedLearningService.citationsForBookDocuments that makes
retrievalService.toCitations throw for one document while returning citations
for another. Assert the failing document is skipped, enhanceWithPageAnchors
receives only the remaining citations without a size mismatch, and the
successful citation retains its expected page URL and anchor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02948cc1-769b-4c70-bcde-eae1ec6cab56
📒 Files selected for processing (22)
docs/configuration.mddocs/pipeline-commands.mdfrontend/src/lib/services/sse.test.tsfrontend/src/lib/services/sse.tsscripts/fetch_all_docs.shscripts/lib/documentation_fetch_metadata.shscripts/test_java_api_fetch_projection.shsrc/main/java/com/williamcallahan/javachat/adapters/in/web/security/CsrfAccessDeniedHandler.javasrc/main/java/com/williamcallahan/javachat/adapters/in/web/security/ExpiringCookieCsrfTokenRepository.javasrc/main/java/com/williamcallahan/javachat/cli/DocumentationSetCatalog.javasrc/main/java/com/williamcallahan/javachat/config/SecurityConfig.javasrc/main/java/com/williamcallahan/javachat/service/GuidedLearningService.javasrc/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.javasrc/main/resources/application-dev.propertiessrc/main/resources/application.propertiessrc/test/java/com/williamcallahan/javachat/cli/JavaApiDocumentationSourceParityTest.javasrc/test/java/com/williamcallahan/javachat/config/SecurityConfigTest.javasrc/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAIStreamingTimeoutContractTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java
💤 Files with no reviewable changes (3)
- src/main/java/com/williamcallahan/javachat/adapters/in/web/security/ExpiringCookieCsrfTokenRepository.java
- src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java
- src/main/java/com/williamcallahan/javachat/cli/DocumentationSetCatalog.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/williamcallahan/javachat/cli/JavaApiDocumentationSourceParityTest.java
- src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java
- docs/pipeline-commands.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.java (1)
164-176: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated transient-message heuristic.
The 7-line normalized-message substring check is now byte-for-byte duplicated between
isStreamingFallbackEligible(Lines 169-175) andisRecoverableStreamingFailure(Lines 210-216). Since this diff switchedisRecoverableStreamingFailurefrom delegating toisStreamingFallbackEligibleinto its own explicit copy, any future tweak to the pattern list (e.g. adding a new transient-error keyword) will need to be made in two places to stay in sync — a classic DRY trap for error-classification logic that both fallback selection and recoverability checks depend on.As per coding guidelines,
**/*.java: "Apply Clean Code, DDD, DRY, YAGNI, KISS, and Clean Architecture."♻️ Proposed extraction
+ private static boolean isTransientStreamMessage(String normalizedMessage) { + return normalizedMessage.contains("invalid stream") + || normalizedMessage.contains("malformed") + || normalizedMessage.contains("unexpected end of json input") + || normalizedMessage.contains("timeout") + || normalizedMessage.contains("temporarily unavailable") + || normalizedMessage.contains("connection reset") + || normalizedMessage.contains("connection closed"); + }Then in both
isStreamingFallbackEligibleandisRecoverableStreamingFailure:- return normalizedMessage.contains("invalid stream") - || normalizedMessage.contains("malformed") - || normalizedMessage.contains("unexpected end of json input") - || normalizedMessage.contains("timeout") - || normalizedMessage.contains("temporarily unavailable") - || normalizedMessage.contains("connection reset") - || normalizedMessage.contains("connection closed"); + return isTransientStreamMessage(normalizedMessage);Also applies to: 199-217
🤖 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/OpenAiProviderRoutingService.java` around lines 164 - 176, Extract the shared normalized-message substring heuristic from isStreamingFallbackEligible and isRecoverableStreamingFailure into one private helper, preserving the null handling and all existing keywords. Update both methods to delegate to that helper so future transient-error pattern changes are maintained in a single location.
🤖 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 `@scripts/lib/documentation_sources.sh`:
- Around line 33-79: Update is_absolute_https_remote_base_url and
is_normalized_relative_mirror_path to reject literal asterisk characters in
their inputs, alongside the existing whitespace/backslash validation. Ensure
both validators return failure before accepting values containing “*”,
preserving all existing URL and mirror-path checks.
---
Outside diff comments:
In
`@src/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.java`:
- Around line 164-176: Extract the shared normalized-message substring heuristic
from isStreamingFallbackEligible and isRecoverableStreamingFailure into one
private helper, preserving the null handling and all existing keywords. Update
both methods to delegate to that helper so future transient-error pattern
changes are maintained in a single location.
🪄 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: e8142bf6-c56e-46af-8547-44e155721246
📒 Files selected for processing (17)
frontend/src/lib/services/chat.test.tsfrontend/src/lib/services/guided.test.tsfrontend/src/lib/services/sse.test.tsscripts/fetch_all_docs.shscripts/lib/documentation_sources.shscripts/test_java_api_fetch_projection.shsrc/main/java/com/williamcallahan/javachat/cli/DocumentProcessor.javasrc/main/java/com/williamcallahan/javachat/cli/DocumentationSet.javasrc/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.javasrc/main/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifest.javasrc/main/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingService.javasrc/main/java/com/williamcallahan/javachat/service/RetrievalService.javasrc/test/java/com/williamcallahan/javachat/cli/JavaApiDocumentationSourceParityTest.javasrc/test/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifestTest.javasrc/test/java/com/williamcallahan/javachat/config/SecurityConfigTest.javasrc/test/java/com/williamcallahan/javachat/service/OpenAiProviderRoutingServiceTest.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/lib/services/guided.test.ts
- frontend/src/lib/services/chat.test.ts
- src/test/java/com/williamcallahan/javachat/config/SecurityConfigTest.java
- src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java
- src/main/java/com/williamcallahan/javachat/service/RetrievalService.java
- src/test/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifestTest.java
- src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java
- scripts/fetch_all_docs.sh
This was referenced Jul 14, 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 free
to 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
PR #77 makes stateful chat streaming single-execution, preserves citation evidence through reranking and redaction, and gives Java API ingestion one validated source catalog with truthful partial-fetch behavior. It also keeps CSRF protection stateless without breaking secure cookie deletion.
Changes
Streaming
Retrieval and citations
Documentation ingestion
java-api-documentation-sources.manifestis the sole owner of complete Java API source rows for Java 21, 24, and 25.rejectRegexandallowPartialreach both Java and generic fetch boundaries, and incomplete retained mirrors still block overall completion.Security and frontend contracts
SameSite=Lax, rejects missing or mismatched token pairs, and emits a secure immediate-deletion cookie on HTTPS logout.Breaking Changes
java/java25-complete; legacy shorthand, display-name, generated-ID, and case-insensitive aliases are removed.OPENAI_STREAMING_READ_TIMEOUT_SECONDSis no longer read; the SDK read timeout inheritsOPENAI_STREAMING_REQUEST_TIMEOUT_SECONDS.Verification
Related Issues
None.