feat(chat): reasoning-effort override scoped to chat generation, actionable 422 gateway errors, and touch-device copy action - #159
Conversation
Keep assistant copy actions discoverable across touch and hybrid input while preserving desktop hover behavior. - Reserve the action slot during streaming and verify visibility, focus, and disabled state
JSON-object completions (reranker, enrichment) own small output budgets whose visible answers reasoning tokens would starve, so they now inherit the gateway/model default. A gateway HTTP 422 unpreservable_reasoning_intent is deterministic and retrying unchanged can never succeed. - OpenAiRequestFactory applies the configured reasoning effort only to non-JSON requests - ChatController maps HTTP 422 unpreservable_reasoning_intent to a non-retryable error naming the configured effort and app.llm.reasoning-effort - docs/configuration.md documents the chat-only scope and the 422 meaning
Gradle default test logging truncates exception chains, and the JVM OmitStackTraceInFastThrow optimization drops frames from repeatedly thrown hot exceptions; both hid the root cause while diagnosing streaming failure regressions. - Log FULL exception format with stack traces for all Test tasks - Pass -XX:-OmitStackTraceInFastThrow to test JVMs
ChatControllerStreamingFailureTest exceeded the CouplingBetweenObjects threshold (37 > 35) after the unpreservable_reasoning_intent coverage added Headers, ErrorObject, and UnprocessableEntityException references, failing the pmdTest pre-push gate. The straightforward mock replacement also crashed the test container: a bare mocked Throwable reaches Reactor and logging machinery that dereferences surfaces a mock leaves null. - Simulate the gateway 422 with a mocked InternalServerException (an OpenAIServiceException) stubbing only statusCode and code - Wrap it in ReportedTerminalStreamingFailure, matching the existing gateway-timeout test and the production path where the service reports terminal failures before the controller classifies them
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change updates LLM reasoning behavior for chat and JSON completions, adds targeted handling for reasoning-related streaming failures, adjusts message copy-action visibility during streaming and across pointer types, and improves test diagnostics. ChangesLLM reasoning and failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChatController
participant OpenAiRequestFactory
participant OpenAIService
ChatController->>OpenAiRequestFactory: Build chat request with configured reasoning
OpenAiRequestFactory->>OpenAIService: Send streaming or JSON-object request
OpenAIService-->>ChatController: Return HTTP 422 reasoning-intent failure
ChatController-->>ChatController: Create effort-specific failure message
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@src/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.java`:
- Around line 216-219: Replace the hardcoded "error" event names in the stream
filtering assertions around streamEvents with the canonical EVENT_ERROR constant
from SseConstants, including the occurrence near the later assertion. Preserve
the existing filtering and assertion 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 Plus
Run ID: 20c6ae5a-727c-4d92-9d92-1d008f84e168
📒 Files selected for processing (8)
build.gradle.ktsdocs/configuration.mdfrontend/src/lib/components/MessageBubble.sveltefrontend/src/lib/components/MessageBubble.test.tssrc/main/java/com/williamcallahan/javachat/service/OpenAiRequestFactory.javasrc/main/java/com/williamcallahan/javachat/web/ChatController.javasrc/test/java/com/williamcallahan/javachat/service/OpenAiRequestFactoryTest.javasrc/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.java
| ServerSentEvent<String> errorEvent = streamEvents.stream() | ||
| .filter(streamEvent -> "error".equals(streamEvent.event())) | ||
| .findFirst() | ||
| .orElseThrow(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the canonical SSE error-event constant.
Lines 217 and 235 duplicate "error" instead of using EVENT_ERROR, which this test already uses elsewhere. Tiny consistency win.
Proposed fix
- .filter(streamEvent -> "error".equals(streamEvent.event()))+ .filter(streamEvent -> EVENT_ERROR.equals(streamEvent.event()))Based on learnings, reference event type strings from SseConstants.java instead of hardcoding values like "error".
Also applies to: 234-237
🤖 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/ChatControllerStreamingFailureTest.java`
around lines 216 - 219, Replace the hardcoded "error" event names in the stream
filtering assertions around streamEvents with the canonical EVENT_ERROR constant
from SseConstants, including the occurrence near the later assertion. Preserve
the existing filtering and assertion behavior.
Source: Learnings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:739ff6652b
ℹ️ 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".
| if (isUnpreservableReasoningIntent(error)) { | ||
| return unpreservableReasoningIntentMessage(); |
There was a problem hiding this comment.
When the same configured reasoning effort is used through /api/guided/stream, GuidedLearningController.guidedStreamError() still maps unpreservable_reasoning_intent to the generic "Streaming error" response whose details tell the user to try again. Because this new classification is local to ChatController, guided-chat users receive no instruction to change the deterministic failing setting; apply the classification to that streaming boundary as well.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The
app.llm.reasoning-effortoverride now applies only to chat generation so reranker/enrichment JSON completions can no longer be starved by reasoning tokens, and a gateway rejection of an unsatisfiable effort now surfaces an actionable non-retryable error instead of a generic failure. Also ships a touch-device fix for the assistant copy action and full test-failure stack traces for developers.Changes
Features
OpenAiRequestFactory.prepareRequest)HTTP 422 unpreservable_reasoning_intent), the stream now ends with a non-retryable error naming the configured effort and the owningapp.llm.reasoning-effortsetting, instead of the generic "something went wrong" message that invited futile retries (ChatController.isUnpreservableReasoningIntent,ChatController.unpreservableReasoningIntentMessage)Bug Fixes
MessageBubble.svelte)Developer Experience
-XX:-OmitStackTraceInFastThrowstops the JVM from dropping frames on repeatedly thrown hot exceptions, restoring root-cause visibility when diagnosing streaming failures (build.gradle.kts)ChatControllerStreamingFailureTestexceeded the CouplingBetweenObjects threshold (37 > 35) and blocked the pmdTest pre-push gate; the 422 gateway failure is now simulated with a stubbedInternalServerExceptionwrapped inReportedTerminalStreamingFailure, matching the production path and removing the excess type references (ChatControllerStreamingFailureTest.unprocessableEntityFailure)Documentation
docs/configuration.md)Breaking Changes
None
Test Plan
./gradlew test --tests OpenAiRequestFactoryTest --tests ChatControllerStreamingFailureTest— green./gradlew pmdTest— green (previously failing at 37 > 35)make build-with-lock: frontend build +gradle build -x test+ lint) — green