feat(groups): I6 — humans as group members (AWAITING_HUMAN_INPUT turns) - #640
Merged
Conversation
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.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Warning Review limit reached
Next review available in: 24 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds HUMAN members to group conversations. Human turns pause execution in ChangesHuman group-member HITL
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GroupMember
participant REST_or_MCP
participant HitlAccessGuard
participant GroupHitlCoordinator
participant GroupConversationService
GroupMember->>REST_or_MCP: submit human input
REST_or_MCP->>HitlAccessGuard: verify pending-member access
REST_or_MCP->>GroupHitlCoordinator: submitHumanInput
GroupHitlCoordinator->>GroupConversationService: resume discussion
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}
![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: 12
🧹 Nitpick comments (7)
src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java (1)
297-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
availableActionsbranch.The enum assertion is correct. The
AvailableActionsTestsnest in this file has no case forAWAITING_HUMAN_INPUT, and thenonTerminaltest at lines 392-393 deliberately omits that state. Nothing pins the new"submitHumanInput"value, which is serialized to REST and MCP clients.💚 Proposed test
`@Test` `@DisplayName`("AWAITING_HUMAN_INPUT offers submitHumanInput") 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 nest covering GroupConversationState.AWAITING_HUMAN_INPUT, setting that state on a GroupConversation and asserting getAvailableActions() returns exactly ["submitHumanInput"].src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java (1)
825-827: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the action name with the existing casing convention.
The existing action strings are lowercase:
"followup","continue","close". The new value is camelCase:"submitHumanInput".availableActionsis serialized to REST and MCP clients, so a client that maps action strings to UI controls now handles two casing styles. Pick one convention before the value ships.♻️ Proposed rename
- case AWAITING_HUMAN_INPUT -> List.of("submitHumanInput"); + case AWAITING_HUMAN_INPUT -> List.of("submit-human-input");🤖 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/GroupConversation.java` around lines 825 - 827, Update the AWAITING_HUMAN_INPUT branch in GroupConversation’s availableActions mapping to use the established lowercase action-name convention instead of "submitHumanInput". Keep the action’s meaning unchanged and ensure the serialized REST and MCP value matches the existing lowercase actions.src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java (1)
160-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a
nullmember list.Both helpers always supply a non-null member list:
config(...)leaves the field at its default empty list, andhumanConfig(...)passesList.of(members). No test reaches theconfig.getMembers() == nullbranch ofhumanMemberProblems, which is the same input shape that NPEs invalidateHumanMembers.🤖 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 160 - 166, Extend agentOnlyGroups_produceNoHumanProblems to cover configurations whose members field is null, asserting humanMemberProblems returns empty and hasHumanMembers remains false without throwing. Construct the test input so config.getMembers() is explicitly null, exercising the same null-member path used by validateHumanMembers.src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java (1)
167-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider counting the human skip like the member-pause skip.
The comment states this branch mirrors
handleMemberPause's SKIP precedent, but it omits both of that method's observability signals:counterGroupMemberPauseSkipped.increment()andlistener.onMemberPauseSkipped(...). A human reached by an automated sub-round (convergence judge, dissent round, task-force wave) therefore produces a transcript entry with no metric and no event, so operators cannot see how often a configuration routes automated turns at people.The
listenerargument is already in scope on this overload.🤖 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 167 - 177, Update the HUMAN-member branch in MemberTurnExecutor to mirror handleMemberPause observability before returning the SKIPPED TranscriptEntry: increment counterGroupMemberPauseSkipped and invoke listener.onMemberPauseSkipped(...) using the available listener and relevant member/turn context.src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java (1)
382-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the executor-saturation rollback.
This class exercises the happy path and every pre-mutation refusal, but not the one post-CAS failure branch. Make
executorService.submit(...)throw aRejectedExecutionExceptionand assert thatresolveHumanTurnremoves the appended entry, restoresAWAITING_HUMAN_INPUTwith the pending record and the original bookmark, re-arms the schedule, and throwsGroupDiscussionException. That branch mutates the most state and currently has no regression guard.🤖 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 - 412, Extend GroupHitlCoordinatorTest with a resolveHumanTurn saturation test that makes executorService.submit(...) throw RejectedExecutionException after the human entry is appended. Assert that the operation throws GroupDiscussionException and rolls back the transcript entry, restores AWAITING_HUMAN_INPUT with the original pending-human record and resume bookmark, and re-arms the schedule using the existing store/service verification patterns.src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java (1)
240-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a resume case for a phase with
repeats > 1.Both resume tests use
phase(TurnOrder.PARALLEL), whose helper fixesrepeats = 1, so every pause and resume in this class carriesrepeatIdx = 0. That is the one repeat index the phase loop inGroupConversationService.executeDiscussioncan match, because the loop always restarts atrepeat = 0. A test that pauses atrepeatIdx = 1would cover the resume path aROUND_TABLEdiscussion phase actually takes.🤖 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 240 - 273, Extend the parallel-phase resume coverage with a case using a phase configuration whose repeats value is greater than one, and set the invocation’s repeat index to 1. Preserve the existing assertions for resuming without rerunning fan-out and for the next-human pause, ensuring the test exercises the repeat-aware resume path used by GroupConversationService.executeDiscussion.src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java (1)
644-650: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe zero-remaining-turns early return also skips the human tail.
When
maxTurns <= 0,remainingTurnsis derived fromagentSpeakers.size(). A phase whose speakers are all HUMAN then hasremainingTurns == 0and returns at Line 646, sopromptHumanTailnever runs and no human is prompted — even thoughmaxTurns <= 0means "no turn cap".
executeDiscussionalways passesmaxTurns > 0(it defaults to 50), so this is currently only reachable through direct callers and tests. Deriving the uncapped size fromspeakers.size()keeps the branch honest.♻️ Proposed adjustment
- int remainingTurns = maxTurns > 0 ? Math.max(0, maxTurns - turnCounter.get()) : agentSpeakers.size(); - if (remainingTurns == 0) { + int remainingTurns = maxTurns > 0 ? Math.max(0, maxTurns - turnCounter.get()) : speakers.size(); + if (remainingTurns == 0) { + // An exhausted turn budget owes no more turns — the human's included. return; } List<GroupMember> batchSpeakers = maxTurns > 0 ? agentSpeakers.subList(0, Math.min(agentSpeakers.size(), remainingTurns)) : agentSpeakers;🤖 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/PhaseExecutionEngine.java` around lines 644 - 650, Update the remaining-turn calculation in executeDiscussion so the uncapped branch uses the full speakers collection, not agentSpeakers.size(). Preserve the zero-turn early return for capped discussions while allowing maxTurns <= 0 phases containing only HUMAN speakers to continue through promptHumanTail.
🤖 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/configs/groups/mongo/AgentGroupStore.java`:
- Around line 90-114: Guard validateHumanMembers in AgentGroupStore.java (lines
90-114) with an early return when config.getMembers() is null, before the
nested-group loop and moderator stream. In AgentGroupStoreTest.java (lines
160-166), add coverage using setMembers(null) and assert humanMemberProblems
returns an empty list and hasHumanMembers returns false.
- Around line 157-165: Update the humanMemberConfig.turnTimeout validation in
AgentGroupStore to retain the parsed Duration, then reject values that are zero
or negative in addition to malformed ISO-8601 values. Add the existing
diagnostic to problems for any non-positive duration, avoiding a separate
pure-method call on the parse result.
In `@src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java`:
- Around line 161-198: Update GroupLifecycleOps.deleteGroupConversation to
include AWAITING_HUMAN_INPUT alongside AWAITING_APPROVAL in the paused-deletion
cleanup branch, ensuring the timeout schedule is canceled and deferred terminal
cleanup proceeds when a human turn is pending.
In `@src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java`:
- Around line 159-160: In HitlAccessGuard, extend authorization for the current
pendingHumanInput.memberId so that only that member receives a minimal
pending-human-input view containing renderedPrompt, without exposing the full
transcript; preserve existing owner/admin/approver access. In
docs/group-conversations.md lines 181-193, document that the UI reads
pendingHumanInput.renderedPrompt only after the member-scoped prompt view is
available.
In `@src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java`:
- Around line 380-386: Update the repair flow containing the AWAITING_APPROVAL
scan and repairGroupHumanPaused() so each scan has independent exception
handling. Ensure an exception from the AWAITING_APPROVAL query or loop is logged
and does not prevent repairGroupHumanPaused() from running, while preserving the
existing return/count behavior for successful repairs.
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 1023-1027: Update the finally-block condition that guards
signingGuard.forgetConversation in GroupConversationService so it preserves the
signing cursor for both AWAITING_APPROVAL and AWAITING_HUMAN_INPUT; only forget
the conversation for other states.
- Around line 721-735: Preserve the resumePoint.repeatIdx() before clearing the
bookmark and use it to initialize the repeat loop in executeDiscussion, rather
than always starting at repeat 0. Apply the saved repeat offset only for the
matching phase and retain the existing speaker and parallel-human offset
handling, so resumed repeats skip all earlier agents and humans without changing
fresh executions.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java`:
- Around line 1160-1163: Update resolveHumanTurn so
counterGroupHitlResume.increment() and auditHumanTurnResolution(...) occur only
after executorService.submit(...) succeeds, preserving rollback behavior when
submission is rejected. In the rollback path, replace the direct
activeTokens.remove(gc.getId()) with removeTokenAndConvertIfSignalled(gc,
listener) so cancellations signalled during restoration are preserved.
- Around line 1209-1210: Update the return path in the surrounding
GroupHitlCoordinator method to re-read and return a fresh group-conversation
copy instead of the live gc instance. Preserve the existing persistence flow,
and ensure RestGroupConversation.submitHumanInput receives the freshly-read
object so resumeWork mutations cannot race response serialization.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java`:
- Around line 496-503: Update the pending retrieval flow in GroupLifecycleOps so
AWAITING_HUMAN_INPUT candidates are not excluded when AWAITING_APPROVAL rows
consume the limit. Fetch and merge sufficient candidates from both states before
HitlAccessGuard filtering, preferably through a state-set query scoped by group
and pending member, then apply the visible-result limit after filtering.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java`:
- Around line 612-633: Update the resume validation in resumeDiscussion for
RESUME_KIND_HUMAN parallel bookmarks to compare speakerIdx against the resolved
humans sublist size, not the full speaker roster. Reject or route through
existing drift handling when the index is outside that human-only range,
preventing executeParallelPhase from passing an invalid humanResumeIdx to
promptHumanTail.
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 887-893: Update onHumanInputRequested in the listener to count
down completionLatch after sending the human-input event and closing the stream,
mirroring the finally behavior in onHitlPause so awaitCompletion is released for
this terminal listener.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java`:
- Around line 825-827: Update the AWAITING_HUMAN_INPUT branch in
GroupConversation’s availableActions mapping to use the established lowercase
action-name convention instead of "submitHumanInput". Keep the action’s meaning
unchanged and ensure the serialized REST and MCP value matches the existing
lowercase actions.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`:
- Around line 167-177: Update the HUMAN-member branch in MemberTurnExecutor to
mirror handleMemberPause observability before returning the SKIPPED
TranscriptEntry: increment counterGroupMemberPauseSkipped and invoke
listener.onMemberPauseSkipped(...) using the available listener and relevant
member/turn context.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java`:
- Around line 644-650: Update the remaining-turn calculation in
executeDiscussion so the uncapped branch uses the full speakers collection, not
agentSpeakers.size(). Preserve the zero-turn early return for capped discussions
while allowing maxTurns <= 0 phases containing only HUMAN speakers to continue
through promptHumanTail.
In `@src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java`:
- Around line 297-302: Add a test in the AvailableActionsTests nest covering
GroupConversationState.AWAITING_HUMAN_INPUT, setting that state on a
GroupConversation and asserting getAvailableActions() returns exactly
["submitHumanInput"].
In `@src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java`:
- Around line 160-166: Extend agentOnlyGroups_produceNoHumanProblems to cover
configurations whose members field is null, asserting humanMemberProblems
returns empty and hasHumanMembers remains false without throwing. Construct the
test input so config.getMembers() is explicitly null, exercising the same
null-member path used by validateHumanMembers.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java`:
- Around line 382-412: Extend GroupHitlCoordinatorTest with a resolveHumanTurn
saturation test that makes executorService.submit(...) throw
RejectedExecutionException after the human entry is appended. Assert that the
operation throws GroupDiscussionException and rolls back the transcript entry,
restores AWAITING_HUMAN_INPUT with the original pending-human record and resume
bookmark, and re-arms the schedule using the existing store/service verification
patterns.
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java`:
- Around line 240-273: Extend the parallel-phase resume coverage with a case
using a phase configuration whose repeats value is greater than one, and set the
invocation’s repeat index to 1. Preserve the existing assertions for resuming
without rerunning fan-out and for the next-human pause, ensuring the test
exercises the repeat-aware resume path used by
GroupConversationService.executeDiscussion.
🪄 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: 360a560f-db68-4ed9-ac9f-93a99086c18c
📒 Files selected for processing (31)
docs/changelog.mddocs/group-conversations.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.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/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/lifecycle/GroupConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.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/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.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.java
- 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.
This was referenced Aug 8, 2026
…pause A human turn pauses mid-repeat, after other speakers appended this repeat's entries; the resumed leg recomputed the slice base from the current transcript size, so the repeat slice covered only post-pause entries - the convergence check (and, once merged with I14, the VOTE tally) silently lost every pre-pause contribution. New persisted pausedRepeatSliceBase, written at the pause commit and consumed exactly once with the speaker-bookmark read-and-clear discipline.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java (1)
159-160: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply the caller filter before the result limit.
listGroupPendingApprovals(groupId, limit)receives the limit before this filter runs. If other users fill that window, the pending member can receive an empty inbox even when their assigned turn exists.Add a scoped service or store query for
ownerId OR pendingMemberId, then applylimitafter that predicate.🤖 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/hitl/HitlAccessGuard.java` around lines 159 - 160, Update listGroupPendingApprovals so the callerId predicate is applied in the scoped service/store query before limiting results; add or reuse a query filtering ownerId or pendingMemberId, then apply limit to the filtered results so the assigned pending member’s approvals are included.
🤖 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/configs/groups/model/GroupConversation.java`:
- Around line 165-174: Increment CURRENT_SCHEMA_VERSION from 3 to 4 and register
the corresponding v3-to-v4 migration for GroupConversation documents. Ensure the
migration preserves legacy behavior by initializing pausedRepeatSliceBase to -1,
while retaining the existing newer-document refusal guard.
In `@src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java`:
- Around line 730-740: Introduce a separate awaitingApproval predicate in the
full-detail authorization logic of RestGroupConversation, and use it instead of
paused for the approver branch so approvers can read transcripts only during
AWAITING_APPROVAL; retain paused for summary fields. Apply the same predicate
distinction in McpHitlTools.getGroupApprovalStatus and add a regression test
covering an approver during AWAITING_HUMAN_INPUT.
In
`@src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java`:
- Around line 171-183: Update midRepeatHumanPause_persistsTheRepeatSliceBase to
capture the GroupConversation passed to conversationStore.update(...) during
service.discuss(), then assert pausedRepeatSliceBase on that persisted argument
rather than only on the returned instance. If the persistence suite has Mongo
serialization coverage, add a round-trip assertion for the same field.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java`:
- Around line 159-160: Update listGroupPendingApprovals so the callerId
predicate is applied in the scoped service/store query before limiting results;
add or reuse a query filtering ownerId or pendingMemberId, then apply limit to
the filtered results so the assigned pending member’s approvals are included.
🪄 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: f722b22a-3150-42db-93c4-484f00aae513
📒 Files selected for processing (18)
docs/changelog.mdsrc/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/hitl/HitlAccessGuard.javasrc/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.javasrc/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.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/mcp/McpHitlToolsCoverageTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java
- src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java
- src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java
- src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java
- src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java
- src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java
- src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
- src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java
…rsist-time test (review round) - CURRENT_SCHEMA_VERSION 3 -> 4: pausedRepeatSliceBase is resume-consumed persisted state; legacy docs default to -1 via Jackson (no migration) - the approver detail=full window now requires AWAITING_APPROVAL on both REST and MCP surfaces — the shared paused predicate also admitted approvers to the transcript during AWAITING_HUMAN_INPUT - the mid-repeat pause test records pausedRepeatSliceBase at persist time inside the update() stub instead of asserting the mutable instance
aisabella-ai
self-requested a review
August 8, 2026 08:09
aisabella-ai
approved these changes
Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Implements I6 — Human as group member from
planning/group-collaboration-improvements-plan.md(queue position 4 inplanning/group-collaboration-NEXT.md, after I17 #637, I14 #638, I8 #639). Real deployments are hybrid teams: amemberType: "HUMAN"member sits in the roster like any agent, but their turn pauses the discussion in a new stateAWAITING_HUMAN_INPUTuntil they answer — deliberately NOTAWAITING_APPROVAL(approval endpoints must never accept free text; inboxes must tell "approve/reject" from "you're up").Design (per the plan, decisions honored)
Turn flow
The phase loops intercept HUMAN speakers before any LLM machinery, render their input exactly like an agent's (
buildPhaseInput), and surface aHumanTurnRequiredcontrol-flow signal;executeDiscussioncatches it andGroupHitlCoordinator.commitHumanTurnPausepersists:PendingHumanInput{memberId, displayName, phaseIdx, repeatIdx, speakerIdx, entryType, renderedPrompt, onTimeout, requestedAt}— the prompt survives restarts and the UI reads it from the conversation;ResumePoint— writer-less until now; this is its first producer;pausedTurnCount = turns + 1) so a human turn is never free.Submission
POST /groups/{groupId}/conversations/{id}/human-input{memberId, content}+ MCPsubmit_group_human_input. Authorization is a new, deliberately narrower guard (HitlAccessGuard.requireGroupHumanInputAccess): the pending member's own principal or an admin — aneddi-approvermay decide approvals they don't own, but speaking as another human is impersonation, not review. The answer is recorded as the phase's natural entry type (captured at pause time so a config edit can't re-type it), the bookmark advances past the answered speaker (speakerIdx + 1, per the plan), the CAS out ofAWAITING_HUMAN_INPUTturns double-submits into 409s, and the discussion re-enters on a background thread exactly like an approval resume. Drift checks run before any mutation — a stale bookmark refuses the submission instead of needing a rollback; the one post-CAS failure (executor saturation) rolls the append back and restores the pause.Timeouts
humanMemberConfig {turnTimeout (ISO-8601, null = wait indefinitely), onTimeout = SKIP_TURN | ABORT}reuses the HITL schedule machinery on a new surfacegroup-human— SKIP_TURN/ABORT are notHitlTimeoutPolicyvalues, soHitlTimeoutHandlerbranches on the surface before parsing. SKIP_TURN writes the plan's SKIPPED entry ("No response from within ") and resumes; ABORT cancels gracefully. Crash recovery re-arms human-turn timeouts (policy bookmarked on the pending record, so a config edit while paused cannot change what the pause promised).PARALLEL phases
Humans never join the fan-out (an LLM answers in seconds; a paused future would pin the batch). Agents run first, then humans are prompted sequentially against the pre-fan-out snapshot — an independent round stays independent. The one carve-out from "PARALLEL never honors a bookmark": a
HUMAN_TURN_PARALLELresume skips the fan-out and resumes the human tail, so agent turns are not duplicated on resume.Save-time matrix (
AgentGroupStore.validateHumanMembers, hard-throws)Hard rejection is safe where the older checks had to warn: no legacy document can contain the new enum value. displayName required; humans rejected in task-force groups (PLAN/EXECUTE/VERIFY — wave workers can't pause) and
targetEachPeerphases (a human on both axes owes up to 2(N−1) pauses and the flat bookmark has no (speaker,target) coordinate) — preset-expanded, or the check is inert for preset-style groups; nested groups containing humans rejected one level deep, with a runtime backstop (MemberTurnExecutorcancels a strandedAWAITING_HUMAN_INPUTchild, like nested HITL).turnTimeoutmust parse. A human moderator is allowed + warned — andresolveParticipantsnow preserves the roster member's type for the moderator id (the 4-arg ctor silently demoted a human moderator to an agent).Surfaces
human_input_requestedevent (constant → record → listener default → SSE forward + OpenAPI list → Slack "you're up" notice, mrkdwn-escaped); pending human turns join the existing inbox aspauseType: "HUMAN_TURN"+pendingMemberId(no third inbox) and the member sees their own turns without owning the conversation;availableActionsgainssubmitHumanInput; MCPget_group_approval_statusreports the pending member + rendered prompt; every cancel path treats the new state as a first-class pause. Defense in depth: a HUMAN reachingexecuteAgentTurn(judge/dissent/task-wave/nested — contexts that cannot pause) yields SKIPPED.Deviation, recorded
The I14
HUMAN_DECIDEStie-policy stays save-time-rejected: I14 (#638) is not merged, so the wiring is a small follow-up once both branches land (I12 needs both anyway).Tests (+23 across 8 classes; 1869 tests green across
engine.internal+configs.groups+engine.hitl; checkstyle clean)Sequential pause (rendered prompt, absolute index, budget-before-human ordering); parallel fan-out-then-human with pre-fan-out blindness asserted via captor, resume-tail without fan-out re-run, post-resume blindness; pause-commit shape (bookmark/pending/schedule surface + policy); submit→record→advance→CAS→re-enter (captured runnable proves the re-entry coordinates); wrong member / wrong state / blank / oversize / config drift all refuse before mutation; SKIP_TURN timeout writes the named SKIPPED entry and advances; cancel-of-human-pause clears the pending turn; timeout-handler routing (SKIP_TURN / ABORT / unknown-degrades-to-skip); guard matrix (member ✓, admin ✓, owner+approver ✗ FORBIDDEN, wrong-group 404, auth-off no-op, inbox shows members their own turns); save-time matrix; human-moderator preservation; defense-in-depth skip; 4 enum pins updated (the I14/I8 CI lesson — caught locally this time).
Summary by CodeRabbit