feat(groups): facilitator with bounded moves (I12) - #643
Merged
Conversation
VOTE phases collect explicit ballots; the deliverable is the auditable artifact - weighted tally, raw ballots, losing-side dissents - because LLM ballots are correlated. Independence is enforced at save time (PARALLEL + NONE scope, hard-rejected otherwise), ballots stay peer-hidden via F4's commit-reveal, and parsing is three-tier with out-of-contract votes as non-ballots that count against quorum (as do abstentions). Ties go to the tiePolicy: one moderator tiebreak turn under a separate conversation key, or an honest NONE. HUMAN_DECIDES is save-time rejected until I6 ships. Also folds in the paragraph-4 gap: decision_reached finally has producers (votes and debate verdicts), with a bounded Slack tally block.
AgentGroupConfigurationTest.phaseType_allValues pins the enum size; I14 added VOTE as the 12th value.
…sent carry - recordVoteDecision now takes (turnCounter, maxTurns): the moderator tiebreak is a real LLM turn, gated on both the turn budget and the cost ceiling like every other extra call, and counted when it runs. - TallyOutcome carries the parsed ballots so a tie-policy resolution computes losing-side dissents against ITS chosen option; reusing the unresolved record dropped the minority report for the closest votes. - Weighted totals tie by epsilon (1e-9), not ==; ballot weights must be finite at save time (NaN passes every < comparison). - Slack tally lines width-bounded via buildPreview so a synthesis- derived option paragraph cannot push the message past Slack limits. - CodeQL: 6 log-injection sinks sanitized; useless null check removed.
A MemberType.HUMAN member sits in the roster like any agent, but their
turn pauses the discussion in a new AWAITING_HUMAN_INPUT state until
they submit - deliberately not AWAITING_APPROVAL: approval endpoints
never accept free text, and an inbox must tell "approve/reject" from
"you are up".
- Phase loops intercept HUMAN speakers, render their input exactly like
an agent turn, and commit a pause carrying PendingHumanInput + the F2
ResumePoint (its first producer). The turn is counted at the pause.
- Submission via POST .../human-input + MCP submit_group_human_input;
only the pending member''s own principal (or admin) may submit - an
approver may decide approvals, but speaking as another human is
impersonation. Entry lands as the phase''s natural type; bookmark
advances; CAS makes double-submits a 409; drift-checks refuse before
any mutation.
- humanMemberConfig {turnTimeout, onTimeout=SKIP_TURN|ABORT} rides the
HITL schedule machinery on a new group-human surface; crash recovery
re-arms human-turn timeouts; SKIP_TURN writes a named SKIPPED entry.
- PARALLEL: agents fan out first, humans prompted sequentially against
the pre-fan-out snapshot; a HUMAN_TURN_PARALLEL resume skips the
fan-out instead of duplicating agent turns.
- Save-time matrix: displayName required; no humans in task-force or
targetEachPeer groups; nested groups with humans rejected (+ runtime
backstop); human moderator allowed + warned, and resolveParticipants
no longer demotes them to an agent.
- human_input_requested event (SSE/Slack), HUMAN_TURN inbox entries
with pendingMemberId, availableActions submitHumanInput.
- McpToolFilter whitelists submit_group_human_input (CI guard test). - New requireGroupConversationReadAccess: the pending human member may read the status carrying their rendered prompt (REST + MCP, summary now includes pendingMemberId/prompt on both surfaces); the full transcript view stays role-gated. - Mid-phase resume starts at the bookmarked repeat instead of replaying earlier repeats (duplicate turns and spend). - Human-turn resolution: metric/audit/resume-event deferred until the executor submit succeeds; rollback re-checks the control token; the caller gets a freshly-read copy, not the live instance the background leg mutates. - Slack listener releases its completion latch on a human pause; deleting an AWAITING_HUMAN_INPUT conversation runs the paused cleanup; the signing cursor survives a human pause; crash-recovery sweeps isolated; inbox merges both pause states oldest-first. - turnTimeout must be positive; null members list cannot NPE; F2 drift guard scoped to approval bookmarks; CodeQL logs sanitized; HumanTurnRequired @PARAM docs moved to the constructor.
…tator # Conflicts: # docs/changelog.md # docs/group-conversations.md # src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java # src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java # src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java
A facilitator agent is briefed at configured checkpoints (EACH_PHASE or EACH_REPEAT) with a compact state summary - never the full transcript - and selects one config-enumerated move: CONTINUE, END_PHASE, EXTEND_PHASE (<=2/phase), CALL_VOTE (one-off I14 vote phase, PARALLEL+ NONE by construction), RECRUIT (I7 validation path), ESCALATE_HUMAN (I6 pending-input pause on the configured principal). Every selection is validated, capped (maxMovesPerDiscussion), and recorded: executed moves as peer-hidden FACILITATION entries + audit events + metrics, rejected attempts as FACILITATION entries that never consume budget. Unparseable, disallowed, invalid-in-context or failed -> CONTINUE. CALL_VOTE/EXTEND_PHASE diverge a runtime copy of the phase list, persisted on the conversation (schema v4); every resume surface now resolves effectivePhases() so bookmarks and drift checks compare against the list the pause was taken from. Divergence is one-off: completion and continuation rounds clear it.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughChangesGroup conversation collaboration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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}
…sions - Briefing task summary reads one getTasks() snapshot instead of an emptiness check plus a second read (CodeQL TOCTOU, high). - facilitatorExtensions getter returns an unmodifiable view; mutation goes through recordFacilitatorExtension/clearFacilitatorExtensions. - Three caller-influenced log values sanitized.
Two findings, one root cause: the EACH_REPEAT checkpoint ran after the last-repeat decision block. END_PHASE broke past it (a VOTE phase it ended never tallied its cast ballots; verdicts/dissents/retro skipped alike), and EXTEND_PHASE at a final repeat re-ran an already-fired block (duplicate dissent rounds, decision_reached twice). The consult now precedes the block: END_PHASE folds into the phase outcome, EXTEND_PHASE defers the block to the true final repeat, and INSERT_VOTE/ESCALATE apply after it.
aisabella-ai
self-requested a review
August 8, 2026 08:12
aisabella-ai
approved these changes
Aug 8, 2026
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 11
🧹 Nitpick comments (10)
src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java (1)
297-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
availableActionscase forAWAITING_HUMAN_INPUT.The count and membership assertions here are correct. The
AvailableActionsTestsnested class in this same file does not cover the new switch arm.GroupConversation.getAvailableActionsnow returnsList.of("submitHumanInput")forAWAITING_HUMAN_INPUT, and clients drive the input prompt off that value. ThenonTerminalloop at lines 392-393 correctly omits the state, so nothing asserts the new behavior.💚 Proposed test
`@Test` `@DisplayName`("AWAITING_HUMAN_INPUT offers submitHumanInput only") void awaitingHumanInput() { var gc = new GroupConversation(); gc.setState(GroupConversationState.AWAITING_HUMAN_INPUT); assertEquals(List.of("submitHumanInput"), gc.getAvailableActions()); }🤖 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/model/GroupConversationTest.java` around lines 297 - 302, Add a test in the AvailableActionsTests nested class covering GroupConversationState.AWAITING_HUMAN_INPUT: create a GroupConversation, set that state, and assert getAvailableActions() returns only "submitHumanInput".src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java (2)
330-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the inclusive
100boundary.The test rejects
101. It does not assert that exactly100is accepted. The validator uses> 100and its message promises "at most 100". A change from>to>=would pass this test while breaking the documented boundary.💚 Proposed addition
`@Test` void facilitator_excessiveMoveBudget_isRejected() { var ex = assertThrows(IllegalArgumentException.class, () -> AgentGroupStore.validateFacilitator( facilitatorConfig(new AgentGroupConfiguration.FacilitatorConfig(true, "fac", null, null, 101, null)))); assertTrue(ex.getMessage().contains("100"), ex.getMessage()); + assertDoesNotThrow(() -> AgentGroupStore.validateFacilitator( + facilitatorConfig(new AgentGroupConfiguration.FacilitatorConfig(true, "fac", null, null, 100, null))), + "the bound is inclusive"); }🤖 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/AgentGroupStoreTest.java` around lines 330 - 335, Add a test alongside facilitator_excessiveMoveBudget_isRejected that passes a move budget of exactly 100 to AgentGroupStore.validateFacilitator and asserts validation succeeds, preserving the documented inclusive upper boundary while retaining the existing rejection test for 101.
99-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
targetEachPeerrejection for VOTE phases.These tests cover every other branch of
validateVotePhases: turn order, context scope, option count, tie policy, and weights. ThetargetEachPeerrejection atsrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.javalines 276-278 is the one remaining uncovered branch. The existingvotePhasehelper hard-codestargetEachPeertofalse, so a small variant is needed.💚 Proposed test
`@Test` void votePhase_targetEachPeer_isRejected() { var peerVote = new DiscussionPhase("Ballot", PhaseType.VOTE, "ALL", TurnOrder.PARALLEL, ContextScope.NONE, true, null, 1, false, null, false, null); var ex = assertThrows(IllegalArgumentException.class, () -> AgentGroupStore.validateVotePhases(voteGroup(peerVote))); assertTrue(ex.getMessage().contains("targetEachPeer"), ex.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/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java` around lines 99 - 174, Add a test named votePhase_targetEachPeer_isRejected alongside the existing VOTE validation tests, constructing a VOTE DiscussionPhase with targetEachPeer set to true and otherwise valid settings. Pass it through voteGroup and assert validateVotePhases throws IllegalArgumentException with a message containing “targetEachPeer”.src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java (1)
246-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd matching normalization tests for
VoteConfigandHumanMemberConfig.This file now pins
FacilitatorConfignormalization well. The two other records added in the same change have no equivalent test here.VoteConfigcarries the most normalization logic: nullmethod, nulloptionsSource, nulloptions, thequorumrange clamp, nullweights, and nulltiePolicy. A quorum test would also pin the boundary behavior discussed onAgentGroupConfiguration.javalines 496-498.
HumanMemberConfigneeds one assertion that a nullonTimeoutnormalizes toSKIP_TURN.💚 Proposed tests
`@Test` void voteConfig_compactConstructor_normalizesDefaults() { var sparse = new VoteConfig(null, null, null, 0.0, null, false, null); assertEquals(VoteMethod.MAJORITY, sparse.method()); assertEquals(OptionsSource.LAST_SYNTHESIS, sparse.optionsSource()); assertTrue(sparse.options().isEmpty()); assertEquals(VoteConfig.DEFAULT_QUORUM, sparse.quorum()); assertTrue(sparse.weights().isEmpty()); assertEquals(TiePolicy.NO_DECISION, sparse.tiePolicy()); } `@Test` void voteConfig_quorumOutOfRange_fallsBackToDefault() { for (double bad : new double[]{-0.1, 0.0, 1.5}) { var config = new VoteConfig(null, null, null, bad, null, false, null); assertEquals(VoteConfig.DEFAULT_QUORUM, config.quorum(), "quorum " + bad); } assertEquals(1.0, new VoteConfig(null, null, null, 1.0, null, false, null).quorum(), "an inclusive upper bound stays as written"); } `@Test` void humanMemberConfig_nullTimeoutPolicy_defaultsToSkipTurn() { assertEquals(OnHumanTimeout.SKIP_TURN, new HumanMemberConfig("PT4H", null).onTimeout()); assertNull(new HumanMemberConfig().turnTimeout(), "no timeout means wait indefinitely"); }🤖 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/model/AgentGroupConfigurationTest.java` around lines 246 - 288, Add normalization coverage in AgentGroupConfigurationTest alongside the FacilitatorConfig tests: add VoteConfig tests covering null defaults, empty collections, tie policy, and invalid quorum values falling back to DEFAULT_QUORUM while preserving an inclusive quorum of 1.0; add a HumanMemberConfig test confirming null onTimeout becomes OnHumanTimeout.SKIP_TURN and the no-argument constructor leaves turnTimeout null.src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java (1)
660-675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises a
NO_DECISIONoutcome but never asserts it.
voteTiebreak_turnBudgetExhausted_keepsNoDecision_andSpendsNothingasserts only that no moderator turn ran and that the counter is unchanged. It passes a freshgc()and discards it, so the recordedDecisionRecordis unobserved. A regression that recorded a winner without a tiebreak turn would still pass.💚 Proposed assertion
- engine.recordVoteDecision(gc(), config, votePhase(), protocol(), 0, ballots, - List.of(member("a"), member("b")), null, turnCounter, 5); + var gc = gc(); + engine.recordVoteDecision(gc, config, votePhase(), protocol(), 0, ballots, + List.of(member("a"), member("b")), null, turnCounter, 5); verify(memberTurnExecutor, never()).executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any(), any(), any()); assertEquals(5, turnCounter.get(), "a blocked tiebreak must not consume a turn"); + assertEquals(DecisionType.NONE, gc.getDecision().type(), "the tally's honest NONE survives the blocked tiebreak"); + assertNull(gc.getDecision().winner());🤖 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 660 - 675, Update voteTiebreak_turnBudgetExhausted_keepsNoDecision_andSpendsNothing to retain the GameContext returned by gc(), then assert its recorded DecisionRecord has a NO_DECISION outcome. Keep the existing assertions verifying that no moderator turn executes and the turnCounter remains unchanged.src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java (1)
499-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a separate counter for human input instead of a synthetic
verdicttag.The call reuses
eddi.mcp.hitl.decisionand setsverdictto"HUMAN_INPUT". A submitted turn is not an approval verdict. Dashboards that aggregateeddi.mcp.hitl.decisionbyverdictnow mix approvals with speech turns.Emit a dedicated counter, for example
eddi.mcp.hitl.human_inputwith asurfacetag.🤖 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/McpHitlTools.java` at line 499, Update the counter emission in McpHitlTools to record human input with a dedicated metric such as eddi.mcp.hitl.human_input, retaining only the surface tag. Do not increment eddi.mcp.hitl.decision or attach a synthetic verdict tag for submitted turns.src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java (1)
266-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@NotNull@Valid`` and size ceilings to align with the other request bodies.The
requestparameter has no@Valid. The@NotBlankconstraints onHumanInputRequestare therefore never evaluated.discuss,followUpWithMember, andcontinueDiscussionall declare@NotNull@Valid``.The implementation re-checks blank
memberIdandcontent, so behaviour is currently correct. The annotations are inert, which is misleading for the next reader.
memberIdalso has noMAX_IDENTIFIER_CHARSceiling, unlikeuserIdandtargetAgentId. Add a@Sizeceiling for parity.♻️ Proposed change
Response submitHumanInput(`@PathParam`("groupId") String groupId, `@PathParam`("groupConversationId") String gcId, - HumanInputRequest request); + `@NotNull` + `@Valid` HumanInputRequest request); /** * Request body for {`@link` `#submitHumanInput`}: which HUMAN member is speaking, * and what they said. */ record HumanInputRequest( - `@NotBlank`(message = "'memberId' must not be blank") String memberId, + `@NotBlank`(message = "'memberId' must not be blank") + `@Size`(max = MAX_IDENTIFIER_CHARS, + message = "'memberId' must be at most {max} characters") String memberId, `@NotBlank`(message = "'content' must not be blank") String content) { }🤖 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/api/IRestGroupConversation.java` around lines 266 - 277, Update submitHumanInput in IRestGroupConversation to annotate the request parameter with `@NotNull` and `@Valid`, enabling HumanInputRequest validation. Add the existing MAX_IDENTIFIER_CHARS-based `@Size` constraint to HumanInputRequest.memberId, matching the ceilings used by userId and targetAgentId while preserving the current `@NotBlank` constraints.src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java (1)
97-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare against the
OnHumanTimeoutenum constants instead of string literals.The method compares
policyStrto the literals"ABORT"and"SKIP_TURN". If a constant in theOnHumanTimeoutenum is renamed, this code compiles and silently degrades every timeout to SKIP_TURN.Use
OnHumanTimeout.ABORT.name()andOnHumanTimeout.SKIP_TURN.name(), or parse the value once and switch on it. Keep the current fallback for unparsable values.The behaviour is correct today. This is a robustness improvement only.
🤖 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/HitlTimeoutHandler.java` around lines 97 - 107, Update the timeout policy comparisons in the handler containing the ABORT and SKIP_TURN branches to use OnHumanTimeout.ABORT.name() and OnHumanTimeout.SKIP_TURN.name(), or parse policyStr once and switch on the enum. Preserve the existing ABORT cancellation behavior and the fallback that logs unknown or unparsable values before calling skipHumanTurnOnTimeout.src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java (1)
1020-1057: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the
phaseList.get(resumePhaseIdx)index.Line 1029 indexes
phaseListwithout a bounds check. One caller passesphaseIdx + 1for a boundary checkpoint, so the index equalsphaseList.size()at the last phase.FacilitatorEngine.executeEscalaterejects the move whenctx.escalationResumeTargetExists()is false, so the invariant is enforced, but it is enforced in a different class and computed by the caller.The read happens before any mutation, so a failure leaves the conversation state clean. A local check turns a possible
IndexOutOfBoundsExceptioninto a typed refusal.🛡️ Proposed bounds check
var humanConfig = config != null && config.getHumanMemberConfig() != null ? config.getHumanMemberConfig() : new AgentGroupConfiguration.HumanMemberConfig(); + if (phaseList == null || resumePhaseIdx < 0 || resumePhaseIdx >= phaseList.size()) { + throw new IResourceStore.ResourceStoreException( + "Facilitator escalation has no resume phase at index " + resumePhaseIdx + + " — refusing to commit a pause with no resume target"); + } DiscussionPhase resumePhase = phaseList.get(resumePhaseIdx);🤖 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/GroupHitlCoordinator.java` around lines 1020 - 1057, Guard the resumePhaseIdx lookup in commitFacilitatorEscalationPause by validating that it is within phaseList bounds before calling phaseList.get. If the index is invalid, refuse the operation using the method’s existing typed exception contract, before mutating GroupConversation or scheduling a timeout; retain the current behavior for valid indices.src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java (1)
382-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the executor-saturation rollback.
These tests cover the happy path and every pre-mutation refusal. They do not cover the one post-CAS failure branch in
resolveHumanTurn(GroupHitlCoordinatorLines 1255-1285). That branch removes the appended transcript entry, restores six bookmark fields, CASes back toAWAITING_HUMAN_INPUT, re-arms the timeout schedule and re-checks the cancel token. If it is wrong, a member's submitted turn is lost silently.The branch is easy to reach: make the mocked
executorService.submitthrow aRejectedExecutionException.💚 Proposed test
`@Test` void submitHumanInput_executorRejects_rollsBackTheAppendAndRestoresThePause() throws Exception { var coordinator = coordinator(); var gc = humanPausedGc(); when(conversationStore.read(GC_ID)).thenReturn(gc); var resId = mock(IResourceStore.IResourceId.class); when(resId.getVersion()).thenReturn(1); when(groupStore.getCurrentResourceId(GROUP_ID)).thenReturn(resId); var config = humanGroupConfig("PT4H"); when(groupStore.read(GROUP_ID, 1)).thenReturn(config); when(groupConversationService.effectivePhases(any(), eq(config))).thenReturn(List.of(opinionPhase())); when(executorService.submit(any(Runnable.class))) .thenThrow(new java.util.concurrent.RejectedExecutionException("saturated")); assertThrows(ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionException.class, () -> coordinator.submitHumanInput(GC_ID, "h-1", "My answer.", "h-1", null)); assertTrue(gc.getTranscript().isEmpty(), "the append must be rolled back"); assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, gc.getState()); assertNotNull(gc.getPendingHumanInput(), "the turn is still owed"); assertEquals(1, gc.getResumePoint().speakerIdx(), "the bookmark is not advanced"); verify(conversationStore).updateIfState(gc, GroupConversationState.IN_PROGRESS); }🤖 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/GroupHitlCoordinatorTest.java` around lines 382 - 468, Add a test beside the existing submitHumanInput tests that configures executorService.submit to throw RejectedExecutionException after the human turn is accepted. Invoke submitHumanInput and assert it raises GroupDiscussionException, removes the transcript entry, restores AWAITING_HUMAN_INPUT with pending input, preserves the original resume bookmark, and verifies the conversation is CAS-restored via updateIfState with IN_PROGRESS.
🤖 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 42: Align the transcript-size references in the changelog bullets
describing the bounded-briefing test. Verify the actual fixture size, then
update the conflicting 50k- and 100k-character claims to match it; if they refer
to different tests, distinguish the tests explicitly.
- Line 17: Update the changelog references to the vote-insertion facilitator
move so every occurrence uses the confirmed canonical FacilitatorMove value.
Replace the inconsistent name across the entries at the referenced locations,
including the main description and both related mentions, without changing the
surrounding behavior or wording.
In `@docs/group-conversations.md`:
- Around line 199-203: Clarify the `HUMAN_DECIDES` description in the tie-policy
documentation to state that it remains unimplemented and is rejected at save
time in this release, even when human group members are configured. Keep the
existing references to human members and the `I6` context while removing the
implication that availability alone enables the policy.
In
`@src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java`:
- Around line 496-498: Update the quorum validation in AgentGroupConfiguration
to treat any non-finite value, including NaN and infinities, as invalid and
replace it with DEFAULT_QUORUM; preserve the existing 0-to-1 range validation
for finite values.
In `@src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java`:
- Around line 191-202: Update readChildConfig’s LOGGER.debugf call to wrap the
caller-controlled groupId with LogSanitizer.sanitize, matching the existing
sanitization pattern used by the warning log in AgentGroupStore. Keep the
exception message handling and existing fallback behavior unchanged.
- Around line 286-289: Update the IllegalArgumentException message in the
HUMAN_DECIDES branch of AgentGroupStore so it states that the human tie-break
path is not implemented, while preserving the existing rejection and guidance to
use MODERATOR_DECIDES or NO_DECISION.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/FacilitatorEngine.java`:
- Around line 461-503: Update recordRejection to submit a REJECTED audit entry
through auditMove, preserving the attempted raw move and rejection reason. Add
an overload of auditMove that accepts the raw move string and rejection details,
populating rejectionReason while retaining existing parsed-move auditing for
executed moves. Ensure unparseable and unknown-move rejection paths reach this
audit submission.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java`:
- Around line 1139-1155: Update the log statements in skipHumanTurnOnTimeout to
wrap groupConversationId and pending.memberId() with LogSanitizer.sanitize,
matching the existing identifier-sanitization pattern used elsewhere in
GroupHitlCoordinator. Apply this to both timeout status logs and the failure
log.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java`:
- Around line 504-529: Replace the full-document `findByState` calls in the
pending-list flow with a summary projection method added to
`IGroupConversationStore` and implemented by its Mongo store. Project only the
pause-bookmark fields required to build `PendingApprovalSummary`, while
retaining the existing full-limit queries, merge ordering, and cap across both
`AWAITING_APPROVAL` and `AWAITING_HUMAN_INPUT` states.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/VoteTallyEngine.java`:
- Around line 101-117: Update resolveOptions so the EXPLICIT branch normalizes a
null config.options() to an empty list before returning, while preserving
configured options when non-null. Keep the existing transcript-derived
resolution unchanged so callers such as PhaseExecutionEngine.recordVoteDecision
and GroupContextBuilder always receive a non-null list.
In
`@src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java`:
- Around line 305-323: Update appendVoteTally to pass each option key through
the existing escapeMrkdwnHuman helper before appending it to the Slack message,
while retaining buildPreview’s length limit. Apply the same escaping to
decision.winner() in the nearby winner-rendering logic, since it can contain
model-derived option text.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java`:
- Around line 266-277: Update submitHumanInput in IRestGroupConversation to
annotate the request parameter with `@NotNull` and `@Valid`, enabling
HumanInputRequest validation. Add the existing MAX_IDENTIFIER_CHARS-based `@Size`
constraint to HumanInputRequest.memberId, matching the ceilings used by userId
and targetAgentId while preserving the current `@NotBlank` constraints.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java`:
- Around line 1020-1057: Guard the resumePhaseIdx lookup in
commitFacilitatorEscalationPause by validating that it is within phaseList
bounds before calling phaseList.get. If the index is invalid, refuse the
operation using the method’s existing typed exception contract, before mutating
GroupConversation or scheduling a timeout; retain the current behavior for valid
indices.
In `@src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java`:
- Around line 97-107: Update the timeout policy comparisons in the handler
containing the ABORT and SKIP_TURN branches to use OnHumanTimeout.ABORT.name()
and OnHumanTimeout.SKIP_TURN.name(), or parse policyStr once and switch on the
enum. Preserve the existing ABORT cancellation behavior and the fallback that
logs unknown or unparsable values before calling skipHumanTurnOnTimeout.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java`:
- Line 499: Update the counter emission in McpHitlTools to record human input
with a dedicated metric such as eddi.mcp.hitl.human_input, retaining only the
surface tag. Do not increment eddi.mcp.hitl.decision or attach a synthetic
verdict tag for submitted turns.
In
`@src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java`:
- Around line 246-288: Add normalization coverage in AgentGroupConfigurationTest
alongside the FacilitatorConfig tests: add VoteConfig tests covering null
defaults, empty collections, tie policy, and invalid quorum values falling back
to DEFAULT_QUORUM while preserving an inclusive quorum of 1.0; add a
HumanMemberConfig test confirming null onTimeout becomes
OnHumanTimeout.SKIP_TURN and the no-argument constructor leaves turnTimeout
null.
In `@src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java`:
- Around line 297-302: Add a test in the AvailableActionsTests nested class
covering GroupConversationState.AWAITING_HUMAN_INPUT: create a
GroupConversation, set that state, and assert getAvailableActions() returns only
"submitHumanInput".
In `@src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java`:
- Around line 330-335: Add a test alongside
facilitator_excessiveMoveBudget_isRejected that passes a move budget of exactly
100 to AgentGroupStore.validateFacilitator and asserts validation succeeds,
preserving the documented inclusive upper boundary while retaining the existing
rejection test for 101.
- Around line 99-174: Add a test named votePhase_targetEachPeer_isRejected
alongside the existing VOTE validation tests, constructing a VOTE
DiscussionPhase with targetEachPeer set to true and otherwise valid settings.
Pass it through voteGroup and assert validateVotePhases throws
IllegalArgumentException with a message containing “targetEachPeer”.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java`:
- Around line 382-468: Add a test beside the existing submitHumanInput tests
that configures executorService.submit to throw RejectedExecutionException after
the human turn is accepted. Invoke submitHumanInput and assert it raises
GroupDiscussionException, removes the transcript entry, restores
AWAITING_HUMAN_INPUT with pending input, preserves the original resume bookmark,
and verifies the conversation is CAS-restored via updateIfState with
IN_PROGRESS.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java`:
- Around line 660-675: Update
voteTiebreak_turnBudgetExhausted_keepsNoDecision_andSpendsNothing to retain the
GameContext returned by gc(), then assert its recorded DecisionRecord has a
NO_DECISION outcome. Keep the existing assertions verifying that no moderator
turn executes and the turnCounter remains unchanged.
🪄 Autofix
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: 9849140c-0612-4053-9640-0c2022778b6f
📒 Files selected for processing (42)
docs/changelog.mddocs/group-conversations.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/configs/groups/model/DiscussionStylePresets.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.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/hitl/HitlAccessGuard.javasrc/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.javasrc/main/java/ai/labs/eddi/engine/hitl/HitlSchedules.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/FacilitatorEngine.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.javasrc/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.javasrc/main/java/ai/labs/eddi/engine/internal/groups/VoteTallyEngine.javasrc/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/main/java/ai/labs/eddi/engine/model/PendingApprovalSummary.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.javasrc/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.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/AgentGroupStoreTest.javasrc/test/java/ai/labs/eddi/engine/hitl/HitlAccessGuardTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/FacilitatorEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/VoteTallyEngineTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsCoverageTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.javasrc/test/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListenerTest.java
| - **END_PHASE skipped the decisions:** it fired only mid-phase (where `lastRepeat` is false) and took a plain `break` past the block — a VOTE phase it ended never tallied its cast ballots; verdicts, dissent rounds and retro harvests were skipped the same way. | ||
| - **EXTEND_PHASE at a final repeat re-ran them:** the block had already fired for that repeat, and the extension made the next repeat "final" again — duplicate dissent rounds, `decision_reached` twice, the tally overwritten. | ||
|
|
||
| **Fix:** the consult now runs after convergence but BEFORE `lastRepeat` is computed, with effects split by kind — END_PHASE folds into the phase outcome (the block sees a real phase end and records everything), EXTEND_PHASE applies immediately (deferring the block to the true final repeat), and INSERT_VOTE/ESCALATE are stashed and applied after the block, so an escalation on a final repeat cannot skip that repeat's decisions on its way out. Two new mutation-check E2Es: END_PHASE on a VOTE phase still tallies (fails against the old `break`), and always-EXTEND on a dissent-recording SYNTHESIS runs exactly one dissent round (fails against the old ordering). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one canonical facilitator move name.
Lines 17, 43, and 47 describe the same vote-insertion move with two names: INSERT_VOTE and CALL_VOTE. Confirm the canonical FacilitatorMove value, then use it consistently throughout the changelog.
Also applies to: 43-43, 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/changelog.md` at line 17, Update the changelog references to the
vote-insertion facilitator move so every occurrence uses the confirmed canonical
FacilitatorMove value. Replace the inconsistent name across the entries at the
referenced locations, including the main description and both related mentions,
without changing the surrounding behavior or wording.
|
|
||
| - **Integration merge first (big-merge memory applied):** merged `feat/group-i6-human-members` into a branch cut from `feat/group-i14-voting`; 5 conflicted files resolved by keeping both sides (AgentGroupStore create/update now run vote + human + facilitator validation; Slack listener keeps tally block AND human notice; both test blocks; both doc sections; both changelog entries). Verified per the memory: clean compile, 2912 tests green across `engine.internal`+`configs.groups`+`engine.hitl`+`engine.mcp`, hot files (GroupConversationService/PhaseExecutionEngine/AgentGroupConfiguration) spot-checked for both features, enum pins consistent (PhaseType 12, MemberType 3, DiscussionStyle 7 — no NEGOTIATION here, that's I11's branch). | ||
| - **Config:** `FacilitatorConfig {enabled=false, agentId (required when enabled), allowedMoves (default [CONTINUE] — an enabled-but-unconfigured facilitator is a pure observer), checkAfter=EACH_PHASE|EACH_REPEAT, maxMovesPerDiscussion=10 (non-CONTINUE only), escalateTo}`. Save-time matrix (`AgentGroupStore.validateFacilitator`): enabled needs agentId; END_PHASE/EXTEND_PHASE + EACH_PHASE rejected (they act on remaining repeats — a boundary checkpoint has none, the config could only produce noise); ESCALATE_HUMAN needs escalateTo; cap ≤ 100. | ||
| - **`FacilitatorEngine` (new R1-style collaborator):** compact briefing (position, budget arithmetic, roster, per-type entry counts, capped excerpts — bounded-by-construction, asserted in a test with a 50k-char transcript); the consult runs under the judge precedent (own `__facilitator` conversation key, skipped at either budget, counts a turn, cost on the I1 ledger); three-tier parse mirroring VoteTallyEngine; per-move context validation (a convergence exit is never overruled). Executed → peer-hidden FACILITATION entry + `group.facilitator` audit event + `eddi_group_facilitator_moves_total{move,outcome}`; rejected → CONTINUE + WARN + FACILITATION entry recording the attempt (never consumes the budget); null reply/exception → CONTINUE with no entry (nothing was tried). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the bounded-briefing test size.
Line 42 states that the test uses a 50k-character transcript. Line 47 states that it uses a 100k-character transcript. If these bullets describe the same test, update one value to match the actual fixture. Otherwise, identify the tests separately.
Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/changelog.md` at line 42, Align the transcript-size references in the
changelog bullets describing the bounded-briefing test. Verify the actual
fixture size, then update the conflicting 50k- and 100k-character claims to
match it; if they refer to different tests, distinguish the tests explicitly.
Comment on lines
+199
to
+203
| - **Ties and quorum failures** go to `tiePolicy`: `MODERATOR_DECIDES` runs one | ||
| moderator turn choosing among the unresolved options (method | ||
| `vote+moderator-tiebreak`); `NO_DECISION` (default) records an honest | ||
| `type: NONE` and the discussion continues. `HUMAN_DECIDES` is reserved for | ||
| human group members (I6) and is rejected at save time until then. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the HUMAN_DECIDES status now that I6 ships in the same change.
Lines 202-203 state that HUMAN_DECIDES "is reserved for human group members (I6) and is rejected at save time until then". The next section documents human group members as available. AgentGroupStore.validateVotePhases still rejects HUMAN_DECIDES with the message "needs human group members (I6), which are not available yet". A reader who just enabled a HUMAN member will expect HUMAN_DECIDES to work. State explicitly that HUMAN_DECIDES remains unimplemented in this release even with human members configured.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/group-conversations.md` around lines 199 - 203, Clarify the
`HUMAN_DECIDES` description in the tie-policy documentation to state that it
remains unimplemented and is rejected at save time in this release, even when
human group members are configured. Keep the existing references to human
members and the `I6` context while removing the implication that availability
alone enables the policy.
Comment on lines
+496
to
+498
| if (quorum <= 0.0 || quorum > 1.0) { | ||
| quorum = DEFAULT_QUORUM; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize a non-finite quorum to the default.
The guard quorum <= 0.0 || quorum > 1.0 does not catch NaN. Both comparisons are false for NaN, so a NaN quorum is stored as-is. A NaN quorum makes every quorum comparison false, so the vote never reaches quorum and silently records no decision.
AgentGroupStore.validateVotePhases already applies Double.isFinite to weights for exactly this reason (see the comment at src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java lines 290-295). Apply the same reasoning to quorum, which no save-time check covers.
🛡️ Proposed fix
- if (quorum <= 0.0 || quorum > 1.0) {
+ if (!Double.isFinite(quorum) || quorum <= 0.0 || quorum > 1.0) {
quorum = DEFAULT_QUORUM;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (quorum <= 0.0 || quorum > 1.0) { | |
| quorum = DEFAULT_QUORUM; | |
| } | |
| if (!Double.isFinite(quorum) || quorum <= 0.0 || quorum > 1.0) { | |
| quorum = DEFAULT_QUORUM; | |
| } |
🤖 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/model/AgentGroupConfiguration.java`
around lines 496 - 498, Update the quorum validation in AgentGroupConfiguration
to treat any non-finite value, including NaN and infinities, as invalid and
replace it with DEFAULT_QUORUM; preserve the existing 0-to-1 range validation
for finite values.
Comment on lines
+191
to
+202
| /** Latest version of a (possible) child group config, or null if unreadable. */ | ||
| private AgentGroupConfiguration readChildConfig(String groupId) { | ||
| try { | ||
| IResourceStore.IResourceId resId = getCurrentResourceId(groupId); | ||
| return resId != null ? read(groupId, resId.getVersion()) : null; | ||
| } catch (Exception e) { | ||
| // Deployment-order tolerance: an unreadable/absent child cannot block | ||
| // the parent save; the runtime backstop covers it. | ||
| LOGGER.debugf("Nested-group human check could not read child '%s': %s", groupId, e.getMessage()); | ||
| return null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sanitize groupId in the debug log.
groupId comes from a submitted member agentId, so it is caller-controlled. Line 199 interpolates it unsanitized. The warn at lines 120-121 in the same class wraps both values in LogSanitizer.sanitize. Apply the same treatment here for consistency with the log-sanitization work in this PR.
🛡️ Proposed fix
- LOGGER.debugf("Nested-group human check could not read child '%s': %s", groupId, e.getMessage());
+ LOGGER.debugf("Nested-group human check could not read child '%s': %s",
+ LogSanitizer.sanitize(groupId), LogSanitizer.sanitize(e.getMessage()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Latest version of a (possible) child group config, or null if unreadable. */ | |
| private AgentGroupConfiguration readChildConfig(String groupId) { | |
| try { | |
| IResourceStore.IResourceId resId = getCurrentResourceId(groupId); | |
| return resId != null ? read(groupId, resId.getVersion()) : null; | |
| } catch (Exception e) { | |
| // Deployment-order tolerance: an unreadable/absent child cannot block | |
| // the parent save; the runtime backstop covers it. | |
| LOGGER.debugf("Nested-group human check could not read child '%s': %s", groupId, e.getMessage()); | |
| return null; | |
| } | |
| } | |
| /** Latest version of a (possible) child group config, or null if unreadable. */ | |
| private AgentGroupConfiguration readChildConfig(String groupId) { | |
| try { | |
| IResourceStore.IResourceId resId = getCurrentResourceId(groupId); | |
| return resId != null ? read(groupId, resId.getVersion()) : null; | |
| } catch (Exception e) { | |
| // Deployment-order tolerance: an unreadable/absent child cannot block | |
| // the parent save; the runtime backstop covers it. | |
| LOGGER.debugf("Nested-group human check could not read child '%s': %s", | |
| LogSanitizer.sanitize(groupId), LogSanitizer.sanitize(e.getMessage())); | |
| return null; | |
| } | |
| } |
🤖 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/AgentGroupStore.java` around
lines 191 - 202, Update readChildConfig’s LOGGER.debugf call to wrap the
caller-controlled groupId with LogSanitizer.sanitize, matching the existing
sanitization pattern used by the warning log in AgentGroupStore. Keep the
exception message handling and existing fallback behavior unchanged.
Comment on lines
+461
to
+503
| /** | ||
| * A rejected attempt: CONTINUE + WARN + a FACILITATION entry that shows the | ||
| * model tried. Never consumes the move budget. | ||
| */ | ||
| private void recordRejection(GroupConversation gc, FacilitatorConfig fc, DiscussionPhase phase, | ||
| CheckpointContext ctx, String attemptedMove, String reason, String why) { | ||
| String content = "Facilitator attempted " + attemptedMove + " — rejected: " + why | ||
| + (reason != null && !reason.isBlank() ? ". Facilitator's reason: " + reason.trim() : ""); | ||
| gc.getTranscript().add(new TranscriptEntry(fc.agentId(), "Facilitator", content, ctx.phaseIdx(), phase.name(), | ||
| TranscriptEntryType.FACILITATION, Instant.now(), null, null)); | ||
| meterRegistry.counter("eddi_group_facilitator_moves_total", | ||
| "move", attemptedMove, "outcome", "rejected").increment(); | ||
| LOGGER.warnf("Facilitator move %s rejected for group %s at phase %d: %s", | ||
| LogSanitizer.sanitize(attemptedMove), LogSanitizer.sanitize(gc.getId()), ctx.phaseIdx(), | ||
| LogSanitizer.sanitize(why)); | ||
| } | ||
|
|
||
| private void auditMove(GroupConversation gc, ParsedMove parsed, CheckpointContext ctx, String outcome, String why) { | ||
| if (auditLedgerService == null || !auditLedgerService.isEnabled()) { | ||
| return; | ||
| } | ||
| try { | ||
| var detail = new LinkedHashMap<String, Object>(); | ||
| detail.put("move", parsed.move() != null ? parsed.move().name() : parsed.rawMove()); | ||
| detail.put("outcome", outcome); | ||
| detail.put("phaseIdx", ctx.phaseIdx()); | ||
| detail.put("repeat", ctx.repeat()); | ||
| if (parsed.reason() != null) { | ||
| detail.put("reason", parsed.reason()); | ||
| } | ||
| if (why != null) { | ||
| detail.put("rejectionReason", why); | ||
| } | ||
| auditLedgerService.submit(new AuditEntry( | ||
| UUID.randomUUID().toString(), gc.getId(), gc.getGroupId(), null, gc.getUserId(), | ||
| null, -1, "group.facilitator", "group", -1, 0L, | ||
| Map.of(), detail, null, null, List.of(), 0.0, | ||
| Instant.now(), null, null)); | ||
| } catch (Exception e) { | ||
| LOGGER.warnf("Failed to submit facilitator audit entry for group conversation %s: %s", | ||
| LogSanitizer.sanitize(gc.getId()), e.getMessage()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Rejected facilitator moves never reach the audit ledger.
recordExecution calls auditMove(...,"EXECUTED", null). recordRejection writes a transcript entry, a metric and a WARN log, but it never calls auditMove. The class javadoc at Lines 61-64 states that a rejected attempt must be recorded "because the audit trail must show the model tried".
auditMove already carries the rejection-only machinery: the why parameter maps to rejectionReason and is passed null by its only caller, and the parsed.move() != null ? ... : parsed.rawMove() branch exists only for an unrecognized move. Both are unreachable today.
Add a REJECTED audit submission to recordRejection. The unparseable and unknown-move call sites pass a raw string, so auditMove needs a rejection-shaped overload.
🛡️ Proposed fix to audit rejected attempts
private void recordRejection(GroupConversation gc, FacilitatorConfig fc, DiscussionPhase phase,
CheckpointContext ctx, String attemptedMove, String reason, String why) {
String content = "Facilitator attempted " + attemptedMove + " — rejected: " + why
+ (reason != null && !reason.isBlank() ? ". Facilitator's reason: " + reason.trim() : "");
gc.getTranscript().add(new TranscriptEntry(fc.agentId(), "Facilitator", content, ctx.phaseIdx(), phase.name(),
TranscriptEntryType.FACILITATION, Instant.now(), null, null));
meterRegistry.counter("eddi_group_facilitator_moves_total",
"move", attemptedMove, "outcome", "rejected").increment();
+ auditMove(gc, new ParsedMove(null, attemptedMove, MAPPER.createObjectNode(), reason), ctx, "REJECTED", why);
LOGGER.warnf("Facilitator move %s rejected for group %s at phase %d: %s",
LogSanitizer.sanitize(attemptedMove), LogSanitizer.sanitize(gc.getId()), ctx.phaseIdx(),
LogSanitizer.sanitize(why));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * A rejected attempt: CONTINUE + WARN + a FACILITATION entry that shows the | |
| * model tried. Never consumes the move budget. | |
| */ | |
| private void recordRejection(GroupConversation gc, FacilitatorConfig fc, DiscussionPhase phase, | |
| CheckpointContext ctx, String attemptedMove, String reason, String why) { | |
| String content = "Facilitator attempted " + attemptedMove + " — rejected: " + why | |
| + (reason != null && !reason.isBlank() ? ". Facilitator's reason: " + reason.trim() : ""); | |
| gc.getTranscript().add(new TranscriptEntry(fc.agentId(), "Facilitator", content, ctx.phaseIdx(), phase.name(), | |
| TranscriptEntryType.FACILITATION, Instant.now(), null, null)); | |
| meterRegistry.counter("eddi_group_facilitator_moves_total", | |
| "move", attemptedMove, "outcome", "rejected").increment(); | |
| LOGGER.warnf("Facilitator move %s rejected for group %s at phase %d: %s", | |
| LogSanitizer.sanitize(attemptedMove), LogSanitizer.sanitize(gc.getId()), ctx.phaseIdx(), | |
| LogSanitizer.sanitize(why)); | |
| } | |
| private void auditMove(GroupConversation gc, ParsedMove parsed, CheckpointContext ctx, String outcome, String why) { | |
| if (auditLedgerService == null || !auditLedgerService.isEnabled()) { | |
| return; | |
| } | |
| try { | |
| var detail = new LinkedHashMap<String, Object>(); | |
| detail.put("move", parsed.move() != null ? parsed.move().name() : parsed.rawMove()); | |
| detail.put("outcome", outcome); | |
| detail.put("phaseIdx", ctx.phaseIdx()); | |
| detail.put("repeat", ctx.repeat()); | |
| if (parsed.reason() != null) { | |
| detail.put("reason", parsed.reason()); | |
| } | |
| if (why != null) { | |
| detail.put("rejectionReason", why); | |
| } | |
| auditLedgerService.submit(new AuditEntry( | |
| UUID.randomUUID().toString(), gc.getId(), gc.getGroupId(), null, gc.getUserId(), | |
| null, -1, "group.facilitator", "group", -1, 0L, | |
| Map.of(), detail, null, null, List.of(), 0.0, | |
| Instant.now(), null, null)); | |
| } catch (Exception e) { | |
| LOGGER.warnf("Failed to submit facilitator audit entry for group conversation %s: %s", | |
| LogSanitizer.sanitize(gc.getId()), e.getMessage()); | |
| } | |
| } | |
| /** | |
| * A rejected attempt: CONTINUE + WARN + a FACILITATION entry that shows the | |
| * model tried. Never consumes the move budget. | |
| */ | |
| private void recordRejection(GroupConversation gc, FacilitatorConfig fc, DiscussionPhase phase, | |
| CheckpointContext ctx, String attemptedMove, String reason, String why) { | |
| String content = "Facilitator attempted " + attemptedMove + " — rejected: " + why | |
| (reason != null && !reason.isBlank() ? ". Facilitator's reason: " + reason.trim() : ""); | |
| gc.getTranscript().add(new TranscriptEntry(fc.agentId(), "Facilitator", content, ctx.phaseIdx(), phase.name(), | |
| TranscriptEntryType.FACILITATION, Instant.now(), null, null)); | |
| meterRegistry.counter("eddi_group_facilitator_moves_total", | |
| "move", attemptedMove, "outcome", "rejected").increment(); | |
| auditMove(gc, new ParsedMove(null, attemptedMove, MAPPER.createObjectNode(), reason), ctx, "REJECTED", why); | |
| LOGGER.warnf("Facilitator move %s rejected for group %s at phase %d: %s", | |
| LogSanitizer.sanitize(attemptedMove), LogSanitizer.sanitize(gc.getId()), ctx.phaseIdx(), | |
| LogSanitizer.sanitize(why)); | |
| } | |
| private void auditMove(GroupConversation gc, ParsedMove parsed, CheckpointContext ctx, String outcome, String why) { | |
| if (auditLedgerService == null || !auditLedgerService.isEnabled()) { | |
| return; | |
| } | |
| try { | |
| var detail = new LinkedHashMap<String, Object>(); | |
| detail.put("move", parsed.move() != null ? parsed.move().name() : parsed.rawMove()); | |
| detail.put("outcome", outcome); | |
| detail.put("phaseIdx", ctx.phaseIdx()); | |
| detail.put("repeat", ctx.repeat()); | |
| if (parsed.reason() != null) { | |
| detail.put("reason", parsed.reason()); | |
| } | |
| if (why != null) { | |
| detail.put("rejectionReason", why); | |
| } | |
| auditLedgerService.submit(new AuditEntry( | |
| UUID.randomUUID().toString(), gc.getId(), gc.getGroupId(), null, gc.getUserId(), | |
| null, -1, "group.facilitator", "group", -1, 0L, | |
| Map.of(), detail, null, null, List.of(), 0.0, | |
| Instant.now(), null, null)); | |
| } catch (Exception e) { | |
| LOGGER.warnf("Failed to submit facilitator audit entry for group conversation %s: %s", | |
| LogSanitizer.sanitize(gc.getId()), 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/groups/FacilitatorEngine.java`
around lines 461 - 503, Update recordRejection to submit a REJECTED audit entry
through auditMove, preserving the attempted raw move and rejection reason. Add
an overload of auditMove that accepts the raw move string and rejection details,
populating rejectionReason while retaining existing parsed-move auditing for
executed moves. Ensure unparseable and unknown-move rejection paths reach this
audit submission.
Comment on lines
+1139
to
+1155
| public void skipHumanTurnOnTimeout(String groupConversationId) { | ||
| try { | ||
| var gc = GroupConversationSchemaMigrations.prepareForResume(conversationStore.read(groupConversationId)); | ||
| if (gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT || gc.getPendingHumanInput() == null) { | ||
| LOGGER.infof("Human-turn timeout for %s skipped — no longer awaiting human input", groupConversationId); | ||
| return; | ||
| } | ||
| var pending = gc.getPendingHumanInput(); | ||
| String reason = "No response from " + pending.displayName() | ||
| + (gc.getHitlApprovalTimeout() != null ? " within " + gc.getHitlApprovalTimeout() : ""); | ||
| resolveHumanTurn(gc, pending, null, reason, "system:timeout", null); | ||
| LOGGER.infof("Human turn for %s timed out (SKIP_TURN) — member '%s' skipped", | ||
| groupConversationId, pending.memberId()); | ||
| } catch (Exception e) { | ||
| LOGGER.errorf(e, "Failed to skip timed-out human turn for %s", groupConversationId); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sanitize the identifiers in these two log statements.
Lines 1143, 1150 and 1151 pass groupConversationId and pending.memberId() to the logger unsanitized. pending.memberId() originates from the group configuration's member list, so an operator-supplied id can forge log lines. Every other new log in this file wraps its identifiers in LogSanitizer.sanitize, including Lines 996, 1055 and 1092. Two findings of this class were already fixed in this PR.
🔒️ Proposed fix
if (gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT || gc.getPendingHumanInput() == null) {
- LOGGER.infof("Human-turn timeout for %s skipped — no longer awaiting human input", groupConversationId);
+ LOGGER.infof("Human-turn timeout for %s skipped — no longer awaiting human input",
+ LogSanitizer.sanitize(groupConversationId));
return;
}
@@
resolveHumanTurn(gc, pending, null, reason, "system:timeout", null);
LOGGER.infof("Human turn for %s timed out (SKIP_TURN) — member '%s' skipped",
- groupConversationId, pending.memberId());
+ LogSanitizer.sanitize(groupConversationId), LogSanitizer.sanitize(pending.memberId()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void skipHumanTurnOnTimeout(String groupConversationId) { | |
| try { | |
| var gc = GroupConversationSchemaMigrations.prepareForResume(conversationStore.read(groupConversationId)); | |
| if (gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT || gc.getPendingHumanInput() == null) { | |
| LOGGER.infof("Human-turn timeout for %s skipped — no longer awaiting human input", groupConversationId); | |
| return; | |
| } | |
| var pending = gc.getPendingHumanInput(); | |
| String reason = "No response from " + pending.displayName() | |
| + (gc.getHitlApprovalTimeout() != null ? " within " + gc.getHitlApprovalTimeout() : ""); | |
| resolveHumanTurn(gc, pending, null, reason, "system:timeout", null); | |
| LOGGER.infof("Human turn for %s timed out (SKIP_TURN) — member '%s' skipped", | |
| groupConversationId, pending.memberId()); | |
| } catch (Exception e) { | |
| LOGGER.errorf(e, "Failed to skip timed-out human turn for %s", groupConversationId); | |
| } | |
| } | |
| public void skipHumanTurnOnTimeout(String groupConversationId) { | |
| try { | |
| var gc = GroupConversationSchemaMigrations.prepareForResume(conversationStore.read(groupConversationId)); | |
| if (gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT || gc.getPendingHumanInput() == null) { | |
| LOGGER.infof("Human-turn timeout for %s skipped — no longer awaiting human input", | |
| LogSanitizer.sanitize(groupConversationId)); | |
| return; | |
| } | |
| var pending = gc.getPendingHumanInput(); | |
| String reason = "No response from " + pending.displayName() | |
| (gc.getHitlApprovalTimeout() != null ? " within " + gc.getHitlApprovalTimeout() : ""); | |
| resolveHumanTurn(gc, pending, null, reason, "system:timeout", null); | |
| LOGGER.infof("Human turn for %s timed out (SKIP_TURN) — member '%s' skipped", | |
| LogSanitizer.sanitize(groupConversationId), LogSanitizer.sanitize(pending.memberId())); | |
| } catch (Exception e) { | |
| LOGGER.errorf(e, "Failed to skip timed-out human turn for %s", 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/internal/groups/GroupHitlCoordinator.java`
around lines 1139 - 1155, Update the log statements in skipHumanTurnOnTimeout to
wrap groupConversationId and pending.memberId() with LogSanitizer.sanitize,
matching the existing identifier-sanitization pattern used elsewhere in
GroupHitlCoordinator. Apply this to both timeout status logs and the failure
log.
Comment on lines
+504
to
+529
| // I6: pending human turns join the SAME inbox — pauseType "HUMAN_TURN" | ||
| // (plus pendingMemberId) is the kind discriminator; no third inbox. Both | ||
| // states are queried with the FULL limit and merged oldest-pause-first, | ||
| // then capped — filling the window with approvals before ever querying | ||
| // human turns would starve exactly the entries a member's own inbox | ||
| // filter needs to see. | ||
| var pending = new ArrayList<>( | ||
| conversationStore.findByState(GroupConversationState.AWAITING_APPROVAL, groupId, clamped)); | ||
| pending.addAll(conversationStore.findByState(GroupConversationState.AWAITING_HUMAN_INPUT, groupId, clamped)); | ||
| pending.sort(Comparator.comparing(GroupConversation::getPausedAt, | ||
| Comparator.nullsLast(Comparator.naturalOrder()))); | ||
| return pending.stream() | ||
| .limit(clamped) | ||
| .map(gc -> { | ||
| var summary = new PendingApprovalSummary( | ||
| gc.getId(), null, gc.getUserId(), gc.getPausedAt(), | ||
| gc.getHitlPauseReason(), | ||
| gc.getHitlTimeoutPolicy() != null ? gc.getHitlTimeoutPolicy().name() : null); | ||
| summary.setGroupId(gc.getGroupId()); | ||
| summary.setApprovalTimeout(gc.getHitlApprovalTimeout()); | ||
| if (gc.getHitlPauseType() != null) { | ||
| summary.setPauseType(gc.getHitlPauseType().name()); | ||
| } | ||
| if (gc.getPendingHumanInput() != null) { | ||
| summary.setPendingMemberId(gc.getPendingHumanInput().memberId()); | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
This listing now loads up to two full-limit pages of whole group conversation documents.
findByState(state, groupId, clamped) returns List<GroupConversation>, so each element carries the full transcript. The change issues a second query with the same clamped limit and merges, so the endpoint can hold 2 × clamped full documents in heap — up to 2000 — to emit at most 1000 summaries.
The comment at Line 500 states "never hand full transcripts to a listing endpoint", but the projection happens in memory after the load. The regular surface avoids this with a real projection query: HitlCrashRecoveryObserver Lines 242-246 records that the equivalent scan there never loads "the (potentially multi-MB) full documents".
The full-limit-per-state fan-out is deliberate and correct for fairness, as the comment explains. The gap is the missing projection on the group store. Add a summary projection to IGroupConversationStore that returns only the pause bookmark fields, and query both states through it.
Run the following script to confirm the store exposes no projection variant:
#!/bin/bash
# Description: Check IGroupConversationStore for a summary/projection query and inspect findByState's Mongo implementation.
set -euo pipefail
fd -t f 'IGroupConversationStore.java' | while IFS= read -r f; do
echo "=== $f ==="
cat -n "$f"
done
# The Mongo implementation of findByState — does it project fields?
rg -n -C 20 'findByState' --type=java -g '**/mongo/**'
# The regular surface's projection, for the shape to mirror.
rg -n -C 10 'findPendingApprovalSummaries' --type=java🤖 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/GroupLifecycleOps.java`
around lines 504 - 529, Replace the full-document `findByState` calls in the
pending-list flow with a summary projection method added to
`IGroupConversationStore` and implemented by its Mongo store. Project only the
pause-bookmark fields required to build `PendingApprovalSummary`, while
retaining the existing full-limit queries, merge ordering, and cap across both
`AWAITING_APPROVAL` and `AWAITING_HUMAN_INPUT` states.
Comment on lines
+101
to
+117
| public static List<String> resolveOptions(VoteConfig config, List<TranscriptEntry> transcript) { | ||
| if (config.optionsSource() == OptionsSource.EXPLICIT) { | ||
| return config.options(); | ||
| } | ||
| for (int i = transcript.size() - 1; i >= 0; i--) { | ||
| TranscriptEntry entry = transcript.get(i); | ||
| if (entry != null && entry.type() == TranscriptEntryType.SYNTHESIS && entry.content() != null) { | ||
| List<String> options = new ArrayList<>(); | ||
| Matcher matcher = OPTION_LINE.matcher(entry.content()); | ||
| while (matcher.find()) { | ||
| options.add(matcher.group(2).trim()); | ||
| } | ||
| return options; | ||
| } | ||
| } | ||
| return List.of(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
resolveOptions returns the configured list verbatim, including null.
For OptionsSource.EXPLICIT the method returns config.options() without a null guard. Every caller then calls size() or iterates: PhaseExecutionEngine.recordVoteDecision at options.size() < 2, and GroupContextBuilder puts the value straight into the template data. A VoteConfig whose options is null therefore throws an NPE inside the phase rather than degrading to the honest NONE record this class promises. Normalize here.
🛡️ Proposed guard
public static List<String> resolveOptions(VoteConfig config, List<TranscriptEntry> transcript) {
if (config.optionsSource() == OptionsSource.EXPLICIT) {
- return config.options();
+ return config.options() != null ? config.options() : List.of();
}This is the same root cause raised on GroupContextBuilder.java lines 189-196; the guard belongs here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static List<String> resolveOptions(VoteConfig config, List<TranscriptEntry> transcript) { | |
| if (config.optionsSource() == OptionsSource.EXPLICIT) { | |
| return config.options(); | |
| } | |
| for (int i = transcript.size() - 1; i >= 0; i--) { | |
| TranscriptEntry entry = transcript.get(i); | |
| if (entry != null && entry.type() == TranscriptEntryType.SYNTHESIS && entry.content() != null) { | |
| List<String> options = new ArrayList<>(); | |
| Matcher matcher = OPTION_LINE.matcher(entry.content()); | |
| while (matcher.find()) { | |
| options.add(matcher.group(2).trim()); | |
| } | |
| return options; | |
| } | |
| } | |
| return List.of(); | |
| } | |
| public static List<String> resolveOptions(VoteConfig config, List<TranscriptEntry> transcript) { | |
| if (config.optionsSource() == OptionsSource.EXPLICIT) { | |
| return config.options() != null ? config.options() : List.of(); | |
| } | |
| for (int i = transcript.size() - 1; i >= 0; i--) { | |
| TranscriptEntry entry = transcript.get(i); | |
| if (entry != null && entry.type() == TranscriptEntryType.SYNTHESIS && entry.content() != null) { | |
| List<String> options = new ArrayList<>(); | |
| Matcher matcher = OPTION_LINE.matcher(entry.content()); | |
| while (matcher.find()) { | |
| options.add(matcher.group(2).trim()); | |
| } | |
| return options; | |
| } | |
| } | |
| return List.of(); | |
| } |
🤖 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/VoteTallyEngine.java`
around lines 101 - 117, Update resolveOptions so the EXPLICIT branch normalizes
a null config.options() to an empty list before returning, while preserving
configured options when non-null. Keep the existing transcript-derived
resolution unchanged so callers such as PhaseExecutionEngine.recordVoteDecision
and GroupContextBuilder always receive a non-null list.
Comment on lines
+305
to
+323
| private static void appendVoteTally(StringBuilder sb, DecisionRecord decision) { | ||
| if (decision.type() != DecisionType.VOTE || decision.tally() == null) { | ||
| return; | ||
| } | ||
| Object totals = decision.tally().get("totals"); | ||
| if (totals instanceof Map<?, ?> totalsMap && !totalsMap.isEmpty()) { | ||
| sb.append("Tally:\n"); | ||
| totalsMap.entrySet().stream().limit(MAX_TALLY_LINES).forEach(entry -> sb.append(String.format("• %s — %s\n", | ||
| buildPreview(String.valueOf(entry.getKey()), MAX_TALLY_OPTION_CHARS), entry.getValue()))); | ||
| if (totalsMap.size() > MAX_TALLY_LINES) { | ||
| sb.append(String.format("… and %d more option(s)\n", totalsMap.size() - MAX_TALLY_LINES)); | ||
| } | ||
| } | ||
| Object valid = decision.tally().get("validBallots"); | ||
| Object participants = decision.tally().get("participants"); | ||
| if (valid instanceof Number && participants instanceof Number) { | ||
| sb.append(String.format("Ballots: %s of %s\n", valid, participants)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape the tally option text before posting it to Slack.
appendVoteTally writes each option key into the message unescaped. Option text is not always operator-authored: with optionsSource: LAST_SYNTHESIS, VoteTallyEngine.resolveOptions extracts the options from a model-written synthesis. An option such as <!channel> or <!here> then renders as a Slack broadcast rather than as text.
This is the exact threat escapeMrkdwnHuman was added for on line 365. Apply the same escaping here.
🔒️ Proposed fix
totalsMap.entrySet().stream().limit(MAX_TALLY_LINES).forEach(entry -> sb.append(String.format("• %s — %s\n",
- buildPreview(String.valueOf(entry.getKey()), MAX_TALLY_OPTION_CHARS), entry.getValue())));
+ escapeMrkdwnHuman(buildPreview(String.valueOf(entry.getKey()), MAX_TALLY_OPTION_CHARS)),
+ escapeMrkdwnHuman(String.valueOf(entry.getValue())))));Consider the same treatment for decision.winner() on line 289, which carries the same model-derived option text.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static void appendVoteTally(StringBuilder sb, DecisionRecord decision) { | |
| if (decision.type() != DecisionType.VOTE || decision.tally() == null) { | |
| return; | |
| } | |
| Object totals = decision.tally().get("totals"); | |
| if (totals instanceof Map<?, ?> totalsMap && !totalsMap.isEmpty()) { | |
| sb.append("Tally:\n"); | |
| totalsMap.entrySet().stream().limit(MAX_TALLY_LINES).forEach(entry -> sb.append(String.format("• %s — %s\n", | |
| buildPreview(String.valueOf(entry.getKey()), MAX_TALLY_OPTION_CHARS), entry.getValue()))); | |
| if (totalsMap.size() > MAX_TALLY_LINES) { | |
| sb.append(String.format("… and %d more option(s)\n", totalsMap.size() - MAX_TALLY_LINES)); | |
| } | |
| } | |
| Object valid = decision.tally().get("validBallots"); | |
| Object participants = decision.tally().get("participants"); | |
| if (valid instanceof Number && participants instanceof Number) { | |
| sb.append(String.format("Ballots: %s of %s\n", valid, participants)); | |
| } | |
| } | |
| private static void appendVoteTally(StringBuilder sb, DecisionRecord decision) { | |
| if (decision.type() != DecisionType.VOTE || decision.tally() == null) { | |
| return; | |
| } | |
| Object totals = decision.tally().get("totals"); | |
| if (totals instanceof Map<?, ?> totalsMap && !totalsMap.isEmpty()) { | |
| sb.append("Tally:\n"); | |
| totalsMap.entrySet().stream().limit(MAX_TALLY_LINES).forEach(entry -> sb.append(String.format("• %s — %s\n", | |
| escapeMrkdwnHuman(buildPreview(String.valueOf(entry.getKey()), MAX_TALLY_OPTION_CHARS)), | |
| escapeMrkdwnHuman(String.valueOf(entry.getValue()))))); | |
| if (totalsMap.size() > MAX_TALLY_LINES) { | |
| sb.append(String.format("… and %d more option(s)\n", totalsMap.size() - MAX_TALLY_LINES)); | |
| } | |
| } | |
| Object valid = decision.tally().get("validBallots"); | |
| Object participants = decision.tally().get("participants"); | |
| if (valid instanceof Number && participants instanceof Number) { | |
| sb.append(String.format("Ballots: %s of %s\n", valid, participants)); | |
| } | |
| } |
🤖 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/integrations/slack/SlackGroupDiscussionListener.java`
around lines 305 - 323, Update appendVoteTally to pass each option key through
the existing escapeMrkdwnHuman helper before appending it to the Slack message,
while retaining buildPreview’s length limit. Apply the same escaping to
decision.winner() in the nearby winner-rendering logic, since it can contain
model-derived option text.
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
3 participants
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
I12 from
planning/group-collaboration-improvements-plan.md— the plan's resolution of adaptive orchestration vs deterministic governance (pillar 2): a facilitator agent is briefed at configured checkpoints with a compact state summary (never the full transcript) and selects exactly one config-enumerated move. Every selection is validated against the checkpoint's context, capped, and audit-logged; anything unparseable, disallowed, invalid-in-context or failed degrades to CONTINUE.Config
Save-time validation: enabled needs
agentId; END_PHASE/EXTEND_PHASE act mid-phase and are rejected unlesscheckAfter: EACH_REPEAT; ESCALATE_HUMAN needsescalateTo; cap ≤ 100.Moves (v1)
maxTurns). A convergence exit is never overruled — the checkpoint context makes that structural.ContextScope.NONEshape by construction (args.options, 2–10).RecruitAgentTool's full validation matrix (already-member, recruit cap, deployed-and-ready, synchronized double-check commit).AWAITING_HUMAN_INPUTon the configured principal, same submission endpoint/guard/inbox/timeout schedule. The bookmark points at the resume position withspeakerIdx=-1so the shared+1advance lands at speaker 0; the answer records as a peer-visibleFOLLOW_UP.Honesty & bounds
maxTurns, dollars on the I1 ledger, skipped once either budget is gone.maxMovesPerDiscussion, each recorded as a peer-hiddenFACILITATIONentry +group.facilitatoraudit event +eddi_group_facilitator_moves_total{move,outcome}.FACILITATIONentry too (the audit trail must show the model tried) but never consume the budget.Runtime phase divergence (schema v4)
CALL_VOTE / EXTEND_PHASE diverge a runtime copy of the phase list, persisted as
gc.runtimePhases; every resume surface (approval resume, human-turn submission, timeout skip) now resolveseffectivePhases(gc, config)so bookmarks and drift checks compare against the list the pause was taken from.CURRENT_SCHEMA_VERSION3→4 — an older pod resuming a diverged document would mis-index every bookmark, which is exactly what the newer-than-current refusal exists to prevent. Divergence is one-off: completion and continuation rounds clear it.Checkpoint placement is deliberate on both cadences: EACH_REPEAT runs after the repeat's own bookkeeping and before the outcome break; EACH_PHASE runs after the HITL gate, so a gated phase's approval can never be silently skipped by an escalation.
Tests
~55 new across 5 classes: parse tiers; every move happy/disallowed/malformed/invalid-in-context; move + extension caps (E2E: repeats=1 with an always-extend facilitator runs exactly 3 rounds); budget gates never call the model; briefing boundedness (<5k chars against a 100k-char transcript, no full entry content); mutation-check that an un-listed move degrades to CONTINUE with zero effect; CALL_VOTE E2E proving the runtime insertion ran (ballots + VOTE DecisionRecord from a config with no vote phase); escalation E2E + coordinator submit test proving the drift check passes only because
effectivePhasesreturns the runtime list; facilitator-unavailable and CONTINUE-everywhere leave the discussion untouched. Fullengine.internal+configs.groups+engine.hitl+engine.mcpsuites green.Summary by CodeRabbit