feat(groups): follow-up, continuation, and close for group conversations - #595
Merged
Conversation
Add three new operations to the group conversation lifecycle:
1. Follow-up with member (POST /{gcId}/followup)
- Send a follow-up question to any specific member agent
- Agent retains full context from prior discussion rounds
- Accepts agent ID or display name (case-insensitive)
- Question + response recorded on group transcript
- State: COMPLETED -> IN_PROGRESS -> COMPLETED
2. Continue discussion (POST /{gcId}/continue + /continue/stream)
- Re-run all discussion phases with a new question
- All agents retain memory via reused private conversations
- Round counter increments, SSE emits round_start event
- State: COMPLETED -> IN_PROGRESS -> COMPLETED
3. Close conversation (POST /{gcId}/close)
- Ends all member conversations permanently
- Cleans up ephemeral agents
- State: COMPLETED|FAILED -> CLOSED (terminal)
Client experience:
- All endpoints return full GroupConversation (consistent shapes)
- memberDisplayNames map (agentId -> displayName) on every response
- Computed availableActions property based on state (READ_ONLY)
- Lifecycle documented in OpenAPI descriptions
Concurrency & error recovery:
- All state transitions use compareAndSetState (CAS)
- followUpWithMember: finally block restores COMPLETED on any failure
- continueDiscussion: failConversation() on pre-execution errors
- closeGroupConversation: CAS for COMPLETED->CLOSED, then FAILED->CLOSED
Files: GroupConversation model, IGroupConversationStore (CAS),
GroupConversationStore (CAS impl), IGroupConversationService,
GroupConversationService, IRestGroupConversation, RestGroupConversation,
GroupConversationEventSink (RoundStartEvent), McpGroupTools
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:
📝 WalkthroughWalkthroughAdds group conversation follow-up, continuation, and closure capabilities with persisted round state, atomic transitions, authorization, concurrency guards, REST/SSE and MCP integrations, cleanup handling, and regression coverage. ChangesGroup Conversation Follow-Up/Continue/Close
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RestGroupConversation
participant GroupConversationService
participant GroupConversationStore
Client->>RestGroupConversation: submit follow-up, continue, or close
RestGroupConversation->>GroupConversationService: validate and invoke operation
GroupConversationService->>GroupConversationStore: compareAndSetState
GroupConversationService-->>RestGroupConversation: updated conversation or SSE events
RestGroupConversation-->>Client: HTTP response or streamed event
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds post-discussion workflow support for group conversations (follow-ups to individual members, multi-round continuation, and explicit close), extending the model/state machine, service layer, REST API (including SSE), and MCP tools to support richer client interactions.
Changes:
- Introduces follow-up, continuation (sync + SSE), and close operations for group conversations, with a new terminal
CLOSEDstate and round tracking. - Adds client-discoverability improvements (
memberDisplayNames, computedavailableActions) and an SSEround_startevent for continuation rounds. - Updates REST + MCP surfaces to expose the new operations and return full
GroupConversationobjects consistently.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.java | Adds MCP tools for member follow-up, group continuation, and explicit close. |
| src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java | Adds round_start SSE event + payload record. |
| src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java | Adds REST endpoints for follow-up/continue/continue-stream/close. |
| src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java | Implements follow-up/continue/close workflows; defers ephemeral cleanup until explicit close. |
| src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java | Defines new REST endpoints + request DTO for follow-up. |
| src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java | Extends service contract for follow-up/continue/close + round-start listener callback. |
| src/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.java | Adds compareAndSetState() implementation used for optimistic state transitions. |
| src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java | Adds round, memberDisplayNames, FOLLOW_UP transcript entry type, CLOSED state, and availableActions. |
| src/main/java/ai/labs/eddi/configs/groups/IGroupConversationStore.java | Adds compareAndSetState() store API. |
| docs/changelog.md | Documents the feature set and design decisions for the new group conversation workflows. |
💡 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/engine/internal/RestGroupConversation.java (1)
251-308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared SSE listener factory to eliminate duplication with
discussStreaming.The listener in
continueDiscussionStreaming(lines 251–308) is nearly identical to the one indiscussStreaming(lines 84–136), differing only by theonRoundStartoverride. A shared factory method accepting an optionalonRoundStarthandler would remove ~50 lines of duplication and ensure both stay in sync when new event types are added.♻️ Proposed refactor
+ private GroupDiscussionEventListener createSseListener(SseEventSink eventSink, Sse sse, + java.util.function.Consumer<GroupConversationEventSink.RoundStartEvent> roundStartHandler) { + return new GroupDiscussionEventListener() { + `@Override` + public void onRoundStart(GroupConversationEventSink.RoundStartEvent event) { + if (roundStartHandler != null) roundStartHandler.accept(event); + } + // ... all other overrides identical to existing code ... + }; + } + // In discussStreaming: - GroupDiscussionEventListener listener = new GroupDiscussionEventListener() { ... }; + GroupDiscussionEventListener listener = createSseListener(eventSink, sse, null); + // In continueDiscussionStreaming: - GroupDiscussionEventListener listener = new GroupDiscussionEventListener() { ... }; + GroupDiscussionEventListener listener = createSseListener(eventSink, sse, event -> sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_ROUND_START, toJson(event)));🤖 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/internal/RestGroupConversation.java` around lines 251 - 308, The SSE listener in continueDiscussionStreaming duplicates the one used by discussStreaming, so extract the common GroupDiscussionEventListener construction into a shared factory/helper in RestGroupConversation and reuse it in both paths. Keep the event forwarding logic centralized in the factory, and allow continueDiscussionStreaming to customize only the onRoundStart behavior (for example via an optional callback or override) while preserving the existing closeQuietly handling for completion/error events.src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (2)
465-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded 120s follow-up timeout — prefer the configured protocol timeout. Elsewhere agent calls use
protocol.agentTimeoutSeconds()(defaulting to 60), but the follow-up path hardcodes 120s, which is inconsistent and not tunable by agent designers. Consider resolving the timeout from the group'sProtocolConfig(with a sensible default) so follow-up latency limits stay configurable and consistent with the rest of the engine.As per coding guidelines: "prefer making it configurable for agent designers rather than hardcoding a single approach; expose sensible defaults in config."
🤖 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/internal/GroupConversationService.java` around lines 465 - 472, The follow-up call in GroupConversationService currently hardcodes a 120-second wait in the responseFuture.get path, which makes it inconsistent with the rest of the engine. Update the timeout to be resolved from the group’s ProtocolConfig (using the same agentTimeoutSeconds pattern and a sensible default when unset) so follow-up latency is configurable for agent designers. Keep the existing exception handling in the follow-up flow, but have the timeout value come from the protocol settings rather than a literal.Source: Coding guidelines
360-365: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftDeferred ephemeral cleanup can leak agents for abandoned conversations. Cleanup now runs only for
FAILEDdiscussions; successful rounds defer tocloseGroupConversation(). If a client completes a discussion but never calls close (the common case), created/deployed ephemeral agents persist indefinitely. Consider a reaper/TTL for staleCOMPLETEDconversations, or documenting that clients must close to reclaim resources.🤖 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/internal/GroupConversationService.java` around lines 360 - 365, The deferred cleanup in GroupConversationService only handles FAILED conversations, so ephemeral agents created during COMPLETED discussions can persist if closeGroupConversation() is never called. Update the GroupConversationService flow to add a stale-conversation cleanup path for completed rounds, such as a TTL/reaper for completed conversations or equivalent automatic cleanup, and keep cleanupEphemeralAgents as the shared removal mechanism; if automatic cleanup is not added here, document that callers must invoke closeGroupConversation() to reclaim resources.
🤖 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`:
- Around line 6-51: The changelog entry in docs/changelog.md is missing the
required “what’s next if interrupted” section. Add that section to the Group
Conversation Follow-Ups entry, near the end with the other structured
subsections, and summarize the next follow-up work or remaining risks for this
feature; keep it consistent with the existing headings and style used in this
entry.
In `@src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java`:
- Around line 76-103: The `followUpWithMember` and `continueDiscussion`
endpoints in `IRestGroupConversation` are missing documented 404 responses even
though their implementations return 404 for
`IResourceStore.ResourceNotFoundException`. Update both method annotations to
include an `@APIResponse(responseCode = "404")`, matching the existing
`closeGroupConversation` contract and keeping the interface aligned with the
implementation.
- Around line 105-114: The continueDiscussionStreaming operation is missing the
same OpenAPI documentation as discussStreaming, including response metadata and
SSE event details. Update the continueDiscussionStreaming method in
IRestGroupConversation to add `@APIResponse` annotations for the successful and
not-found cases, and expand the `@Operation` description to list all emitted SSE
event types, including the new round_start event, so the streaming contract is
documented consistently.
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 320-329: The `continueDiscussionStreaming` exception handling is
swallowing `ForbiddenException` inside the generic `catch (Exception e)` and
turning it into an SSE error instead of a 403. Update the `try`/`catch` around
`requireOwnerOrAdmin` and the streaming setup in
`RestGroupConversation.continueDiscussionStreaming` to rethrow
`ForbiddenException` before the SSE error path, matching the behavior of
`followUpWithMember`, `continueDiscussion`, and `closeGroupConversation`. Keep
the existing SSE error handling only for non-authorization failures, and
preserve the current `sendEvent`/`closeQuietly` flow for those cases.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 465-472: The follow-up call in GroupConversationService currently
hardcodes a 120-second wait in the responseFuture.get path, which makes it
inconsistent with the rest of the engine. Update the timeout to be resolved from
the group’s ProtocolConfig (using the same agentTimeoutSeconds pattern and a
sensible default when unset) so follow-up latency is configurable for agent
designers. Keep the existing exception handling in the follow-up flow, but have
the timeout value come from the protocol settings rather than a literal.
- Around line 360-365: The deferred cleanup in GroupConversationService only
handles FAILED conversations, so ephemeral agents created during COMPLETED
discussions can persist if closeGroupConversation() is never called. Update the
GroupConversationService flow to add a stale-conversation cleanup path for
completed rounds, such as a TTL/reaper for completed conversations or equivalent
automatic cleanup, and keep cleanupEphemeralAgents as the shared removal
mechanism; if automatic cleanup is not added here, document that callers must
invoke closeGroupConversation() to reclaim resources.
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 251-308: The SSE listener in continueDiscussionStreaming
duplicates the one used by discussStreaming, so extract the common
GroupDiscussionEventListener construction into a shared factory/helper in
RestGroupConversation and reuse it in both paths. Keep the event forwarding
logic centralized in the factory, and allow continueDiscussionStreaming to
customize only the onRoundStart behavior (for example via an optional callback
or override) while preserving the existing closeQuietly handling for
completion/error events.
🪄 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: 86ce4982-5ff5-4154-bf89-b69c326d29b9
📒 Files selected for processing (10)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/IGroupConversationStore.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.javasrc/main/java/ai/labs/eddi/engine/api/IGroupConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.java
…lose
Static-analysis and code-review response (CodeQL, Copilot, CodeRabbit,
GitHub Code Quality).
Security & correctness:
- Sanitize user-controlled groupConversationId in logs (CWE-117)
- Validate conversation belongs to {groupId} path (404 on mismatch) via
shared loadInGroup(); resolves unused-groupId findings + wrong-path gap
- Rethrow ForbiddenException in continueDiscussionStreaming so ownership
failures map to 403 instead of a 200 SSE error
Concurrency:
- Fail-fast per-conversation guard around follow-up/continue/close
(single-node; cluster-wide would need storage-level CAS)
Resource lifecycle:
- @PreDestroy the RestGroupConversation virtual-thread executor
- Clean up ephemeral agents on delete (deferral had orphaned them);
shared cleanupEphemeralAgentsForGroup() for close + delete
API cleanup:
- Remove dead userId param (service iface/impl, REST, MCP tools)
- Configurable follow-up timeout via protocol.agentTimeoutSeconds()
- Unmodifiable getMemberDisplayNames() + addMemberDisplayName()
- OpenAPI 404 + SSE event listing additions
CI (green):
- Update GroupConversationState (6->7, CLOSED) and TranscriptEntryType
(14->15, FOLLOW_UP) enum-count guard tests
- Whitelist the 3 follow-up MCP tools in McpToolFilter — they were
filtered out entirely (never exposed to MCP clients), a functional fix
Review findings (adversarial multi-dimension review):
- Guard deleteGroupConversation with the per-conversation lock: as a
terminal op it could race an in-flight continue/follow-up and resurrect
a zombie document via upsert-by-id update()
- close: throw GroupDiscussionException for business conflicts (->409);
genuine store failures now map to 500, not 409
- Route readGroupConversation/deleteGroupConversation through loadInGroup
so every /groups/{groupId}/conversations/{id} endpoint verifies the
conversation belongs to the path group (404 on mismatch)
- Sanitize e.getMessage() in the 3 new MCP tool catch blocks (CWE-117)
Tests (+35): getAvailableActions per state, memberDisplayNames
encapsulation, round default, compareAndSetState branches, follow-up /
continue / close service logic (display-name resolution, wrong-state,
concurrency guard, state restore, round increment), and the new REST
endpoints incl. the loadInGroup 404 guard.
Contributor
There was a problem hiding this comment.
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/engine/internal/RestGroupConversation.java (1)
264-351: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winForbiddenException rethrow fix confirmed; new sanitization gap in the executor lambda.
The previously-flagged
ForbiddenExceptionswallowing is fixed (lines 339-341 now rethrow before the SSE error path). However, the newly-added catch insideexecutorService.submit(...)(line 334) logse.getMessage()raw.continueDiscussion/followUpWithMemberexception messages can embed user-controlled data (e.g.,targetAgentId/question text used in "target not found" style messages per the display-name resolution feature), so this is the same CWE-117 log-injection class the changelog says was hardened inGroupConversationServiceandMcpGroupTools— this call site in the new streaming code path was missed.🔒 Proposed fix
try { groupConversationService.continueDiscussion(gcId, request.question(), listener); } catch (Exception e) { - LOGGER.errorf("Continue discussion streaming failed: %s", e.getMessage()); + LOGGER.errorf("Continue discussion streaming failed: %s", LogSanitizer.sanitize(e.getMessage())); listener.onGroupError(new GroupConversationEventSink.GroupErrorEvent(e.getMessage())); }🤖 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/internal/RestGroupConversation.java` around lines 264 - 351, Sanitize the exception detail logged in the executor lambda inside continueDiscussionStreaming before passing it to LOGGER.errorf. Reuse the existing log-message sanitization utility used by GroupConversationService or McpGroupTools, while preserving the raw exception message for the SSE GroupErrorEvent response.
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java (1)
306-440: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test coverage for
continueDiscussionStreaming.The other three post-discussion endpoints are well covered, but the streaming variant — which contains the exact
ForbiddenException-vs-SSE-error logic flagged in a prior review round and now fixed — has no test verifying the 403 rethrow, the SSEgroup_errormapping forResourceNotFoundException, or the executor-submitted task's error handling.🤖 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/engine/internal/RestGroupConversationTest.java` around lines 306 - 440, Add tests in the PostDiscussionOperations suite for RestGroupConversation.continueDiscussionStreaming covering ForbiddenException rethrow as 403, ResourceNotFoundException mapped to an SSE group_error, and errors raised by the executor-submitted task. Reuse the existing group/conversation fixtures and mocks, and verify both the response behavior and relevant delegation/error handling.
🤖 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.
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 264-351: Sanitize the exception detail logged in the executor
lambda inside continueDiscussionStreaming before passing it to LOGGER.errorf.
Reuse the existing log-message sanitization utility used by
GroupConversationService or McpGroupTools, while preserving the raw exception
message for the SSE GroupErrorEvent response.
---
Nitpick comments:
In `@src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java`:
- Around line 306-440: Add tests in the PostDiscussionOperations suite for
RestGroupConversation.continueDiscussionStreaming covering ForbiddenException
rethrow as 403, ResourceNotFoundException mapped to an SSE group_error, and
errors raised by the executor-submitted task. Reuse the existing
group/conversation fixtures and mocks, and verify both the response behavior and
relevant delegation/error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33ae0b3b-9f01-497a-9518-f06b98b30f4f
📒 Files selected for processing (12)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/api/IGroupConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java
- src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java
…or, executor error) Adds the streaming variant's missing coverage flagged in review: - ForbiddenException is rethrown (maps to 403), not converted to an SSE error - group-mismatch (loadInGroup) sends group_error and closes without delegating - happy path delegates continue to the background executor - executor task failure routes the error to the SSE sink and closes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExecutor never shut down between tests.
Each
ContinueDiscussionStreamingtest rebuildsrestGroupConversationin@BeforeEach, creating a newExecutors.newVirtualThreadPerTaskExecutor()that is never stopped (the@PreDestroy shutdown()method is package-private and reachable from this test class but isn't invoked). Over a full suite run this accumulates un-shutdown executors.♻️ Suggested cleanup
+ `@AfterEach` + void tearDown() { + restGroupConversation.shutdown(); + }Also applies to: 344-417
🤖 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/engine/internal/RestGroupConversationExtendedTest.java` around lines 51 - 52, Ensure each test-created RestGroupConversation executor is shut down after every test, by invoking its package-private shutdown() from the test lifecycle (for example, in `@AfterEach`). Keep the existing `@BeforeEach` construction and ContinueDiscussionStreaming test behavior unchanged while preventing executor accumulation across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java`:
- Around line 51-52: Ensure each test-created RestGroupConversation executor is
shut down after every test, by invoking its package-private shutdown() from the
test lifecycle (for example, in `@AfterEach`). Keep the existing `@BeforeEach`
construction and ContinueDiscussionStreaming test behavior unchanged while
preventing executor accumulation across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c9651f6-6c2b-46b6-a8c8-6cac0ee35b8a
📒 Files selected for processing (1)
src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java
Reconcile the group follow-up/continuation/close feature with main's HITL
framework + multimodal-attachments group work. All 23 conflict hunks across
12 files resolved as a union of both feature sets. Key decisions:
- executeDiscussion setup: interleave round-aware start events (ours) with
attachment rehydration, resume-seeded turn counter and HITL control-token
registration (theirs); fire a start event only on fresh execution
(startPhaseIndex == 0), GROUP_START on round 1 else ROUND_START.
- executeDiscussion finally: clean up ephemeral agents only on FAILED/CANCELLED
(defer COMPLETED to close/delete so follow-ups reuse dynamic agents); always
remove the control token; keep the verification cursor while AWAITING_APPROVAL.
- GroupConversationState now has 8 values (CLOSED + CANCELLED);
getAvailableActions() handles CANCELLED; enum-count guard test fixed 7 -> 8.
- continueDiscussion passes startPhaseIndex=0 to the new 6-arg executeDiscussion.
- Group-path guard unified on loadInGroup() for all six per-conversation
endpoints; theirs' redundant requireGroupMembership() removed.
- Restore the /{groupId}/conversations prefix on the four follow-up endpoints
after main flattened the class-level @path to /groups (otherwise they bind
groupId=null and 404 on every call).
- RestGroupConversationExtendedTest: @AfterEach shuts down the virtual-thread
executor so a full suite run does not leak executors.
Verified: mvnw clean test-compile green; ~187 group-conversation unit tests
pass (0 failures/errors).
A deep adversarial review of the merge surfaced interaction bugs between the group follow-up/continuation/close feature and main's HITL pause/cancel/resume machinery — latent because the two never coexisted before the merge, and invisible to the impl-level unit tests. Fixes: - A: continuation resume used the round-1 question (resumeDiscussion read originalQuestion, which continueDiscussion never updated) -> silent wrong output. Added a dedicated GroupConversation.resumeQuestion field; resume reads it with a fallback to originalQuestion so the UI title is untouched. - B: continueDiscussionStreaming's inline SSE listener dropped HITL/cancel events, hanging the client. Unified on createStreamingListener (added onRoundStart there); removed the duplicated listener. - C: executeDiscussion's finally leaked ephemeral agents on the cross-pod terminal-override and lost-completion-CAS exits (stale in-memory state). Both exits now align in-memory state to the persisted terminal value. - D: followUpWithMember's success write was a bare update() that could clobber a racing CANCELLED; switched to updateIfState(gc, IN_PROGRESS) -> 409 on conflict, no resurrection. - E: continueDiscussion now pre-registers the DiscussionControlToken after its CAS (mirrors startAndDiscussAsync/resumeDiscussion) so a racing cancel takes the signal path; removed on the pre-exec failure path. - F: closeGroupConversation accepts CANCELLED -> CLOSED and getAvailableActions returns [close] for CANCELLED, giving a reclaim path for orphaned agents. Added regression tests (CANCELLED available-actions, close-of-CANCELLED) and updated the follow-up success-write assertion. mvnw test green: 189 group-conversation unit tests pass.
…kfill Third critical review of the merge targeted the files git auto-merged (never reviewed), the whole-branch PR surface, coverage, and security. 14 verified findings, all fixed: Security - McpGroupTools gated the conversation tools on a ROLE check only, with no ownership check, while the REST equivalents enforce requireOwnerOrAdmin. Any viewer could read/append/re-run another user's group conversation and an editor could close or delete it. Injected OwnershipValidator and gated all five conversation-scoped tools (read/delete/followup/continue/close). Correctness — CLOSED was not recognised as terminal by main's HITL/cancel code - persistedTerminalOverride ignored CLOSED, so a running leg kept going past a concurrent close and its whole-document write RESURRECTED the closed conversation (members already ended, ephemeral agents already deleted). - cancelDiscussion ignored CLOSED, so a cancel could un-terminalize it. Correctness — compareAndSetState was a read-check-write, not a CAS - It read, compared in Java, then wrote unconditionally: two racing callers could both pass the check and both write, and it is the only cross-process guard behind follow-up/continue/close. Now uses storeIfFieldEquals (a conditional write), returning false on a lost race. Contract / robustness - followUpWithMember NPE'd into a 500 on a blank targetAgentId -> now 400. - POST /continue advertised attachments but dropped them -> now honored via an attachment-aware continueDiscussion overload (union of prior + new). - Follow-up by display name now resolves dynamically recruited agents. - DELETE during an in-flight operation returned 500 -> now 409. - OpenAPI updated for the new 400/409 responses and CANCELLED-closeable. Tests - Backfilled the previously untested fixes (resumeQuestion, token pre-registration, terminal-state alignment, cancel-of-CLOSED, conditional CAS, MCP ownership, blank-input 400s, continue attachments, delete 409, and the streaming listener forwarding HITL + round_start). - Fixed a latent broken test the merge introduced: GroupConversationHitlTest still asserted 7 enum states (CLOSED makes it 8). mvnw test green: 773 group/MCP unit tests pass.
Pre-push review of the previous commit found that two of its fixes were wrong. Corrected: - BLOCKER: the new MCP ownership gate locked the creator out of their own conversation. MCP recorded the owner as the literal "mcp-client" (or any caller-supplied userId), so with auth enabled requireOwnerOrAdmin denied the very caller who created it — the whole MCP group workflow was unusable for non-admins. discuss_with_group / start_group_discussion now resolve the owner via validateAndResolveUserId (the calling principal; impersonation rejected), falling back to "mcp-client" only when auth is off. - list_group_conversations is now owner-filtered (mirrors REST). It returns full conversation documents, so without it the per-conversation gate was pointless: a non-owner could just list the group and read everyone's transcripts. - Reverted the "honour attachments on /continue" change: it was a functional no-op. Attachments are granted/injected to a member only on its first-EVER turn (privateConvId == null), and on a continuation every member conversation already exists — so the new code stored an orphaned blob and still returned 200. /continue and /continue/stream now REJECT attachments (400 / terminal group_error) instead of silently dropping them. Honouring them requires reworking the attachment fan-out; recorded as follow-up. - Reverted the dynamic-member follow-up scan: it was dead code. Nothing in production populates GroupConversation.dynamicMembers (sub-agent creation records only createdAgentIds), so recruited agents are not addressable as follow-up targets at all. Registering them as real members is follow-up work. Tests: MCP owner-resolution + impersonation rejection + list filtering; /continue and /continue/stream attachment rejection. mvnw test green: 760 group/MCP unit tests pass. mvnw validate (Checkstyle) clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.java (1)
316-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the test to expect
ResourceNotFoundException.Following the removal of the unchecked
GroupConversationGoneExceptionwrapper fromupdateIfState, this test must be updated to expect the nativeResourceNotFoundExceptionthat will now propagate directly from the storage layer.♻️ Proposed fix
`@Test` - `@DisplayName`("updateIfState — storage ResourceNotFoundException maps to GroupConversationGoneException (404)") + `@DisplayName`("updateIfState — storage ResourceNotFoundException propagates as-is") void updateIfStateGoneWhenDeleted() throws Exception { var gc = new GroupConversation(); gc.setId("gc-1"); IResourceStorage.IResource<GroupConversation> resource = mock(IResourceStorage.IResource.class); when(storage.newResource(eq("gc-1"), anyInt(), eq(gc))).thenReturn(resource); // The storage-level CAS reports the row is gone. doThrow(new IResourceStore.ResourceNotFoundException("gone")) .when(storage).storeIfFieldEquals(eq(resource), eq("state"), anyString()); - assertThrows(GroupConversationGoneException.class, + assertThrows(IResourceStore.ResourceNotFoundException.class, () -> store.updateIfState(gc, GroupConversationState.AWAITING_APPROVAL)); }🤖 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/configs/groups/mongo/GroupConversationStoreTest.java` around lines 316 - 330, Update updateIfStateGoneWhenDeleted to expect the native IResourceStore.ResourceNotFoundException from store.updateIfState instead of GroupConversationGoneException, preserving the existing storage mock and invocation setup.src/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.java (1)
135-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate
ResourceNotFoundExceptiondirectly instead of wrapping it in an unchecked exception.
updateIfStatewraps the checkedResourceNotFoundExceptioninto an uncheckedGroupConversationGoneException. However, callers likeGroupConversationService.resumeDiscussion(which callsupdateIfStatedirectly) already declarethrows IResourceStore.ResourceNotFoundExceptionin their signatures.Crucially, the REST layer (
RestGroupConversation) specifically catchesResourceNotFoundExceptionto return a404 Not Foundresponse. By throwing an unchecked exception instead, the REST layer's specificcatchblock is bypassed, resulting in a generic500 Internal Server Error(or generic error event) whenupdateIfStateis called on a concurrently deleted conversation.Update the method signature in both this class and the
IGroupConversationStoreinterface to declareResourceNotFoundException, allowing it to propagate natively without theGroupConversationGoneExceptionworkaround. Please also remove theGroupConversationGoneExceptionclass if it's no longer used.♻️ Proposed fix to propagate the checked exception natively
`@Override` - public void updateIfState(GroupConversation gc, GroupConversation.GroupConversationState expectedState) - throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException { + public void updateIfState(GroupConversation gc, GroupConversation.GroupConversationState expectedState) + throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { try { IResourceStorage.IResource<GroupConversation> resource = storage.newResource(gc.getId(), SINGLE_VERSION, gc); storage.storeIfFieldEquals(resource, "state", expectedState.name()); - } catch (IResourceStore.ResourceNotFoundException e) { - // deleted-vs-mismatch distinction from the storage CAS: surface the - // deletion as its own (unchecked) type so callers can answer 404 - throw new GroupConversationGoneException( - "Group conversation " + gc.getId() + " no longer exists", e); } catch (IOException e) { throw new IResourceStore.ResourceStoreException("Failed conditional update: " + e.getMessage(), e); } }Then, simplify
compareAndSetStateby removing the unwrap block:public boolean compareAndSetState(String id, GroupConversation.GroupConversationState expectedState, GroupConversation.GroupConversationState newState) throws IResourceStore.ResourceStoreException, IResourceStore.ResourceNotFoundException { GroupConversation gc = read(id); if (gc.getState() != expectedState) { // Fast path: clearly the wrong state — no need to attempt the write. return false; } gc.setState(newState); gc.setLastModified(java.time.Instant.now()); try { // Conditional write — the persisted state must STILL be expectedState. The // read-check above alone was a read-check-update (single-node only): two // racing callers could both pass it and both write. storeIfFieldEquals makes // the transition atomic across processes. updateIfState(gc, expectedState); return true; } catch (IResourceStore.ResourceModifiedException e) { // Another writer transitioned the conversation between our read and our // write — this CAS lost the race. return false; - } catch (GroupConversationGoneException e) { - throw new IResourceStore.ResourceNotFoundException( - "Group conversation " + id + " no longer exists"); } }🤖 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/groups/mongo/GroupConversationStore.java` around lines 135 - 177, Update updateIfState in GroupConversationStore and IGroupConversationStore to declare and propagate IResourceStore.ResourceNotFoundException directly from storage.storeIfFieldEquals, removing the GroupConversationGoneException wrapping. Simplify compareAndSetState by removing its GroupConversationGoneException catch and preserve direct ResourceNotFoundException propagation. Remove GroupConversationGoneException if no remaining references exist.
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.java (2)
54-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOwnership helpers look correct; consider WARN for the access-denial log.
requireConversationOwner,resolveOwner, andaccessDeniedare well-documented and match the test expectations (owner/admin gate, "mcp-client" fallback, uniform denial). One nit:accessDeniedlogs a security-relevant denial event atinfof(Line 100) rather than a warn-level log, which under-signals the event to log monitoring/alerting compared to other denial paths.As per coding guidelines, "Use JBoss Logger rather than
System.out; include conversation context and use appropriate log levels."📝 Proposed change
- LOGGER.infof("%s denied: caller does not own group conversation %s", + LOGGER.warnf("%s denied: caller does not own group conversation %s", tool, LogSanitizer.sanitize(groupConversationId));🤖 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/mcp/McpGroupTools.java` around lines 54 - 104, Update the accessDenied method to log security-relevant ownership denials at warn level instead of info level, while preserving the existing tool name, sanitized conversation ID, and uniform denial response.Source: Coding guidelines
449-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew follow-up tools have no Micrometer instrumentation.
followup_with_member,continue_group_discussion, andclose_group_conversationare new features but add no counters/timers viaMeterRegistry(e.g., invocation count, denial count, failure count per tool).As per coding guidelines, "Add Micrometer metrics to new features, using counters, timers, or gauges registered through
MeterRegistry."🤖 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/mcp/McpGroupTools.java` around lines 449 - 514, Add Micrometer instrumentation to followup_with_member, continue_group_discussion, and close_group_conversation using the class’s existing MeterRegistry conventions. Record per-tool invocation and failure/denial outcomes, and time the operations where established patterns support it, while preserving the current authorization, service calls, and responses.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/mcp/McpGroupToolsTest.java (1)
443-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid denial/allow coverage, but no test for the admin-bypass branch.
toolsAsUser/ownedByand the denial tests correctly exercise the non-owner path for followup/continue/close/read/delete plus owner-resolution-on-creation and list-filtering. However, none of these tests sethasRole("eddi-admin")to verify that an admin can access/mutate another user's conversation (the other half ofrequireOwnerOrAdminand the list-filter's admin exemption) — a security-critical branch with zero regression coverage.🤖 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/engine/mcp/McpGroupToolsTest.java` around lines 443 - 586, Add admin-bypass coverage to McpGroupToolsTest using toolsAsUser and ownedBy: configure an eddi-admin caller accessing a conversation owned by another user, then verify representative read/mutation operations succeed and the underlying service is invoked. Also add an admin listing test confirming list_group_conversations serializes conversations owned by multiple users, covering both requireOwnerOrAdmin and the list-filter exemption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java`:
- Around line 1217-1237: Update
close_completed_transitionsToClosedAndEndsMembers to stub the final conversation
read with a CLOSED-state result after the successful compareAndSetState call,
then assert that the returned conversation has GroupConversationState.CLOSED
instead of relying only on assertSame(gc, result).
- Around line 1053-1075: The continueDiscussion test fixtures must model the
post-CAS read returning an IN_PROGRESS conversation rather than the original
COMPLETED instance. Update the conversationStore.read stubbing in
continue_incrementsRoundAndAppendsQuestion and the related test around the
second referenced range, then assert that the failed recovery state is persisted
as FAILED after the configuration-load exception.
In
`@src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java`:
- Around line 434-454: The test method
continueListenerForwardsHitlAndCancelEvents currently invokes only onHitlPause,
so it does not verify cancellation delivery. Invoke the listener’s cancellation
callback as well, then assert that the cancelled SSE event is sent and the event
sink is closed, preserving the existing HITL pause assertion.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.java`:
- Around line 135-177: Update updateIfState in GroupConversationStore and
IGroupConversationStore to declare and propagate
IResourceStore.ResourceNotFoundException directly from
storage.storeIfFieldEquals, removing the GroupConversationGoneException
wrapping. Simplify compareAndSetState by removing its
GroupConversationGoneException catch and preserve direct
ResourceNotFoundException propagation. Remove GroupConversationGoneException if
no remaining references exist.
In
`@src/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.java`:
- Around line 316-330: Update updateIfStateGoneWhenDeleted to expect the native
IResourceStore.ResourceNotFoundException from store.updateIfState instead of
GroupConversationGoneException, preserving the existing storage mock and
invocation setup.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.java`:
- Around line 54-104: Update the accessDenied method to log security-relevant
ownership denials at warn level instead of info level, while preserving the
existing tool name, sanitized conversation ID, and uniform denial response.
- Around line 449-514: Add Micrometer instrumentation to followup_with_member,
continue_group_discussion, and close_group_conversation using the class’s
existing MeterRegistry conventions. Record per-tool invocation and
failure/denial outcomes, and time the operations where established patterns
support it, while preserving the current authorization, service calls, and
responses.
In `@src/test/java/ai/labs/eddi/engine/mcp/McpGroupToolsTest.java`:
- Around line 443-586: Add admin-bypass coverage to McpGroupToolsTest using
toolsAsUser and ownedBy: configure an eddi-admin caller accessing a conversation
owned by another user, then verify representative read/mutation operations
succeed and the underlying service is invoked. Also add an admin listing test
confirming list_group_conversations serializes conversations owned by multiple
users, covering both requireOwnerOrAdmin and the list-filter exemption.
🪄 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: 1c5c9f85-268a-4b41-9951-fee9a426e8b4
📒 Files selected for processing (18)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/IGroupConversationStore.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.javasrc/main/java/ai/labs/eddi/engine/api/IGroupConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/test/java/ai/labs/eddi/configs/groups/model/GroupConversationHitlTest.javasrc/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpGroupToolsTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java
- src/main/java/ai/labs/eddi/configs/groups/IGroupConversationStore.java
- src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java
- src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java
- src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java
- src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java
- src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java
- src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
Correctness: - continueDiscussion persisted the round/question mutation with an unconditional update() after its CAS, so a cancel/close/delete winning the window was overwritten and the conversation resurrected as IN_PROGRESS. Now a conditional write (updateIfState on IN_PROGRESS) -> 409 on conflict. Same defect class already fixed in followUpWithMember; continue was missed. Security (reflected input / error exposure): - loadInGroup embedded the caller-supplied groupId in its exception message, which followup/continue/close echo into the 404 body. Curated body now; ids logged server-side via LogSanitizer. - followUpWithMember's "not a member" 409 echoed the caller-supplied targetAgentId. No longer reflected (the member list is server data, kept). - The delete-conflict 409 returned the raw exception text (CodeQL: information exposure through an error message). Curated body + sanitized server log. Observability / docs: - Added eddi_group_followup_count / _continue_count / _close_count. Instrumented in the service rather than the MCP tools so the counters cover REST and MCP. - Authorization denials log at WARN, not INFO. - getAvailableActions javadoc no longer claims "not persisted" (Jackson does serialize it; it is READ_ONLY so it is never read back and always recomputed). Tests: model the post-CAS IN_PROGRESS read so the FAILED recovery path is really exercised; assert close returns CLOSED instead of assertSame on a stale instance; add the concurrent-terminal-transition conflict test, the SSE cancelled-callback test, admin-bypass tests, and assertions that neither the raw exception text nor the caller-supplied groupId reaches the client. Not actioned: CodeRabbit's claim that GroupConversationGoneException bypasses the REST 404 handling is a false positive — RestGroupConversation catches it in a multi-catch alongside ResourceNotFoundException on every surface that exposes the operation and maps it to 404. mvnw test green (633 group/MCP tests, 0 failures); mvnw validate clean.
Contributor
There was a problem hiding this comment.
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/engine/internal/RestGroupConversation.java (1)
427-447: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize generic exception messages in SSE error events.
The PR objectives highlight fixing "Information exposure through an error message" by curating error bodies. However,
continueDiscussionStreamingcurrently reflects the rawe.getMessage()for any genericExceptionto the client in two places (the background task's catch block and the setup catch block). This could inadvertently leak internal system details or database errors.Catch specific business exceptions (
IllegalArgumentException,GroupDiscussionException) if their messages are safe to return, and use a curated message for generic exceptions, matching the pattern used inapproveGroupPhaseStreaming.🔐 Proposed fix
executorService.submit(() -> { try { groupConversationService.continueDiscussion(gcId, request.question(), listener); + } catch (IllegalArgumentException | IGroupConversationService.GroupDiscussionException e) { + listener.onGroupError(new GroupConversationEventSink.GroupErrorEvent(e.getMessage())); } catch (Exception e) { LOGGER.errorf("Continue discussion streaming failed: %s", e.getMessage()); - listener.onGroupError(new GroupConversationEventSink.GroupErrorEvent(e.getMessage())); + listener.onGroupError(new GroupConversationEventSink.GroupErrorEvent("Failed to continue group discussion.")); } }); } catch (ForbiddenException e) { throw e; } catch (IResourceStore.ResourceNotFoundException e) { sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_GROUP_ERROR, toJson(new GroupConversationEventSink.GroupErrorEvent(e.getMessage()))); closeQuietly(eventSink); } catch (Exception e) { LOGGER.errorf("Continue discussion streaming setup failed: %s", e.getMessage()); sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_GROUP_ERROR, - toJson(new GroupConversationEventSink.GroupErrorEvent(e.getMessage()))); + toJson(new GroupConversationEventSink.GroupErrorEvent("Failed to setup continuation."))); closeQuietly(eventSink); }🤖 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/internal/RestGroupConversation.java` around lines 427 - 447, Update continueDiscussionStreaming’s generic exception handling in both the executor task and setup catch blocks to stop sending raw e.getMessage() values in GroupErrorEvent responses. Catch and expose messages only for safe business exceptions such as IllegalArgumentException and GroupDiscussionException, and use the same curated generic-error message pattern as approveGroupPhaseStreaming for all other exceptions while retaining server-side logging.
🤖 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.
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 427-447: Update continueDiscussionStreaming’s generic exception
handling in both the executor task and setup catch blocks to stop sending raw
e.getMessage() values in GroupErrorEvent responses. Catch and expose messages
only for safe business exceptions such as IllegalArgumentException and
GroupDiscussionException, and use the same curated generic-error message pattern
as approveGroupPhaseStreaming for all other exceptions while retaining
server-side logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c4e406f-ecf3-4fcd-abe0-13bebe567b59
📒 Files selected for processing (9)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpGroupToolsTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java
- src/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.java
- src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java
- src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java
- src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
…odies Found by an adversarial review of the previous PR-review-response commit, which hardened the SUCCESS write in continueDiscussion but left the FAILURE write — and most of the reflected-value surface — open. Terminal states are now irreversible: - failConversation did an unconditional whole-document write, i.e. an UPSERT. It could RE-CREATE a conversation another pod had deleted, and could overwrite a terminal CANCELLED with FAILED, clobbering that writer's transcript. It is now a conditional write. - The CAS expectation comes from the PERSISTED state, not the in-memory one. A first attempt CASed on gc.getState() and was itself a blocker: executeDiscussion sets SYNTHESIZING in memory BEFORE the synthesis phase and persists it only afterwards, so a synthesis-phase failure would have lost the CAS, skipped the write and stranded the conversation IN_PROGRESS forever. failConversation now re-reads the persisted state, skips the write when it is already terminal (aligning the in-memory state so the finally makes the right ephemeral-agent decision), and counts the failure metric unconditionally so a lost race cannot hide a failure from operators. No exception text reaches the client (CodeQL: information exposure / reflected value) — the previous commit curated one handler; this closes the class: - Throw sites: the store and service no longer embed caller-supplied ids in exception messages. - Sinks: RestGroupConversation returns no raw exception text in any body; every 400/404/409 is curated (and deliberately non-committal, since these exceptions cover several causes), with the detail logged via LogSanitizer. - SSE: the SERVICE was pushing raw e.getMessage() into GroupErrorEvent, which the streaming listener forwards to the browser — so LLM/DB/driver detail reached the client despite the REST catch sites being curated. Now curated; the raw cause is logged with its stack trace. - Malformed ids (Mongo's ObjectId parser embeds the raw caller string — the most exploitable sink) are caught INSIDE loadInGroup, scoped to the id lookup only, and answered with a curated 404. Deliberately not a blanket catch around the whole operation, which would mask a genuine internal bug as a false "not found". 636 group/MCP unit tests pass (incl. regression tests for the persisted-state CAS and the already-terminal skip); mvnw validate clean.
"Do we have coverage for everything we fixed?" answered by mutation testing
rather than by reading test names: each fix was reverted in turn and the suite
re-run. A surviving mutant = no coverage = the bug can silently come back.
Mutants that SURVIVED (zero coverage) and are now covered:
- JAX-RS routing: the /{groupId}/conversations prefix on the four post-discussion
endpoints. The unit tests invoke resource methods directly and never exercise
path binding, so the highest-severity bug of the whole effort (every
followup/continue/close would 404) could be reintroduced with a green suite.
- SSE error curation: the SERVICE pushing raw e.getMessage() into GroupErrorEvent.
Nothing asserted what actually reaches the browser.
- Malformed-id reflected value (Mongo's ObjectId parser echoes the raw string).
- Curated exception messages: the old test asserted the response body, which the
REST layer curates anyway — the message itself (surfaced by read/delete via the
global mapper) was unguarded.
- Continuation startPhaseIndex=0: a mutant skipping phase 0 passed.
- finally cleanup condition: nothing asserted a COMPLETED round keeps its
ephemeral agents for follow-ups.
- resumeQuestion: only the write was tested, not that resume actually reads it.
- The three new metrics counters.
Added (each verified to KILL its mutant):
- IRestGroupConversationRoutingTest (new): reflective JAX-RS assertions — every
@PathParam must have a matching {template} segment (a mismatch binds null, the
exact production failure), per-conversation routes keep the group prefix, and
the four endpoints resolve to their documented URLs. This is the invariant a
direct-invocation test structurally cannot check.
- GroupConversationServiceExtendedTest.MergeRegressionGuards (new): curated SSE
error on failure; continuation restarts at phase 0 and emits round_start;
COMPLETED keeps its ephemeral agents.
- GroupConversationServiceHitlTest: a paused continuation resumes with the
follow-up question, not the stale round-1 one.
- Store/REST: not-found message carries no caller id; malformed id -> 404 without
reflecting the payload; group-mismatch exception message is curated.
- Service: the three operation counters; failure counter increments even when the
CAS is lost.
Already-covered fixes (mutants killed before this pass): failConversation's
upsert, the MCP ownership gate, CLOSED-blindness in persistedTerminalOverride,
continueDiscussion's conditional write, control-token pre-registration,
cancelDiscussion's CLOSED guard, availableActions for CANCELLED, MCP list filter.
647 group/MCP tests pass; mvnw validate clean. No production code changed.
…ause followUpWithMember/continueDiscussion mapped every GroupDiscussionException to 409 Conflict — including an unknown target agent (client error) and mid-round server/upstream failures (LLM/DB down, agent timeout). A 409 wrongly tells the client "retryable conflict". Pre-existing feature behaviour, not a regression, but a real API-semantics issue — fixed properly. - Cause-differentiated subtypes (all extend GroupDiscussionException, so existing catch (GroupDiscussionException) in MCP/tests keeps working): GroupMemberNotFoundException (404), GroupExecutionException (502), GroupTimeoutException extends GroupExecutionException (504). The base type now means ONLY a state/concurrency conflict (409). - executeDiscussion re-throws every phase-loop failure as GroupExecutionException (preserving GroupTimeoutException) — one interception point instead of editing the many deep agent/quota/config throw sites. followUpWithMember's own agent-call/timeout throws are re-typed directly. - REST maps most-specific first: member -> 404, timeout -> 504, agent/model failure -> 502 (logged with stack trace), state/concurrency -> 409. The @apiresponse annotations now list the full set. close is unchanged (409 only). - Nits: new 400 bodies set .type(TEXT_PLAIN); the follow-up InterruptedException path restores the interrupt flag. Tests: REST 404/502/504 + still-409-for-conflict; service asserts the specific subtypes are thrown. Each mapping mutation-verified — reverting the split makes the matching test fail. 654 group/MCP tests pass; mvnw validate clean.
executeAgentTurn's ABORT-policy timeout branch still threw a base GroupDiscussionException, so a genuine member-agent timeout mid-round mapped to 502 Bad Gateway instead of the 504 the status-split documents. It now throws GroupTimeoutException; executeDiscussion's re-wrap preserves the subtype through to REST. Also hedge continueDiscussion's 502 body: that catch covers both the agent-call and unrunnable-config paths, so it no longer asserts a single cause it cannot verify. Coverage: new FailurePolicies#abortPolicy_agentTimesOut_throwsGroupTimeoutException drives a real timeout under ABORT and asserts the subtype; mutation-verified (reverting to the parent flips exactly this test to failing).
…IN, stale comment
All 5 Copilot findings verified against the code and confirmed:
- MCP raw-exception leak (x3): followup_with_member, continue_group_discussion
and close_group_conversation returned errorJson(e.getMessage()), forwarding
raw internal exception text to callers. Now log the full throwable server-side
and return a stable curated errorJson("Failed to ...", "INTERNAL", null),
matching the McpHitlTools convention.
- rejectAttachmentsOnContinue() 400 now sets .type(TEXT_PLAIN), consistent with
every other error response in RestGroupConversation.
- Corrected the stale operationsInProgress comment: compareAndSetState is an
atomic storage-layer CAS (storeIfFieldEquals), not a best-effort
read-check-update; the Set is only an in-node fast-fail optimization.
Coverage: three new McpGroupToolsTest cases assert each tool's generic catch
returns the curated message + INTERNAL errorCode and never the raw exception
text; mutation-verified (reverting the curations fails exactly those three).
The identical leak in ~10 pre-existing catch blocks (some covered by tests that
assert the forwarded message) is left as a separate follow-up, not bundled here.
Pre-existing bug on main, unrelated to the group-followups work, that surfaced as an intermittent full-suite CI failure on this branch: ToolExecutionServiceBranchTest.executeMultipleInParallel got "Error executing tool: ConcurrentModificationException". executeToolsParallel shares one ToolExecutionTrace across every concurrent task, but addToolCall/addFailedToolCall mutated a plain ArrayList, a plain HashMap (toolMetrics) and non-atomic counters without synchronization. Under real parallelism HashMap.computeIfAbsent throws ConcurrentModificationException, which executeTool returns as the observed error string. This is a genuine production bug — executeToolsParallelAndWait is a live API. Fix: synchronize both trace mutators. updateMetrics is private and only called under those locks; trace reads happen after allOf(...).join(), so writer synchronization is sufficient. Coverage: new concurrentToolsShareTraceWithoutCorruption stress test (50 rounds x 32 parallel tasks on a shared trace) fails reliably without the fix (CME) and passes with it. The original 2-task test's race window was too small to be a reliable regression guard.
aisabella-ai
approved these changes
Jul 20, 2026
Merges the multi-model cascade enterprise hardening and MCP/REST conversation-ownership security work from main. Two files conflicted: - ToolExecutionTrace.java: both branches documented the same synchronized rationale (ours as Javadoc, main's as inline comment); kept the Javadoc, dropped the redundant inline comment. - docs/changelog.md: both branches appended entries at the top; kept as a union with a new merge entry summarizing the resolution.
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 introduces a comprehensive set of features enabling richer post-discussion workflows for group conversations. It adds support for member-specific follow-ups, multi-round group continuations, and explicit conversation closure, along with important client-facing improvements for discoverability and consistency.
New group conversation actions:
FOLLOW_UPentries. [1] [2] [3]roundcounter and preserving agent memory across rounds. [1] [2] [3] [4]CLOSED. [1] [2] [3] [4]Model and API enhancements:
CLOSEDstate toGroupConversationState, aroundfield, and acompareAndSetState()method for atomic state transitions, preventing race conditions during follow-ups or continuations. [1] [2] [3] [4]GroupConversationobject. The newmemberDisplayNamesmap andavailableActionscomputed property make it easy for clients to display member names and discover allowed operations. [1] [2]REST and tool layer updates:
Design decisions and client experience:
COMPLETEDstate, with optimistic concurrency to prevent overlap.QUESTIONentries.These changes significantly improve the flexibility, reliability, and usability of group conversations for both clients and developers.
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
round_startSSE events and state-drivenavailableActions.CLOSED) behavior.