feat(llm): multi-model cascade — enterprise hardening - #587
Merged
Conversation
Design spec for the full enterprise pass over multi-model cascading: audit correctness, SSE wiring, token/cost observability, judge_model, config-driven heuristics, configure-time validation, cancellation safety, streaming the final step, and honest docs.
Full enterprise pass over multi-model cascading: - Audit correctness (#5): record the cascade-selected provider/model in audit:model_name and audit:cascade_model; add cost + token_usage keys. - SSE events (#1): wire onCascadeStepStart/onCascadeEscalation end-to-end (handler default methods -> ConversationService sink -> SSE endpoint). - judge_model (#2): real judgeModel config block built via ChatModelRegistry. - Agent-mode confidence (#6): auto-route structured_output to judge/heuristic. - convertToObject + cascade (#7): honor jsonMode, force non-wrapper strategy. - Global-var/Qute step consistency (#8); cancellation safety (#9). - Token + cost evidence in trace + responseMetadata; Micrometer metrics (eddi.llm.cascade.*); maxTotalDurationMs + maxCostPerRun ceilings. - Configure-time validation (CascadeConfigValidator). - JSON-parse-first confidence parsing; config-driven + language-agnostic heuristic; stream the always-accepted final step live; returnBestAcrossSteps; lazy base-model creation. CascadingModelExecutor converted to an instance; AgentOrchestrator.ExecutionResult gains responseMetadata (2-arg ctor retained). Existing executor + LlmTask tests updated. Backward compatible: all new config optional; enabled:false unaffected.
New tests: CascadeConfigValidatorTest, CascadingModelExecutorEnterpriseTest (judge reachability, cost computation, metrics, returnBestAcrossSteps, trace), ConfidenceEvaluatorEnterpriseTest (JSON-parse-first, stray-confidence safety, config-driven heuristics, judge parsing). Fixes surfaced by the tests: - ConfidenceEvaluator.evaluateStructuredOutput now only treats a response as a confidence wrapper when it IS a single JSON object, eliminating false positives from a stray "confidence" value inside answer content (e.g. a code sample). - CascadingModelExecutor.buildJudgeModel defaults null judge params to an empty map, avoiding an NPE in ChatModelRegistry.getOrCreate for judges without params.
Documents judgeModel/heuristic/ceilings/pricing/returnBestAcrossSteps config, real SSE event fields, real audit keys and trace key (langchain:cascade:trace), token/cost observability + Micrometer metrics, agent-mode confidence routing, convertToObject interaction, final-step streaming, cancellation semantics, and configure-time validation. Corrects prior drift (429 retried-then-escalate).
- CascadeConfigValidator warns at deploy time when a cascade step or judgeModel targets a different provider than the task but omits its own apiKey (it would otherwise inherit the task's key, wrong for a different provider). Documented. - isRetryableError message matching collapsed to a single regex (fewer branches). - Broadened tests: agent mode, live streaming, cost/duration ceilings, timeout + retryable escalation, convertToObject downgrade, confidence edge cases, judge fallbacks, streaming metadata capture, fully-configured heuristic, cross-provider credential validation. New-code coverage ~92% instruction / ~78% branch.
From a multi-lens adversarial review (7 reviewers + independent skeptics): - returnBestAcrossSteps no longer supersedes a final step already streamed live (would mismatch tokens already sent to the client); trace marks it superseded. - Agent-mode cascade now emits its final response to the SSE stream as a single chunk, matching the standard agent path (was dropped); docs corrected. - CascadeConfigValidator warns (not throws) for conditions older releases tolerated at load — protects previously-loading stored agents from failing to deploy on upgrade; only new pricing/ceiling fields hard-fail. - Heuristic scores clamped to [0,1]; unescapeJsonString rewritten single-pass (chained replace corrupted \n); judge regex scoped to the extracted object. - Streaming-timeout caveat documented. - Regression tests + previously-missing SSE-forwarding and cooperative-cancellation tests. New-code coverage ~92% instruction / ~79% branch.
…sions A lean second adversarial review (5 reviewers + synthesizer) found 5 real issues: - HIGH: a live-streamed step no longer runs under the per-step/duration timeout — cancelling it couldn't stop the provider callback thread, leaking tokens while the cascade re-emitted a different response (concurrent SSE writes). Streamed steps now run under the streaming executor's own ~120s bound; result (even if partial) is the accepted answer. streamLive tightened to guaranteed-accept steps. - MED: judge regex fallback restored to full-text (scoping to the first balanced object dropped the score when a reasoning object preceded the rating). - MED: docs corrected to state the real two validation tiers (error vs warn). - LOW: stripCodeFences now unwraps single-line fenced JSON. - LOW: returnBestAcrossSteps relabels the winning step's trace entry. Regression tests added for each; full touched-area suite green.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends multi-model cascading with configurable evaluation strategies, pricing and duration limits, deploy-time validation, hardened confidence parsing, instance-based execution, metadata capture, live-streaming controls, SSE callbacks, audit integration, and expanded tests and documentation. ChangesMulti-Model Cascade Enterprise Hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 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 |
![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: 7
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/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java (1)
332-381: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
escalationsmetric fires even on the last step, where no escalation actually occurs.In both the
TimeoutException(lines 338-339) and generalException(lines 364-365) handlers,increment("eddi.llm.cascade.escalations", ...)runs unconditionally, while the SSEonCascadeEscalationevent is correctly gated by!isLastStep(lines 343, 369). When the last step fails, there is no next step to escalate to — the cascade just returns bestSoFar or throws — so counting this as an "escalation" over-reports the metric.🤖 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/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java` around lines 332 - 381, The `eddi.llm.cascade.escalations` metric is being incremented even when `isLastStep` is true, which overcounts failures as escalations. Update `CascadingModelExecutor` so the `increment("eddi.llm.cascade.escalations", ...)` calls in both the `TimeoutException` and general `Exception` branches are only executed when there is actually a next step to escalate to, matching the existing `eventSink.onCascadeEscalation` `!isLastStep` guard. Keep the current last-step fallback/throw behavior unchanged.
🧹 Nitpick comments (4)
src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.java (1)
103-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated executor-construction helper across 5 test files.
This
runCascadehelper (mockedGlobalVariableResolver+CascadingModelExecutorconstruction +execute(...)call) is repeated almost verbatim inCascadingModelExecutorExecuteTest.javaandCascadingModelExecutorExtendedTest.java, and in structurally equivalent form inCascadingModelExecutorCoverageTest.java(executor(...)) andCascadingModelExecutorEnterpriseTest.java(run(...)). Consider extracting a shared test utility/base class so futureexecute(...)signature changes only need updating in one place.🤖 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/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.java` around lines 103 - 113, The executor-construction helper is duplicated across multiple cascading model tests, so changes to CascadingModelExecutor.execute(...) must be repeated in several places. Extract the shared setup into a common test utility or base class used by runCascade, executor(...), and run(...) so the mocked GlobalVariableResolver, CascadingModelExecutor construction, and execute(...) invocation live in one reusable helper.src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExtendedTest.java (1)
32-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisabled test class duplicates coverage already provided by enabled sibling tests.
This entire class is
@Disabled, so the updatedrunCascadehelper and its call sites provide no actual verification.CascadingModelExecutorTest.javaandCascadingModelExecutorExecuteTest.javaexercise the same null/empty-steps, escalation, error-handling, and bestSoFar-fallback scenarios using the same threadedexecute()path and Mockito mocking style, and are enabled. Worth re-validating whether the "mocks don't work across threads" rationale still holds; if not, either re-enable this class or remove it to reduce redundant maintenance.🤖 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/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExtendedTest.java` around lines 32 - 64, The CascadingModelExecutorExtendedTest class is disabled and only duplicates coverage already exercised by the enabled CascadingModelExecutorTest and CascadingModelExecutorExecuteTest suites. Re-check whether the thread-pool/mock limitation still applies for CascadingModelExecutor and its execute/runCascade path; if it no longer does, remove the `@Disabled` from CascadingModelExecutorExtendedTest and keep the helper-based coverage, otherwise delete the redundant test class to avoid duplicate maintenance.src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
484-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
TokenUsage.add()over hand-rolled summation.
dev.langchain4j.model.output.TokenUsagealready provides a null-safeadd(TokenUsage that)method — "If one of the token usages is null, the other is returned without changes." Reimplementing this viasumTokens/sumIntduplicates library behavior and diverges slightly in semantics (the custom version coerces null-null pairs to0rather than preservingnull).♻️ Possible simplification
- private static TokenUsage sumTokens(TokenUsage a, TokenUsage b) { - if (a == null) { - return b; - } - if (b == null) { - return a; - } - return new TokenUsage(sumInt(a.inputTokenCount(), b.inputTokenCount()), sumInt(a.outputTokenCount(), b.outputTokenCount()), - sumInt(a.totalTokenCount(), b.totalTokenCount())); - } - - private static Integer sumInt(Integer a, Integer b) { - return (a != null ? a : 0) + (b != null ? b : 0); - } + private static TokenUsage sumTokens(TokenUsage a, TokenUsage b) { + return a == null ? b : (b == null ? a : a.add(b)); + }🤖 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/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 484 - 500, The TokenUsage merging logic in AgentOrchestrator.sumTokens duplicates library behavior and should use TokenUsage.add(TokenUsage) instead. Update the code path that combines token counts to delegate to add() and remove the custom sumInt helper, keeping the existing null-safe behavior where one null returns the other. This also avoids changing semantics for the null-null case and keeps the implementation aligned with dev.langchain4j.model.output.TokenUsage.docs/changelog.md (1)
8-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChangelog entry is missing an explicit "what's next if interrupted" section.
The new entry covers date, title, repo/branch, and an extensive "what changed"/design-decisions narrative, but has no closing note on next steps/continuation state.
As per coding guidelines, "Changelog entries must include the date, short title, repo and branch, what changed, design decisions, and what's next if interrupted."
🤖 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 `@docs/changelog.md` around lines 8 - 80, The changelog entry is missing the required “what’s next if interrupted” section; add a brief closing note to the existing entry in changelog.md that states the continuation state or next steps after this work. Keep the current date/title/repo/branch/what changed/design decisions content intact, and append a concise “what’s next” paragraph so the entry satisfies the changelog guideline.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.
Inline comments:
In `@docs/model-cascade.md`:
- Around line 60-75: The `strategy` description in the cascade fields table is
inconsistent with `CascadeConfigValidator.validateTask` and the doc’s validation
section: unknown values are not rejected, they only trigger a warning and
continue with sequential behavior. Update the `strategy` entry in
`docs/model-cascade.md` to say unknown strategies warn and default to sequential
execution, while keeping the note that `parallel` is accepted but runs
sequentially. Make sure the wording matches the actual `CascadeConfigValidator`
behavior and the rest of the document.
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java`:
- Around line 105-111: The cascade_escalation payload in
RestAgentEngineStreaming.onCascadeEscalation can emit invalid JSON when
confidence or threshold is NaN/Infinity because String.format with %.4f writes
non-JSON tokens. Update the event construction to sanitize these values before
formatting, using the existing onCascadeEscalation method and sendEvent call, so
non-finite numbers are converted to valid JSON-safe output (for example, a
quoted string or null) before escaping the reason and sending the SSE.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 351-358: The interrupt-handling in AgentOrchestrator should clear
the thread’s interrupted status before throwing the cancellation
LifecycleException, because the current isInterrupted() check leaves the flag
set on a pooled worker. Update the cooperative cancellation checks in
AgentOrchestrator to consume the interrupt (for example by using
Thread.interrupted()) and apply the same change to the later pre-tool
cancellation check so no stale interrupt leaks into the next task.
- Around line 328-334: The retry logic in AgentOrchestrator.executeWithRetry
currently resets tokenHolder on each attempt, so responseMetadata.tokenUsage
only reflects the last successful try. Keep a separate accumulator for total
token usage across all attempts in the executeWithRetry block, update it
whenever tokenHolder is set, and use that accumulated value when building the
final response metadata.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java`:
- Around line 283-318: The accepted-step metric is recorded too early in the
cascade flow, so `increment("eddi.llm.cascade.accepted.step", ...)` can count
step i even when `returnBestAcrossSteps` later returns `bestSoFar` instead.
Update `CascadingModelExecutor` so the metric is emitted only after the
`returnBestAcrossSteps` branch has been resolved, or split it into separate
counters for truly returned accepted steps versus superseded ones. Keep the
trace updates around `stepTrace`, `bestSoFar`, and the `return withRun(...)`
path aligned with the final returned result.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.java`:
- Around line 111-140: The malformed structured-output fallback in
ConfidenceEvaluator.evaluateStructuredOutput is using the raw response when
extracting actualResponse, which can preserve markdown fences if the input was
fenced. Update the regex-fallback branch so the stripJsonWrapper fallback
operates on the already fence-stripped candidate rather than response, and keep
the existing confidence parsing behavior intact. This change should be made in
the evaluateStructuredOutput method, alongside the CONFIDENCE_JSON_PATTERN and
RESPONSE_JSON_PATTERN fallback logic.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java`:
- Around line 443-458: The skipCascade branch in LlmTask should match the
standard streaming fallback behavior when
agentOrchestrator.executeIfToolsEnabled returns null. Update the else path to
forward the legacyChatExecutor.execute responseContent to eventSink the same way
the non-skipCascade flow does, honoring addToOutputExplicitlyFalse and the
existing null checks. Keep the fix localized to the skipCascade handling in
LlmTask so SSE output is emitted consistently regardless of which fallback path
is used.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java`:
- Around line 332-381: The `eddi.llm.cascade.escalations` metric is being
incremented even when `isLastStep` is true, which overcounts failures as
escalations. Update `CascadingModelExecutor` so the
`increment("eddi.llm.cascade.escalations", ...)` calls in both the
`TimeoutException` and general `Exception` branches are only executed when there
is actually a next step to escalate to, matching the existing
`eventSink.onCascadeEscalation` `!isLastStep` guard. Keep the current last-step
fallback/throw behavior unchanged.
---
Nitpick comments:
In `@docs/changelog.md`:
- Around line 8-80: The changelog entry is missing the required “what’s next if
interrupted” section; add a brief closing note to the existing entry in
changelog.md that states the continuation state or next steps after this work.
Keep the current date/title/repo/branch/what changed/design decisions content
intact, and append a concise “what’s next” paragraph so the entry satisfies the
changelog guideline.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 484-500: The TokenUsage merging logic in
AgentOrchestrator.sumTokens duplicates library behavior and should use
TokenUsage.add(TokenUsage) instead. Update the code path that combines token
counts to delegate to add() and remove the custom sumInt helper, keeping the
existing null-safe behavior where one null returns the other. This also avoids
changing semantics for the null-null case and keeps the implementation aligned
with dev.langchain4j.model.output.TokenUsage.
In
`@src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExtendedTest.java`:
- Around line 32-64: The CascadingModelExecutorExtendedTest class is disabled
and only duplicates coverage already exercised by the enabled
CascadingModelExecutorTest and CascadingModelExecutorExecuteTest suites.
Re-check whether the thread-pool/mock limitation still applies for
CascadingModelExecutor and its execute/runCascade path; if it no longer does,
remove the `@Disabled` from CascadingModelExecutorExtendedTest and keep the
helper-based coverage, otherwise delete the redundant test class to avoid
duplicate maintenance.
In `@src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.java`:
- Around line 103-113: The executor-construction helper is duplicated across
multiple cascading model tests, so changes to
CascadingModelExecutor.execute(...) must be repeated in several places. Extract
the shared setup into a common test utility or base class used by runCascade,
executor(...), and run(...) so the mocked GlobalVariableResolver,
CascadingModelExecutor construction, and execute(...) invocation live in one
reusable helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09909925-f9fb-416d-bf22-675e4ef9f956
📒 Files selected for processing (29)
docs/changelog.mddocs/model-cascade.mdplanning/model-cascade-enterprise-hardening-plan.mdsrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorEnterpriseTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExecuteTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluatorEnterpriseTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskConfigureTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorCoverageTest.java
ginccc
force-pushed
the
feat/model-cascade-enterprise-hardening
branch
from
July 3, 2026 08:47
415b52d to
7cb312b
Compare
There was a problem hiding this comment.
Pull request overview
Hardens the multi-model cascade execution path in the Quarkus LLM module to make documented enterprise behaviors real end-to-end (SSE cascade events + judge model), correct audit attribution under cascade, and add measurable observability (token usage, cost, and Micrometer metrics) while preserving backward compatibility for stored configs.
Changes:
- Extends cascade configuration with judge model + heuristic overrides + ceilings/pricing + best-across-steps selection; adds deploy-time validation.
- Reworks cascade execution to support judge-based confidence, per-step trace enrichment (tokenUsage/cost/duration/status), live streaming of guaranteed-accept steps, and cooperative cancellation in the agent tool loop.
- Wires cascade SSE events through the streaming stack and adds comprehensive regression/coverage tests plus updated docs/changelog.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds cascade config fields (judgeModel, heuristic, ceilings/pricing, returnBestAcrossSteps). |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Integrates cascade executor, lazy base model creation, audit fixes, metadata propagation, and deploy-time validation call. |
| src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java | Refactors cascade execution to support judge/heuristic routing, templated step params, ceilings, cost/token trace, metrics, and live streaming. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.java | Implements JSON-parse-first structured output parsing, configurable heuristics, and judge-model evaluation. |
| src/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.java | Adds metadata capture (finish reason/token usage) while streaming tokens. |
| src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java | New deploy-time validator with warn-vs-fail-fast behavior for backward compatibility. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Adds cooperative cancellation checks and aggregates token usage across tool-loop iterations. |
| src/main/java/ai/labs/eddi/engine/api/IConversationService.java | Adds default cascade callbacks to the streaming response handler interface. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Forwards cascade events from the event sink to the streaming handler. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Emits cascade_step_start / cascade_escalation SSE events. |
| src/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorCoverageTest.java | New coverage for streaming capture (token usage/finish reason + error path). |
| src/test/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluatorEnterpriseTest.java | New enterprise-focused tests for structured output parsing, heuristic config, judge parsing, edge cases. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorEnterpriseTest.java | New tests for judge reachability, metrics, returnBestAcrossSteps, and cost computation. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorCoverageTest.java | New coverage for agent mode, live streaming, ceilings, timeouts, retryable errors, and convertToObject/jsonMode behavior. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java | New tests for validator warn/fail-fast boundaries and cross-provider credential warnings. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.java | Updates tests to the new executor instance API/signature. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExtendedTest.java | Updates tests to the new executor instance API/signature. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExecuteTest.java | Updates tests to the new executor instance API/signature. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskConfigureTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java | Updates LlmTask construction to pass a MeterRegistry. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java | Adds regression test for cooperative cancellation behavior. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java | Adds test coverage for forwarding cascade SSE events. |
| planning/model-cascade-enterprise-hardening-plan.md | Adds the design plan/spec for the enterprise hardening work. |
| docs/model-cascade.md | Updates user-facing documentation for new cascade features/semantics. |
| docs/changelog.md | Adds a detailed changelog entry describing the hardening work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Accumulate agent-mode token usage across ALL retry attempts, not just the last (every LLM call is billed). - Clear the cancellation interrupt flag (Thread.interrupted()) so it can't leak. - Under returnBestAcrossSteps, the accepted.step metric + trace status now name the actual returned step (deferred past the supersede decision). - Normalize an unknown evaluationStrategy to structured_output at runtime (matches the validator warning + evaluator default; keeps streaming/wrapper gating consistent). - SSE cascade_escalation guards non-finite confidence/threshold (valid JSON). - structured_output regex fallback uses the fence-stripped text (no leaked fences). - Cascade-disabled agent path forwards its buffered response to the SSE stream. - Remove an unused parameter from executeStepWithTimeout; delete a dead @disabled test. - Docs/changelog/plan corrected: the validator WARNS (not rejects/fails-fast) on legacy conditions; judgeModel omitted -> warn + heuristic fallback.
Line-by-line audit of docs/model-cascade.md, docs/changelog.md, and the planning doc against the final shipped code: - model-cascade.md: maxTotalDurationMs/timeoutMs field docs and the Error Handling 'Timeout' row unconditionally claimed the per-step timeout is always capped by the remaining duration budget — no longer true for a live-streamed step (round-2 fix exempted it). Qualified both + cross-linked to the Streaming section. - 'Streaming the Final Step' main paragraph described only 'the final step', narrower than the actual guaranteed-accept condition (last step, null- threshold step, or none-strategy step with threshold<=1.0) already documented in the note below it. Aligned the two, removed the redundant closing sentence in the note. - Trace status enum was missing 'superseded_by_best' / 'accepted_as_best', introduced by the returnBestAcrossSteps trace-relabeling fix. - Noted that configured heuristic scores are clamped to [0,1]. - changelog.md: the 'Cascade ceilings' bullet had the same stale unconditional timeout-capping claim; qualified to buffered steps only. - plan doc: corrected 'Status: In progress' to Complete/PR #587; corrected CascadingModelExecutor's description from a speculative '@ApplicationScoped CDI bean with IMemoryItemConverter' (never built that way) to what was actually shipped (a plain instance constructed by LlmTask, no IMemoryItemConverter dependency); corrected the streaming-final-step and returnBestAcrossSteps sections to describe the round-2/round-3 shipped behavior instead of the original round-1 intent; recorded all three adversarial/bot review rounds under phase 11.
niedch
reviewed
Jul 9, 2026
niedch
left a comment
niedch
left a comment
Collaborator
There was a problem hiding this comment.
Looks good overall, just one thing which could avoid some confustions in the future!
| @Override | ||
| public void onCascadeStepStart(int stepIndex, String modelType, String modelName, int totalSteps) { | ||
| sendEvent(eventSink, sse, "cascade_step_start", | ||
| String.format("{\"stepIndex\":%d,\"modelType\":\"%s\",\"modelName\":\"%s\",\"totalSteps\":%d}", stepIndex, |
Collaborator
There was a problem hiding this comment.
I like to be type safe so i would probably wrap this in a Object and serialize that
| return new EvaluationResult("", 0.0); | ||
| } | ||
|
|
||
| return switch (strategy != null ? strategy.toLowerCase() : "structured_output") { |
Collaborator
There was a problem hiding this comment.
I noticed that a couple of time already, is there a reason why the strategy should be string and not a enum with the values? At the moment you have to trust the comment on the definition. I think adding a enum for the evaluationStrategry and general CacasdingStrategy would be benefitial.
Integrate the tool-level HITL framework (from main) with the multi-model cascade enterprise hardening (this branch). Conflict resolutions: - CascadingModelExecutor keeps the instance-based design; HITL tool-approval params (effectiveToolApprovals, llmTaskIndex, transcriptMaxBytes) threaded through execute -> executeStepWithTimeout -> executeAgentModeStep -> executeIfToolsEnabled, plus a ToolApprovalRequiredException immediate rethrow in the per-step catch so a HITL pause is never demoted to a failed step. Added a backward-compatible 11-arg execute overload for non-HITL callers. - AgentOrchestrator: main's HITL-gating/resume version as the base, with cascade's token accumulation, cooperative-cancellation checks, and ExecutionResult.responseMetadata grafted in (tokenHolder threaded through runToolCallLoop; both callers build responseMetadata). - LlmTask: cascade's if/else-if/else execution branch structure retained; HITL params threaded into the cascade executor and both executeIfToolsEnabled call sites; meterRegistry + hitlToolJournalStore both added to the constructor. - docs/changelog.md: union of both branches' entries. - Tests adapted to the merged API (instance executor + dual constructor arg); added a regression test for the cascade tool-approval-pause rethrow.
…iew) Address two @niedch review comments after merging origin/main into the branch. - RestAgentEngineStreaming: serialize the cascade_step_start / cascade_escalation SSE payloads via typed records through the existing Jackson mapper (new sendJsonEvent helper) instead of hand-built String.format JSON. Non-finite confidence/threshold are still sanitized. - Introduce EvaluationStrategy and CascadingStrategy enums as the single source of truth for the recognized strategy tokens; ConfidenceEvaluator, CascadingModelExecutor, and CascadeConfigValidator now resolve to them instead of scattered magic strings. The config wire fields stay lenient Strings (documented) so an unknown/future value still loads, warns, falls back to the enum DEFAULT, and round-trips unchanged; parsing happens at the boundary via fromConfig/fromConfigOrDefault. New StrategyEnumsTest locks that lenient contract; runtime behavior is otherwise unchanged.
A critical whole-branch review found the branch merge-ready (no blockers). Acted on the confirmed nits and backfilled coverage for new/adapted paths the existing suite missed. Fixes: - CascadeConfigValidator: the convertToObject-incompatibility warning now uses EvaluationStrategy.fromConfigOrDefault, so an unknown evaluationStrategy — which resolveEffectiveStrategy also resolves to structured_output and downgrades at runtime — warns too (validator <-> runtime parity). - CascadingModelExecutor: when a live-streamed final step fails after emitting partial tokens, the buffered-best fallback is marked streamedLive so LlmTask does not re-emit a duplicate token stream after the partial the client already received (withRun overload + per-step stepStreamedLive flag). Coverage backfill (all green): - AgentOrchestrator: token accumulation into ExecutionResult.responseMetadata (the cascade-cost feed), sumTokens/tokenUsageMap unit tests (helpers made package-private), and the before-tool cooperative-cancellation check. - CascadingModelExecutor: step-param templating + credential skip, a deterministic duration-ceiling test, and the streaming mid-failure de-dup. - LlmTask: skipCascade legacy-fallback SSE emit + cascade token-usage audit. - ConfidenceEvaluator: stripJsonWrapper fallback, extractFirstBalancedObject escaped-quote handling, judge readTree-throw -> regex fallback. - CascadeConfigValidator: cascade-level negative pricing + convertToObject + unknown-strategy warn.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
148-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
volatilefor lazily-wired attachment fields.
attachmentStore/attachmentTextExtractorare written once post-construction bysetAttachmentServicesand then read fromaddReadAttachmentToolIfEnabledon every conversation turn. IfAgentOrchestratorinstances are shared across threads (its own comments describe it as effectively non-CDI, held/wired byLlmTask), a plain field write with novolatile/synchronization has no guaranteed happens-before relationship with later reads on other threads, unless publication is otherwise safe (e.g. via afinalreference assigned after this call completes). Hard to confirm withoutLlmTask's wiring code.🛡️ Proposed defensive fix
- private IAttachmentStore attachmentStore; - private AttachmentTextExtractor attachmentTextExtractor; + private volatile IAttachmentStore attachmentStore; + private volatile AttachmentTextExtractor attachmentTextExtractor;As per coding guidelines, "Backend code must be thread-safe and non-blocking."
🤖 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/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 148 - 161, Make the lazily wired attachment services safely visible across conversation threads by declaring both attachmentStore and attachmentTextExtractor volatile. Keep setAttachmentServices and addReadAttachmentToolIfEnabled behavior unchanged, without introducing blocking synchronization.Source: Path instructions
🤖 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/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 953-973: Update the abandoned-thread guard in the surrounding
orchestration method to use the interrupt-checking operation that clears the
current thread’s interrupted flag before throwing LifecycleInterruptedException.
Preserve the existing no-shared-memory-mutation behavior and keep the sibling
cancellation checks consistent.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 148-161: Make the lazily wired attachment services safely visible
across conversation threads by declaring both attachmentStore and
attachmentTextExtractor volatile. Keep setAttachmentServices and
addReadAttachmentToolIfEnabled behavior unchanged, without introducing blocking
synchronization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d963674-1b03-480f-92bf-f132f1f41eee
📒 Files selected for processing (15)
docs/changelog.mddocs/model-cascade.mdplanning/model-cascade-enterprise-hardening-plan.mdsrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/CascadingStrategy.javasrc/main/java/ai/labs/eddi/modules/llm/model/EvaluationStrategy.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- planning/model-cascade-enterprise-hardening-plan.md
- src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.java
- src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
148-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
volatilefor lazily-wired attachment fields.
attachmentStore/attachmentTextExtractorare written once post-construction bysetAttachmentServicesand then read fromaddReadAttachmentToolIfEnabledon every conversation turn. IfAgentOrchestratorinstances are shared across threads (its own comments describe it as effectively non-CDI, held/wired byLlmTask), a plain field write with novolatile/synchronization has no guaranteed happens-before relationship with later reads on other threads, unless publication is otherwise safe (e.g. via afinalreference assigned after this call completes). Hard to confirm withoutLlmTask's wiring code.🛡️ Proposed defensive fix
- private IAttachmentStore attachmentStore; - private AttachmentTextExtractor attachmentTextExtractor; + private volatile IAttachmentStore attachmentStore; + private volatile AttachmentTextExtractor attachmentTextExtractor;As per coding guidelines, "Backend code must be thread-safe and non-blocking."
🤖 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/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 148 - 161, Make the lazily wired attachment services safely visible across conversation threads by declaring both attachmentStore and attachmentTextExtractor volatile. Keep setAttachmentServices and addReadAttachmentToolIfEnabled behavior unchanged, without introducing blocking synchronization.Source: Path instructions
🤖 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/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 953-973: Update the abandoned-thread guard in the surrounding
orchestration method to use the interrupt-checking operation that clears the
current thread’s interrupted flag before throwing LifecycleInterruptedException.
Preserve the existing no-shared-memory-mutation behavior and keep the sibling
cancellation checks consistent.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 148-161: Make the lazily wired attachment services safely visible
across conversation threads by declaring both attachmentStore and
attachmentTextExtractor volatile. Keep setAttachmentServices and
addReadAttachmentToolIfEnabled behavior unchanged, without introducing blocking
synchronization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d963674-1b03-480f-92bf-f132f1f41eee
📒 Files selected for processing (15)
docs/changelog.mddocs/model-cascade.mdplanning/model-cascade-enterprise-hardening-plan.mdsrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/CascadingStrategy.javasrc/main/java/ai/labs/eddi/modules/llm/model/EvaluationStrategy.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- planning/model-cascade-enterprise-hardening-plan.md
- src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/modules/llm/impl/ConfidenceEvaluator.java
- src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
🛑 Comments failed to post (1)
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
953-973: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abandoned-thread guard doesn't clear the interrupt flag, unlike the other two cancellation checks in this same method.
Lines 891 and 989 both use
Thread.interrupted()(clears the flag) per the earlier fix for interrupt-leak-to-pooled-threads. This third check at Line 970 usesThread.currentThread().isInterrupted(), which leaves the flag set. If the executor backing this call ever reuses platform threads (rather than one-shot virtual threads), the next task scheduled on that thread would inherit a stale interrupted state and could fail unexpectedly. The surrounding comment explains why the pause must be aborted here, but doesn't explain why the flag is deliberately left set — this looks like it was missed when the sibling checks were fixed.🛡️ Proposed fix
- if (Thread.currentThread().isInterrupted()) { + if (Thread.interrupted()) { throw new LifecycleException.LifecycleInterruptedException( "Tool-approval pause abandoned: executing thread was interrupted before commit"); }Clearing the flag here doesn't change any of the documented "abandoned thread, no shared-memory mutation" semantics — it only prevents the interrupted status from leaking past this throw.
🤖 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/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 953 - 973, Update the abandoned-thread guard in the surrounding orchestration method to use the interrupt-checking operation that clears the current thread’s interrupted flag before throwing LifecycleInterruptedException. Preserve the existing no-shared-memory-mutation behavior and keep the sibling cancellation checks consistent.
AgentOrchestrator.attachmentStore/attachmentTextExtractor are set once by LlmTask#wireAttachmentServices (@PostConstruct) — past the constructor's final-field freeze — then read on every conversation turn. `volatile` makes that single write-at-init visible to the reader threads explicitly rather than relying on CDI singleton publication semantics (PR-review nitpick).
executeToolsParallel shares one ToolExecutionTrace across parallel
CompletableFuture tasks, but addToolCall/addFailedToolCall mutated a plain
ArrayList (+ counters + metrics map) without synchronization — concurrent
ArrayList.add corrupts the backing array and throws ArrayIndexOutOfBounds,
surfacing as a flaky ToolExecutionServiceBranchTest.executeMultipleInParallel
failure ("Index 1 out of bounds for length 0"). Synchronized both mutators.
Pre-existing race (the parallel machinery is otherwise unwired in production);
verified with 7 consecutive green runs of the parallel test.
aisabella-ai
approved these changes
Jul 20, 2026
This was referenced Jul 21, 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
Full enterprise pass over the multi-model cascading feature. A code review found that two documented capabilities didn't exist end-to-end (SSE events, judge model), the audit trail recorded the wrong model under cascade, and token/cost/metrics were discarded so the cost-savings pitch was unmeasurable. This PR lands all of it — plus two rounds of adversarial review and the resulting fixes.
Design spec:
planning/model-cascade-enterprise-hardening-plan.md. Docs rewritten:docs/model-cascade.md.What changed
Correctness / compliance
provider/model(was task-level default), plus cascade cost + token usage.{response,confidence}wrapper can't wrap a tool loop); agent-mode response now streamed to SSE as a chunk.jsonModeand forces a non-wrapper strategy.type+ params (parity with the standard path).Broken promises made real
cascade_step_start/cascade_escalationwired end-to-end (handler default →ConversationServicesink →RestAgentEngineStreaming).judgeModelconfig block built viaChatModelRegistry.strategy: parallelwarns; the false "budget" javadoc removed.Enterprise gaps
responseMetadata; Micrometer metrics undereddi.llm.cascade.*.maxTotalDurationMs+ dollarmaxCostPerRun(from configurable per-step pricing).returnBestAcrossSteps; lazy base-model creation.Extras requested during review
apiKey(and would inherit the wrong one) is flagged at deploy time + documented.Review & fixes
Two adversarial review passes (multi-lens reviewers → independent skeptics) surfaced real bugs that are all fixed with regression tests, notably:
returnBestAcrossStepsclobbering a live-streamed step; the validator hard-failing previously-loadable stored configs; unclamped heuristic scores; anunescapeJsonStringordering bug; judge-confidence and single-line-fence regressions.Two more latent bugs were caught while writing tests (stray-
"confidence"false positive; judge-null-params NPE).Backward compatibility
All new config fields are optional with today's behavior as defaults. Configs without
modelCascade, andenabled: false, are unaffected.StreamingResponseHandler's cascade methods aredefault. Stored-config load behavior is preserved (validator warns rather than hard-failing legacy conditions).Testing
New-code coverage ≈ 92% instruction / 79% branch (JaCoCo); the project aggregate gate (>90% / >80%) is unaffected. Full touched-area unit suite is green locally. The full
@QuarkusTestIT suite runs in CI.Summary by CodeRabbit
Summary of changes