Feat/error handling recovery - #593
Merged
Merged
Conversation
…ture - Shared RetryConfiguration with exponential backoff and retryable error classification - LifecycleManager: error classification, failure audit, SSE task_failed events, Micrometer counters - Admin PATCH endpoint to reset stuck conversations (DB + cache atomic update) - HTTP: error body storage for 4xx/5xx, softened JSON parsing - MCP: continueOnError, call-site retry, circuit breaker (3 failures/60s cooldown) - LLM: ResponseValidation config (onEmpty/onTruncation/onContentFilter/onRefusal/onStreamingTimeout) - Streaming: retry on zero-token failures, partial response metadata, configurable timeout - Conversation auto-recovery from EXECUTION_INTERRUPTED state - Code review fixes: JSON injection, cache invalidation, tightened string matching, LifecycleException - 83+ new tests across 7 test files (all passing)
Prevents checked exceptions from runPostResponse() from masking the original LifecycleException or defeating continueOnError when post-response processing fails during MCP call error handling.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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:
📝 WalkthroughWalkthroughThis PR adds shared retry and recovery infrastructure across LLM, MCP, API, streaming, lifecycle, and conversation-state flows. It adds response validation, circuit breaking, structured task-failure reporting, HTTP error persistence, an admin reset endpoint, tests, and changelog entries. ChangesError Handling and Recovery Infrastructure
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant LifecycleManager
participant ConversationService
participant RestAgentEngineStreaming
participant Client
LifecycleManager->>ConversationService: onTaskFailed(taskId, taskType, durationMs, errorType, errorSummary)
ConversationService->>RestAgentEngineStreaming: streamingHandler.onTaskFailed(...)
RestAgentEngineStreaming->>Client: task_failed SSE event
sequenceDiagram
participant LlmTask
participant StreamingLegacyChatExecutor
participant StreamingModel
LlmTask->>StreamingLegacyChatExecutor: execute(model, messages, eventSink, task)
StreamingLegacyChatExecutor->>StreamingModel: stream request
StreamingModel-->>StreamingLegacyChatExecutor: partial response, completion, or error
StreamingLegacyChatExecutor-->>LlmTask: StreamingResult(response, metadata)
LlmTask->>LlmTask: applyResponseValidation(response, metadata, task)
sequenceDiagram
participant Admin
participant RestAgentEngine
participant IConversationMemoryStore
participant ConversationService
Admin->>RestAgentEngine: PATCH /{conversationId}/state?state=READY
RestAgentEngine->>IConversationMemoryStore: load conversation snapshot
RestAgentEngine->>ConversationService: resetConversationState(conversationId, READY)
ConversationService-->>Admin: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Auto-recover from transient interrupted state | ||
| if (getConversationState() == ConversationState.EXECUTION_INTERRUPTED) { | ||
| LOGGER.infof("Auto-recovering conversation %s from EXECUTION_INTERRUPTED", | ||
| conversationMemory.getConversationId()); |
There was a problem hiding this comment.
Pull request overview
This PR strengthens EDDI’s resilience and observability by introducing shared retry utilities, richer error classification/reporting, MCP/LLM recovery behaviors, and an admin capability to recover stuck conversations—aimed at making pipeline execution more robust across LLM, HTTP, and MCP subsystems.
Changes:
- Added a shared
RetryConfiguration(and integrated it into LLM + MCP execution paths) plus expanded error classification/audit/SSE reporting inLifecycleManager. - Introduced LLM response validation policies and improved streaming execution metadata/timeout handling.
- Added an admin-only REST endpoint to reset stuck conversation state, and extended streaming/event interfaces to emit structured task-failure events.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java | New shared retry/backoff utility used across subsystems. |
| src/test/java/ai/labs/eddi/configs/shared/RetryConfigurationTest.java | Tests for shared retry/backoff behavior. |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds response validation + streaming timeout fields; extracts retry config to shared model while keeping compat wrapper. |
| src/test/java/ai/labs/eddi/modules/llm/model/ResponseValidationTest.java | Tests defaulting/serialization for new response-validation configuration. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentExecutionHelper.java | Switches LLM retry implementation to shared RetryConfiguration. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutor.java | Null-safety for AiMessage; adds finish-reason warnings into response metadata. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutorErrorHandlingTest.java | Tests finish-reason warnings and null-safety. |
| src/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.java | Adds task-aware streaming execution with timeout/retry and metadata capture. |
| src/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorRetryTest.java | Tests retry/timeout/metadata behavior for streaming legacy executor. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Applies response validation policies after model execution; ingests streaming metadata. |
| src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java | Adds circuit breaker tracking to reduce repeated MCP discovery failures. |
| src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java | Tests circuit breaker behavior for MCP tool discovery. |
| src/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCall.java | Adds per-call continueOnError and optional retry config. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.java | Tests new MCP call fields and JSON round-trips. |
| src/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.java | Adds per-call retry and continueOnError handling for MCP tool execution. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java | Stores non-2xx error body/code in memory; softens JSON parsing when content-type is JSON but body isn’t. |
| src/main/java/ai/labs/eddi/engine/lifecycle/ConversationEventSink.java | Extends sink with onTaskFailed callback for structured failure monitoring. |
| src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java | Adds error classification + audit summary + SSE task-failure emission + metrics tags. |
| src/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerErrorClassificationTest.java | Tests classification + audit summary behavior. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Passes through new onTaskFailed streaming callback; adds resetConversationState. |
| src/main/java/ai/labs/eddi/engine/api/IConversationService.java | Adds resetConversationState API + default streaming callback. |
| src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java | Adds admin-only PATCH /{conversationId}/state API contract. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Implements admin state reset endpoint and wires in conversation memory snapshot loading. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Updates constructor wiring for new RestAgentEngine dependency. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Emits task_failed SSE events with structured error metadata. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java | Adds automatic recovery from EXECUTION_INTERRUPTED to READY at step start. |
| docs/changelog.md | Documents the error-handling/recovery overhaul and design decisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 4
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java (1)
169-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider appending a truncation indicator to the error body.
When the error body exceeds 2000 chars, the truncated string is stored without any marker. Downstream templates or rules inspecting the error body cannot distinguish a truncated response from a complete one. Appending a simple suffix (e.g.,
"…[truncated]") would make truncation visible to consumers.This is a minor usability improvement and can be deferred if not a priority.
♻️ Optional: add truncation indicator
- if (errorBody.length() > 2000 - ? errorBody.substring(0, 2000) - : errorBody; + String truncatedError = errorBody.length() > 2000 + ? errorBody.substring(0, 2000) + "…[truncated]" + : errorBody;🤖 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/apicalls/impl/ApiCallExecutor.java` around lines 169 - 175, The fallback path in ApiCallExecutor around responseBody handling currently stores a truncated error body without any visible marker, so consumers can’t tell it was shortened. Update the truncation logic in this JSON-deserialization fallback flow to append a clear suffix like “…[truncated]” whenever the body is cut off, and keep the behavior localized to the responseObject assignment used after the deserialize attempt.src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java (1)
20-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfiguration POJO should use Java records per coding guidelines.
RetryConfiguration.javamatches the**/*Configuration.javapattern, and the coding guidelines state: "New feature configuration classes must use Java records for configuration POJOs." The current mutable class with getters/setters violates this guideline.Consider splitting into a
RetryConfigrecord for the configuration fields and a separate utility class (e.g.,RetryExecutor) for the static methods (executeWithRetry,backoff,isRetryableError). Jackson deserialization of records is fully supported via the canonical constructor.As per coding guidelines: "New feature configuration classes must use Java records for configuration POJOs."
♻️ Proposed refactor: split configuration and utility
// RetryConfig.java — configuration record public record RetryConfig( Integer maxAttempts, Long backoffDelayMs, Double backoffMultiplier, Long maxBackoffDelayMs ) { public RetryConfig { if (maxAttempts == null) maxAttempts = 3; if (backoffDelayMs == null) backoffDelayMs = 1000L; if (backoffMultiplier == null) backoffMultiplier = 2.0; if (maxBackoffDelayMs == null) maxBackoffDelayMs = 10000L; } } // RetryExecutor.java — static utility methods public final class RetryExecutor { public static <T> T executeWithRetry(Callable<T> action, RetryConfig config, String desc) { ... } public static void backoff(int attempt, RetryConfig config) { ... } public static boolean isRetryableError(Exception e) { ... } }🤖 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/configs/shared/RetryConfiguration.java` around lines 20 - 58, The RetryConfiguration POJO should be converted to a Java record to match the configuration coding guideline, since the current mutable class with getters/setters violates the record requirement. Replace the mutable configuration fields in RetryConfiguration with an immutable record form (or rename to RetryConfig) and keep only the config data there, while moving the retry logic methods such as executeWithRetry, backoff, and isRetryableError into a separate utility class like RetryExecutor. Make sure any existing uses of RetryConfiguration are updated to the new record type and that default values remain preserved through the record canonical constructor.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java (1)
570-667: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew Response Validation feature has no Micrometer metrics.
The validation actions (
warn/fallback/errorper signal) are a good candidate for counters (e.g., validation triggers by type/action), useful for tracking how often responses are being modified or rejected in production.As per path instructions, "Use Micrometer
MeterRegistrymetrics for new features, initializing counters/timers in@PostConstruct."🤖 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/LlmTask.java` around lines 570 - 667, The new response validation flow in applyResponseValidation/applyValidationAction has no Micrometer instrumentation yet. Add MeterRegistry-based counters for each validation signal/action pair (for example, empty/truncated/content_filter/streaming_timeout/refusal and warn/fallback/error) so triggers are tracked in production. Initialize the meters in a `@PostConstruct` method on LlmTask, wire in MeterRegistry via the class constructor or injection, and increment the appropriate counter inside applyValidationAction before storing data, returning fallback content, or throwing LifecycleException.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/StreamingLegacyChatExecutor.java`:
- Around line 90-96: The retry flow in StreamingLegacyChatExecutor is blocking
the caller by waiting on CountDownLatch and sleeping in
RetryConfiguration.backoff across multiple attempts. Refactor the attempt loop
and related timeout/error handling in the streaming execution path to run
asynchronously so the calling thread is not held for long periods; use the
existing StreamingLegacyChatExecutor methods and retry/backoff logic to complete
work off-thread and signal results without blocking, and update the
retry-related blocks around the latch await and backoff calls accordingly.
- Around line 95-186: The retry loop in StreamingLegacyChatExecutor leaves
timed-out or errored streaming attempts alive, so stale tokens can keep reaching
eventSink during the next attempt. Update the streaming flow around
streamingModel.chat and the StreamingChatResponseHandler callbacks to use a
cancelable context/handle or an active-attempt guard, and ensure the previous
attempt is cancelled or ignored before any continue to the next retry.
In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java`:
- Around line 671-759: The new ResponseValidation configuration in
LlmConfiguration should be converted from a mutable bean into a Java record to
match the configuration POJO guideline. Update the ResponseValidation type
itself (including its default values and accessors), then adjust any Jackson
binding or deserialization assumptions and update ResponseValidationTest to
construct and verify the record instead of using setters. Keep the existing
field names and semantics so callers can still configure enabled, onEmpty,
onTruncation, onContentFilter, onRefusal, and onStreamingTimeout.
In
`@src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java`:
- Around line 122-138: Strengthen the circuit-breaker test in
circuitClosed_attemptsConnection by asserting a recorded failure after
discoverTools(List.of(config)) runs, not just a non-null result. Use the
McpToolProviderManager instance to verify the circuit state or failure count
changed as expected so the test proves the connection attempt actually happened
instead of being skipped by an always-open breaker.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java`:
- Around line 20-58: The RetryConfiguration POJO should be converted to a Java
record to match the configuration coding guideline, since the current mutable
class with getters/setters violates the record requirement. Replace the mutable
configuration fields in RetryConfiguration with an immutable record form (or
rename to RetryConfig) and keep only the config data there, while moving the
retry logic methods such as executeWithRetry, backoff, and isRetryableError into
a separate utility class like RetryExecutor. Make sure any existing uses of
RetryConfiguration are updated to the new record type and that default values
remain preserved through the record canonical constructor.
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java`:
- Around line 169-175: The fallback path in ApiCallExecutor around responseBody
handling currently stores a truncated error body without any visible marker, so
consumers can’t tell it was shortened. Update the truncation logic in this
JSON-deserialization fallback flow to append a clear suffix like “…[truncated]”
whenever the body is cut off, and keep the behavior localized to the
responseObject assignment used after the deserialize attempt.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java`:
- Around line 570-667: The new response validation flow in
applyResponseValidation/applyValidationAction has no Micrometer instrumentation
yet. Add MeterRegistry-based counters for each validation signal/action pair
(for example, empty/truncated/content_filter/streaming_timeout/refusal and
warn/fallback/error) so triggers are tracked in production. Initialize the
meters in a `@PostConstruct` method on LlmTask, wire in MeterRegistry via the
class constructor or injection, and increment the appropriate counter inside
applyValidationAction before storing data, returning fallback content, or
throwing LifecycleException.
🪄 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: c600bc68-357b-4451-8fd5-1dc75f137356
📒 Files selected for processing (27)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCall.javasrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/engine/lifecycle/ConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentExecutionHelper.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.javasrc/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.javasrc/test/java/ai/labs/eddi/configs/shared/RetryConfigurationTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerErrorClassificationTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutorErrorHandlingTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorRetryTest.javasrc/test/java/ai/labs/eddi/modules/llm/model/ResponseValidationTest.java
…ration The nested LlmConfiguration.RetryConfiguration was an empty subclass of the extracted ai.labs.eddi.configs.shared.RetryConfiguration, added as a backward-compat shim. It overrode nothing and shadowed the imported shared type within LlmConfiguration's body, triggering the "class has same name as super class" code-quality finding on PR #593. Delete the shim and point the retry field/getter/setter plus all tests at the shared class directly. The subclass added no fields, so the retry JSON structure is byte-for-byte identical and existing stored configs deserialize unchanged. Verified with a clean test-compile and 123 passing unit tests.
Integrates 180 commits from main — the HITL (human-in-the-loop) framework, multimodal-attachments completion, and related work — into the holistic error-handling/recovery branch. Conflicts resolved as unions preserving BOTH feature sets: - IConversationService / IRestAgentEngine: kept error-handling's resetConversationState + PATCH /state endpoint alongside main's HITL endConversation(id, endedBy), cancel/resume/approval endpoints, and onSkipped. - AgentExecutionHelper: kept HEAD's one-liner delegating to the shared RetryConfiguration.executeWithRetry; main's inline retry loop (which referenced the now-removed nested RetryConfiguration and duplicated the shared util) was dropped. Its HITL ToolApprovalRequiredException pass-through was relocated into the shared RetryConfiguration so both LLM and MCP retries still let a tool-approval pause propagate intact. - LlmConfiguration / LlmTask: unioned response-validation (HEAD) with multimodal + tool-approval config and the HITL resume path (main). - RestAgentEngine constructor: unioned to take conversationMemoryStore (HEAD) plus hitlAccessGuard/hitlToolJournalStore (main); updated all affected tests. - Five of main's test files updated for the removed nested RetryConfiguration and the new constructor arity. Verified: clean compile (main + tests); 437 affected unit tests pass; adversarial multi-agent review of all 8 conflict resolutions plus the 3 silent auto-merges (Conversation, ConversationService, LifecycleManager) found no dropped or corrupted code.
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 (4)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (4)
379-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog SSE callback failures with conversation context.
The current message drops the throwable, conversation ID, and task ID, making callback failures difficult to diagnose.
As per coding guidelines, JBoss Logger entries must include conversation context and use appropriate log levels.
🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 379 - 380, Update the SSE callback failure handler around the catch block in LifecycleManager to log the full throwable rather than only its message, and include the conversation ID and task ID in the structured log context. Use the appropriate warning/error level for a callback failure while preserving the existing exception handling flow.Source: Coding guidelines
384-400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let audit collection bypass strict-write recovery.
If
auditCollector.collect()throws, Lines 403-408 never run, partial task writes remain, and the audit exception masks the original failure. Apply strict-write recovery first, then shield audit collection while attaching any reporting failure as suppressed.As per coding guidelines, recoverable exceptions must not kill the pipeline.
🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 384 - 400, The failure path must execute strict-write recovery before attempting audit reporting. In the block around the failure AuditEntry and auditCollector.collect, move or invoke the existing recovery logic first, then wrap auditCollector.collect in guarded recoverable-exception handling; attach any audit exception as suppressed to the original task failure without replacing it, and ensure the pipeline continues through the existing recovery flow.Source: Coding guidelines
834-840: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact exception messages before persisting or streaming them.
Truncation does not make arbitrary exception text safe. This value is written to audit storage and emitted through
task_failedSSE, potentially exposing URLs, payloads, credentials, or PII. Apply a shared redactor before returning it.🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 834 - 840, Update summarizeForAudit(Throwable e) to pass the selected exception message or class-name fallback through the project’s shared redactor before truncation and return. Preserve the existing 500-character limit, and ensure the redacted value is used for both audit persistence and task_failed SSE output.
802-823: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive typed inner causes precedence over wrapper messages.
Each outer message is checked before its cause. For example, a
"429"wrapper aroundSocketTimeoutExceptionis classified asrate_limit, nottimeout. Scan the entire chain for typed causes before applying message heuristics.🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 802 - 823, Update classifyError to scan the full Throwable cause chain for typed timeout or transport exceptions before evaluating any error-message heuristics. Preserve the existing message classifications for rate_limit and content_filter, but only apply them after no higher-priority typed cause is found.
🤖 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/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 534-538: Update getLifecycleStartIndex to guard both nullable
values before prefix matching: skip lifecycle tasks whose task.getType() is null
and ignore null entries in lifecycleTaskTypes. Preserve the existing
startsWith-based matching and return the first matching task index.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 379-380: Update the SSE callback failure handler around the catch
block in LifecycleManager to log the full throwable rather than only its
message, and include the conversation ID and task ID in the structured log
context. Use the appropriate warning/error level for a callback failure while
preserving the existing exception handling flow.
- Around line 384-400: The failure path must execute strict-write recovery
before attempting audit reporting. In the block around the failure AuditEntry
and auditCollector.collect, move or invoke the existing recovery logic first,
then wrap auditCollector.collect in guarded recoverable-exception handling;
attach any audit exception as suppressed to the original task failure without
replacing it, and ensure the pipeline continues through the existing recovery
flow.
- Around line 834-840: Update summarizeForAudit(Throwable e) to pass the
selected exception message or class-name fallback through the project’s shared
redactor before truncation and return. Preserve the existing 500-character
limit, and ensure the redacted value is used for both audit persistence and
task_failed SSE output.
- Around line 802-823: Update classifyError to scan the full Throwable cause
chain for typed timeout or transport exceptions before evaluating any
error-message heuristics. Preserve the existing message classifications for
rate_limit and content_filter, but only apply them after no higher-priority
typed cause is found.
🪄 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: eaa7c192-5fbf-473e-9247-344608980c19
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolPauseTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java
- src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java
- src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- docs/changelog.md
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (4)
379-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog SSE callback failures with conversation context.
The current message drops the throwable, conversation ID, and task ID, making callback failures difficult to diagnose.
As per coding guidelines, JBoss Logger entries must include conversation context and use appropriate log levels.
🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 379 - 380, Update the SSE callback failure handler around the catch block in LifecycleManager to log the full throwable rather than only its message, and include the conversation ID and task ID in the structured log context. Use the appropriate warning/error level for a callback failure while preserving the existing exception handling flow.Source: Coding guidelines
384-400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let audit collection bypass strict-write recovery.
If
auditCollector.collect()throws, Lines 403-408 never run, partial task writes remain, and the audit exception masks the original failure. Apply strict-write recovery first, then shield audit collection while attaching any reporting failure as suppressed.As per coding guidelines, recoverable exceptions must not kill the pipeline.
🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 384 - 400, The failure path must execute strict-write recovery before attempting audit reporting. In the block around the failure AuditEntry and auditCollector.collect, move or invoke the existing recovery logic first, then wrap auditCollector.collect in guarded recoverable-exception handling; attach any audit exception as suppressed to the original task failure without replacing it, and ensure the pipeline continues through the existing recovery flow.Source: Coding guidelines
834-840: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact exception messages before persisting or streaming them.
Truncation does not make arbitrary exception text safe. This value is written to audit storage and emitted through
task_failedSSE, potentially exposing URLs, payloads, credentials, or PII. Apply a shared redactor before returning it.🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 834 - 840, Update summarizeForAudit(Throwable e) to pass the selected exception message or class-name fallback through the project’s shared redactor before truncation and return. Preserve the existing 500-character limit, and ensure the redacted value is used for both audit persistence and task_failed SSE output.
802-823: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive typed inner causes precedence over wrapper messages.
Each outer message is checked before its cause. For example, a
"429"wrapper aroundSocketTimeoutExceptionis classified asrate_limit, nottimeout. Scan the entire chain for typed causes before applying message heuristics.🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 802 - 823, Update classifyError to scan the full Throwable cause chain for typed timeout or transport exceptions before evaluating any error-message heuristics. Preserve the existing message classifications for rate_limit and content_filter, but only apply them after no higher-priority typed cause is found.
🤖 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/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 534-538: Update getLifecycleStartIndex to guard both nullable
values before prefix matching: skip lifecycle tasks whose task.getType() is null
and ignore null entries in lifecycleTaskTypes. Preserve the existing
startsWith-based matching and return the first matching task index.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 379-380: Update the SSE callback failure handler around the catch
block in LifecycleManager to log the full throwable rather than only its
message, and include the conversation ID and task ID in the structured log
context. Use the appropriate warning/error level for a callback failure while
preserving the existing exception handling flow.
- Around line 384-400: The failure path must execute strict-write recovery
before attempting audit reporting. In the block around the failure AuditEntry
and auditCollector.collect, move or invoke the existing recovery logic first,
then wrap auditCollector.collect in guarded recoverable-exception handling;
attach any audit exception as suppressed to the original task failure without
replacing it, and ensure the pipeline continues through the existing recovery
flow.
- Around line 834-840: Update summarizeForAudit(Throwable e) to pass the
selected exception message or class-name fallback through the project’s shared
redactor before truncation and return. Preserve the existing 500-character
limit, and ensure the redacted value is used for both audit persistence and
task_failed SSE output.
- Around line 802-823: Update classifyError to scan the full Throwable cause
chain for typed timeout or transport exceptions before evaluating any
error-message heuristics. Preserve the existing message classifications for
rate_limit and content_filter, but only apply them after no higher-priority
typed cause is found.
🪄 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: eaa7c192-5fbf-473e-9247-344608980c19
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolPauseTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java
- src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java
- src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- docs/changelog.md
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java
🛑 Comments failed to post (1)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (1)
534-538: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard nullable task types before prefix matching.
Elsewhere
task.getType()is explicitly treated as nullable, but this dereference crashes selective execution. Null-filter requested types as well.Proposed fix
ILifecycleTask task = this.lifecycleTasks.get(i); -if (lifecycleTaskTypes.stream().anyMatch(type -> task.getType().startsWith(type))) { +String taskType = task.getType(); +if (taskType != null && lifecycleTaskTypes.stream() + .filter(Objects::nonNull) + .anyMatch(taskType::startsWith)) { return i; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private int getLifecycleStartIndex(List<String> lifecycleTaskTypes) { for (int i = 0; i < this.lifecycleTasks.size(); i++) { ILifecycleTask task = this.lifecycleTasks.get(i); String taskType = task.getType(); if (taskType != null && lifecycleTaskTypes.stream() .filter(Objects::nonNull) .anyMatch(taskType::startsWith)) { return i;🤖 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/engine/lifecycle/internal/LifecycleManager.java` around lines 534 - 538, Update getLifecycleStartIndex to guard both nullable values before prefix matching: skip lifecycle tasks whose task.getType() is null and ignore null entries in lifecycleTaskTypes. Preserve the existing startsWith-based matching and return the first matching task index.
Both failures pre-date the origin/main merge — CI was already red on c054b43 ("Tests run: 9776, Failures: 1, Errors: 1") with these exact two tests. Each asserts a contract this PR deliberately superseded, so the fix is in the tests; no production behavior changes. StreamingLegacyChatExecutorTest.execute_error_throwsRuntimeException asserted that a streaming error always throws. The PR intentionally changed this: an error after partial tokens returns the partial text with a streaming_error_partial warning, and only a zero-content error throws. Retarget the test at the zero-content case (matching its name) and add execute_errorAfterPartial_returnsPartialContent for the partial contract, keeping the original token-forwarding assertion. ConversationExtendedTest.saySucceeds stubbed getConversationState() with a call-count-sensitive consecutive-return sequence (READY, then IN_PROGRESS). The PR's EXECUTION_INTERRUPTED auto-recovery added a state read at the top of runStep, which consumed the READY, so the in-progress guard saw IN_PROGRESS and threw. The mock now tracks state like real memory, making it robust to how often production reads it.
Four findings on this PR's own failure path, all verified against the code. Each fix reuses existing infrastructure rather than adding a new utility. Audit must not bypass strict-write recovery (Major): if auditCollector.collect() threw, the strict-write rollback was skipped — leaving the partial task writes it exists to remove — and the audit exception propagated out of the catch, replacing the original task failure. Strict-write recovery now runs first, and audit collection is shielded, attaching any reporting error to the original exception via addSuppressed instead of masking it. Redact credentials from audit/SSE summaries (Major): summarizeForAudit() only truncated, yet its output is persisted to the audit ledger and streamed to admins over task_failed SSE. It now applies SecretRedactionFilter.redact() before truncating — cutting first can split a secret so the pattern no longer matches. URLs and class names stay: the audience is privileged and needs them. Typed causes outrank wrapper messages (Minor): classifyError() checked each level's message before descending, so a "429" wrapper around a SocketTimeoutException classified as rate_limit. It now scans the chain for typed causes first, falling back to message heuristics only when none match. SSE failure logging (Minor): the task_failed emission catch logged at DEBUG and dropped the throwable and all context. Now WARN with the throwable, sanitized conversation id (LogSanitizer, CWE-117) and task id. Adds four regression tests, each failing under the previous behavior.
aisabella-ai
approved these changes
Jul 20, 2026
…conflicts Brings 43 commits from main (group conversations, MCP/REST ownership hardening, multi-model cascade enterprise work) into the error-handling branch. Conflicts resolved (7): - RestAgentEngine + its three tests: main moved the descriptor-based ownership check into ConversationAccessGuard (removing the last conversationDescriptorStore usage); ours added IConversationMemoryStore for the new admin resetState() endpoint. Kept the memory store, dropped the descriptor store. - CascadingModelExecutorCoverageTest: resolved onto the shared ai.labs.eddi.configs.shared.RetryConfiguration. - StreamingLegacyChatExecutor: ours added a retry loop and configurable timeout, main added executeCapturing() capturing full ChatResponse metadata for cascade cost evidence. Unified on one core implementation that does both; both public entry points retained. - docs/changelog.md: both entry blocks kept whole. Beyond the reported conflicts: - Repaired two files broken by a silent auto-merge: main added new LlmConfiguration.RetryConfiguration usages while ours deleted that shim, so the merge compiled on neither side (AgentOrchestratorExtendedTest, CascadingModelExecutorEnterpriseTest). - executeCapturing now always propagates a mid-stream error instead of salvaging partial text. CascadingModelExecutor has no try/catch there and relies on the throw to fall back to the best previous step, so salvaging caused a failed final step to be accepted as successful. Verified: clean test-compile green; full unit suite 11,409 tests with no assertion failures (remaining failures/errors are all sandbox loopback-socket and Docker/Testcontainers limitations, verified per-message).
…Executor
Both pre-date the merge — they came in with this branch's retry loop — but
both sit inside the method the merge rewrote and are squarely in this PR's
subject area, so they are fixed here rather than deferred.
1. A failed attempt's metadata leaked into a successful retry. The metadata
map is declared outside the retry loop; when attempt 1 timed out with no
tokens it set streamingTimeout=true, and a successful attempt 2 returned
carrying that stale flag. LlmTask.applyResponseValidation acts on it and
fires responseValidation.onStreamingTimeout, so a good answer was either
silently replaced by the timeout fallback string or turned into a thrown
turn. The loop now clears the map at the start of each attempt.
2. maxAttempts <= 0 skipped the model entirely. RetryConfiguration does no
clamping, so retry: {maxAttempts: 0} — a natural way to write "no retries"
— made the loop body never execute: the model was never invoked and a null
response propagated into a TextOutputItem(null, 0) with no exception and
no log. Clamped with Math.max(1, ...).
Not clamped in RetryConfiguration.setMaxAttempts itself: the shared
executeWithRetry already fails loudly at 0 (throws LifecycleException), and
changing the setter would silently alter that contract for the MCP and
HTTP-call consumers and their tests. The clamp belongs at the call site that
was failing silently.
Tests: three regression tests, each verified to fail before the fix and pass
after. A fourth pins executeCapturing's throw-on-partial-token contract at
the executor level, which until now was guarded only by a cascade-level test.
155 tests green across the streaming, cascade, orchestrator and LlmTask suites.
Also fixes an import-ordering slip introduced by the merge resolution in
AgentOrchestratorExtendedTest.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/StreamingLegacyChatExecutor.java (1)
176-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInterrupted streaming attempt is silently returned as a success.
When
latch.await(...)throwsInterruptedException(176-185), neithertimedOutnorerrorRefis set. Execution then falls through both theif (timedOut)andif (errorRef.get() != null)checks and reachesbreak→ returnsnew StreamingResult(responseText, metadata)as if the attempt completed normally — even ifresponseTextis empty and noChatResponsewas ever received. Callers (e.g.LlmTask, the cascade) have no signal that this "successful" result is actually the product of an interrupted/aborted attempt.🐛 Proposed fix — treat interruption as a distinct failure signal
boolean timedOut = false; + boolean interrupted = false; try { if (!latch.await(timeoutSeconds, TimeUnit.SECONDS)) { LOGGER.warn("Streaming chat timed out"); timedOut = true; } } catch (InterruptedException e) { Thread.currentThread().interrupt(); LOGGER.warn("Streaming chat was interrupted"); + interrupted = true; } responseText = fullResponse.toString(); metadata.putAll(buildMetadata(responseRef.get())); + if (interrupted) { + metadata.put("streamingInterrupted", true); + return new StreamingResult(responseText, metadata); + } + if (timedOut) {🤖 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/StreamingLegacyChatExecutor.java` around lines 176 - 230, Update the interruption handling around latch.await in the streaming retry flow to record interruption as a distinct failure outcome instead of falling through to the success break. Preserve the interrupt flag, propagate or retry the interruption consistently with other failed attempts, and ensure an interrupted empty attempt cannot return a successful StreamingResult. Anchor the change to the timedOut/errorRef checks and the retry loop in StreamingLegacyChatExecutor.
🤖 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/changelog.md`:
- Line 90: Reconcile the full-suite summary in the changelog entry by rechecking
the Surefire aggregation and correcting the reported failures, errors, and
category counts so the totals are mathematically consistent. Update the
surrounding test-count statement if necessary, while preserving the explanation
that these are sandbox infrastructure issues rather than assertion failures.
In
`@src/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerTest.java`:
- Line 438: In LifecycleManagerTest, add a top-level import for
IAuditEntryCollector and replace the fully qualified type in the auditCollector
mock declaration with the simple class name.
---
Outside diff comments:
In
`@src/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.java`:
- Around line 176-230: Update the interruption handling around latch.await in
the streaming retry flow to record interruption as a distinct failure outcome
instead of falling through to the success break. Preserve the interrupt flag,
propagate or retry the interruption consistently with other failed attempts, and
ensure an interrupted empty attempt cannot return a successful StreamingResult.
Anchor the change to the timedOut/errorRef checks and the retry loop in
StreamingLegacyChatExecutor.
🪄 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: 9f0df1a5-6050-4a39-b276-2ca07dc50d05
📒 Files selected for processing (21)
docs/changelog.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/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.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/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerErrorClassificationTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.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/CascadingModelExecutorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverage2Test.javasrc/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorRetryTest.java
🚧 Files skipped from review as they are similar to previous changes (11)
- src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorTest.java
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.java
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/test/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutorExecuteTest.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java
|
|
||
| `mvnw clean test-compile` green — a *clean* build deliberately, since the `RestAgentEngine` signature change would be masked by a stale incremental one. | ||
|
|
||
| Full unit suite: **11,409 tests run, no assertion failures.** The 8 reported failures and 304 errors were each inspected via the surefire XML and categorised by message: 297 `Unable to establish loopback connection`, 16 Docker/Testcontainers unavailable (Mongo + Postgres store tests), 5 socket-selector/event-loop creation errors. All are this sandbox's inability to open loopback sockets or run Docker — none is a code assertion. CI remains the source of truth for those suites. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the full-suite failure totals.
The entry reports 8 failures + 304 errors (312 total), but the listed categories sum to 318 (297 + 16 + 5). Please recheck the Surefire aggregation and make the totals consistent.
🤖 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` at line 90, Reconcile the full-suite summary in the
changelog entry by rechecking the Surefire aggregation and correcting the
reported failures, errors, and category counts so the totals are mathematically
consistent. Update the surrounding test-count statement if necessary, while
preserving the explanation that these are sandbox infrastructure issues rather
than assertion failures.
…n as failure Clears the four follow-ups recorded after the merge, plus three CodeRabbit review comments. Each was pre-existing; together they meant responseValidation — a headline feature of this PR — silently did nothing on the paths it is most likely to be configured for. Every change is TDD'd: test written first and observed failing. - Streaming now derives `warning` from finishReason (LENGTH -> truncated, CONTENT_FILTER -> content_filter), mirroring LegacyChatExecutor. Previously onTruncation/onContentFilter could never fire on a streaming task. - CascadeResult and the internal StepResult now carry responseMetadata, so the winning step's warning/streamingTimeout/finishReason reach LlmTask. With modelCascade and responseValidation both enabled, only onEmpty and onRefusal could fire before this; a truncated or filtered answer reached the user even with action "error". - A live-streamed step that hits its timeout is treated as failed rather than as a candidate: executeCapturing returns partial text instead of throwing, so such a step was accepted on equal footing with a real answer and, being last, beat a good earlier one. It no longer becomes bestSoFar, escalates when not last, and falls back to bestSoFar when last. This required executeCapturing to honour streamingTimeoutSeconds, which it ignored by passing task = null. Retry is still deliberately not applied on the cascade path — escalation is the cascade's retry, and stacking both multiplies spend. - An interrupted streaming attempt no longer returns an empty success. Neither timedOut nor errorRef was set on InterruptedException, so a cancelled call was reported as a completed answer — contradicting AgentOrchestrator, which treats a set interrupt flag as a hard abort. It now records streamingInterrupted, salvages partial text with a warning on the LlmTask path, and otherwise throws. It is never retried; retrying would ignore the cancellation. - Cascade failures are diagnosable: the retry wrapper throws a generic "failed after N attempts", and the cascade recorded only e.getMessage(), so a fully failed cascade read identically for a rate limit, an auth failure and a network outage. describeFailure() appends the root cause via a bounded chain walk. - New CascadingModelExecutorErrorPathTest restores coverage lost when CascadingModelExecutorExtendedTest was deleted and CascadingModelExecutorCoverageTest was rewritten on main: last-step timeout with and without a bestSoFar, timeout escalation events, exception-cause handling, aggregated all-steps-failed errors, and enableInAgentMode=false never consulting the orchestrator. - Review nits: replaced eight inline fully-qualified names in LifecycleManagerTest with top-level imports, and corrected a changelog verification claim whose per-bucket counts were measured over a surefire directory that still held XML from earlier targeted runs. Verified on a clean run of the final code: 11,658 tests, 8 failures, 287 errors, zero assertion failures — every failing case carries a blocked-loopback or socket-selector message from this sandbox. CI remains the authority for the socket- and Docker-dependent suites.
…ecutorErrorPathTest The executor(...) helper took an ITemplatingEngineStub it never used, and that marker interface existed only to make the argument look deliberate — a readability device introduced in the previous commit that conveyed nothing. Removed both; the seven call sites now read executor(registry). Flagged by github-code-quality on PR #593. No behaviour change; the 7 tests in the class still pass.
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
This pull request delivers a comprehensive overhaul of error handling and recovery infrastructure across the EDDI platform. The changes introduce a shared, configurable retry mechanism, robust error classification and reporting, enhanced admin recovery tools, and improved monitoring hooks. These updates span multiple subsystems (LLM, HTTP, MCP), enabling more resilient and observable operations, and providing administrators with better tools to recover from failures.
Error Handling & Retry Infrastructure
RetryConfigurationclass for consistent, exponential-backoff retry logic across all subsystems (LLM, MCP, etc.), including static utility methods for executing actions with retries and detecting retryable errors.McpCallmodel, includingcontinueOnErrorand a configurableretryfield. This allows MCP calls to be retried on transient failures and to optionally not abort the pipeline on error. [1] [2] [3]Admin Recovery & State Management
PATCH /{conversationId}/stateto reset the state of stuck conversations (ERROR or EXECUTION_INTERRUPTED) back to READY, with appropriate validation and error handling. [1] [2] [3] [4] [5]IConversationServiceinterface and implementation to support atomic conversation state resets, affecting both persistent and in-memory state. [1] [2]Monitoring & Observability
ConversationEventSink, streaming handlers) to include structuredonTaskFailedcallbacks, enabling real-time admin monitoring of failed tasks with detailed error metadata. [1] [2] [3] [4]Documentation
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
task_failedevents witherrorTypeand a safe, truncatederrorSummary.continueOnErrorand per-call retry settings.