refactor: Wave R — decompose GroupConversationService, AgentOrchestrator and ConversationService - #626
Merged
Merged
Conversation
…nService Add the Rev 2.1 group-collaboration implementation plan and begin its Wave R refactoring workstream: GroupConversationService (4,417 lines) is decomposed into focused collaborator classes before the plan's ~18 feature items land, so they get clean homes instead of piling onto an already-oversized file. R1 step 1 (smallest, pure-move extraction first per the plan's ordering): move the attachment cluster (materializeAttachments, rehydrateAttachmentsFromStore, grantAndInjectAttachments) into a new GroupAttachmentBinder. Plain class, not a CDI bean, constructed per call site — the existing test suite constructs GroupConversationService directly and attachmentStore is field-injected specifically to keep that compiling, so the extraction must not change the constructor signature. Baselined the full 12-class/470-test GroupConversationService*Test suite before touching code (all green, 82% instruction / 72% branch JaCoCo) as the refactor's regression budget. Moved the 12 dedicated attachment tests to a new focused GroupAttachmentBinderTest testing the extracted class directly. Re-verified: still 470 tests total, all green, clean compile, clean formatter/Checkstyle.
…ervice R1 step 2 of planning/group-collaboration-improvements-plan.md: move the phase-input-construction and scope-filtering cluster (buildPhaseInput, selectDefaultTemplate, filterByScope, findLatestResponse, mapPhaseToEntryType, extractResponse, buildPlainTextFallback) into a new GroupContextBuilder, constructed once in GroupConversationService's constructor. Several of these methods are reached by characterization tests via GroupConversationService.class.getDeclaredMethod(...) reflection, which requires the method to stay declared directly on that class. Kept all seven as thin private delegators rather than inlining at call sites, after an exhaustive sweep of every reflection lookup across the test package. Added a focused GroupContextBuilderTest (direct construction, no reflection) alongside the untouched characterization suites. Full group suite + both new test classes green; clean compile; formatter/Checkstyle clean.
…vice R1 step 3 of planning/group-collaboration-improvements-plan.md: move the Ed25519 inter-agent signing cluster into a new GroupSigningGuard -- verifyPriorEntriesIfRequired, the signing-creation block that was inline inside executeAgentTurn, and the lastVerifiedIndex cursor map both share. The signing block's four loose local variables (signature, nonce, timestampMs, keyVersion) become a SigningResult record with an UNSIGNED singleton for "not signed, for any reason". verifyPriorEntriesIfRequired stays a declared delegator (reflection dependency, same pattern as step 2). Added a focused GroupSigningGuardTest covering the guard-clause branches directly; the full sign/verify/nonce-validate happy path needs real Ed25519 key material and stays covered by the untouched characterization suites. Full group suite + all three new focused test classes green; clean compile; formatter/Checkstyle clean.
Independent 5-agent review of the 3 R1 extraction commits before push surfaced one real pre-existing bug worth fixing in place, plus five stale comments left behind by the moves. GroupSigningGuard resolved a signer's public key via getKeyValidAt(timestamp) -- "whichever key is valid right now" -- at both the self-verify-after-signing and peer-verify-on-receipt call sites, instead of getKeyForVersion(exactVersion) using the key version actually recorded on the signature. During a key rotation's overlap window (both old and new key simultaneously valid -- the scenario AgentPublicKey is explicitly designed to support) this could self-discard a good signature, or verify an entry against the wrong key entirely. The peer-verify cache was also keyed by agent ID alone, so a second entry from the same agent signed with a different key version silently reused the first entry's cached key without ever consulting its own key version. Fixed both lookups; changed the verify-side cache key to agentId#keyVersion. Added two regression tests that construct an overlapping-validity two-key identity and assert the exact key material passed to verifyEnvelope via ArgumentCaptor. Mutation-checked: temporarily reverted the production fix via git stash on just that file, confirmed both new tests fail and only those two, then restored it. Two related pre-existing findings (no replay check on the verify path; requirePeerVerification is audit-only) were deliberately not fixed here -- both are security-sensitive design decisions, not mechanical bugs, and are filed as separate follow-up tasks with full context instead. Also fixed: an inline FQN in GroupSigningGuardTest (AGENTS.md Sec4.4), a stale "is now private" comment on a method that's actually public, two Javadoc comments still naming lastVerifiedIndex after it moved into GroupSigningGuard, and an overstated test-coverage claim in a class Javadoc. Full 15-class group suite green; clean compile; formatter/Checkstyle clean.
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:
📝 WalkthroughWalkthroughThe change extracts group discussion and LLM tool orchestration responsibilities into dedicated components. It adds focused tests for execution, lifecycle, signing, attachments, providers, and budgeting. It also updates the changelog and implementation plan. ChangesGroup collaboration refactor
LLM tool orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GroupConversationService
participant PhaseExecutionEngine
participant TaskForceEngine
participant MemberTurnExecutor
participant GroupContextBuilder
participant GroupSigningGuard
participant ConversationService
GroupConversationService->>PhaseExecutionEngine: execute discussion phase
GroupConversationService->>TaskForceEngine: execute task phase
PhaseExecutionEngine->>MemberTurnExecutor: execute member turn
MemberTurnExecutor->>GroupContextBuilder: build member input
MemberTurnExecutor->>GroupSigningGuard: verify and sign messages
MemberTurnExecutor->>ConversationService: execute private or nested conversation
TaskForceEngine->>MemberTurnExecutor: execute assigned task
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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}
| return result != null ? result : ""; | ||
| } | ||
|
|
||
| public String buildPlainTextFallback(DiscussionPhase phase, GroupMember speaker, String question, List<TranscriptEntry> transcript) { |
There was a problem hiding this comment.
Pull request overview
This PR begins decomposing the GroupConversationService god-class into smaller, independently testable collaborators (Wave R / R1 steps 1–3), while keeping IGroupConversationService as the stable facade and preserving characterization-test reflection entry points. It also adds the Rev 2.1 implementation plan for upcoming group-collaboration features and documents the refactor/bugfix work in the changelog.
Changes:
- Extracted attachment handling, phase-input/context building, and signing/peer-verification logic into
GroupAttachmentBinder,GroupContextBuilder, andGroupSigningGuard, withGroupConversationServicedelegating. - Added focused unit tests for the extracted collaborators and moved the attachment tests out of
GroupConversationServiceTest. - Added
planning/group-collaboration-improvements-plan.mdand updateddocs/changelog.mdwith the refactor steps and signing verification fix context.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java | Replaces inline attachment/context/signing clusters with delegations to extracted collaborators; cleans up signing-cursor cleanup to call signingGuard.forgetConversation. |
| src/main/java/ai/labs/eddi/engine/internal/groups/GroupAttachmentBinder.java | New helper for attachment materialize/rehydrate/grant+inject behavior formerly inside the facade. |
| src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java | New helper for building per-phase input and transcript scoping; extracted from the facade. |
| src/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.java | New helper encapsulating inter-agent signing + incremental peer verification + verification cursor state. |
| src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java | Removes the attachment nested test class (moved to a dedicated binder test). |
| src/test/java/ai/labs/eddi/engine/internal/groups/GroupAttachmentBinderTest.java | New focused tests for GroupAttachmentBinder. |
| src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderTest.java | New focused tests for GroupContextBuilder. |
| src/test/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuardTest.java | New focused tests for GroupSigningGuard, including key-rotation regression coverage. |
| planning/group-collaboration-improvements-plan.md | Adds the Rev 2.1 plan and dependency graph for the broader group-collaboration roadmap. |
| docs/changelog.md | Documents the refactor steps and the signing verification/key-rotation fix findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+87
to
+93
| var resourceId = agentStore.getCurrentResourceId(agentId); | ||
| var agentConfig = agentStore.read(agentId, resourceId.getVersion()); | ||
| if (agentConfig.getSecurity() == null | ||
| || !agentConfig.getSecurity().isSignInterAgentMessages() | ||
| || response == null) { | ||
| return SigningResult.UNSIGNED; | ||
| } |
Comment on lines
+134
to
+139
| case PLAN -> { | ||
| // Provide member list for planning template | ||
| List<Map<String, Object>> memberList = new ArrayList<>(); | ||
| // Note: speaker list should be the full member list for planning | ||
| data.put("members", memberList); // populated by caller via template data | ||
| } |
![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/GroupConversationService.java (2)
409-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid allocating a
GroupAttachmentBinderper call.
attachmentBinder()builds a new instance on every attachment operation, includinggrantAndInjectAttachments, which runs on each member's first turn. The class is stateless, so the allocation buys nothing. The stated reason is thatattachmentStoreis field-injected and test-mutable.Read the field lazily inside a single cached binder instead, or inject the store through the constructor together with the other collaborators. See the related comment on the constructor.
🤖 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 409 - 411, Cache a single stateless GroupAttachmentBinder instead of constructing one in every attachment operation. Update attachmentBinder() to lazily initialize and reuse the binder while reading the current attachmentStore field, preserving test-time field mutability; alternatively, move attachmentStore to constructor injection as requested by the related constructor comment and initialize the binder once.
236-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire the three new collaborators as CDI beans. All three extracted components are instantiated by hand inside
GroupConversationService, so the container manages neither their lifetime nor their dependencies. The repository guidelines require Quarkus CDI for components. Annotating each class with@ApplicationScopedand an@Injectconstructor keeps the direct-construction unit tests working, because a CDI constructor is still a plain Java constructor.
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java#L236-L237: replace the twonewcalls with injectedGroupContextBuilderandGroupSigningGuardfields. This matters most forGroupSigningGuard, which owns thelastVerifiedIndexcursor and therefore must exist as exactly one instance.src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java#L409-L411: replace the per-callattachmentBinder()factory with a single injectedGroupAttachmentBinder, or have the binder read the injectedIAttachmentStorelazily so no instance is allocated on each member turn.As per coding guidelines: "Use Quarkus CDI (
@ApplicationScopedand@Inject) for components; do not manually register modules."🤖 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 236 - 237, Make GroupContextBuilder, GroupSigningGuard, and GroupAttachmentBinder CDI-managed with `@ApplicationScoped` and `@Inject` constructors, preserving direct-construction tests. In GroupConversationService.java lines 236-237, inject and reuse GroupContextBuilder and GroupSigningGuard instead of constructing them; at lines 409-411, inject and reuse one GroupAttachmentBinder rather than creating it per call, with lazy IAttachmentStore access if needed.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderTest.java (1)
180-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the selected template, not just non-null.
These four tests name a specific branch of
selectDefaultTemplate, butassertNotNullpasses for every branch. If theContextScope.NONEandContextScope.ANONYMOUSbranches were swapped, all four tests would still pass. Compare against the expectedDiscussionStylePresetsconstant instead.💚 Proposed assertions
`@Test` void selectDefaultTemplate_opinionNone_independentTemplate() { - assertNotNull(builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.NONE), List.of(), 0)); + assertEquals(DiscussionStylePresets.TEMPLATE_OPINION_INDEPENDENT, + builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.NONE), List.of(), 0)); } `@Test` void selectDefaultTemplate_opinionAnonymous_anonymousTemplate() { - assertNotNull(builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.ANONYMOUS), List.of(), 0)); + assertEquals(DiscussionStylePresets.TEMPLATE_OPINION_ANONYMOUS, + builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.ANONYMOUS), List.of(), 0)); } `@Test` void selectDefaultTemplate_opinionFull_withContextTemplate() { - assertNotNull(builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.FULL), List.of(), 0)); + assertEquals(DiscussionStylePresets.TEMPLATE_OPINION_WITH_CONTEXT, + builder.selectDefaultTemplate(phase(PhaseType.OPINION, ContextScope.FULL), List.of(), 0)); } `@Test` void selectDefaultTemplate_nonOpinion_usesPresetDefault() { - assertNotNull(builder.selectDefaultTemplate(phase(PhaseType.CRITIQUE, ContextScope.FULL), List.of(), 0)); + assertEquals(DiscussionStylePresets.defaultTemplate(PhaseType.CRITIQUE), + builder.selectDefaultTemplate(phase(PhaseType.CRITIQUE, ContextScope.FULL), List.of(), 0)); }Add the import for
ai.labs.eddi.configs.groups.model.DiscussionStylePresets.🤖 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/groups/GroupContextBuilderTest.java` around lines 180 - 198, Update the four selectDefaultTemplate tests in GroupContextBuilderTest to assert equality with the expected DiscussionStylePresets constant for each PhaseType and ContextScope branch, replacing assertNotNull. Add the DiscussionStylePresets import and use the independent, anonymous, contextual, and preset-default constants corresponding to the tested branches.
🤖 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 49: Update the characterization test class count and list in the
changelog entry so they agree: either add the omitted class name to the list or
change “four” to “three,” preserving the documented reflection behavior.
In `@planning/group-collaboration-improvements-plan.md`:
- Line 61: Update the signing summary near executeAgentTurn to qualify
NonceCacheService as sender-side nonce validation only. Remove the implication
that receipt verification currently provides replay protection, and mention
replay detection during verification as follow-up work.
- Around line 138-140: The dependency map for GroupAttachmentBinder,
GroupContextBuilder, and GroupSigningGuard does not match their extracted
constructors. Update the plan to list GroupAttachmentBinder as depending on
attachmentStore and defaultTenantId, GroupContextBuilder only on
templatingEngine, and GroupSigningGuard on agentStore, agentSigningService,
nonceCacheService, and defaultTenantId; remove IJsonSerialization from the first
two entries.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.java`:
- Around line 249-267: Update the peer-verification flow in GroupSigningGuard
around the publicKeyCache lookup to detect a null entry.signatureKeyVersion
before constructing cacheKey or calling getKeyForVersion. Treat entries with
missing signature key versions as unsigned/unresolvable without incrementing
failed++, while preserving the intentional zero-version behavior when key
metadata is absent.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 409-411: Cache a single stateless GroupAttachmentBinder instead of
constructing one in every attachment operation. Update attachmentBinder() to
lazily initialize and reuse the binder while reading the current attachmentStore
field, preserving test-time field mutability; alternatively, move
attachmentStore to constructor injection as requested by the related constructor
comment and initialize the binder once.
- Around line 236-237: Make GroupContextBuilder, GroupSigningGuard, and
GroupAttachmentBinder CDI-managed with `@ApplicationScoped` and `@Inject`
constructors, preserving direct-construction tests. In
GroupConversationService.java lines 236-237, inject and reuse
GroupContextBuilder and GroupSigningGuard instead of constructing them; at lines
409-411, inject and reuse one GroupAttachmentBinder rather than creating it per
call, with lazy IAttachmentStore access if needed.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderTest.java`:
- Around line 180-198: Update the four selectDefaultTemplate tests in
GroupContextBuilderTest to assert equality with the expected
DiscussionStylePresets constant for each PhaseType and ContextScope branch,
replacing assertNotNull. Add the DiscussionStylePresets import and use the
independent, anonymous, contextual, and preset-default constants corresponding
to the tested branches.
🪄 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 Plus
Run ID: 496fa6ec-49b1-4e71-93cd-24e8d4dbf202
📒 Files selected for processing (10)
docs/changelog.mdplanning/group-collaboration-improvements-plan.mdsrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupAttachmentBinder.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupAttachmentBinderTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuardTest.java
…rvice
R1 step 4 of planning/group-collaboration-improvements-plan.md -- the biggest
extraction yet, and the one GroupConversationServiceConcurrencyTest exists
specifically to pin. Moves both executeAgentTurn overloads,
tryResolveMemberToolPause, handleMemberPause, executeGroupMemberTurn,
handleAgentFailure and errorEntry into a new MemberTurnExecutor.
Three design decisions resolved during scoping:
- executeGroupMemberTurn recurses into the facade's own public discuss()/
cancelDiscussion() for nested GROUP-type members. Resolved by passing
`this` into MemberTurnExecutor's constructor (safe: it only stores the
reference, never invokes it during construction).
- propagateDynamicAgentTracking stays on GroupConversationService (widened to
public static) rather than moving -- DynamicAgentTrackingPropagationTest
calls it directly by class name, not via reflection, and the plan already
assigns it to a later step (GroupLifecycleOps).
- Attachment granting reaches back through the same self-reference to the
facade's grantAndInjectAttachments (widened to public) rather than giving
MemberTurnExecutor its own IAttachmentStore and duplicating the facade's
per-call GroupAttachmentBinder construction.
Also widened MemberTurnCancellation/MemberTurnCancelledException to public
(MemberTurnExecutor lives in a different package and references them in its
own signatures).
The reflection sweep needed a second pass -- the first only grepped
getDeclaredMethod("...") and missed this codebase's actual method("...")
test-helper-wrapper pattern, which turned up real dependencies in two files
the first pass said were clean. All six moved methods kept as thin
delegators as a result.
Full 12-class group suite + DynamicAgentTrackingPropagationTest (22 tests,
unmodified) + 4 focused collaborator test classes green on the first run
after compile succeeded, including GroupConversationServiceConcurrencyTest
(8/8). Added a focused MemberTurnExecutorTest for the pure-function methods.
GroupConversationService: 3,977 -> 3,605 lines.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.java:90
- signOutgoingMessage() fetches the agent resource/config before checking for a null response, and it assumes agentStore.getCurrentResourceId(agentId) is non-null. When response is null (or the agent has no current resource id / config), this does unnecessary store IO and typically falls into the catch-all warning path, producing noisy logs for a normal “unsigned” outcome. Add early returns for response==null and resourceId/config==null, and keep the security flag check after those guards.
try {
var resourceId = agentStore.getCurrentResourceId(agentId);
var agentConfig = agentStore.read(agentId, resourceId.getVersion());
if (agentConfig.getSecurity() == null
|| !agentConfig.getSecurity().isSignInterAgentMessages()
docs/changelog.md:12
- PR description/title says this PR covers R1 steps 1–3, but this changelog entry states it includes R1 step 4 (MemberTurnExecutor extraction). Please align the PR metadata (title/description) with the actual scope of changes, or split step 4 into its own PR if the intent was to keep this PR limited to steps 1–3.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java (1)
549-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the cause into the thrown
GroupDiscussionException.This throw drops
cause, so the original stack trace is lost for every ABORT failure, including sub-group failures routed here from Line 541. Lines 160 and 179 already use the cause-carrying constructor of the same exception type.cause.getMessage()can also benullfor exceptions without a message.♻️ Proposed refactor to preserve the cause
if (protocol.onAgentFailure() == ProtocolConfig.MemberFailurePolicy.ABORT) { throw new GroupDiscussionException( - "%s for agent %s and onAgentFailure=ABORT: %s".formatted(prefix, member.agentId(), cause.getMessage())); + "%s for agent %s and onAgentFailure=ABORT: %s".formatted(prefix, member.agentId(), cause.getMessage()), cause); }🤖 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/groups/MemberTurnExecutor.java` around lines 549 - 552, Update the ABORT branch in MemberTurnExecutor’s failure handling to construct GroupDiscussionException with cause as the chained throwable, preserving the existing context message while retaining failures with null messages. Match the cause-carrying constructor usage already established at the other GroupDiscussionException call sites.src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer mocks over
nullfor the helper collaborators.
GroupSigningGuardandGroupContextBuilderreceivenulldependencies. The four current tests exerciseerrorEntryandhandleAgentFailure, which touch neither the agent store nor the templating engine, so the fixture works today. If a later change makes either method reach a collaborator, the test fails with a bareNullPointerExceptioninstead of an unmet-stub message. Mocks keep the failure diagnosable and let the fixture be reused for more paths.♻️ Proposed fixture change
private MemberTurnExecutor executor() { return new MemberTurnExecutor( Mockito.mock(IConversationService.class), Mockito.mock(IAgentFactory.class), - new GroupSigningGuard(null, null, null, "default"), - new GroupContextBuilder(null), + new GroupSigningGuard( + Mockito.mock(IAgentStore.class), + Mockito.mock(AgentSigningService.class), + Mockito.mock(NonceCacheService.class), + "default"), + new GroupContextBuilder(Mockito.mock(ITemplatingEngine.class)), Mockito.mock(GroupConversationService.class), new SimpleMeterRegistry().counter("test.member.pause.skipped"), 180, 2); }Add the matching imports for
IAgentStore,AgentSigningService,NonceCacheService, andITemplatingEngine.🤖 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/groups/MemberTurnExecutorTest.java` around lines 44 - 53, Update the executor() test fixture to replace the null dependencies passed to GroupSigningGuard and GroupContextBuilder with Mockito mocks of IAgentStore, AgentSigningService, NonceCacheService, and ITemplatingEngine, adding the corresponding imports while preserving the existing constructor setup.
🤖 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/internal/groups/MemberTurnExecutor.java`:
- Around line 444-455: Update the graceful resume path in MemberTurnExecutor
around the tool-less contribution construction to call
signingGuard.signOutgoingMessage for the generated response, then populate the
returned signature, nonce, timestampMs, and keyVersion in TranscriptEntry.
Preserve the existing UNSIGNED behavior when signing is disabled and keep the
current response fallback and logging unchanged.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`:
- Around line 549-552: Update the ABORT branch in MemberTurnExecutor’s failure
handling to construct GroupDiscussionException with cause as the chained
throwable, preserving the existing context message while retaining failures with
null messages. Match the cause-carrying constructor usage already established at
the other GroupDiscussionException call sites.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java`:
- Around line 44-53: Update the executor() test fixture to replace the null
dependencies passed to GroupSigningGuard and GroupContextBuilder with Mockito
mocks of IAgentStore, AgentSigningService, NonceCacheService, and
ITemplatingEngine, adding the corresponding imports while preserving the
existing constructor setup.
🪄 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 Plus
Run ID: 1e2d6de6-128e-4b1f-8df9-2ee08d9c5748
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.javasrc/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java
…Service R1 step 5 of planning/group-collaboration-improvements-plan.md: move the debate-style turn-order executors -- executeSequentialPhase, executeParallelPhase, executePeerTargetedPhase -- into a new PhaseExecutionEngine. TASK_FORCE's PLAN/EXECUTE/VERIFY routing stays on the facade (separate cluster, R1 step 6). Deliberately skipped the plan's speculative PhaseExecutor/PhaseOutcome/ PhaseExitSignal interface -- that shape assumes F2/ResumePoint and I2/convergence, neither of which exists yet since R1 runs before Wave 0/1. Moved the three methods as concrete methods on a plain class; the interface is a decision for whenever F2/I2 actually need it. PhaseExecutionEngine takes the facade's ExecutorService by reference, not ownership (TaskForceEngine's not-yet-extracted waves share it; the facade keeps the @PreDestroy shutdown hook). parallelBatchBudgetSeconds stays on the facade, widened to public static, called back cross-package. Reflection sweep found a third distinct test-helper-wrapper naming convention (phaseMethod(name), case-sensitively distinct from the method(name) pattern used elsewhere) that broke the first test run with a NoSuchMethodException. Fixed by re-sweeping with a bare-token grep instead of guessing at wrapper names -- adopting that as the standard approach going forward. executeParallelPhase kept as a delegator as a result; executeSequentialPhase/executePeerTargetedPhase confirmed to have no test dependency and were fully inlined. Also caught and fixed a bug in the new PhaseExecutionEngineTest itself (a mock stub hardcoding targetAgentId=null instead of threading through the real argument) before it ever reached CI. Full 12-class group suite + DynamicAgentTrackingPropagationTest + 5 focused collaborator test classes green (540 tests). GroupConversationService: 3,605 -> 3,432 lines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (1)
254-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConfirm the manual construction of the helper components is intended.
GroupContextBuilder,GroupSigningGuard,MemberTurnExecutor, and nowPhaseExecutionEngineare constructed withnewinside the constructor. The coding guidelines require Quarkus CDI for components. The comments explain that direct construction keeps the existing direct-construction unit tests working, so this looks deliberate. If CDI beans are planned for a later wave, record that follow-up in the plan document so the deviation stays visible.As per coding guidelines: "Use Quarkus CDI (
@ApplicationScopedand@Inject) for components; do not manually register modules."🤖 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 254 - 262, Confirm that manual construction of GroupContextBuilder, GroupSigningGuard, MemberTurnExecutor, and PhaseExecutionEngine is intentional, and document the CDI migration as a follow-up in the plan document if it is deferred. Keep the existing direct-construction behavior and unit-test compatibility unchanged.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java (1)
130-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a partial parallel-budget test.
parallelPhase_zeroRemainingBudget_noopcovers only the fully exhausted budget. ThesubList(0, min(speakers.size(), remainingTurns))cap atPhaseExecutionEnginelines 100-102 stays untested, and it is the branch where an off-by-one would appear. Add a case with a partial budget, for example 3 speakers andmaxTurns2 withturnCounter0, then assert two transcript entries andturnCounterequal to 2.♻️ Proposed additional test
`@Test` void parallelPhase_partialBudget_capsBatchToRemainingTurns() throws Exception { var engine = engine(); when(memberTurnExecutor.executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any(), any())) .thenAnswer(inv -> opinionEntry(((GroupMember) inv.getArgument(0)).agentId())); var speakers = List.of(member("a"), member("b"), member("c")); var gc = gc(); var turnCounter = new AtomicInteger(0); engine.executeParallelPhase(gc, new AgentGroupConfiguration(), speakers, phase(TurnOrder.PARALLEL), protocol(), "Q?", 0, null, turnCounter, 2); assertEquals(2, gc.getTranscript().size()); assertEquals(2, turnCounter.get()); verify(memberTurnExecutor, times(2)).executeAgentTurn(any(), eq(gc), any(), any(), eq(0), any(), isNull(), isNull(), any()); }🤖 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/groups/PhaseExecutionEngineTest.java` around lines 130 - 141, Add a partial-budget test alongside parallelPhase_zeroRemainingBudget_noop that invokes executeParallelPhase with three speakers, turnCounter starting at 0, and maxTurns 2; stub memberTurnExecutor to return entries, then assert exactly two transcript entries, turnCounter equals 2, and the executor is called twice.
🤖 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/internal/groups/PhaseExecutionEngine.java`:
- Around line 231-233: Update the comment immediately above the allMembers
stream in PhaseExecutionEngine to state that it collects and sorts all
configured group members, including the moderator; leave the implementation
unchanged unless the surrounding logic explicitly requires excluding moderators.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 254-262: Confirm that manual construction of GroupContextBuilder,
GroupSigningGuard, MemberTurnExecutor, and PhaseExecutionEngine is intentional,
and document the CDI migration as a follow-up in the plan document if it is
deferred. Keep the existing direct-construction behavior and unit-test
compatibility unchanged.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java`:
- Around line 130-141: Add a partial-budget test alongside
parallelPhase_zeroRemainingBudget_noop that invokes executeParallelPhase with
three speakers, turnCounter starting at 0, and maxTurns 2; stub
memberTurnExecutor to return entries, then assert exactly two transcript
entries, turnCounter equals 2, and the executor is called twice.
🪄 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 Plus
Run ID: 6ae141f4-0e12-41ad-a4ed-6bc04134e2b7
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.javasrc/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.java:93
- signOutgoingMessage() assumes agentStore.getCurrentResourceId(agentId) is non-null and dereferences it immediately. When it is null (e.g., agent deleted / store returns no current version), this throws and is caught, but it logs a WARN and treats it like a signing failure. This is a normal “unsigned” condition and should short-circuit without exception/log noise; also response==null can be checked before hitting the store.
try {
var resourceId = agentStore.getCurrentResourceId(agentId);
var agentConfig = agentStore.read(agentId, resourceId.getVersion());
if (agentConfig.getSecurity() == null
|| !agentConfig.getSecurity().isSignInterAgentMessages()
|| response == null) {
return SigningResult.UNSIGNED;
}
src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java:102
- In executeParallelPhase(), maxTurns<=0 currently falls back to treating the budget as unlimited (remainingTurns = speakers.size()), which is inconsistent with executeSequentialPhase() / reserveTurn() semantics where maxTurns==0 means “no turns”. This can cause a parallel phase to run even when the caller intends a zero budget (and makes unit semantics inconsistent).
// Cap batch size to remaining turn budget
int remainingTurns = maxTurns > 0 ? Math.max(0, maxTurns - turnCounter.get()) : speakers.size();
if (remainingTurns == 0) {
return;
}
List<GroupMember> batchSpeakers = maxTurns > 0
? speakers.subList(0, Math.min(speakers.size(), remainingTurns))
: speakers;
R1 step 6 of planning/group-collaboration-improvements-plan.md -- the largest and most concurrency-sensitive extraction in Wave R: the entire TASK_FORCE PLAN/EXECUTE/VERIFY cluster (~840 lines) into a new TaskForceEngine. This is the code GroupConversationServiceConcurrencyTest exists to pin -- the taskList -> transcript lock order, the recordTaskFailure/notifyTaskFailure split, and resetStrandedInProgressTasks's compare-and-set-under-monitor sweep all moved verbatim. Did the bare-token reflection sweep first this time (the step-5 lesson). Found 12 of 16 methods reflected -- more than any prior step -- including several reached via a direct multi-line getDeclaredMethod(...) call that a single-line pattern grep can't see. All 12 kept as delegators; the other 4 (executeTaskPlanPhase, abortWave, stringOrNull, notifyTaskFailure) had zero matches anywhere and were fully inlined. A 13th reflected method, reserveTurn, was hiding just outside the cluster's own banner (in the prior "cooperative cancellation" section) and is reflected as a static invocation (invoke(null, ...)). Missed by the line-range-scoped search, caught before compiling by checking every method actually called from the code being moved rather than trusting banner boundaries. Moved to TaskForceEngine with a static delegator left behind, same pattern as propagateDynamicAgentTracking in step 4. Two bugs caught before reaching CI: a transcription error where I invented non-existent ...ForTest-suffixed delegator method names instead of matching TaskForceEngine's real ones (caught re-reading the diff, not by the compiler); and a stale-TaskItem bug in the new TaskForceEngineTest itself (TaskItem is an immutable record -- completeTask() returns a new instance rather than mutating the one already held locally, so a test was checking status==COMPLETED against a stale PENDING snapshot). Full 12-class group suite + DynamicAgentTrackingPropagationTest + 6 focused collaborator test classes green (555 tests), including GroupConversationServiceConcurrencyTest (8/8) and GroupConversationServiceTaskForceTest (20/20). GroupConversationService: 3,432 -> 2,594 lines. 6 of R1's 10 steps done; GroupConversationService is now smaller than both AgentOrchestrator (2,725) and ConversationService (2,698).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/GroupSigningGuard.java:129
- signOutgoingMessage() can still persist a signed envelope even when no public key is available for the chosen keyVersion (publicKey == null). That skips the self-verification step described in the method’s contract, and produces transcript entries that peers cannot verify (verifyPriorEntriesIfRequired will log failures / cache null). Consider failing closed: if the key for the signed keyVersion cannot be resolved, drop the signature and return UNSIGNED (optionally with a warning log).
String publicKey = agentConfig.getIdentity() != null
? agentConfig.getIdentity().getKeyForVersion(keyVersion)
: null;
if (publicKey != null) {
boolean valid = agentSigningService.verifyEnvelope(signedEnvelope, publicKey);
docs/changelog.md:12
- PR title/description say this PR covers R1 steps 1–3, but this diff includes R1 steps 4–6 as well (new MemberTurnExecutor, PhaseExecutionEngine, TaskForceEngine plus tests, and changelog entries for steps 4–6). Please either update the PR metadata to reflect the actual scope, or split steps 4–6 into a separate PR to match the stated review slice.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineTest.java (3)
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the setup to
@BeforeEachand close the executor.
engine()assigns thetemplatingEngineandjsonSerializationfields as a side effect of building the engine. Tests that callengine()inline and then assert on those fields depend on that ordering.tryParseVerificationJson_noBrackets_returnsFalseWithoutCallingDeserializerat Line 171 to Line 174 is the case:verifyNoInteractions(jsonSerialization)passes only because the field was reassigned during the inlineengine()call.
engine()also creates a newExecutors.newVirtualThreadPerTaskExecutor()per call and never closes it.Build the mocks and the engine in a
@BeforeEachmethod, and close the executor in@AfterEach.♻️ Proposed test setup
private ITemplatingEngine templatingEngine; private IJsonSerialization jsonSerialization; + private ExecutorService executorService; + private TaskForceEngine engine; - private TaskForceEngine engine() { + `@BeforeEach` + void setUp() { templatingEngine = mock(ITemplatingEngine.class); jsonSerialization = mock(IJsonSerialization.class); - return new TaskForceEngine(mock(MemberTurnExecutor.class), templatingEngine, jsonSerialization, - Executors.newVirtualThreadPerTaskExecutor(), new CallerIdentityContext(null, null), - new java.util.concurrent.ConcurrentHashMap<>(), 180, 5); + executorService = Executors.newVirtualThreadPerTaskExecutor(); + engine = new TaskForceEngine(mock(MemberTurnExecutor.class), templatingEngine, jsonSerialization, + executorService, new CallerIdentityContext(null, null), + new ConcurrentHashMap<>(), 180, 5); + } + + `@AfterEach` + void tearDown() { + executorService.close(); }Then replace each
engine()call with theenginefield.🤖 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/groups/TaskForceEngineTest.java` around lines 50 - 56, Move creation of templatingEngine, jsonSerialization, the virtual-thread executor, and TaskForceEngine from engine() into a `@BeforeEach` setup method, storing the executor and engine in fields; replace every engine() invocation with the initialized engine field. Add an `@AfterEach` method that closes the executor to prevent per-test resource leaks.
201-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the reviewer-feedback append.
buildTaskExecutionInputappendstask.verificationNote()to the prompt at Lines 746-748 ofTaskForceEngine.java. That append drives the RETRY rejection policy, so a regression silently removes the reviewer feedback from the re-execution prompt. No test asserts it.Add one case with a non-blank
verificationNote.💚 Proposed additional test
+ `@Test` + void buildTaskExecutionInput_withVerificationNote_appendsReviewerFeedback() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + var base = new TaskItem("Write the report", "cover Q1 results", 1); + var rejected = new TaskItem(base.id(), base.subject(), base.description(), + base.status(), base.assignedAgentId(), base.assignedDisplayName(), + base.dependsOnIds(), base.result(), "missing the revenue table", + false, base.priority(), base.createdAt(), base.completedAt()); + var phase = new DiscussionPhase("EXEC", PhaseType.EXECUTE, "ALL", TurnOrder.SEQUENTIAL, ContextScope.NONE, false, null, 1, false); + var engine = engine(); + when(templatingEngine.processTemplate(any(), any(), any())).thenReturn("Task prompt"); + + String input = engine.buildTaskExecutionInput(rejected, "Q?", phase, gc); + + assertTrue(input.contains("missing the revenue table")); + }🤖 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/groups/TaskForceEngineTest.java` around lines 201 - 215, Add a focused test alongside buildTaskExecutionInput_templateFailure_fallsBackToPlainText that creates a TaskItem with a non-blank verificationNote, invokes TaskForceEngine.buildTaskExecutionInput, and asserts the returned prompt contains that reviewer feedback. Keep the existing template-failure coverage unchanged and verify the appended note is preserved in the re-execution input.
95-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the direct agentId branch.
resolveTaskAssignmenthas three branches: ALL or null,ROLE:prefix, and a direct agentId reference at Line 952 ofTaskForceEngine.java. The direct agentId branch delegates toTaskListParser.resolveAgentand has no test here.Add one case that passes an agentId as
assignToRole.💚 Proposed additional test
`@Test` void resolveTaskAssignment_allWithNoEligibleMembers_fallsBackToFirstMember() { var members = List.of(member(MODERATOR)); assertEquals(MODERATOR, engine().resolveTaskAssignment("ALL", members, MODERATOR, 0)); } + + `@Test` + void resolveTaskAssignment_directAgentIdReference_resolvesThatMember() { + var members = List.of(member(MODERATOR), member(AGENT_A), member(AGENT_B)); + assertEquals(AGENT_B, engine().resolveTaskAssignment(AGENT_B, members, MODERATOR, 0)); + }🤖 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/groups/TaskForceEngineTest.java` around lines 95 - 99, Add a test in TaskForceEngineTest covering resolveTaskAssignment’s direct agentId path: pass a concrete agentId as assignToRole, provide matching members, and assert the returned assignment resolves to that agent through TaskListParser.resolveAgent. Keep the existing ALL and ROLE: branch tests unchanged.src/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.java (1)
814-817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the
passedcoercion into one helper.Lines 814-817 and Lines 890-893 duplicate the same three-line coercion.
tryParseVerificationJsonuses it to set persisted task state.formatVerificationForDisplayuses it to render the pass or fail marker to the user. If one copy changes, the transcript shows a result that does not match the stored task status.Extract a single private static helper and call it from both sites.
♻️ Proposed shared helper
- // Read 'passed' boolean directly from JSON - boolean passed = true; // default to passed - if (map.containsKey("passed")) { - Object passedVal = map.get("passed"); - passed = Boolean.TRUE.equals(passedVal) || "true".equalsIgnoreCase(String.valueOf(passedVal)); - } + boolean passed = readPassed(map);Apply the same replacement at Lines 890-893, and add the helper next to
stringOrNull:/** * Reads the {`@code` passed} flag from a deserialized verification item. * An absent key defaults to {`@code` true}. */ private static boolean readPassed(Map<?, ?> map) { if (!map.containsKey("passed")) { return true; } Object passedVal = map.get("passed"); return Boolean.TRUE.equals(passedVal) || "true".equalsIgnoreCase(String.valueOf(passedVal)); }🤖 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/groups/TaskForceEngine.java` around lines 814 - 817, Extract the duplicated passed-flag coercion from tryParseVerificationJson and formatVerificationForDisplay into one private static readPassed(Map<?, ?>) helper near stringOrNull. Preserve the existing behavior: return true when "passed" is absent, otherwise accept Boolean.TRUE or the case-insensitive string "true", and replace both inline implementations with calls to the helper.src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (1)
256-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider CDI beans for the extracted collaborators.
GroupContextBuilder,GroupSigningGuard,MemberTurnExecutor,PhaseExecutionEngine, andTaskForceEngineare instantiated withnewinside the constructor. The coding guidelines require Quarkus CDI for components.@ApplicationScopedbeans plus@Injectwould remove the manual wiring order constraints documented at Lines 256-261, and would keep the dependency graph visible to the container. The direct-construction unit tests can keep working through constructor injection on the beans themselves.Note that this change also affects
TaskForceEngine's access toactiveTokens: a CDI-managed engine would need that map passed per call or owned by a shared holder rather than injected as constructor state.As per coding guidelines: "Use Quarkus CDI (
@ApplicationScopedand@Inject) for components; do not manually register modules."🤖 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 256 - 266, Convert GroupContextBuilder, GroupSigningGuard, MemberTurnExecutor, PhaseExecutionEngine, and TaskForceEngine into CDI-managed `@ApplicationScoped` components with constructor `@Inject` dependencies, and inject them into GroupConversationService instead of constructing them with new. Remove the constructor-order workaround comments and preserve direct constructor usability for unit tests. Refactor TaskForceEngine so activeTokens is supplied per invocation or through a shared holder rather than constructor state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 14: Update the reflected-method count in the changelog sentence to match
the listed methods: replace “two” with “four” while preserving the remaining
description and totals.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.java`:
- Around line 770-778: Update the fallback loop in parseAndApplyVerification to
re-fetch each task from gc.getTaskList() before checking its status or calling
verifyTask. Use the live task state to process only tasks still in COMPLETED
status, while preserving the existing verification event behavior for tasks that
are actually verified.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 256-266: Convert GroupContextBuilder, GroupSigningGuard,
MemberTurnExecutor, PhaseExecutionEngine, and TaskForceEngine into CDI-managed
`@ApplicationScoped` components with constructor `@Inject` dependencies, and inject
them into GroupConversationService instead of constructing them with new. Remove
the constructor-order workaround comments and preserve direct constructor
usability for unit tests. Refactor TaskForceEngine so activeTokens is supplied
per invocation or through a shared holder rather than constructor state.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.java`:
- Around line 814-817: Extract the duplicated passed-flag coercion from
tryParseVerificationJson and formatVerificationForDisplay into one private
static readPassed(Map<?, ?>) helper near stringOrNull. Preserve the existing
behavior: return true when "passed" is absent, otherwise accept Boolean.TRUE or
the case-insensitive string "true", and replace both inline implementations with
calls to the helper.
In `@src/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineTest.java`:
- Around line 50-56: Move creation of templatingEngine, jsonSerialization, the
virtual-thread executor, and TaskForceEngine from engine() into a `@BeforeEach`
setup method, storing the executor and engine in fields; replace every engine()
invocation with the initialized engine field. Add an `@AfterEach` method that
closes the executor to prevent per-test resource leaks.
- Around line 201-215: Add a focused test alongside
buildTaskExecutionInput_templateFailure_fallsBackToPlainText that creates a
TaskItem with a non-blank verificationNote, invokes
TaskForceEngine.buildTaskExecutionInput, and asserts the returned prompt
contains that reviewer feedback. Keep the existing template-failure coverage
unchanged and verify the appended note is preserved in the re-execution input.
- Around line 95-99: Add a test in TaskForceEngineTest covering
resolveTaskAssignment’s direct agentId path: pass a concrete agentId as
assignToRole, provide matching members, and assert the returned assignment
resolves to that agent through TaskListParser.resolveAgent. Keep the existing
ALL and ROLE: branch tests unchanged.
🪄 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 Plus
Run ID: 52f66241-d774-49f3-8f52-df2c3342cb3c
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.javasrc/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineTest.java
…teps 4-6 Independent review of R1 steps 4-6 (MemberTurnExecutor, PhaseExecutionEngine, TaskForceEngine) before starting step 7. One stale class-Javadoc paragraph fixed; one pre-existing concurrency-test-coverage gap flagged as a follow-up task rather than fixed inline.
…Service R1 step 7: unite the pause/cancel/timeout helpers and the cancelDiscussion/ resumeDiscussion public surface into one collaborator. Widens executeDiscussion, resolvePhases and cleanupEphemeralAgents to public so the coordinator can call back into the facade, same self-reference pattern as MemberTurnExecutor. GroupConversationService: 2,594 -> 1,885 lines. 7 of R1's 10 steps done.
…er's tool From PR review comments. McpToolsProvider.discover added every spec to a list while writing executors into a map, so two MCP servers exposing the same tool name left two specs and only the LAST server's executor. ToolSourceRegistry then keeps the FIRST spec and looks the executor up by name -- pairing server A's signature with server B's implementation, so the model is shown one tool's contract while a different tool runs. Now first-write-wins within the provider, a spec is only added when its executor is present, and the collision is logged with the toolsBlacklist remedy named. Also removed a null guard of mine in addDynamicAgentTools that implied a nullability the method does not honour three lines earlier. Declined with reasoning: getRecruitedAgentIds "exposes internal state" -- RecruitAgentTool synchronizes on that list to make its cap check atomic and it is a CopyOnWriteArrayList; a defensive copy would break the mutator. Suite at baseline: 8 failures / 294 errors, all environmental.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 107 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:93
- There are two consecutive Javadoc blocks here, but only the second one is attached to a member. The first block (ending at line 93) is orphaned and will show up as stray documentation (and can trip Javadoc/checkstyle rules). Remove the first Javadoc block or merge its content into the following one.
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
…e-split # Conflicts: # docs/changelog.md
The plan read as if none of it had been built. Marks Wave R, Wave 0, Wave 1 (I1-I4) and I5/I7 as done, and records the known gaps in what shipped so a follow-up branch starts from truth rather than from the original intent -- notably that I1's ceiling cannot fire for an ordinary group (non-cascade model calls price at zero), that decision_reached has no producer, and that nested-group cost is overwritten rather than accumulated. Also records the two deliberate deviations from the plan's design (I3's two-sided-roster + impartial-judge precondition, I5 assigning at file time) so they read as decisions rather than drift.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 107 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:126
get(String groupConversationId)currently has no Javadoc even though the class-level comments describe important semantics ("live only while running", callers should treat absence as non-running). Adding a short method-level Javadoc makes the contract easier to discover at the call sites.
public Optional<GroupConversation> get(String groupConversationId) {
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:97
- There is an orphaned/duplicated Javadoc block here (two
/** ... */blocks back-to-back), so the first block isn’t attached to any method and the combined comment is harder to read/maintain.
This issue also appears on line 126 of the same file.
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java:340
- This return expression is correct but unnecessarily hard to read (
versioned != null || version != 0 ? ...). Refactoring to an explicit early-return makes the intended fallback behavior (version 0 →publicKey, others → null) much clearer and reduces the risk of future mistakes.
String versioned = keys.stream()
.filter(k -> k.version() == version)
.map(ai.labs.eddi.configs.agents.crypto.AgentPublicKey::publicKeyB64)
.findFirst()
.orElse(null);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 107 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:98
- There are two adjacent Javadoc blocks here; the first one appears to describe
get(...)but is currently attached togetForMember(...), leaving a redundant/incorrect doc comment. This is confusing for readers and can also trip Javadoc/checkstyle rules that expect exactly one Javadoc per member.
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
…next The implementation plan is 500+ lines of design and carried no status, so resuming meant re-deriving what had shipped from git history. Adds planning/group-collaboration-NEXT.md as the sequencing authority and collapses the status block added to the plan earlier today into a pointer -- two files tracking status drift, and the stale one is the one that gets read. The plan stays the design reference. Records the two defects to fix before any new feature (unpriced ordinary model calls, which make I1's ceiling unable to fire; I9 windowing), the ordered queue with dependencies and the reasoning behind the order, the off-critical-path gaps including the two whose obvious fixes were tried and rejected, and the conventions that cost time on this branch.
Traced all three non-cascade paths rather than assuming: agent/tool loop, legacy chat and streaming legacy chat each already emit tokenUsage, and all three produce the exact keys computeCost reads. Confirms the fix is config-field-only with no token-counting plumbing, and saves the next session re-deriving it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 108 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:94
- There are two consecutive Javadoc blocks here; the first one (lines 87–93) is a dangling Javadoc that isn’t attached to any declaration. This is easy to miss in reviews and can trigger Javadoc/Checkstyle noise; it should be removed or merged with the next block.
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
Fifth review pass, aimed at the least-reviewed high-blast-radius code rather than re-walking the group tools. GroupConversation.schemaVersion defaults to CURRENT_SCHEMA_VERSION, so the pre-F6 documents in production -- which have no such key -- load claiming schema 3 while being version-1-shaped, and prepareForResume loops zero times. The migration ladder never runs on the documents F6 exists to protect. Verified by deserialising a key-less document. Zero impact today (MIGRATIONS is empty, no released build ever wrote a version), so deliberately NOT fixed here: it cannot fire, and pushing it would invalidate a green CI. Flagged do-it-before-#626-ships because the fix is free only while no production document carries a version. Filed with its two companions -- stale MIGRATIONS Javadoc, and a test suite that never covers the key-less document, which is why this survived four rounds. Also records three suspicions traced to ground and cleared, so the next reviewer does not repeat the work.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 108 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:93
- There are two consecutive Javadoc blocks before getForMember(). The first one (describing get()) is redundant and isn’t attached to any method, which is easy to miss and can trip Javadoc/Checkstyle tooling. Remove the orphaned block (or merge its content into the get()/getForMember() docs).
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java:33
- GroupConversationSchemaMigrations’ Javadoc claims GroupConversation#CURRENT_SCHEMA_VERSION is 1 and “the first version that has ever existed”, but GroupConversation currently defines CURRENT_SCHEMA_VERSION as 3. This is misleading for future schema bumps (and for understanding why MIGRATIONS is empty). Update the comment to avoid hardcoding an incorrect value and instead state that no migrations are currently registered (identity fallback applies).
* Migration functions, keyed by the version they upgrade <em>from</em> (so
* entry {@code N} takes a version-{@code N} document to version {@code N+1}).
* Empty today — {@link GroupConversation#CURRENT_SCHEMA_VERSION} is {@code 1},
* the first version that has ever existed, so there is nothing yet to migrate
* from. Every future Wave item that adds a resume-relevant field bumps
…e-split # Conflicts: # src/main/java/ai/labs/eddi/engine/internal/ConversationService.java # src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java
| // this path (the 404 check above uses the cheaper getConversationState), and | ||
| // it is skipped entirely when toolDecisions is absent so the overwhelmingly | ||
| // common plain-verdict resume incurs no extra load. | ||
| if (decision != null && decision.getToolDecisions() != null && !decision.getToolDecisions().isEmpty()) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 112 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/internal/groups/LiveDiscussionRegistry.java:93
- There are two Javadoc blocks back-to-back here; the first one (lines 87–93) is not attached to any declaration, which is confusing and can trip Javadoc/Checkstyle tooling. Either remove it or move it to document get(...).
/**
* The live instance for a running discussion, or empty if it is not currently
* running (paused, finished, or never started on this node — group control is
* per-node, like {@code activeTokens}). Callers resolve this via the
* {@code groupConversationId} context var and must turn an empty result into an
aisabella-ai
approved these changes
Aug 7, 2026
This was referenced Aug 7, 2026
Merged
This was referenced Aug 8, 2026
Closed
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
Wave R of
planning/group-collaboration-improvements-plan.mdis complete. All three god-classes are decomposed, ahead of the ~18 feature items the plan lands on top of them.GroupConversationServiceAgentOrchestratorConversationServiceCheckstyle went 8 violations → 6, and every remaining one is a pre-existing
LineLengthin a file this branch never touched. There is noFileLengthviolation left anywhere in the codebase.Every extraction is a behaviour-preserving pure move. The three classes remain the facades (
IGroupConversationService/IConversationServiceunchanged) and delegate. Many methods stay as thin declared delegators specifically because characterization tests reach them viagetDeclaredMethod(...), which resolves by exact parameter types — removing them breaks the safety net even though nothing in production would notice. Per plan rule 3.0-4, collaborators are plain classes constructed by the facade, not CDI beans, because ~34 test classes construct these services directly.R1 —
GroupConversationService→ 8 collaboratorsGroupAttachmentBinder·GroupContextBuilder·GroupSigningGuard·MemberTurnExecutor·PhaseExecutionEngine·TaskForceEngine·GroupHitlCoordinator·GroupLifecycleOpsPlus one deliberate behaviour change in its own commit (plan rule 3.0-1): group discussions now participate in graceful shutdown via
rejectIfShuttingDown()on every entry point that starts, continues or resumes a discussion →RejectedExecutionException, already mapped to HTTP 503.R2 —
AgentOrchestrator→ theToolSourceProviderSPIAll eight tool sources are providers now, and
buildToolSetupiterates them rather than calling each by name. Adding a tool source is adding a provider — the property that gates Wave 2's I5/I7/I17.BuiltinToolsProvider·ContextualToolsProvider·AttachmentToolsProvider·DynamicAgentToolsProvider·HttpCallToolsProvider·McpToolsProvider·A2AToolsProvider, assembled byToolSourceRegistry, withToolContextBudget,ToolApprovalGateSupport,ToolLoopRunner,ToolLoopResumerand theIAgentOrchestratorinterface alongside.Three properties worth calling out:
ToolSourceRegistrywraps everycontributecall, catchingThrowable(the realistic non-ExceptionisNoClassDefFoundErrorfrom an optional integration whose dependency is absent at runtime).toolSourcestags come from the tool's own class, not the provider'ssource()— stamping one tag over a contribution that legitimately spansmemory/recall/builtinwould silently unmatch arequire: ["memory:*"]approval pattern.R3 —
ConversationService→ConversationHitlService+ConversationStepRunnerThe HITL cluster was the back ~43% and had almost nothing to do with the front. What remained was two things wearing one name: the public
IConversationServicesurface, and the machinery that executes a turn.Bugs found and fixed along the way
GroupSigningGuard) — resolved a signer's public key via "whichever key is valid right now" instead of the exact version a signature declares, at both call sites. During a rotation overlap window (a scenario the key model explicitly supports) this could self-discard a good signature or verify against the wrong key. The peer-verify cache was also keyed by agent id alone, so a second entry signed with a different key version reused the first entry's cached key.MemberTurnExecutor) — when a group auto-rejected a member's gated tool call, the resulting transcript entry was built without a signature, while the normal path three lines away signed an identically-shaped entry that peers verify identically.TaskForceEngine) — a verifier LLM repeating a subject re-verified an already-verified task, and the resultingIllegalStateExceptionescaped from a loop sitting outside the enclosingtry, losing the verifier's transcript entry and its event.GroupHitlCoordinator) —notifyCancelledran inside the state-revertingcatch, so an SSE sink on a closed stream desynced memory from the store, and the cleanup path reads memory.snippets/varsunprotected (HttpCallToolsProvider, reported by Copilot) — a prompt-injected tool argument could shadow the deployment-config namespace that httpcall templates read.maxCreatedAgentsPerDiscussionstarted empty on everybuildToolListcall, so a 5-member × 3-phase discussion with the default cap of 5 could deploy up to 75 agents to production.GroupSigningGuard, missingenableBuiltInToolsgates on two providers, a null-returningextractTextdereferenced inside a keep-the-turn-alive fallback, and three logging/privacy fixes.A graceful-shutdown observer was added and then removed, not fixed:
GracefulShutdownServicesetsshuttingDown = trueinsidedrain(), so an earlier-priority observer runs while the reject gate is still open and races the very drain it was meant to precede.Review
Independent multi-agent reviews at four points, plus every automated comment on this PR. All findings are fixed in-branch — see the
fix(review):commits and changelog entries. Beyond the test suite, the extractions were verified structurally: every pre-branch method signature matched against the union of facade + collaborators (none vanished), and a comment-stripped line diff resolved every unmatched statement to an intentional qualifier rewrite.Two findings were declined with reasons: gating
create_sub_agent/teardown_agentat registration (the guardrails are already enforced in the tool bodies, and suppressing registration is a behaviour change inside a pure move), and removing six "unused" parameters (those signatures are pinned by 44 reflective test references that resolve on exact parameter types — the declarations now carry a comment explaining why the obvious fix breaks the build).Test plan
./mvnw clean compile— cleanToolSourceProviderTest(the plan's R2 post-condition: a provider throwing yields an empty contribution and the loop continues)./mvnw formatter:format validate— clean; 6 pre-existingLineLengthviolations remain, all in untouched filesWhat's next
Wave 0 foundations (F1–F6) and the feature items (I1–I18). Full sequencing in the plan's §7 dependency graph. Follow-ups recorded as separate tasks rather than folded in here: group-signing replay protection,
requirePeerVerificationbeing audit-only, whether a moderator should be peer-critiqued, and aTaskForceEnginelock-ordering concurrency test.