diff --git a/docs/changelog.md b/docs/changelog.md index fd9d4ffc6..84757c735 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,66 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## πŸ”Ž fix(groups): I6 review round β€” schema bump, approver full-view scope, persist-time assertion (2026-08-08) + +**Repo:** EDDI (`feat/group-i6-human-members`) + +Three accepted CodeRabbit findings on PR #640 (all against the pausedRepeatSliceBase fix commit): + +1. **Schema version (major)** β€” `pausedRepeatSliceBase` is persisted state the resumed leg depends on, but `CURRENT_SCHEMA_VERSION` stayed 3. Bumped to 4 (no migration entry: Jackson defaults legacy documents to -1, the exact pre-v4 behavior). On the integration branch v4 is the release shape shared with I11's `negotiationState` and I12's `runtimePhases`. +2. **Approver transcript scope (major, security)** β€” the `detail=full` gate used the shared `paused` predicate, which also covers `AWAITING_HUMAN_INPUT`; an `eddi-approver` could read the full transcript of a discussion merely waiting on a human member's turn. Both surfaces (REST `getGroupApprovalStatus`, MCP `get_group_approval_status`) now gate the approver window on a dedicated `awaitingApproval` predicate; summary fields keep the wider one. Regression tests on both surfaces (approver + human-turn pause β†’ 403/FORBIDDEN). +3. **Persist-time assertion (minor)** β€” the mid-repeat pause test asserted only the in-memory instance; a captor would hold the same mutable object, so the test now records `pausedRepeatSliceBase` inside the `update()` stub at persist time and asserts the last persisted value. + +--- + +## πŸ”Ž fix(groups): I6 final-review finding β€” mid-repeat pause loses the repeat slice (2026-08-08) + +**Repo:** EDDI (`feat/group-i6-human-members`) + +Confirmed CRITICAL from the final multi-agent review pass (2 independent verifiers traced it): a human turn pauses MID-repeat, after other speakers already appended this repeat's entries β€” but the resumed leg recomputed `transcriptSizeBeforeRepeat` from the current transcript size, so the repeat slice covered only post-pause entries. Every consumer of that slice silently lost the pre-pause contributions: the convergence check on this branch, and (on the integration tree) VOTE tallies missing every agent ballot cast before the human's β€” a wrong election, reported as legitimate. + +**Fix:** new persisted `pausedRepeatSliceBase` on `GroupConversation` (βˆ’1 = unset; legacy documents keep the old recompute), written when the human-turn pause commits (the catch site has the true base in scope) and consumed exactly once with the same read-and-clear discipline as the speaker bookmark. Tests: the pause persists the base pointing at the top of the repeat (fails without the write), and a resumed leg consumes it exactly once (fails without the consume). + + +## πŸ”Ž fix(groups): I6 PR #640 review round 1 (2026-08-08) + +**Repo:** EDDI (`feat/group-i6-human-members`) + +CI failure + all 20 review comments (CodeQL Γ—5, code-quality Γ—4, CodeRabbit Γ—11) triaged; every one accepted and fixed: + +- **CI**: `submit_group_human_input` added to `McpToolFilter`'s whitelist (a non-whitelisted MCP tool is unreachable dead code β€” the guard test caught exactly that). +- **The pending member can now READ their turn**: new `HitlAccessGuard.requireGroupConversationReadAccess` β€” owner/admin/approver PLUS the human member a pending turn waits on β€” used by the REST and MCP approval-status endpoints (whose summary now carries `pendingMemberId`/`pendingHumanPrompt` on both surfaces). The full-transcript view stays role-gated: the member's working material is the rendered prompt, never the transcript. +- **Mid-phase resume no longer replays earlier repeats**: the phase loop starts at the bookmark's `repeatIdx` (clamped) β€” each replayed repeat was a full round of duplicate turns and spend. +- **Metric/audit/resume-event moved AFTER the successful executor submit** in the human-turn resolution (a rolled-back attempt must not pollute the resume metric or the EU-AI-Act trail β€” the rule `resumeDiscussion` already followed); the rollback path now re-checks the control token (`removeTokenAndConvertIfSignalled`) so a cancel racing the rollback is not dropped; and the method returns a **freshly-read copy** instead of the live instance the background leg mutates under the serializer. +- **Slack listener releases its completion latch on a human pause** (it blocked `awaitCompletion`'s full 300s on every human turn); **deletion of an `AWAITING_HUMAN_INPUT` conversation runs the paused-cleanup branch** (timeout schedule + ephemeral agents + signing cursor); **the signing cursor now survives a human pause** in `executeDiscussion`'s finally; **crash-recovery sweeps are isolated** (a failing approval query no longer skips the human re-arm). +- **Inbox starvation fixed**: both pause states are queried with the full limit, merged oldest-pause-first, then capped β€” approvals can no longer push a member's own turn out of the window. +- **Validation**: `turnTimeout` must be positive (PT0S/PT-4H parsed but armed an immediately-firing timeout that silently skipped every turn); `"members": null` cannot NPE the nested/moderator checks. +- **F2 drift guard explicitly scoped to approval bookmarks** (human bookmarks never reach `resumeDiscussion` β€” disjoint states β€” and their advanced `speakerIdx+1` semantics would false-positive at the last-speaker boundary; the executors clamp instead). +- CodeQL Γ—5 sanitized; the `HumanTurnRequired` `@param` docs moved from class to constructor Javadoc (Γ—4). + +**Tests:** +7 (read-access matrix incl. stranger-refused + wrong-group-404; full-view refusal for the pending member; PT0S/PT-4H rejection; null-members no-NPE; MCP guard/gate re-alignment Γ—2). Suites: 2882 green across `engine.internal` + `configs.groups` + `engine.hitl` + `engine.mcp`; checkstyle clean. + +--- + +## πŸ™‹ feat(groups): I6 β€” humans as group members (2026-08-08) + +**Repo:** EDDI (`feat/group-i6-human-members`) + +Fourth Wave 2 queue item. Humans can finally *speak*, not just gate: a `MemberType.HUMAN` member's turn pauses the discussion in a **new state `AWAITING_HUMAN_INPUT`** until they submit β€” deliberately not `AWAITING_APPROVAL` (approval endpoints must never accept free text; inboxes must tell "approve/reject" from "you're up"). + +- **Turn flow, exactly per the plan:** the phase loops intercept HUMAN speakers before any LLM machinery, render their input *exactly like an agent's* (`buildPhaseInput`), and surface a `HumanTurnRequired` control-flow signal; `executeDiscussion` catches it, and `GroupHitlCoordinator.commitHumanTurnPause` persists `PendingHumanInput{memberId, displayName, phaseIdx, repeatIdx, speakerIdx, entryType, renderedPrompt, onTimeout, requestedAt}` + the F2 `ResumePoint` β€” writer-less until now, this is its first producer. The human's turn is counted at the pause (`pausedTurnCount = turns+1`), so it is never free. +- **Submission:** `POST /groups/{groupId}/conversations/{id}/human-input` + MCP `submit_group_human_input`. Authorization is a NEW guard (`requireGroupHumanInputAccess`): the pending member's own principal or admin β€” deliberately narrower than approve (an `eddi-approver` may decide approvals; speaking as another human is impersonation). The answer lands as the phase's natural entry type (captured at pause time so config edits can't re-type it), the bookmark advances past the answered speaker, the CAS out of `AWAITING_HUMAN_INPUT` makes double-submits a 409, and the discussion re-enters like an approval resume. Drift-checks run BEFORE any mutation β€” a stale bookmark refuses the submission instead of needing rollback; the one post-CAS failure (executor saturation) rolls the append back and restores the pause. +- **Timeouts:** `humanMemberConfig {turnTimeout (ISO-8601, null=wait), onTimeout=SKIP_TURN|ABORT}`, riding the HITL schedule machinery with a new surface `group-human` β€” the SKIP_TURN/ABORT policies are NOT `HitlTimeoutPolicy` values, so the fire handler branches on 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). +- **PARALLEL phases:** humans never join the fan-out; agents run first, humans are then prompted sequentially against the **pre-fan-out snapshot** (blindness preserved). The one carve-out from "PARALLEL never honors a bookmark": a `HUMAN_TURN_PARALLEL` resume skips the fan-out entirely and resumes the human tail β€” no duplicate agent turns on resume. +- **Save-time matrix** (`AgentGroupStore.validateHumanMembers`, hard-throws β€” safe because no legacy doc can contain the new enum value): displayName required; no humans in task-force (PLAN/EXECUTE/VERIFY) or `targetEachPeer` groups (preset-EXPANDED, or the check is inert); nested groups containing humans rejected one level deep (runtime backstop in `MemberTurnExecutor` cancels a stranded `AWAITING_HUMAN_INPUT` child); `turnTimeout` must parse. Human moderator allowed + warned β€” and `resolveParticipants` now preserves the roster's member for the moderator id (the 4-arg ctor silently DEMOTED a human moderator to an agent). +- **Surfaces:** `human_input_requested` event (constant + record + listener default + SSE forward incl. OpenAPI list + Slack "you're up" notice, mrkdwn-escaped); pending human turns join the existing inbox as `pauseType: "HUMAN_TURN"` + `pendingMemberId` (no third inbox) and the member sees their own turns without owning the conversation; `availableActions` gains `submitHumanInput`; MCP `get_group_approval_status` reports the pending member and their rendered prompt; cancel paths (`cancelDiscussion`, pauseβ†’cancel conversion, `removeTokenAndConvertIfSignalled`) all treat the new state as a first-class pause. +- **Defense in depth:** a HUMAN member reaching `executeAgentTurn` (convergence judge, dissent round, task-force wave, nested group β€” contexts that cannot pause) yields a SKIPPED entry, mirroring the member-HITL SKIP precedent. +- **Deviation, recorded:** the I14 `HUMAN_DECIDES` tie-policy wiring stays save-time-rejected β€” I14 (PR #638) is not merged; wiring it is a small follow-up once both branches land (I12 needs both anyway). + +**Tests (+23 across 8 classes; `engine.internal` + `configs.groups` + `engine.hitl` suites 1869 green; checkstyle clean):** sequential pause with rendered prompt + absolute index + budget-before-human ordering; parallel fan-out-then-human with pre-fan-out blindness (captor on the prompt transcript), resume-tail without fan-out re-run, and post-resume blindness; commitHumanTurnPause bookmark/pending/schedule shape (surface + policy asserted); submitβ†’recordβ†’advanceβ†’CASβ†’re-enter (captured runnable proves the re-entry coords); 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); guard matrix (member ok, admin ok, owner+approver FORBIDDEN, wrong-group 404, auth-off no-op, inbox shows the member their turn); save-time matrix; human-moderator preservation; defense-in-depth skip; 4 enum pins updated (the I14/I8 CI lesson β€” caught locally this time). + --- ## πŸ”€ merge: bring `origin/main` (PR #627 HITL request pinning) into the branch (2026-08-07) diff --git a/docs/group-conversations.md b/docs/group-conversations.md index 8f440db3b..9d2052165 100644 --- a/docs/group-conversations.md +++ b/docs/group-conversations.md @@ -160,6 +160,49 @@ Both caps are enforced independently: `maxPerTurn` bounds a runaway single turn, discussion cap counts only agent-filed tasks, so a large planned backlog does not exhaust it. A rejected call does not consume the per-turn budget. +## Humans as group members (I6) + +Real deployments are hybrid teams: a `memberType: "HUMAN"` member sits in the +roster like any agent, but their turn **pauses the discussion** +(`AWAITING_HUMAN_INPUT`) until they answer. + +```json +{ + "members": [ + { "agentId": "agent-1", "displayName": "Analyst", "speakingOrder": 1 }, + { "agentId": "gregor@example.com", "displayName": "Gregor", "speakingOrder": 2, "memberType": "HUMAN" } + ], + "humanMemberConfig": { "turnTimeout": "PT4H", "onTimeout": "SKIP_TURN" } +} +``` + +- The human's `agentId` is their **principal id** β€” the identity that may submit + their turns; `displayName` is required at save time. +- Their prompt is rendered exactly like an agent's and persisted on the + conversation (`pendingHumanInput.renderedPrompt`); the `human_input_requested` + SSE event (and a Slack notice) says who is up. +- Submission: `POST /groups/{groupId}/conversations/{id}/human-input` + `{memberId, content}` or MCP `submit_group_human_input`. **Only the member's + own principal (or an admin) may submit** β€” an `eddi-approver` may decide + approvals, but speaking as another human is impersonation, not review. The + answer is recorded as the phase's natural entry type (a human OPINION is an + OPINION) and the discussion resumes from the next speaker. +- This is deliberately NOT the approval surface: approve/reject endpoints never + accept free text, and the pending-approvals inbox marks these entries + `pauseType: "HUMAN_TURN"` with the member's id, so a human sees their own + pending turns without owning the conversation. +- **Timeouts** (`humanMemberConfig`): `turnTimeout` (ISO-8601; unset = wait + indefinitely) with `onTimeout: SKIP_TURN` (a SKIPPED entry β€” "no response from + within " β€” and the discussion moves on) or `ABORT` (graceful + cancel). Timeout schedules survive restarts via the HITL crash-recovery sweep. +- **PARALLEL phases**: agents fan out first; humans are then prompted one at a + time against the *pre-fan-out* snapshot, so an independent round stays + independent β€” a human answering after the agents cannot read their answers. +- **v1 bounds (save-time rejected)**: no HUMAN members in task-force groups + (PLAN/EXECUTE/VERIFY) or `targetEachPeer` phases, and a group containing + humans cannot be nested as a GROUP member. A human **moderator** is allowed β€” + every synthesis then waits on that person (the save warns about it). + ## Nested Groups (Group-of-Groups) Members can be other groups. The sub-group runs its own discussion and its synthesized answer becomes the member's response. diff --git a/src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java b/src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java index 40da7c9d0..74e3fb569 100644 --- a/src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java +++ b/src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java @@ -91,6 +91,21 @@ public void setTaskListConfig(GroupTaskConfig taskListConfig) { this.taskListConfig = taskListConfig; } + /** + * How HUMAN members' turns are timed out (I6). {@code null} means wait + * indefinitely β€” the same thing a default-constructed {@link HumanMemberConfig} + * means. + */ + private HumanMemberConfig humanMemberConfig; + + public HumanMemberConfig getHumanMemberConfig() { + return humanMemberConfig; + } + + public void setHumanMemberConfig(HumanMemberConfig humanMemberConfig) { + this.humanMemberConfig = humanMemberConfig; + } + /** * Governs agent-filed tasks (I5). The task list is otherwise written only by * the PLAN phase and by config, so work an agent discovers while @@ -145,12 +160,18 @@ public GroupTaskConfig() { } /** - * A member of the group. Members can be individual agents or nested groups. + * A member of the group. Members can be individual agents, nested groups, or + * humans (I6). *

* For {@code MemberType.GROUP} members, the {@code agentId} field contains the * group configuration ID instead. The sub-group runs its own discussion and its * synthesized answer becomes this member's response. *

+ * For {@code MemberType.HUMAN} members, {@code agentId} carries the human's + * principal id (the identity that may submit their turns) and + * {@code displayName} is required at save time β€” a paused discussion must be + * able to say WHO it is waiting on. + *

* The optional {@code role} field controls which phases the member participates * in (e.g. "DEVIL_ADVOCATE", "PRO", "CON"). If null, the member is a default * participant. @@ -164,13 +185,56 @@ public GroupMember(String agentId, String displayName, Integer speakingOrder, St } /** - * Whether a group member is an individual agent or a nested sub-group. + * Whether a group member is an individual agent, a nested sub-group, or a human + * (I6). */ public enum MemberType { /** An individual EDDI agent. */ AGENT, /** A nested group β€” runs its own discussion, returns synthesized answer. */ - GROUP + GROUP, + /** + * A human β€” their turn pauses the discussion ({@code + * AWAITING_HUMAN_INPUT}) until they submit a response or the group's + * {@code humanMemberConfig} timeout policy resolves the turn (I6). + */ + HUMAN + } + + /** + * How the discussion treats HUMAN members' turns (I6). One config for the whole + * group: humans on the same team wait under the same rules. + * + * @param turnTimeout + * ISO-8601 duration a human turn may stay unanswered before + * {@code onTimeout} fires; {@code null} or blank = wait indefinitely + * @param onTimeout + * what an expired turn does β€” defaults to + * {@link OnHumanTimeout#SKIP_TURN} + */ + public record HumanMemberConfig(String turnTimeout, OnHumanTimeout onTimeout) { + + /** Normalization choke point, same shape as {@link GroupTaskConfig}. */ + public HumanMemberConfig { + if (onTimeout == null) { + onTimeout = OnHumanTimeout.SKIP_TURN; + } + } + + /** Wait indefinitely; a timeout would skip the turn if one were set. */ + public HumanMemberConfig() { + this(null, OnHumanTimeout.SKIP_TURN); + } + } + + /** What an expired human turn does (I6). */ + public enum OnHumanTimeout { + /** + * Record a SKIPPED entry ("no response from within ") and move on. + */ + SKIP_TURN, + /** Cancel the discussion. */ + ABORT } // --- Discussion Style --- diff --git a/src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java b/src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java index 7e25568af..d7b6abdd1 100644 --- a/src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java +++ b/src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java @@ -31,7 +31,11 @@ public class GroupConversation { * Wave adds a field resume-time logic depends on, and register that version's * migration in {@code GroupConversationSchemaMigrations}. */ - public static final int CURRENT_SCHEMA_VERSION = 3; + // v4 (this release): adds pausedRepeatSliceBase, which the resumed leg's + // repeat-slice logic depends on. No migration entry needed β€” Jackson defaults + // the field to -1 (no pending slice base) on legacy documents, which is + // exactly the pre-v4 behavior. + public static final int CURRENT_SCHEMA_VERSION = 4; /** * The shape this specific document was last written in. Checked before a * resume: newer than {@link #CURRENT_SCHEMA_VERSION} refuses (this deployment @@ -162,6 +166,24 @@ public void setRoundStartTranscriptIndex(int roundStartTranscriptIndex) { private Set retainedAgentIds = ConcurrentHashMap.newKeySet(); private int pausedAtPhaseIndex = -1; private int pausedTurnCount = 0; + /** + * Transcript index where the PAUSED repeat's entries begin (I6), or {@code -1}. + * A human turn pauses MID-repeat, after other speakers already appended this + * repeat's entries β€” the resumed leg recomputing "size at top of repeat" would + * slice only what came AFTER the pause, so the convergence check (and every + * later consumer of the repeat slice) silently loses the pre-pause + * contributions. Persisted with the pause, consumed exactly once with the + * speaker bookmark. {@code -1} on legacy documents keeps the old recompute. + */ + private int pausedRepeatSliceBase = -1; + + public int getPausedRepeatSliceBase() { + return pausedRepeatSliceBase; + } + + public void setPausedRepeatSliceBase(int pausedRepeatSliceBase) { + this.pausedRepeatSliceBase = pausedRepeatSliceBase; + } private String pausedPhaseName; private Instant pausedAt; private HitlPauseType hitlPauseType; @@ -169,13 +191,18 @@ public void setRoundStartTranscriptIndex(int roundStartTranscriptIndex) { private String hitlPauseReason; /** * Where inside a SEQUENTIAL phase's speaker list a pause landed (Wave 0, F2). - * {@code null} for every pause today β€” {@code PHASE} and {@code TASK} pauses - * (the only kinds that exist) both land at a phase/task boundary, never - * mid-speaker-list. Exists for I6 (human as a group member), which pauses - * between one speaker and the next within a running SEQUENTIAL phase; see - * {@link ResumePoint}'s own Javadoc for why PARALLEL phases never set this. + * {@code null} for {@code PHASE} and {@code TASK} pauses, which land at a + * phase/task boundary, never mid-speaker-list. I6's HUMAN_TURN pauses are the + * producer: they pause ON a specific speaker within a running phase; see + * {@link ResumePoint}'s own Javadoc (and its {@code HUMAN_TURN_PARALLEL} + * carve-out) for the resume semantics. */ private ResumePoint resumePoint; + /** + * The human member's turn an {@code AWAITING_HUMAN_INPUT} pause is waiting on + * (I6). Non-null exactly while the state is AWAITING_HUMAN_INPUT. + */ + private PendingHumanInput pendingHumanInput; /** Timeout policy copied from config at pause time (Phase 6d). */ private HitlTimeoutPolicy hitlTimeoutPolicy; /** @@ -390,13 +417,65 @@ public enum TranscriptEntryType { * pause landed on; on resume, speakers before this index are skipped * rather than re-run * @param pauseKind - * free-text tag for observability (REST/MCP status payloads) β€” not - * consulted by any resume logic, which keys off this record's mere - * presence rather than what kind of mid-phase pause it names + * which kind of mid-phase pause this bookmark records. For most + * kinds it is a pure observability tag (resume logic keys off this + * record's mere presence), with ONE exception: + * {@link #RESUME_KIND_HUMAN_TURN_PARALLEL} tells the resumed leg + * that {@code speakerIdx} indexes the phase's HUMAN-only sublist and + * that the agent fan-out already ran β€” the parallel executor then + * skips straight to the remaining human turns instead of re-running + * the fan-out (the sole carve-out from the "PARALLEL never honors a + * bookmark" rule above; see I6) */ public record ResumePoint(int phaseIdx, int repeatIdx, int speakerIdx, String pauseKind) { } + /** {@link ResumePoint#pauseKind()} of a sequential HUMAN member turn (I6). */ + public static final String RESUME_KIND_HUMAN_TURN = "HUMAN_TURN"; + + /** + * {@link ResumePoint#pauseKind()} of a HUMAN turn in a PARALLEL phase (I6): + * {@code speakerIdx} indexes the phase's human-only sublist, and the resumed + * leg must NOT re-run the agent fan-out. + */ + public static final String RESUME_KIND_HUMAN_TURN_PARALLEL = "HUMAN_TURN_PARALLEL"; + + /** + * The one turn a paused {@code AWAITING_HUMAN_INPUT} discussion is waiting on + * (I6). Persisted with the document so the prompt survives a restart and the + * approval/inbox surfaces can display exactly what the member was asked. + * + * @param memberId + * the HUMAN member's {@code agentId} β€” the human principal id; + * submissions must come from this principal (or an admin) + * @param displayName + * the member's display name, for transcript attribution and UI + * @param phaseIdx + * phase the turn belongs to (matches the {@link ResumePoint}) + * @param repeatIdx + * repeat of that phase + * @param speakerIdx + * the member's index β€” into the phase's resolved speaker list for + * sequential turns, into the human-only sublist for parallel ones + * @param entryType + * {@link TranscriptEntryType} name the submitted content is recorded + * as β€” captured at pause time so a config edit while paused cannot + * re-type the entry (a human OPINION is an OPINION) + * @param renderedPrompt + * the phase input rendered for this member exactly as an agent would + * have received it β€” what the UI shows the human + * @param onTimeout + * the group's {@code humanMemberConfig.onTimeout} policy name at + * pause time (SKIP_TURN or ABORT) β€” bookmarked so crash recovery + * re-arms the same policy the pause promised, config edits + * notwithstanding + * @param requestedAt + * when the turn was requested + */ + public record PendingHumanInput(String memberId, String displayName, int phaseIdx, int repeatIdx, int speakerIdx, + String entryType, String renderedPrompt, String onTimeout, Instant requestedAt) { + } + /** * What kind of conclusion a {@link DecisionRecord} represents (Wave 0, F3). */ @@ -480,6 +559,14 @@ public enum GroupConversationState { CANCELLED, /** Paused for human approval β€” HITL foundation (Phase 9b). */ AWAITING_APPROVAL, + /** + * Paused because a HUMAN group member's turn is up (I6). Deliberately NOT + * {@link #AWAITING_APPROVAL}: approval endpoints must never accept free text, + * and an inbox must be able to tell "approve/reject this" from "you're up". + * Resolved only by {@code submitHumanInput} (or its timeout policy) β€” never by + * the approve/resume surface. + */ + AWAITING_HUMAN_INPUT, /** * Terminal β€” member conversations ended, ephemeral agents cleaned up, no * further follow-ups. @@ -488,7 +575,12 @@ public enum GroupConversationState { } public enum HitlPauseType { - PHASE, TASK + PHASE, TASK, + /** + * A HUMAN group member's turn (I6) β€” see + * {@link GroupConversationState#AWAITING_HUMAN_INPUT}. + */ + HUMAN_TURN } // --- Getters/Setters --- @@ -752,6 +844,9 @@ public List getAvailableActions() { // FAILED and CANCELLED are terminal but closeable β€” close ends member // conversations and reclaims ephemeral agents. case FAILED, CANCELLED -> List.of("close"); + // I6: the one state a human member acts on β€” the UI switches to an + // input prompt instead of approve/reject buttons. + case AWAITING_HUMAN_INPUT -> List.of("submitHumanInput"); case IN_PROGRESS, SYNTHESIZING, CREATED, AWAITING_APPROVAL -> List.of(); case CLOSED -> List.of(); }; @@ -827,6 +922,14 @@ public void setResumePoint(ResumePoint resumePoint) { this.resumePoint = resumePoint; } + public PendingHumanInput getPendingHumanInput() { + return pendingHumanInput; + } + + public void setPendingHumanInput(PendingHumanInput pendingHumanInput) { + this.pendingHumanInput = pendingHumanInput; + } + public AgentGroupConfiguration.ProtocolConfig.CostPolicy getCostCeilingOutcome() { return costCeilingOutcome; } diff --git a/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java b/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java index 04ff7972b..6e082c675 100644 --- a/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java +++ b/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java @@ -14,11 +14,15 @@ import ai.labs.eddi.datastore.IResourceStorageFactory; import ai.labs.eddi.datastore.IResourceStore; import ai.labs.eddi.datastore.serialization.IDocumentBuilder; +import ai.labs.eddi.utils.LogSanitizer; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import org.jboss.logging.Logger; +import java.time.Duration; +import java.util.ArrayList; import java.util.List; +import java.util.Objects; /** * DB-agnostic store for group configurations. Extends @@ -41,6 +45,7 @@ public AgentGroupStore(IResourceStorageFactory storageFactory, IDocumentBuilder public IResourceStore.IResourceId create(AgentGroupConfiguration groupConfiguration) throws IResourceStore.ResourceStoreException { HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); + validateHumanMembers(groupConfiguration); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); return super.create(groupConfiguration); @@ -52,11 +57,145 @@ public Integer update(String id, Integer version, AgentGroupConfiguration groupC throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); + validateHumanMembers(groupConfiguration); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); return super.update(id, version, groupConfiguration); } + /** + * I6 save-time matrix for HUMAN members. Hard rejections + * ({@link IllegalArgumentException}, {@code HitlConfigValidation}'s contract) + * are safe here in a way {@link #warnOnModeratorlessPhases} could not be: no + * pre-existing document can contain {@code MemberType.HUMAN}, so there is no + * legacy config a rejection could strand. + *

    + *
  • a HUMAN member must carry a {@code displayName} β€” a paused discussion + * must be able to say WHO it is waiting on;
    • + *
  • groups with HUMAN members must not run task-force phases + * (PLAN/EXECUTE/VERIFY assign work on agent-latency math and pause inside wave + * workers) nor {@code targetEachPeer} phases (a human on both axes of an + * NΓ—(N-1) round would owe up to 2(N-1) pauses per repeat, and the flat speaker + * bookmark has no (speaker,target) coordinate) β€” preset-expanded like + * {@link #moderatorlessPhaseNames}, or the check is inert for preset-style + * groups;
    • + *
  • a group containing HUMAN members may not be USED as a nested GROUP member + * (one level deep here; {@code MemberTurnExecutor} carries the runtime backstop + * for configs edited afterwards);
    • + *
  • {@code humanMemberConfig.turnTimeout} must parse as an ISO-8601 + * duration;
    • + *
  • a HUMAN moderator is allowed but warned about β€” every synthesis then + * waits on a person.
    • + *
+ */ + void validateHumanMembers(AgentGroupConfiguration config) throws IResourceStore.ResourceStoreException { + List problems = humanMemberProblems(config); + if (!problems.isEmpty()) { + throw new IllegalArgumentException(String.join("; ", problems)); + } + // "members": null in the JSON reaches here as a literal null list β€” the + // pure helper already tolerates it; there is nothing human to validate. + List members = config.getMembers() != null ? config.getMembers() : List.of(); + // Nested check needs the store β€” kept out of the pure helper. + for (var member : members) { + if (member != null && member.memberType() == AgentGroupConfiguration.MemberType.GROUP) { + AgentGroupConfiguration child = readChildConfig(member.agentId()); + if (child != null && hasHumanMembers(child)) { + throw new IllegalArgumentException( + "members['" + member.agentId() + "'] is a nested group that contains HUMAN members β€” " + + "human turns cannot pause a nested discussion (I6 v1); remove the human from the " + + "child group or flatten the hierarchy"); + } + } + } + String moderator = config.getModeratorAgentId(); + if (moderator != null && members.stream() + .anyMatch(m -> m != null && m.memberType() == AgentGroupConfiguration.MemberType.HUMAN + && moderator.equals(m.agentId()))) { + LOGGER.warnf("Group '%s' names HUMAN member '%s' as moderator β€” every synthesis phase will pause and wait " + + "for their input", LogSanitizer.sanitize(config.getName()), LogSanitizer.sanitize(moderator)); + } + } + + /** + * The pure, assertable part of the I6 matrix (same split as + * {@link #moderatorlessPhaseNames}): every problem with this config's HUMAN + * members that needs no store access. Empty list = valid. + */ + static List humanMemberProblems(AgentGroupConfiguration config) { + List humans = config.getMembers() == null + ? List.of() + : config.getMembers().stream() + .filter(m -> m != null && m.memberType() == AgentGroupConfiguration.MemberType.HUMAN) + .toList(); + List problems = new ArrayList<>(); + for (var human : humans) { + if (human.displayName() == null || human.displayName().isBlank()) { + problems.add("HUMAN member '" + human.agentId() + "' needs a displayName"); + } + if (human.agentId() == null || human.agentId().isBlank()) { + problems.add("a HUMAN member needs an agentId carrying the human's principal id"); + } + } + if (!humans.isEmpty()) { + // Preset-expanded, or the check is inert for preset-style groups. + List phases = config.getPhases(); + if (phases == null || phases.isEmpty()) { + DiscussionStyle style = config.getStyle() != null ? config.getStyle() : DiscussionStyle.ROUND_TABLE; + phases = DiscussionStylePresets.expand(style, config.getMaxRounds()); + } + boolean taskPhases = phases.stream().filter(Objects::nonNull).anyMatch( + p -> p.type() == AgentGroupConfiguration.PhaseType.PLAN + || p.type() == AgentGroupConfiguration.PhaseType.EXECUTE + || p.type() == AgentGroupConfiguration.PhaseType.VERIFY); + if (taskPhases) { + problems.add("HUMAN members cannot join task-force groups (PLAN/EXECUTE/VERIFY phases) β€” " + + "task waves assign and time work on agent latencies (I6 v1)"); + } + boolean peerPhases = phases.stream().filter(Objects::nonNull).anyMatch(DiscussionPhase::targetEachPeer); + if (peerPhases) { + problems.add("HUMAN members cannot join groups with targetEachPeer phases β€” a human would owe one " + + "authored critique per peer AND be a target, multiplying pauses (I6 v1)"); + } + } + var humanConfig = config.getHumanMemberConfig(); + if (humanConfig != null && humanConfig.turnTimeout() != null && !humanConfig.turnTimeout().isBlank()) { + try { + Duration parsed = Duration.parse(humanConfig.turnTimeout()); + // Duration.parse accepts PT0S and PT-4H; both would arm a timeout + // that fires effectively immediately (past-due clamps to the grace + // window), silently skipping every human turn. + if (parsed.isZero() || parsed.isNegative()) { + problems.add("humanMemberConfig.turnTimeout must be a positive duration, not '" + + humanConfig.turnTimeout() + "'"); + } + } catch (Exception e) { + problems.add("humanMemberConfig.turnTimeout must be an ISO-8601 duration (e.g. PT4H), not '" + + humanConfig.turnTimeout() + "'"); + } + } + return problems; + } + + /** True if the config lists at least one HUMAN member. */ + static boolean hasHumanMembers(AgentGroupConfiguration config) { + return config.getMembers() != null && config.getMembers().stream() + .anyMatch(m -> m != null && m.memberType() == AgentGroupConfiguration.MemberType.HUMAN); + } + + /** 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; + } + } + /** * I3: a phase restricted to {@code participants: "MODERATOR"} in a group that * names no {@code moderatorAgentId} cannot run as written. The engine picks the diff --git a/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java b/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java index a5a4b77ce..c804f3e7d 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java @@ -158,6 +158,44 @@ GroupConversation resumeDiscussion(String groupConversationId, throws GroupDiscussionException, IResourceStore.ResourceStoreException, IResourceStore.ResourceNotFoundException, IResourceStore.ResourceModifiedException; + /** + * Submits a HUMAN group member's response for the turn an + * {@code AWAITING_HUMAN_INPUT} discussion is waiting on (I6), records it as the + * phase's natural transcript entry, and resumes the discussion from the next + * speaker. + * + * @param groupConversationId + * the paused discussion + * @param memberId + * the human member the submission is for β€” must match the pending + * turn's member (authorization is the caller's concern; the service + * verifies the MATCH, the REST/MCP guard verifies the caller may act + * as this member) + * @param content + * the member's response β€” becomes the transcript entry's content + * @param submittedBy + * attribution for the audit trail (server-derived, never + * caller-supplied) + * @return the conversation, resumed (IN_PROGRESS) β€” the discussion continues on + * a background thread, exactly like an approval resume + * @throws IllegalArgumentException + * blank content, unknown member, or a memberId that does not match + * the pending turn β€” maps to HTTP 400 + * @throws GroupDiscussionException + * if the conversation is not awaiting human input β€” maps to a + * conflict, not a validation error + */ + GroupConversation submitHumanInput(String groupConversationId, String memberId, String content, String submittedBy) + throws GroupDiscussionException, IResourceStore.ResourceStoreException, + IResourceStore.ResourceNotFoundException, IResourceStore.ResourceModifiedException; + + /** + * SKIP_TURN resolution of an expired human turn (I6): records a SKIPPED entry + * for the pending member and resumes the discussion. Called by the HITL timeout + * scheduler; never throws β€” an already-resolved turn is a no-op. + */ + void skipHumanTurnOnTimeout(String groupConversationId); + /** * List group conversations currently awaiting human approval, as bounded * summaries (no transcripts). Used by dashboards and admin UIs. @@ -210,6 +248,8 @@ default void onConvergenceChecked(GroupConversationEventSink.ConvergenceCheckedE } default void onConvergenceReached(GroupConversationEventSink.ConvergenceReachedEvent event) { } + default void onHumanInputRequested(GroupConversationEventSink.HumanInputRequestedEvent event) { + } } // --- Exceptions --- diff --git a/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java b/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java index ac078b93b..cc56997b4 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java @@ -184,7 +184,8 @@ Response continueDiscussion(@PathParam("groupId") String groupId, + "Emits round_start (new round marker) followed by the same events as the " + "initial stream (phase_start, speaker_start, speaker_complete, phase_complete, " + "synthesis_start, group_complete, group_error), plus the HITL events " - + "(awaiting_approval, hitl_resume, cancelled, member_pause_skipped). NOTE: " + + "(awaiting_approval, hitl_resume, cancelled, member_pause_skipped, " + + "human_input_requested). NOTE: " + "'attachments' are NOT supported on a continuation and are rejected with a " + "terminal group_error event rather than silently ignored.") @APIResponse(responseCode = "200", description = "SSE event stream of continuation progress.") @@ -247,6 +248,34 @@ void approveGroupPhaseStreaming(@PathParam("groupId") String groupId, @Context SseEventSink eventSink, @Context Sse sse); + @POST + @Path("/{groupId}/conversations/{groupConversationId}/human-input") + @Consumes(MediaType.APPLICATION_JSON) + @Produces(MediaType.APPLICATION_JSON) + @RolesAllowed({"eddi-admin", "eddi-editor", "eddi-user", "eddi-approver"}) + @Operation(summary = "Submit a human group member's turn (I6)", + description = "Records the pending HUMAN member's response as their transcript entry and resumes an " + + "AWAITING_HUMAN_INPUT discussion from the next speaker. Only the pending member's own " + + "principal (or an admin) may submit β€” this is a member SPEAKING, not an approval; the " + + "approve endpoint never accepts free text and this endpoint never decides approvals.") + @APIResponse(responseCode = "200", description = "Input recorded; discussion resumed.") + @APIResponse(responseCode = "400", description = "Blank content, unknown member, or memberId not matching the pending turn.") + @APIResponse(responseCode = "403", description = "Caller is neither the pending member nor an admin.") + @APIResponse(responseCode = "404", description = "Group conversation not found.") + @APIResponse(responseCode = "409", description = "Not awaiting human input / concurrent modification conflict.") + Response submitHumanInput(@PathParam("groupId") String groupId, + @PathParam("groupConversationId") String gcId, + 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 = "'content' must not be blank") String content) { + } + @GET @Path("/{groupId}/conversations/{groupConversationId}/approval-status") @Produces(MediaType.APPLICATION_JSON) diff --git a/src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java b/src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java index 0ed36d05b..6bceaf463 100644 --- a/src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java +++ b/src/main/java/ai/labs/eddi/engine/hitl/HitlAccessGuard.java @@ -139,8 +139,10 @@ public void requireGroupConversationHitlAccess(String groupId, String groupConve /** * Owner-scoped pending-approval inbox for group conversations: admins and - * approvers see all; other callers see only their own (post-filtered by owner, - * since the group store has no owner-scoped query); anonymous callers see + * approvers see all; other callers see their own β€” or, for I6 human-turn + * pauses, entries waiting on THEM as the pending member, since a human member + * usually does not own the conversation whose turn they owe. Post-filtered, + * since the group store has no owner-scoped query; anonymous callers see * nothing (fail-closed). {@code groupId == null} lists across all groups * (cross-group inbox). */ @@ -154,6 +156,83 @@ public List listScopedGroupPendingApprovals(String group if (callerId == null || callerId.isBlank()) { return List.of(); } - return scoped.filter(summary -> callerId.equals(summary.getUserId())).toList(); + return scoped.filter(summary -> callerId.equals(summary.getUserId()) + || callerId.equals(summary.getPendingMemberId())).toList(); + } + + /** + * Read access to a group conversation's approval/pause STATUS (I6): the + * owner/admin/approver rule, plus the one extra reader the strict guard cannot + * admit β€” the HUMAN member a pending turn is waiting on. The inbox tells that + * member they are up; without this, the status API (where their rendered prompt + * lives) would refuse to show them what they were asked. Read-only surfaces + * only β€” approve/cancel/submit keep their own guards. + * + * @throws ForbiddenException + * if the caller may not read this group conversation's status. + */ + public void requireGroupConversationReadAccess(String groupId, String groupConversationId) { + GroupConversation gc; + try { + gc = groupConversationService.readGroupConversation(groupConversationId); + } catch (ResourceNotFoundException e) { + LOGGER.debugf("Group conversation not found for read-access check: %s", sanitize(groupConversationId)); + return; // the actual operation 404s + } catch (Exception e) { + throw new ForbiddenException("Access denied: unable to verify group conversation"); + } + if (groupId != null && !groupId.equals(gc.getGroupId())) { + LOGGER.infof("Group conversation %s does not belong to group %s", + sanitize(groupConversationId), sanitize(groupId)); + throw new jakarta.ws.rs.NotFoundException("Group conversation not found."); + } + String callerId = identity != null && identity.getPrincipal() != null ? identity.getPrincipal().getName() : null; + if (gc.getPendingHumanInput() != null && callerId != null + && callerId.equals(gc.getPendingHumanInput().memberId())) { + return; + } + ownershipValidator.requireOwnerAdminOrApprover(identity, gc.getUserId(), "group conversation"); + } + + /** + * Authorization for submitting a HUMAN member's turn (I6): the caller must BE + * that member (their principal equals the pending turn's member id) or an + * admin. Deliberately narrower than the approve surface β€” an + * {@code eddi-approver} may decide approvals they do not own, but SPEAKING as + * another human is impersonation, not review; only the admin break-glass + * crosses that line. A wrong-group path 404s without leaking existence, same as + * {@link #requireGroupConversationHitlAccess}. No-op when authorization is + * disabled. + * + * @throws ForbiddenException + * if the caller may not submit for this member. + */ + public void requireGroupHumanInputAccess(String groupId, String groupConversationId, String memberId) { + if (!ownershipValidator.isAuthEnabled()) { + return; + } + GroupConversation gc; + try { + gc = groupConversationService.readGroupConversation(groupConversationId); + } catch (ResourceNotFoundException e) { + LOGGER.debugf("Group conversation not found for human-input access check: %s", sanitize(groupConversationId)); + return; // the actual operation 404s + } catch (Exception e) { + throw new ForbiddenException("Access denied: unable to verify group conversation"); + } + if (groupId != null && !groupId.equals(gc.getGroupId())) { + LOGGER.infof("Group conversation %s does not belong to group %s", + sanitize(groupConversationId), sanitize(groupId)); + throw new jakarta.ws.rs.NotFoundException("Group conversation not found."); + } + if (ownershipValidator.isAdmin(identity)) { + return; + } + String callerId = identity != null && identity.getPrincipal() != null ? identity.getPrincipal().getName() : null; + if (callerId == null || callerId.isBlank() || memberId == null || !callerId.equals(memberId)) { + LOGGER.warnf("Human-input submission for member '%s' of %s denied for caller '%s'", + sanitize(memberId), sanitize(groupConversationId), sanitize(callerId)); + throw new ForbiddenException("Access denied: only the pending member (or an admin) may submit this turn"); + } } } diff --git a/src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java b/src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java index 42db2b956..c582d05a6 100644 --- a/src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java +++ b/src/main/java/ai/labs/eddi/engine/hitl/HitlCrashRecoveryObserver.java @@ -135,7 +135,9 @@ void runRecovery() { LOGGER.info("HITL crash recovery running..."); int rearmedRegular = repairRegularPaused(); int recoveredInProgress = recoverRegularInProgress(); - int rearmedGroup = repairGroupPaused(); + // Two independent sweeps: a failing AWAITING_APPROVAL query must not + // silently skip the AWAITING_HUMAN_INPUT re-arm (I6), or vice versa. + int rearmedGroup = repairGroupPaused() + repairGroupHumanPaused(); if (rearmedRegular > 0 || recoveredInProgress > 0 || rearmedGroup > 0) { LOGGER.warnf("HITL crash recovery: re-armed %d regular + %d group timeout schedule(s), " @@ -384,6 +386,59 @@ private int repairGroupPaused() { } } + /** + * I6: re-arms the turn timeout of discussions waiting on a HUMAN member. The + * SKIP_TURN/ABORT policy is bookmarked on the pending record (it is not a + * {@code HitlTimeoutPolicy}), the duration on the shared bookmark field, and + * the schedule's surface routes the fire to the human-turn handler. + */ + private int repairGroupHumanPaused() { + try { + List pausedGcs = groupConversationStore.findByState(GroupConversationState.AWAITING_HUMAN_INPUT); + int count = 0; + for (GroupConversation gc : pausedGcs) { + try { + var pending = gc.getPendingHumanInput(); + if (pending == null || gc.getHitlApprovalTimeout() == null || gc.getHitlApprovalTimeout().isBlank()) { + continue; // wait-indefinitely turn, or a half-written pause β€” nothing to arm + } + String groupConversationId = gc.getId(); + Instant scannedPausedAt = gc.getPausedAt(); + if (rearmSchedule(HitlSchedules.groupTimeoutScheduleName(groupConversationId), + HitlSchedules.SURFACE_GROUP_HUMAN, groupConversationId, + null, pending.onTimeout() != null ? pending.onTimeout() : "SKIP_TURN", + gc.getHitlApprovalTimeout(), scannedPausedAt, + () -> groupStillHumanPaused(groupConversationId, scannedPausedAt))) { + count++; + } + } catch (Exception e) { + LOGGER.warnf("Failed to repair human-paused group conversation %s: %s", gc.getId(), e.getMessage()); + } + } + return count; + } catch (Exception e) { + LOGGER.warnf("Error during group human-pause repair: %s", e.getMessage()); + return 0; + } + } + + /** {@code groupStillPaused}'s I6 twin β€” same pause-identity re-check. */ + private boolean groupStillHumanPaused(String groupConversationId, Instant scannedPausedAt) { + try { + var current = groupConversationStore.read(groupConversationId); + if (current == null || current.getState() != GroupConversationState.AWAITING_HUMAN_INPUT) { + return false; + } + if (scannedPausedAt == null) { + return true; + } + return scannedPausedAt.equals(current.getPausedAt()); + } catch (Exception e) { + LOGGER.debugf("Human-pause re-check failed for %s: %s", groupConversationId, e.getMessage()); + return true; // keep the schedule; the handler no-ops on a non-paused state + } + } + /** * Idempotently replaces the one-shot HITL timeout schedule: deletes any * existing schedule of that name and creates a fresh one at the original due @@ -399,6 +454,18 @@ private int repairGroupPaused() { private boolean rearmSchedule(String scheduleName, String surface, String conversationId, String agentId, HitlTimeoutPolicy policy, String approvalTimeout, Instant pausedAt, java.util.function.BooleanSupplier stillPaused) { + return rearmSchedule(scheduleName, surface, conversationId, agentId, policy.name(), approvalTimeout, + pausedAt, stillPaused); + } + + /** + * String-policy variant (I6): human-turn schedules carry {@code OnHumanTimeout} + * names (SKIP_TURN/ABORT), which are not {@code HitlTimeoutPolicy} values β€” the + * fire handler branches on the surface before parsing. + */ + private boolean rearmSchedule(String scheduleName, String surface, String conversationId, + String agentId, String policyName, String approvalTimeout, + Instant pausedAt, java.util.function.BooleanSupplier stillPaused) { if (approvalTimeout == null || approvalTimeout.isBlank() || pausedAt == null) { LOGGER.warnf("Cannot re-arm HITL timeout for %s: missing approvalTimeout/pausedAt in bookmark", conversationId); @@ -431,7 +498,7 @@ private boolean rearmSchedule(String scheduleName, String surface, String conver schedule.setCreatedAt(Instant.now()); schedule.setMetadata(Map.of( HitlSchedules.METADATA_TYPE_KEY, HitlSchedules.METADATA_TYPE_TIMEOUT, - HitlSchedules.METADATA_POLICY_KEY, policy.name(), + HitlSchedules.METADATA_POLICY_KEY, policyName, HitlSchedules.METADATA_SURFACE_KEY, surface, HitlSchedules.METADATA_CONVERSATION_ID_KEY, conversationId)); scheduleStore.createSchedule(schedule); @@ -453,7 +520,7 @@ private boolean rearmSchedule(String scheduleName, String surface, String conver } LOGGER.infof("Re-armed HITL timeout for %s (%s) at %s (policy: %s)", - conversationId, surface, fireAt, policy); + conversationId, surface, fireAt, policyName); return true; } catch (Exception e) { LOGGER.warnf("Failed to re-arm HITL timeout for %s: %s", conversationId, e.getMessage()); diff --git a/src/main/java/ai/labs/eddi/engine/hitl/HitlSchedules.java b/src/main/java/ai/labs/eddi/engine/hitl/HitlSchedules.java index 9a3ae30b3..fe39da843 100644 --- a/src/main/java/ai/labs/eddi/engine/hitl/HitlSchedules.java +++ b/src/main/java/ai/labs/eddi/engine/hitl/HitlSchedules.java @@ -32,6 +32,13 @@ private HitlSchedules() { public static final String SURFACE_REGULAR = "regular"; public static final String SURFACE_GROUP = "group"; + /** + * A HUMAN group member's turn timeout (I6). Its {@link #METADATA_POLICY_KEY} + * carries {@code OnHumanTimeout} names (SKIP_TURN/ABORT), NOT a + * {@code HitlTimeoutPolicy} β€” the fire handler must branch on this surface + * BEFORE parsing the policy. + */ + public static final String SURFACE_GROUP_HUMAN = "group-human"; private static final String NAME_PREFIX_REGULAR = "hitl-timeout-"; private static final String NAME_PREFIX_GROUP = "hitl-timeout-group-"; diff --git a/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java b/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java index a999ec46e..bbd7a93c6 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java @@ -677,7 +677,19 @@ public GroupConversation executeDiscussion(GroupConversation gc, AgentGroupConfi // comparison across two different phases would be meaningless. List previousRepeatEntries = null; - for (int repeat = 0; repeat < Math.max(phase.repeats(), 1); repeat++) { + // I6: a mid-phase bookmark names the repeat the pause landed on β€” + // start THERE instead of replaying every earlier repeat (each + // replayed repeat is a full round of duplicate turns and spend). + // Peeked, not consumed: the loop's own read-and-clear below still + // owns the speaker offset. Clamped so a bookmark from a config + // whose repeats shrank cannot skip the phase entirely. + int startRepeat = 0; + GroupConversation.ResumePoint repeatBookmark = gc.getResumePoint(); + if (repeatBookmark != null && repeatBookmark.phaseIdx() == phaseIdx) { + startRepeat = Math.min(Math.max(repeatBookmark.repeatIdx(), 0), Math.max(phase.repeats(), 1) - 1); + } + + for (int repeat = startRepeat; repeat < Math.max(phase.repeats(), 1); repeat++) { // --- maxTurns safety cap --- if (turnCounter.get() >= maxTurns) { @@ -718,34 +730,72 @@ public GroupConversation executeDiscussion(GroupConversation gc, AgentGroupConfi // what guarantees this matches on the very first (phaseIdx, repeat) // this leg visits, before executeDiscussion is ever called. int startSpeakerIdx = 0; + Integer parallelHumanResumeIdx = null; GroupConversation.ResumePoint resumePoint = gc.getResumePoint(); if (resumePoint != null) { gc.setResumePoint(null); if (resumePoint.phaseIdx() == phaseIdx && resumePoint.repeatIdx() == repeat) { - startSpeakerIdx = resumePoint.speakerIdx(); + // I6: a parallel human bookmark indexes the phase's + // human-only sublist and means the agent fan-out already + // ran β€” routed to executeParallelPhase below instead of + // the sequential offset. + if (GroupConversation.RESUME_KIND_HUMAN_TURN_PARALLEL.equals(resumePoint.pauseKind())) { + parallelHumanResumeIdx = resumePoint.speakerIdx(); + } else { + startSpeakerIdx = resumePoint.speakerIdx(); + } } } // I2: mark where this repeat's entries begin. TranscriptEntry // carries phaseIndex but no repeat index, so with repeats > 1 the // only way to say "what this repeat produced" is by position. + // I6: a human-turn pause landed MID-repeat β€” the persisted base + // (taken when the pause committed) wins over a recompute that + // would only see post-pause entries. Read-and-clear with the same + // one-shot discipline as the speaker bookmark above. int transcriptSizeBeforeRepeat = gc.getTranscript().size(); + if (gc.getPausedRepeatSliceBase() >= 0) { + transcriptSizeBeforeRepeat = Math.min(gc.getPausedRepeatSliceBase(), transcriptSizeBeforeRepeat); + gc.setPausedRepeatSliceBase(-1); + } // --- Task-oriented phase routing --- - if (phase.type() == PhaseType.PLAN || phase.type() == PhaseType.EXECUTE || phase.type() == PhaseType.VERIFY) { - executeTaskPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns); - } else if (phase.targetEachPeer()) { - phaseExecutionEngine.executePeerTargetedPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, - turnCounter, - maxTurns); - } else if (phase.turnOrder() == TurnOrder.PARALLEL) { - // F2: PARALLEL never honors a speaker offset β€” see - // GroupConversation.ResumePoint's Javadoc for why a parallel - // resume always re-runs its whole fan-out instead. - executeParallelPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns); - } else { - phaseExecutionEngine.executeSequentialPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, - maxTurns, startSpeakerIdx); + // I6: the dispatch is wrapped so a HUMAN member's turn β€” surfaced + // by the executors as a HumanTurnRequired signal β€” commits an + // AWAITING_HUMAN_INPUT pause and ends this leg, exactly like the + // commitPause call sites below end theirs. + try { + if (phase.type() == PhaseType.PLAN || phase.type() == PhaseType.EXECUTE || phase.type() == PhaseType.VERIFY) { + executeTaskPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns); + } else if (phase.targetEachPeer()) { + phaseExecutionEngine.executePeerTargetedPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, + turnCounter, + maxTurns); + } else if (phase.turnOrder() == TurnOrder.PARALLEL) { + // F2: PARALLEL never honors a speaker offset β€” with I6's + // one carve-out: a HUMAN_TURN_PARALLEL bookmark resumes + // the phase's human tail instead of re-running the + // fan-out; see GroupConversation.ResumePoint's Javadoc. + executeParallelPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns, + parallelHumanResumeIdx); + } else { + phaseExecutionEngine.executeSequentialPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, + turnCounter, + maxTurns, startSpeakerIdx); + } + } catch (PhaseExecutionEngine.HumanTurnRequired humanTurn) { + // I6 slice-base fix: the pause lands MID-repeat, after other + // speakers appended this repeat's entries. Persist where the + // repeat began so the resumed leg's convergence slice (and + // every later consumer of repeatEntries) still covers the + // pre-pause contributions instead of only what follows. + gc.setPausedRepeatSliceBase(transcriptSizeBeforeRepeat); + hitlCoordinator.commitHumanTurnPause(gc, phaseIdx, phase, repeat, humanTurn, + turnCounter.get() + 1, contextBuilder.mapPhaseToEntryType(phase.type()).name(), + listener, config); + convertPauseToCancelIfSignalled(gc, listener); + return gc; } // I1: a phase executor hit the cost ceiling and stopped scheduling @@ -996,8 +1046,9 @@ public GroupConversation executeDiscussion(GroupConversation gc, AgentGroupConfi } } - // Don't overwrite AWAITING_APPROVAL with COMPLETED - if (gc.getState() == GroupConversationState.AWAITING_APPROVAL) { + // Don't overwrite AWAITING_APPROVAL (or a human-turn pause) with COMPLETED + if (gc.getState() == GroupConversationState.AWAITING_APPROVAL + || gc.getState() == GroupConversationState.AWAITING_HUMAN_INPUT) { return gc; } // #27/#45: complete with a CAS on the running state this leg believes it @@ -1108,9 +1159,11 @@ public GroupConversation executeDiscussion(GroupConversation gc, AgentGroupConfi // the no-op signal branch. Resume re-registers a fresh token. Re-check the // removed token so a cancel that raced this remove is not silently dropped. removeTokenAndConvertIfSignalled(gc, listener); - // Drop the incremental verification cursor once this leg ends, but keep it - // across an HITL pause so a resume continues from where it left off. - if (gc.getState() != GroupConversationState.AWAITING_APPROVAL) { + // Drop the incremental verification cursor once this leg ends, but keep + // it across ANY pause (approval or a human turn, I6) so a resume + // continues from where it left off. + if (gc.getState() != GroupConversationState.AWAITING_APPROVAL + && gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT) { signingGuard.forgetConversation(gc.getId()); } // Defer ephemeral cleanup to closeGroupConversation()/deleteGroupConversation() @@ -1366,6 +1419,17 @@ public List resolveParticipants(DiscussionPhase phase, List moderatorAgentId.equals(m.agentId())).findFirst().orElse(null) + : null; + if (rosterModerator != null) { + return List.of(new GroupMember(rosterModerator.agentId(), rosterModerator.displayName(), 0, "MODERATOR", + rosterModerator.memberType())); + } return List.of(new GroupMember(moderatorAgentId, "Moderator", 0, "MODERATOR")); } @@ -1610,6 +1674,17 @@ private void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration phaseExecutionEngine.executeParallelPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns); } + /** + * I6 overload: {@code humanResumeIdx} resumes a parallel phase's human tail. + */ + private void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration config, List speakers, DiscussionPhase phase, + ProtocolConfig protocol, String question, int phaseIdx, GroupDiscussionEventListener listener, + java.util.concurrent.atomic.AtomicInteger turnCounter, int maxTurns, Integer humanResumeIdx) + throws GroupDiscussionException { + phaseExecutionEngine.executeParallelPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns, + humanResumeIdx); + } + // ================================================================= // Agent turn execution // ================================================================= @@ -1785,6 +1860,19 @@ public GroupConversation resumeDiscussion(String groupConversationId, GroupAppro return hitlCoordinator.resumeDiscussion(groupConversationId, request, listener); } + @Override + public GroupConversation submitHumanInput(String groupConversationId, String memberId, String content, String submittedBy) + throws GroupDiscussionException, IResourceStore.ResourceStoreException, + IResourceStore.ResourceNotFoundException, IResourceStore.ResourceModifiedException { + rejectIfShuttingDown(); + return hitlCoordinator.submitHumanInput(groupConversationId, memberId, content, submittedBy, null); + } + + @Override + public void skipHumanTurnOnTimeout(String groupConversationId) { + hitlCoordinator.skipHumanTurnOnTimeout(groupConversationId); + } + private void restoreGroupPause(GroupConversation gc, int phaseIndex, String phaseName, GroupConversation.HitlPauseType pauseType, Instant pausedAt, AgentGroupConfiguration configOrNull, diff --git a/src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java b/src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java index 99abdee3b..84f634365 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java +++ b/src/main/java/ai/labs/eddi/engine/internal/HitlTimeoutHandler.java @@ -44,6 +44,12 @@ public void handleTimeout(Map metadata) { LOGGER.error("HITL timeout metadata missing 'policy' key"); return; } + // I6: human-turn timeouts carry OnHumanTimeout policies (SKIP_TURN/ABORT), + // not HitlTimeoutPolicy values β€” branch on the surface BEFORE the parse. + if (HitlSchedules.SURFACE_GROUP_HUMAN.equals(surface)) { + handleHumanTurnTimeout(metadata, policyStr); + return; + } HitlTimeoutPolicy policy; try { policy = HitlTimeoutPolicy.valueOf(policyStr); @@ -80,6 +86,30 @@ public void handleTimeout(Map metadata) { } } + /** + * I6: an expired human turn resolves per the group's {@code humanMemberConfig} + * β€” SKIP_TURN records a SKIPPED entry and moves on; ABORT cancels the + * discussion (the same graceful cancel the approval ABORT policy uses). + */ + private void handleHumanTurnTimeout(Map metadata, String policyStr) { + String gcId = (String) metadata.get(HitlSchedules.METADATA_CONVERSATION_ID_KEY); + try { + if ("ABORT".equals(policyStr)) { + boolean cancelled = groupConversationService.cancelDiscussion(gcId, + ai.labs.eddi.engine.lifecycle.model.ControlSignal.CANCEL_GRACEFUL); + LOGGER.infof("Human-turn timeout ABORT for group conversation %s%s", gcId, + cancelled ? "" : " skipped β€” already terminal"); + return; + } + if (!"SKIP_TURN".equals(policyStr)) { + LOGGER.errorf("Unknown human-turn timeout policy '%s' for %s β€” treating as SKIP_TURN", policyStr, gcId); + } + groupConversationService.skipHumanTurnOnTimeout(gcId); + } catch (Exception e) { + LOGGER.errorf(e, "Failed to resolve timed-out human turn for group conversation %s", gcId); + } + } + private void resumeRegular(Map metadata, HitlDecision decision) { String conversationId = (String) metadata.get(HitlSchedules.METADATA_CONVERSATION_ID_KEY); try { diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java index fb54f43b6..368c3221c 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java @@ -461,6 +461,51 @@ public Response approveGroupPhase(String groupId, String gcId, GroupApprovalRequ } } + @Override + public Response submitHumanInput(String groupId, String gcId, HumanInputRequest request) { + if (request == null || request.memberId() == null || request.memberId().isBlank() + || request.content() == null || request.content().isBlank()) { + return Response.status(Response.Status.BAD_REQUEST).type(TEXT_PLAIN) + .entity("Request body must include a non-blank 'memberId' and 'content'").build(); + } + // I6: NOT the approve guard β€” speaking as a member is impersonation unless + // the caller IS that member (or an admin); an eddi-approver may decide + // approvals but not talk for people. + hitlAccessGuard.requireGroupHumanInputAccess(groupId, gcId, request.memberId()); + String submittedBy = identity != null && identity.getPrincipal() != null + ? identity.getPrincipal().getName() + : "anonymous"; + try { + var gc = groupConversationService.submitHumanInput(gcId, request.memberId(), request.content(), submittedBy); + return Response.ok(gc).build(); + } catch (IResourceStore.ResourceModifiedException e) { + // Double-submit or a concurrent cancel won the CAS. + return Response.status(Response.Status.CONFLICT).type(TEXT_PLAIN) + .entity("The group conversation was modified concurrently β€” reload and retry.").build(); + } catch (IResourceStore.ResourceNotFoundException + | ai.labs.eddi.configs.groups.IGroupConversationStore.GroupConversationGoneException e) { + LOGGER.infof("Human input for group conversation %s β†’ not found: %s", sanitize(gcId), e.getMessage()); + return Response.status(Response.Status.NOT_FOUND).type(TEXT_PLAIN) + .entity("Group conversation not found.").build(); + } catch (IGroupConversationService.GroupDiscussionException e) { + LOGGER.infof("Human input for group conversation %s rejected (wrong state): %s", sanitize(gcId), e.getMessage()); + return Response.status(Response.Status.CONFLICT).type(TEXT_PLAIN) + .entity("Group conversation is not awaiting human input β€” the turn may have been resolved, " + + "timed out, or cancelled.") + .build(); + } catch (IllegalArgumentException e) { + LOGGER.infof("Human input for group conversation %s rejected (invalid request): %s", sanitize(gcId), e.getMessage()); + return Response.status(Response.Status.BAD_REQUEST).type(TEXT_PLAIN) + .entity("Invalid submission: the memberId does not match the pending turn, or the content is " + + "blank or too long.") + .build(); + } catch (Exception e) { + LOGGER.error("Failed to submit human input for group conversation " + sanitize(gcId), e); + return Response.status(Response.Status.INTERNAL_SERVER_ERROR).type(TEXT_PLAIN) + .entity("Failed to submit human input.").build(); + } + } + /** * A continuation round cannot share NEW files: attachments are granted and * injected to a member only on its first-ever turn, and on a continuation every @@ -676,16 +721,28 @@ private void sendErrorEvent(SseEventSink eventSink, Sse sse, String message) { @Override public Response getGroupApprovalStatus(String groupId, String gcId, String detail) { - validateGroupConversationOwnership(groupId, gcId, true); + // I6: the READ guard, not the strict HITL guard β€” the pending HUMAN + // member must be able to see the status of the turn they owe (their + // rendered prompt lives in the summary below). + hitlAccessGuard.requireGroupConversationReadAccess(groupId, gcId); try { var gc = groupConversationService.readGroupConversation(gcId); - boolean paused = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL; + boolean paused = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL + || gc.getState() == GroupConversation.GroupConversationState.AWAITING_HUMAN_INPUT; + // The approver full-view window is the APPROVAL pause only β€” `paused` + // also covers AWAITING_HUMAN_INPUT, where nothing awaits an approver's + // decision and the transcript would leak outside their remit (review + // finding). Summary fields keep the wider predicate. + boolean awaitingApproval = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL; if ("full".equals(detail)) { // Approver-only callers (not owner, not admin) may read the full // conversation (incl. transcript) only while it is actually awaiting - // approval β€” mirrors the regular surface's read-scope gate. - if (!paused && !ownershipValidator.isAdmin(identity) - && !ownershipValidator.isOwner(identity, gc.getUserId())) { + // approval β€” mirrors the regular surface's read-scope gate. The + // pending human member does NOT get the full view either: their + // working material is the rendered prompt in the summary. + if (!ownershipValidator.isAdmin(identity) + && !ownershipValidator.isOwner(identity, gc.getUserId()) + && !(awaitingApproval && ownershipValidator.isApprover(identity))) { return Response.status(Response.Status.FORBIDDEN) .entity("Full approval status is available to approvers only while the group " + "conversation is awaiting approval β€” use the summary view") @@ -711,6 +768,13 @@ public Response getGroupApprovalStatus(String groupId, String gcId, String detai summary.put("pauseReason", paused && gc.getHitlPauseReason() != null ? gc.getHitlPauseReason() : ""); summary.put("timeoutPolicy", paused && gc.getHitlTimeoutPolicy() != null ? gc.getHitlTimeoutPolicy().name() : ""); summary.put("awaitingApprovalTaskIds", awaitingTaskIds); + // I6: a human-turn pause carries WHO is up and WHAT they were asked β€” + // the summary is that member's working view, no transcript needed. + if (gc.getPendingHumanInput() != null) { + summary.put("pendingMemberId", gc.getPendingHumanInput().memberId()); + summary.put("pendingMemberDisplayName", gc.getPendingHumanInput().displayName()); + summary.put("pendingHumanPrompt", gc.getPendingHumanInput().renderedPrompt()); + } return Response.ok(summary).build(); } catch (IResourceStore.ResourceNotFoundException e) { return Response.status(Response.Status.NOT_FOUND).type(TEXT_PLAIN) @@ -839,6 +903,14 @@ public void onHitlPause(GroupConversationEventSink.HitlPauseEvent event) { closeQuietly(eventSink); } + @Override + public void onHumanInputRequested(GroupConversationEventSink.HumanInputRequestedEvent event) { + // I6: terminal for THIS stream, like an approval pause β€” the leg + // ends; a submission opens its own resumed stream if it wants one. + sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_HUMAN_INPUT_REQUESTED, toJson(event)); + closeQuietly(eventSink); + } + @Override public void onHitlResume(GroupConversationEventSink.HitlResumeEvent event) { // Deliberately does NOT close β€” the stream continues with the diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java index a618a5db1..9e11d2968 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinator.java @@ -32,6 +32,7 @@ import ai.labs.eddi.engine.schedule.IScheduleStore; import ai.labs.eddi.engine.schedule.model.ScheduleConfiguration; import ai.labs.eddi.engine.security.CallerIdentityContext; +import ai.labs.eddi.utils.LogSanitizer; import io.micrometer.core.instrument.Counter; import org.jboss.logging.Logger; @@ -288,7 +289,9 @@ public void convertPauseToCancelIfSignalled(GroupConversation gc, GroupDiscussio */ public void removeTokenAndConvertIfSignalled(GroupConversation gc, GroupDiscussionEventListener listener) { var removed = activeTokens.remove(gc.getId()); - if (removed != null && removed.isCancelled() && gc.getState() == GroupConversationState.AWAITING_APPROVAL) { + if (removed != null && removed.isCancelled() + && (gc.getState() == GroupConversationState.AWAITING_APPROVAL + || gc.getState() == GroupConversationState.AWAITING_HUMAN_INPUT)) { convertPauseToCancelIfSignalled(gc, listener, removed); } } @@ -298,24 +301,32 @@ public void convertPauseToCancelIfSignalled(GroupConversation gc, GroupDiscussio if (token == null || !token.isCancelled()) { return; } + // I6: the conversion serves BOTH pause kinds β€” CAS on whichever paused + // state this leg just committed (approval or human turn). + final GroupConversationState pausedState = gc.getState() == GroupConversationState.AWAITING_HUMAN_INPUT + ? GroupConversationState.AWAITING_HUMAN_INPUT + : GroupConversationState.AWAITING_APPROVAL; // Only the persist itself may revert the in-memory state. Past the commit - // below, CANCELLED is durable, and reverting memory to AWAITING_APPROVAL + // below, CANCELLED is durable, and reverting memory to the paused state // would make executeDiscussion's finally block read the wrong state: it - // skips signingGuard.forgetConversation for AWAITING_APPROVAL (leaking the + // skips signingGuard.forgetConversation for paused states (leaking the // verification cursor) and only runs cleanupEphemeralAgents for // FAILED/CANCELLED β€” so dynamically created agents would stay deployed. // The realistic post-commit thrower is the listener: an SSE sink on a closed // stream. It used to sit inside this try. + final GroupConversation.PendingHumanInput savedPending = gc.getPendingHumanInput(); try { gc.setState(GroupConversationState.CANCELLED); gc.setPausedAt(null); + gc.setPendingHumanInput(null); gc.setLastModified(Instant.now()); - conversationStore.updateIfState(gc, GroupConversationState.AWAITING_APPROVAL); + conversationStore.updateIfState(gc, pausedState); } catch (IResourceStore.ResourceModifiedException e) { // Someone else moved the state concurrently (approve/timeout) β€” restore // the in-memory state so the executeDiscussion finally block does not // release paused-state resources for a conversation still paused in DB. - gc.setState(GroupConversationState.AWAITING_APPROVAL); + gc.setState(pausedState); + gc.setPendingHumanInput(savedPending); LOGGER.infof("Pauseβ†’cancel conversion for GC %s lost a state race β€” leaving persisted state", gc.getId()); return; } catch (IGroupConversationStore.GroupConversationGoneException e) { @@ -323,7 +334,8 @@ public void convertPauseToCancelIfSignalled(GroupConversation gc, GroupDiscussio LOGGER.infof("Pauseβ†’cancel conversion for GC %s skipped β€” conversation was deleted", gc.getId()); return; } catch (Exception e) { - gc.setState(GroupConversationState.AWAITING_APPROVAL); + gc.setState(pausedState); + gc.setPendingHumanInput(savedPending); LOGGER.warnf("Failed to convert just-committed pause of GC %s to CANCELLED: %s", gc.getId(), e.getMessage()); return; @@ -441,9 +453,11 @@ public boolean cancelDiscussion(String conversationId, ControlSignal mode) LOGGER.infof("Cancel skipped: GC %s already in terminal state %s", conversationId, state); return false; } - boolean wasPaused = state == GroupConversationState.AWAITING_APPROVAL; + boolean wasPaused = state == GroupConversationState.AWAITING_APPROVAL + || state == GroupConversationState.AWAITING_HUMAN_INPUT; gc.setState(GroupConversationState.CANCELLED); gc.setPausedAt(null); // keep isPaused() consistent with the terminal state + gc.setPendingHumanInput(null); // I6: a cancelled turn is no longer owed gc.setLastModified(Instant.now()); try { conversationStore.updateIfState(gc, state); @@ -762,7 +776,16 @@ public GroupConversation resumeDiscussion(String groupConversationId, GroupAppro // the pause instead, mirroring the phase-name drift branch above. // Independent of savedPhaseName: a resumePoint's own phaseIdx is the // authority for which phase's roster to check. - if (savedResumePoint != null) { + // I6 scoping: this guard's ">= size means drift" arithmetic is the + // APPROVAL bookmark's contract (the bookmarked speaker itself + // re-runs, so its index must exist). Human-turn bookmarks cannot + // reach this method β€” their state is AWAITING_HUMAN_INPUT, which + // the CAS above rejects β€” and their advanced (speakerIdx+1) + // semantics would false-positive here at the last-speaker + // boundary; the executors clamp their indices safely instead. + if (savedResumePoint != null + && !GroupConversation.RESUME_KIND_HUMAN_TURN.equals(savedResumePoint.pauseKind()) + && !GroupConversation.RESUME_KIND_HUMAN_TURN_PARALLEL.equals(savedResumePoint.pauseKind())) { List currentSpeakers = savedResumePoint.phaseIdx() < phases.size() // I7: the same roster the phase loop will use, recruits // included β€” resolving against the config alone would @@ -904,6 +927,344 @@ public void restoreGroupPause(GroupConversation gc, int phaseIndex, String phase } } + // ================================================================= + // I6 β€” human member turns: pause commit, submission, timeout + // ================================================================= + + /** + * Upper bound on one human submission β€” transcripts are documents, not blobs. + */ + static final int MAX_HUMAN_INPUT_LENGTH = 100_000; + + /** + * Commits an {@code AWAITING_HUMAN_INPUT} pause for a HUMAN member's turn (I6). + * The commitPause sibling, deliberately NOT folded into it: the states, the + * pause payloads, the timeout policies and the resolution surfaces all differ β€” + * sharing a method body would couple what the design keeps apart. + * + * @param turnCountIncludingThisTurn + * the leg's turn count PLUS ONE for the human turn being paused on β€” + * the turn is spent when it resolves (submission or SKIP_TURN), and + * the resumed leg's counter is seeded from this bookmark, so + * counting it here is what keeps a human turn from being free + * @param entryTypeName + * {@link TranscriptEntryType} name the eventual submission is + * recorded as β€” the phase's natural type, captured now so a config + * edit while paused cannot re-type the entry + */ + public void commitHumanTurnPause(GroupConversation gc, int phaseIdx, DiscussionPhase phase, int repeatIdx, + PhaseExecutionEngine.HumanTurnRequired turn, int turnCountIncludingThisTurn, + String entryTypeName, GroupDiscussionEventListener listener, + AgentGroupConfiguration config) + throws IResourceStore.ResourceStoreException { + GroupMember member = turn.member(); + var humanConfig = config != null && config.getHumanMemberConfig() != null + ? config.getHumanMemberConfig() + : new AgentGroupConfiguration.HumanMemberConfig(); + + gc.setResumePoint(new GroupConversation.ResumePoint(phaseIdx, repeatIdx, turn.speakerIdx(), + turn.parallel() + ? GroupConversation.RESUME_KIND_HUMAN_TURN_PARALLEL + : GroupConversation.RESUME_KIND_HUMAN_TURN)); + gc.setPendingHumanInput(new GroupConversation.PendingHumanInput( + member.agentId(), member.displayName(), phaseIdx, repeatIdx, turn.speakerIdx(), + entryTypeName, turn.renderedPrompt(), humanConfig.onTimeout().name(), Instant.now())); + gc.setState(GroupConversationState.AWAITING_HUMAN_INPUT); + gc.setPausedAt(Instant.now()); + gc.setPausedAtPhaseIndex(phaseIdx); + gc.setPausedPhaseName(phase.name()); + gc.setPausedTurnCount(turnCountIncludingThisTurn); + gc.setHitlPauseType(HitlPauseType.HUMAN_TURN); + gc.setHitlPauseReason("Waiting for input from " + member.displayName() + " β€” phase: " + phase.name()); + // The ISO-8601 duration rides the shared bookmark field (REST visibility + + // crash-recovery re-arm); the SKIP_TURN/ABORT policy rides the pending + // record β€” it is not a HitlTimeoutPolicy and must not pretend to be one. + gc.setHitlApprovalTimeout(humanConfig.turnTimeout()); + gc.setHitlTimeoutPolicy(null); + conversationStore.update(gc); + + scheduleHumanTurnTimeout(gc); + counterGroupHitlPause.increment(); + + if (listener != null) { + listener.onHumanInputRequested(new GroupConversationEventSink.HumanInputRequestedEvent( + member.agentId(), member.displayName(), phaseIdx, phase.name())); + } + LOGGER.infof("Group discussion %s paused for human input from member '%s' at phase %d", + LogSanitizer.sanitize(gc.getId()), LogSanitizer.sanitize(member.agentId()), phaseIdx); + } + + /** + * One-shot timeout schedule for a human turn. Same schedule NAME as the + * approval timeout (one armed timeout per conversation, ever), but surface + * {@code group-human} and a SKIP_TURN/ABORT policy string β€” the fire handler + * branches on the surface before it ever parses a {@code HitlTimeoutPolicy}. + */ + public void scheduleHumanTurnTimeout(GroupConversation gc) { + try { + var pending = gc.getPendingHumanInput(); + String timeoutStr = gc.getHitlApprovalTimeout(); + if (pending == null || timeoutStr == null || timeoutStr.isBlank()) { + return; // wait indefinitely + } + Duration timeout = Duration.parse(timeoutStr); + Instant pausedAt = gc.getPausedAt(); + Instant now = Instant.now(); + Instant fireAt = pausedAt != null ? pausedAt.plus(timeout) : now.plus(timeout); + if (fireAt.isBefore(now)) { + fireAt = now.plus(GROUP_HITL_REARM_GRACE); + } + + var schedule = new ScheduleConfiguration(); + schedule.setName(HitlSchedules.groupTimeoutScheduleName(gc.getId())); + schedule.setEnabled(true); + schedule.setOneTimeAt(fireAt.toString()); + schedule.setNextFire(fireAt); + schedule.setCreatedAt(Instant.now()); + schedule.setMetadata(Map.of( + HitlSchedules.METADATA_TYPE_KEY, HitlSchedules.METADATA_TYPE_TIMEOUT, + HitlSchedules.METADATA_POLICY_KEY, pending.onTimeout(), + HitlSchedules.METADATA_SURFACE_KEY, HitlSchedules.SURFACE_GROUP_HUMAN, + HitlSchedules.METADATA_CONVERSATION_ID_KEY, gc.getId())); + scheduleStore.createSchedule(schedule); + LOGGER.infof("Scheduled human-turn timeout for %s at %s (policy: %s)", + LogSanitizer.sanitize(gc.getId()), fireAt, LogSanitizer.sanitize(pending.onTimeout())); + } catch (Exception e) { + LOGGER.warnf("Failed to schedule human-turn timeout for %s: %s", + LogSanitizer.sanitize(gc.getId()), LogSanitizer.sanitize(e.getMessage())); + } + } + + /** + * Records a HUMAN member's submitted response and resumes the discussion (I6). + * Validation and the config-drift check run BEFORE any mutation, so a stale + * bookmark refuses the submission instead of needing a rollback; the one + * failure after the CAS (executor saturation) rolls the append back and + * restores the pause. + */ + public GroupConversation submitHumanInput(String groupConversationId, String memberId, String content, + String submittedBy, GroupDiscussionEventListener listener) + throws GroupDiscussionException, IResourceStore.ResourceStoreException, + IResourceStore.ResourceNotFoundException, IResourceStore.ResourceModifiedException { + var gc = GroupConversationSchemaMigrations.prepareForResume(conversationStore.read(groupConversationId)); + if (gc.getState() != GroupConversationState.AWAITING_HUMAN_INPUT) { + throw new GroupDiscussionException("Group conversation is not awaiting human input"); + } + var pending = gc.getPendingHumanInput(); + if (pending == null) { + throw new GroupDiscussionException( + "Group conversation is awaiting human input but records no pending turn β€” cancel the discussion"); + } + if (memberId == null || !memberId.equals(pending.memberId())) { + throw new IllegalArgumentException( + "This conversation is waiting on member '" + pending.memberId() + "'"); + } + if (content == null || content.isBlank()) { + throw new IllegalArgumentException("Content must not be blank"); + } + if (content.length() > MAX_HUMAN_INPUT_LENGTH) { + throw new IllegalArgumentException( + "Content exceeds the maximum length of " + MAX_HUMAN_INPUT_LENGTH + " characters"); + } + return resolveHumanTurn(gc, pending, content, null, submittedBy, listener); + } + + /** + * SKIP_TURN timeout resolution: the turn resolves to a SKIPPED entry ("no + * response within the configured window") and the discussion moves on β€” the + * same resume path a submission takes, with a different entry. + */ + 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); + } + } + + /** + * The shared resolution path: append the turn's entry (the submission, or a + * SKIPPED record), advance the bookmark past the answered speaker, CAS out of + * AWAITING_HUMAN_INPUT, and re-enter the discussion on a background thread. + */ + private GroupConversation resolveHumanTurn(GroupConversation gc, GroupConversation.PendingHumanInput pending, + String content, String skipReason, String resolvedBy, + GroupDiscussionEventListener listener) + throws GroupDiscussionException, IResourceStore.ResourceStoreException, + IResourceStore.ResourceNotFoundException, IResourceStore.ResourceModifiedException { + String groupConversationId = gc.getId(); + + // Config + drift check BEFORE any mutation. + IResourceStore.IResourceId currentGroupId = groupStore.getCurrentResourceId(gc.getGroupId()); + if (currentGroupId == null) { + throw new IResourceStore.ResourceNotFoundException("Group not found."); + } + AgentGroupConfiguration groupConfig = groupStore.read(gc.getGroupId(), currentGroupId.getVersion()); + List phases = groupConversationService.resolvePhases(groupConfig); + GroupConversation.ResumePoint bookmark = gc.getResumePoint(); + if (bookmark == null || bookmark.phaseIdx() != pending.phaseIdx()) { + throw new GroupDiscussionException( + "The paused turn's bookmark is missing or inconsistent β€” cancel the discussion"); + } + if (pending.phaseIdx() >= phases.size() + || (gc.getPausedPhaseName() != null + && !gc.getPausedPhaseName().equals(phases.get(pending.phaseIdx()).name()))) { + throw new GroupDiscussionException( + "Group config changed while paused β€” the bookmarked phase no longer matches; fix the config or cancel"); + } + + TranscriptEntry entry; + if (skipReason != null) { + entry = new TranscriptEntry(pending.memberId(), pending.displayName(), null, pending.phaseIdx(), + phases.get(pending.phaseIdx()).name(), TranscriptEntryType.SKIPPED, Instant.now(), skipReason, null); + } else { + TranscriptEntryType entryType; + try { + entryType = TranscriptEntryType.valueOf(pending.entryType()); + } catch (Exception e) { + // hand-edited or legacy β€” a mis-typed entry beats a refused turn + LOGGER.warnf("Pending human turn for %s carries unknown entry type '%s' β€” recording as OPINION", + groupConversationId, pending.entryType()); + entryType = TranscriptEntryType.OPINION; + } + entry = new TranscriptEntry(pending.memberId(), pending.displayName(), content, pending.phaseIdx(), + phases.get(pending.phaseIdx()).name(), entryType, Instant.now(), null, null); + } + + // Saved for the one rollback path below (executor saturation). + final Instant savedPausedAt = gc.getPausedAt(); + final String savedPausedPhaseName = gc.getPausedPhaseName(); + final int savedPausedPhaseIdx = gc.getPausedAtPhaseIndex(); + final String savedApprovalTimeout = gc.getHitlApprovalTimeout(); + final String savedPauseReason = gc.getHitlPauseReason(); + + gc.getTranscript().add(entry); + // Advance past the answered speaker; RESUME_KIND is preserved so a + // parallel tail resumes as a parallel tail. + gc.setResumePoint(new GroupConversation.ResumePoint(pending.phaseIdx(), pending.repeatIdx(), + pending.speakerIdx() + 1, bookmark.pauseKind())); + gc.setPendingHumanInput(null); + gc.setPausedAt(null); + gc.setPausedAtPhaseIndex(-1); + gc.setPausedPhaseName(null); + gc.setHitlPauseType(null); + gc.setHitlPauseReason(null); + gc.setHitlTimeoutPolicy(null); + gc.setHitlApprovalTimeout(null); + gc.setState(GroupConversationState.IN_PROGRESS); + gc.setLastModified(Instant.now()); + // Double-submit / concurrent-cancel race β†’ ResourceModifiedException β†’ 409. + conversationStore.updateIfState(gc, GroupConversationState.AWAITING_HUMAN_INPUT); + + // Same post-CAS order as resumeDiscussion: token first (a racing cancel + // must find a signalable token), then the schedule. The metric, the audit + // entry and the resume event are deliberately deferred until the resume is + // actually ENQUEUED below β€” a submit failure rolls the pause back, and a + // rolled-back attempt must not pollute the resume metric or the EU-AI-Act + // audit trail (the same rule resumeDiscussion follows). + activeTokens.put(gc.getId(), new DiscussionControlToken()); + deleteGroupHitlTimeoutSchedule(groupConversationId); + + final int startFromPhase = pending.phaseIdx(); + var question = gc.getResumeQuestion() != null ? gc.getResumeQuestion() : gc.getOriginalQuestion(); + Runnable resumeWork = () -> { + try { + groupConversationService.executeDiscussion(gc, groupConfig, phases, question, listener, startFromPhase); + } catch (Exception e) { + // executeDiscussion persisted the terminal state itself. + LOGGER.errorf(e, "Resumed group discussion %s failed after human turn resolution", groupConversationId); + } + }; + try { + executorService.submit(callerIdentityContext.withIdentity(callerIdentityContext.captureOrCurrent(), resumeWork)); + } catch (RuntimeException e) { + // No thread will run the resume β€” roll the append back and restore the + // pause, so the member's turn is not silently swallowed. + LOGGER.errorf(e, "Could not schedule resumed discussion %s after human turn β€” restoring the pause", + groupConversationId); + synchronized (gc.getTranscript()) { + gc.getTranscript().remove(entry); + } + gc.setResumePoint(bookmark); + gc.setPendingHumanInput(pending); + gc.setState(GroupConversationState.AWAITING_HUMAN_INPUT); + gc.setPausedAt(savedPausedAt); + gc.setPausedAtPhaseIndex(savedPausedPhaseIdx); + gc.setPausedPhaseName(savedPausedPhaseName); + gc.setHitlPauseType(HitlPauseType.HUMAN_TURN); + gc.setHitlPauseReason(savedPauseReason); + gc.setHitlApprovalTimeout(savedApprovalTimeout); + gc.setLastModified(Instant.now()); + try { + conversationStore.updateIfState(gc, GroupConversationState.IN_PROGRESS); + scheduleHumanTurnTimeout(gc); + } catch (Exception restoreEx) { + LOGGER.errorf(restoreEx, "Failed to restore human pause for %s", groupConversationId); + } + // Remove-and-recheck, not a plain remove: a cancel signalled between + // the token registration above and this rollback would otherwise be + // dropped with the discarded token, leaving a "cancelled" discussion + // restored to AWAITING_HUMAN_INPUT with an armed timer. + removeTokenAndConvertIfSignalled(gc, listener); + throw new GroupDiscussionException("Could not schedule the resumed discussion β€” try again", e); + } + + // The resume is committed and enqueued β€” now count it, audit it, and tell + // SSE subscribers the discussion is live again. + counterGroupHitlResume.increment(); + auditHumanTurnResolution(gc, pending, resolvedBy, skipReason != null); + if (listener != null) { + listener.onHitlResume(new GroupConversationEventSink.HitlResumeEvent( + skipReason != null ? "HUMAN_TURN_SKIPPED" : "HUMAN_INPUT", null, resolvedBy)); + } + + // A freshly-read copy, not the live instance: resumeWork mutates gc on a + // background thread (transcript appends, cost maps) while the caller + // serializes the return value β€” handing back the live object is a + // serialization-time race. + try { + return conversationStore.read(groupConversationId); + } catch (Exception e) { + LOGGER.debugf("Post-resume re-read of %s failed (%s) β€” returning the pre-resume snapshot state", + groupConversationId, e.getMessage()); + return gc; + } + } + + /** Audit entry for a human turn's resolution (submission or timeout skip). */ + private void auditHumanTurnResolution(GroupConversation gc, GroupConversation.PendingHumanInput pending, + String resolvedBy, boolean skipped) { + if (auditLedgerService == null || !auditLedgerService.isEnabled()) { + return; + } + try { + var detail = new LinkedHashMap(); + detail.put("verdict", skipped ? "HUMAN_TURN_SKIPPED" : "HUMAN_INPUT_SUBMITTED"); + detail.put("memberId", pending.memberId()); + detail.put("decidedBy", resolvedBy != null ? resolvedBy : "unknown"); + detail.put("automated", resolvedBy != null && resolvedBy.startsWith("system:")); + detail.put("surface", "group"); + auditLedgerService.submit(new AuditEntry( + UUID.randomUUID().toString(), gc.getId(), gc.getGroupId(), null, gc.getUserId(), + null, -1, "hitl.approval", "hitl", -1, 0L, + Map.of(), detail, null, null, List.of(), 0.0, + Instant.now(), null, null)); + } catch (Exception e) { + LOGGER.warnf("Failed to submit human-turn audit entry for group conversation %s: %s", + gc.getId(), e.getMessage()); + } + } + /** * Submits an {@code hitl.approval} audit entry for a group HITL decision (#15, * EU AI Act). Covers human and automated timeout decisions. diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java index f02879588..e3ecfce2a 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java @@ -36,6 +36,8 @@ import org.jboss.logging.Logger; import java.time.Instant; +import java.util.ArrayList; +import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -138,7 +140,8 @@ public void deleteGroupConversation(String groupConversationId) // schedule fires against a deleted conversation, ephemeral dynamic // agents stay deployed forever, and the signing guard's verification // cursor leaks. - if (gc.getState() == GroupConversationState.AWAITING_APPROVAL) { + if (gc.getState() == GroupConversationState.AWAITING_APPROVAL + || gc.getState() == GroupConversationState.AWAITING_HUMAN_INPUT) { groupConversationService.deleteGroupHitlTimeoutSchedule(groupConversationId); groupConversationService.cleanupAfterTerminalState(gc); } @@ -492,7 +495,19 @@ public List listGroupPendingApprovals(String groupId, in // The groupId filter is applied in the QUERY (not post-limit), so a busy // deployment cannot push this group's items past the limit window. int clamped = Math.max(1, Math.min(limit, 1000)); - return conversationStore.findByState(GroupConversationState.AWAITING_APPROVAL, groupId, clamped).stream() + // 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(), @@ -500,6 +515,12 @@ public List listGroupPendingApprovals(String groupId, in 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()); + } return summary; }) .toList(); diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java b/src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java index aba17f729..a1172c9d6 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java @@ -164,6 +164,18 @@ public TranscriptEntry executeAgentTurn(GroupMember member, GroupConversation gc return executeGroupMemberTurn(member, gc, input, protocol, phaseIdx, phase, entryType, targetAgentId); } + // --- HUMAN member (I6): defense in depth, never the main path --- + // The phase loops intercept HUMAN speakers BEFORE this method and pause + // the discussion. Reaching here means an automated sub-round (convergence + // judge, dissent round, vote tiebreak, task-force wave, nested group) + // asked a human for an automated turn β€” those contexts cannot pause, so + // the turn is skipped, mirroring handleMemberPause's SKIP precedent. + if (member.memberType() == AgentGroupConfiguration.MemberType.HUMAN) { + return new TranscriptEntry(member.agentId(), member.displayName(), null, phaseIdx, phase.name(), + TranscriptEntryType.SKIPPED, Instant.now(), + "Human member does not take automated turns in this context β€” skipped", targetAgentId); + } + // Check agent availability try { var agent = agentFactory.getLatestReadyAgent(DEFAULT_ENV, member.agentId()); @@ -644,9 +656,14 @@ public TranscriptEntry executeGroupMemberTurn(GroupMember member, GroupConversat // supported in v1; cancel the stranded sub-pause (releases its timeout // schedule and removes it from pending-approval listings) and return a // SKIPPED entry with explanation. - if (subConversation.getState() == GroupConversationState.AWAITING_APPROVAL) { - LOGGER.warnf("Sub-group '%s' is awaiting approval β€” nested HITL not supported in v1; cancelling sub-pause", - subGroupId); + // I6: AWAITING_HUMAN_INPUT is the same stranded-nested-pause problem β€” + // save-time validation rejects HUMAN members in nested groups, but the + // child config can gain a human AFTER the parent saved; this is the + // runtime backstop. + if (subConversation.getState() == GroupConversationState.AWAITING_APPROVAL + || subConversation.getState() == GroupConversationState.AWAITING_HUMAN_INPUT) { + LOGGER.warnf("Sub-group '%s' paused (%s) β€” nested pauses are not supported in v1; cancelling sub-pause", + subGroupId, subConversation.getState()); try { groupConversationService.cancelDiscussion(subConversation.getId(), ControlSignal.CANCEL_GRACEFUL); } catch (Exception cancelEx) { @@ -656,7 +673,10 @@ public TranscriptEntry executeGroupMemberTurn(GroupMember member, GroupConversat } return new TranscriptEntry(member.agentId(), member.displayName(), null, phaseIdx, phase.name(), TranscriptEntryType.SKIPPED, Instant.now(), - "Sub-group awaiting approval β€” nested HITL not supported in v1", targetAgentId); + subConversation.getState() == GroupConversationState.AWAITING_HUMAN_INPUT + ? "Sub-group waiting on a human member β€” nested human turns are not supported in v1" + : "Sub-group awaiting approval β€” nested HITL not supported in v1", + targetAgentId); } // Extract the synthesized answer, or concatenate all responses diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java b/src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java index b974d2e71..90c0bccf2 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java @@ -89,6 +89,60 @@ public PhaseExecutionEngine(MemberTurnExecutor memberTurnExecutor, GroupContextB this.callerIdentityContext = callerIdentityContext; } + /** + * Control-flow signal (I6): the phase reached a HUMAN member's turn and the + * discussion must pause for their input. Thrown by the sequential loop and the + * parallel human tail, caught ONLY by {@code executeDiscussion}'s phase + * dispatch, which commits the {@code AWAITING_HUMAN_INPUT} pause and returns. A + * RuntimeException on purpose β€” it must fly through method signatures that + * declare {@code GroupDiscussionException} WITHOUT being caught by the generic + * failure handling (the same reasoning as + * {@code GroupConversationService.MemberTurnCancelledException}). + */ + public static final class HumanTurnRequired extends RuntimeException { + private final transient GroupMember member; + private final int speakerIdx; + private final String renderedPrompt; + private final boolean parallel; + + /** + * @param member + * the HUMAN member whose turn is up + * @param speakerIdx + * the member's index β€” into the phase's resolved speaker list for + * sequential turns, into the phase's human-only sublist for parallel + * ones ({@code parallel} distinguishes the two) + * @param renderedPrompt + * the phase input rendered for this member, exactly as an agent + * would have received it + * @param parallel + * whether this turn belongs to a PARALLEL phase's human tail + */ + public HumanTurnRequired(GroupMember member, int speakerIdx, String renderedPrompt, boolean parallel) { + super("Human member turn required: " + member.agentId(), null, false, false); + this.member = member; + this.speakerIdx = speakerIdx; + this.renderedPrompt = renderedPrompt; + this.parallel = parallel; + } + + public GroupMember member() { + return member; + } + + public int speakerIdx() { + return speakerIdx; + } + + public String renderedPrompt() { + return renderedPrompt; + } + + public boolean parallel() { + return parallel; + } + } + public void executeSequentialPhase(GroupConversation gc, AgentGroupConfiguration config, List speakers, DiscussionPhase phase, ProtocolConfig protocol, String question, int phaseIdx, GroupDiscussionEventListener listener, AtomicInteger turnCounter, int maxTurns) @@ -496,7 +550,8 @@ public void executeSequentialPhase(GroupConversation gc, AgentGroupConfiguration AtomicInteger turnCounter, int maxTurns, int startSpeakerIdx) throws GroupDiscussionException { int from = Math.min(Math.max(startSpeakerIdx, 0), speakers.size()); - for (GroupMember speaker : speakers.subList(from, speakers.size())) { + for (int idx = from; idx < speakers.size(); idx++) { + GroupMember speaker = speakers.get(idx); if (turnCounter.get() >= maxTurns) { break; } @@ -505,6 +560,16 @@ public void executeSequentialPhase(GroupConversation gc, AgentGroupConfiguration if (GroupCostLedger.enforceCeiling(gc, protocol, phaseIdx, phase)) { break; } + // I6: a human's turn pauses the discussion. AFTER the budget checks β€” + // an exhausted budget means no more turns, the human's included β€” and + // BEFORE the turn count (the pause commit accounts for the turn, so a + // restored pause does not double-count it). The prompt is rendered + // HERE, against exactly the transcript an agent speaker would see. + if (speaker.memberType() == AgentGroupConfiguration.MemberType.HUMAN) { + String humanInput = contextBuilder.buildPhaseInput(phase, speaker, question, gc.getTranscript(), phaseIdx, null, + GroupConversationService.rosterWithRecruits(config, gc)); + throw new HumanTurnRequired(speaker, idx, humanInput, false); + } turnCounter.incrementAndGet(); if (listener != null) { listener.onSpeakerStart( @@ -525,6 +590,48 @@ public void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration c ProtocolConfig protocol, String question, int phaseIdx, GroupDiscussionEventListener listener, AtomicInteger turnCounter, int maxTurns) throws GroupDiscussionException { + executeParallelPhase(gc, config, speakers, phase, protocol, question, phaseIdx, listener, turnCounter, maxTurns, null); + } + + /** + * @param humanResumeIdx + * {@code null} for every fresh run. Non-null only when resuming an + * I6 {@code HUMAN_TURN_PARALLEL} pause: the agent fan-out already + * ran before the pause, so the resumed leg skips straight to the + * phase's human tail, starting at this index into the human-only + * sublist (see {@code GroupConversation.ResumePoint}). + */ + public void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration config, List speakers, DiscussionPhase phase, + ProtocolConfig protocol, String question, int phaseIdx, GroupDiscussionEventListener listener, + AtomicInteger turnCounter, int maxTurns, Integer humanResumeIdx) + throws GroupDiscussionException { + + // I6: humans never join the fan-out β€” an LLM answers in seconds, a human + // in minutes-to-days, and a paused future would pin the whole batch. + // Agents run first (concurrently), then humans are prompted sequentially, + // each pausing the discussion in turn. + List humans = speakers.stream() + .filter(s -> s.memberType() == AgentGroupConfiguration.MemberType.HUMAN).toList(); + List agentSpeakers = humans.isEmpty() + ? speakers + : speakers.stream().filter(s -> s.memberType() != AgentGroupConfiguration.MemberType.HUMAN).toList(); + + if (humanResumeIdx != null) { + // Resumed leg: the fan-out already ran before the pause. The remaining + // humans' prompts must stay blind to their peers β€” the fresh leg uses + // the pre-fan-out snapshot for that, which no longer exists here, so + // the resumed bound is "no entries of this phase at all" (deliberately + // a touch stronger for repeats > 1 than the fresh-leg bound; a blind + // round that stays blind is the honest failure direction). + List blindTranscript; + synchronized (gc.getTranscript()) { + blindTranscript = gc.getTranscript().stream() + .filter(e -> e == null || e.phaseIndex() != phaseIdx).toList(); + } + promptHumanTail(gc, config, humans, phase, protocol, question, phaseIdx, turnCounter, maxTurns, + humanResumeIdx, blindTranscript); + return; + } // I1: whole-batch check β€” a PARALLEL phase fans every speaker out at once, // so there is no per-speaker checkpoint to gate individually; this is the @@ -535,13 +642,13 @@ public void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration c } // Cap batch size to remaining turn budget - int remainingTurns = maxTurns > 0 ? Math.max(0, maxTurns - turnCounter.get()) : speakers.size(); + int remainingTurns = maxTurns > 0 ? Math.max(0, maxTurns - turnCounter.get()) : agentSpeakers.size(); if (remainingTurns == 0) { return; } List batchSpeakers = maxTurns > 0 - ? speakers.subList(0, Math.min(speakers.size(), remainingTurns)) - : speakers; + ? agentSpeakers.subList(0, Math.min(agentSpeakers.size(), remainingTurns)) + : agentSpeakers; // SAFETY: Snapshot the transcript so parallel tasks each see a consistent view. // Iterating a Collections.synchronizedList requires holding its monitor. @@ -678,6 +785,34 @@ public void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration c } // Count all completed turns for this batch (parallel turns are atomic batches) turnCounter.addAndGet(batchSpeakers.size()); + + // I6: the human tail, prompted against the PRE-fan-out snapshot so a + // "parallel" (independent) round stays independent β€” a human answering + // after the agents must not read their answers first. + promptHumanTail(gc, config, humans, phase, protocol, question, phaseIdx, turnCounter, maxTurns, 0, snapshotTranscript); + } + + /** + * Prompts a PARALLEL phase's HUMAN members one at a time, from + * {@code startIdx}. Throws {@link HumanTurnRequired} for the first human whose + * turn is still owed β€” one pause per human, sequentially. Budget checks mirror + * the sequential loop: an exhausted budget owes no more turns, human or not. + */ + private void promptHumanTail(GroupConversation gc, AgentGroupConfiguration config, List humans, DiscussionPhase phase, + ProtocolConfig protocol, String question, int phaseIdx, AtomicInteger turnCounter, int maxTurns, + int startIdx, List promptTranscript) { + for (int i = Math.max(0, startIdx); i < humans.size(); i++) { + if (maxTurns > 0 && turnCounter.get() >= maxTurns) { + return; + } + if (GroupCostLedger.enforceCeiling(gc, protocol, phaseIdx, phase)) { + return; + } + GroupMember human = humans.get(i); + String input = contextBuilder.buildPhaseInput(phase, human, question, promptTranscript, phaseIdx, null, + GroupConversationService.rosterWithRecruits(config, gc)); + throw new HumanTurnRequired(human, i, input, true); + } } /** diff --git a/src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java b/src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java index 8182bcebd..8eba5c8fc 100644 --- a/src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java +++ b/src/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.java @@ -61,6 +61,14 @@ private GroupConversationEventSink() { * Always preceded by an {@link #EVENT_CONVERGENCE_CHECKED} for the same repeat. */ public static final String EVENT_CONVERGENCE_REACHED = "convergence_reached"; + /** + * A HUMAN group member's turn is up (I6): the discussion paused + * ({@code AWAITING_HUMAN_INPUT}) until that member submits their response β€” or + * the group's {@code humanMemberConfig} timeout policy resolves the turn. + * Distinct from {@link #EVENT_AWAITING_APPROVAL}: this is "you're up", not + * "approve/reject". + */ + public static final String EVENT_HUMAN_INPUT_REQUESTED = "human_input_requested"; // --- Event payloads --- @@ -119,6 +127,15 @@ public record HitlPauseEvent(int phaseIndex, String phaseName, String reason, St public record HitlResumeEvent(String verdict, String note, String decidedBy) { } + /** + * A HUMAN member's turn is up (I6). Carries identifiers only β€” the rendered + * prompt lives on the conversation's {@code pendingHumanInput}, which the + * member's UI reads; an SSE frame is the wrong place for a full transcript + * rendering. + */ + public record HumanInputRequestedEvent(String memberId, String displayName, int phaseIndex, String phaseName) { + } + /** * Emitted when a member agent's private conversation requested human approval * (PAUSE_CONVERSATION) during its group turn. Member-level HITL is not diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java index dbd4d23d0..b6a87e3c3 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java @@ -337,11 +337,23 @@ public String getGroupApprovalStatus( return errorJson("groupId and conversationId are required", "BAD_REQUEST", null); } try { - hitlAccessGuard.requireGroupConversationHitlAccess(groupId, conversationId); + // I6: the READ guard β€” the pending HUMAN member may read the status + // of the turn they owe (their rendered prompt is in the summary). + hitlAccessGuard.requireGroupConversationReadAccess(groupId, conversationId); GroupConversation gc = groupConversationService.readGroupConversation(conversationId); - boolean paused = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL; + boolean paused = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL + || gc.getState() == GroupConversation.GroupConversationState.AWAITING_HUMAN_INPUT; + // The approver full-view window is the APPROVAL pause only β€” `paused` + // also covers AWAITING_HUMAN_INPUT, where there is nothing for an + // approver to decide and the transcript would leak outside their + // remit (review finding). Summary fields keep the wider predicate. + boolean awaitingApproval = gc.getState() == GroupConversation.GroupConversationState.AWAITING_APPROVAL; if ("full".equals(detail)) { - if (!paused && !ownershipValidator.isAdmin(identity) && !ownershipValidator.isOwner(identity, gc.getUserId())) { + // The pending human member (admitted by the READ guard) does NOT + // get the full view β€” their working material is the rendered + // prompt in the summary. Same gate as the REST surface. + if (!ownershipValidator.isAdmin(identity) && !ownershipValidator.isOwner(identity, gc.getUserId()) + && !(awaitingApproval && ownershipValidator.isApprover(identity))) { return errorJson("Full approval status is available to approvers only while the group conversation " + "is awaiting approval β€” use the summary view", "FORBIDDEN", null); } @@ -362,6 +374,13 @@ public String getGroupApprovalStatus( summary.put("pauseReason", paused && gc.getHitlPauseReason() != null ? gc.getHitlPauseReason() : ""); summary.put("timeoutPolicy", paused && gc.getHitlTimeoutPolicy() != null ? gc.getHitlTimeoutPolicy().name() : ""); summary.put("awaitingApprovalTaskIds", awaitingTaskIds); + // I6: a human-turn pause carries WHO is up and WHAT they were asked β€” + // the summary is that member's working view, no transcript needed. + if (gc.getPendingHumanInput() != null) { + summary.put("pendingMemberId", gc.getPendingHumanInput().memberId()); + summary.put("pendingMemberDisplayName", gc.getPendingHumanInput().displayName()); + summary.put("pendingHumanPrompt", gc.getPendingHumanInput().renderedPrompt()); + } return jsonSerialization.serialize(summary); } catch (ForbiddenException e) { return errorJson("Access denied", "FORBIDDEN", null); @@ -459,6 +478,52 @@ public String approveGroupPhase( } } + @Tool(name = "submit_group_human_input", + description = "Submit a HUMAN group member's response for the turn an AWAITING_HUMAN_INPUT discussion is " + + "waiting on (I6). The response is recorded as the member's transcript entry and the discussion " + + "resumes from the next speaker. Only the pending member's own principal (or an admin) may submit " + + "β€” this is the member SPEAKING, not an approval.") + @Blocking + public String submitGroupHumanInput( + @ToolArg(description = "Group ID") String groupId, + @ToolArg(description = "Group conversation ID") String conversationId, + @ToolArg(description = "The pending HUMAN member's id (their principal id)") String memberId, + @ToolArg(description = "The member's response text") String content) { + String disabled = disabledIfMutationsOff(); + if (disabled != null) { + return disabled; + } + if (groupId == null || groupId.isBlank() || conversationId == null || conversationId.isBlank() + || memberId == null || memberId.isBlank() || content == null || content.isBlank()) { + return errorJson("groupId, conversationId, memberId and content are required", "BAD_REQUEST", null); + } + try { + hitlAccessGuard.requireGroupHumanInputAccess(groupId, conversationId, memberId); + GroupConversation result = groupConversationService.submitHumanInput( + conversationId, memberId, content, principalWithMcpPrefix()); + meterRegistry.counter("eddi.mcp.hitl.decision", "surface", "group", "verdict", "HUMAN_INPUT").increment(); + return jsonSerialization.serialize(result); + } catch (ForbiddenException e) { + return errorJson("Access denied", "FORBIDDEN", null); + } catch (jakarta.ws.rs.NotFoundException e) { + return errorJson("Group conversation not found", "NOT_FOUND", null); + } catch (IResourceStore.ResourceModifiedException e) { + return errorJson("The group conversation was modified concurrently β€” reload and retry", "CONFLICT", null); + } catch (ResourceNotFoundException + | ai.labs.eddi.configs.groups.IGroupConversationStore.GroupConversationGoneException e) { + return errorJson("Group conversation not found", "NOT_FOUND", null); + } catch (IGroupConversationService.GroupDiscussionException e) { + return errorJson("Group conversation is not awaiting human input β€” the turn may have been resolved, " + + "timed out, or cancelled", "WRONG_STATE", null); + } catch (IllegalArgumentException e) { + return errorJson("Invalid submission: the memberId does not match the pending turn, or the content is " + + "blank or too long", "BAD_REQUEST", null); + } catch (Exception e) { + LOGGER.warn("MCP submit_group_human_input failed", e); + return errorJson("Failed to submit human input", "INTERNAL", null); + } + } + @Tool(name = "cancel_group_discussion", description = "Cancel an in-progress or paused group discussion. Attributed to the authenticated caller.") @Blocking diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.java b/src/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.java index 954ed44a2..3d524dacd 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.java @@ -60,7 +60,7 @@ public class McpToolFilter implements ToolFilter { // HITL approval surface (McpHitlTools) β€” mirrors the REST HITL endpoints "list_pending_approvals", "get_approval_status", "resume_conversation", "cancel_conversation", "list_group_pending_approvals", "list_all_group_pending_approvals", "get_group_approval_status", - "approve_group_phase", "cancel_group_discussion", + "approve_group_phase", "cancel_group_discussion", "submit_group_human_input", // Persistent user memory (McpMemoryTools) β€” role-guarded (viewer read / admin // write) + per-user ownership "list_user_memories", "get_visible_memories", "search_user_memories", "get_memory_by_key", diff --git a/src/main/java/ai/labs/eddi/engine/model/PendingApprovalSummary.java b/src/main/java/ai/labs/eddi/engine/model/PendingApprovalSummary.java index 04fe92463..22ba5bda6 100644 --- a/src/main/java/ai/labs/eddi/engine/model/PendingApprovalSummary.java +++ b/src/main/java/ai/labs/eddi/engine/model/PendingApprovalSummary.java @@ -22,10 +22,21 @@ public class PendingApprovalSummary { private String timeoutPolicy; /** ISO-8601 duration of the configured approval timeout (may be null). */ private String approvalTimeout; - /** null/"RULE" = behavior-rule pause, "TOOL_CALL" = gated tool pause. */ + /** + * null/"RULE" = behavior-rule pause, "TOOL_CALL" = gated tool pause; group + * pauses carry "PHASE"/"TASK", and "HUMAN_TURN" (I6) marks a pending human + * member turn β€” the inbox's kind discriminator, per the plan's "no third inbox" + * rule. + */ private String pauseType; /** Names only (no arguments) of the gated tool calls β€” badges inbox lists. */ private List toolNames; + /** + * The HUMAN member a "HUMAN_TURN" pause is waiting on (I6) β€” lets that member's + * own inbox surface the turn even though they do not own the conversation. + * {@code null} for every other pause kind. + */ + private String pendingMemberId; public PendingApprovalSummary() { } @@ -102,4 +113,10 @@ public List getToolNames() { public void setToolNames(List toolNames) { this.toolNames = toolNames; } + public String getPendingMemberId() { + return pendingMemberId; + } + public void setPendingMemberId(String pendingMemberId) { + this.pendingMemberId = pendingMemberId; + } } diff --git a/src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java b/src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java index b6e8cee99..898427c09 100644 --- a/src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java +++ b/src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java @@ -295,6 +295,41 @@ public void onDecisionReached(GroupConversationEventSink.DecisionReachedEvent ev postSafe(channelId, threadTs, sb.toString().stripTrailing()); } + /** + * I6: a HUMAN member's turn is up. Slack is notification-only in v1 β€” the + * member responds through the EDDI UI/API (free-text reply capture from Slack + * is a planned follow-up); this message tells them they are up and where. + */ + @Override + public void onHumanInputRequested(GroupConversationEventSink.HumanInputRequestedEvent event) { + if (event == null || event.memberId() == null) { + return; + } + String name = event.displayName() != null && !event.displayName().isBlank() + ? event.displayName() + : event.memberId(); + String msg = String.format("πŸ™‹ *%s* β€” you're up in *%s*. Respond in EDDI (conversation `%s`).", + escapeMrkdwnHuman(name), escapeMrkdwnHuman(event.phaseName()), + groupConversationId != null ? groupConversationId : "unknown"); + String threadTs = expandedMode ? null : userThreadTs; + postSafe(channelId, threadTs, msg); + // A human pause is terminal for THIS listener instance β€” the discussion + // leg ends, and a resumed leg gets its own listener. Without the count + // down, SlackEventHandler blocks its full awaitCompletion timeout on + // every human pause (the same rule onHitlPause follows). + completionLatch.countDown(); + } + + /** + * Escapes Slack's three mrkdwn control characters β€” a member display name + * containing {@code } must render as text, not broadcast. + */ + private static String escapeMrkdwnHuman(String value) { + return value == null + ? "" + : value.replace("&", "&").replace("<", "<").replace(">", ">"); + } + // ─── HITL (human-in-the-loop) ─── @Override diff --git a/src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java b/src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java index 4e047a6a1..9f80ba9da 100644 --- a/src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java +++ b/src/test/java/ai/labs/eddi/configs/groups/model/AgentGroupConfigurationTest.java @@ -236,9 +236,10 @@ void protocol_setAndGet() { @Test void memberType_allValues() { - assertEquals(2, MemberType.values().length); + assertEquals(3, MemberType.values().length); assertNotNull(MemberType.valueOf("AGENT")); assertNotNull(MemberType.valueOf("GROUP")); + assertNotNull(MemberType.valueOf("HUMAN")); } // ==================== LifecyclePolicy ==================== diff --git a/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationHitlTest.java b/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationHitlTest.java index 0cdd4bc00..9f579c37e 100644 --- a/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationHitlTest.java +++ b/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationHitlTest.java @@ -49,9 +49,10 @@ void awaitingApprovalExists() { void valuesContainAllExpected() { var values = GroupConversationState.values(); // CREATED, IN_PROGRESS, SYNTHESIZING, COMPLETED, FAILED, CANCELLED, - // AWAITING_APPROVAL, CLOSED (CLOSED added by the group follow-up feature) - assertEquals(8, values.length); + // AWAITING_APPROVAL, AWAITING_HUMAN_INPUT (I6), CLOSED + assertEquals(9, values.length); assertEquals(GroupConversationState.CLOSED, GroupConversationState.valueOf("CLOSED")); + assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, GroupConversationState.valueOf("AWAITING_HUMAN_INPUT")); } } @@ -74,9 +75,10 @@ void taskExists() { } @Test - @DisplayName("values() has exactly 2 members") + @DisplayName("values() has exactly 3 members") void valuesCount() { - assertEquals(2, HitlPauseType.values().length); + assertEquals(3, HitlPauseType.values().length); + assertNotNull(HitlPauseType.valueOf("HUMAN_TURN")); } } diff --git a/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java b/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java index 94891f118..29e89fdf8 100644 --- a/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java +++ b/src/test/java/ai/labs/eddi/configs/groups/model/GroupConversationTest.java @@ -294,11 +294,12 @@ void transcriptEntryTypes() { @DisplayName("GroupConversationState β€” all values") void groupConversationStates() { var values = GroupConversationState.values(); - assertEquals(8, values.length); + assertEquals(9, values.length); assertNotNull(GroupConversationState.valueOf("CREATED")); assertNotNull(GroupConversationState.valueOf("COMPLETED")); assertNotNull(GroupConversationState.valueOf("FAILED")); assertNotNull(GroupConversationState.valueOf("AWAITING_APPROVAL")); + assertNotNull(GroupConversationState.valueOf("AWAITING_HUMAN_INPUT")); assertNotNull(GroupConversationState.valueOf("CLOSED")); assertNotNull(GroupConversationState.valueOf("CANCELLED")); } diff --git a/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java b/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java index d9c1c087b..67db6d5be 100644 --- a/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java +++ b/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java @@ -8,6 +8,10 @@ import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextScope; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.DiscussionPhase; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.DiscussionStyle; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.GroupMember; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.HumanMemberConfig; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.MemberType; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.OnHumanTimeout; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.PhaseType; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.TurnOrder; import org.junit.jupiter.api.Test; @@ -87,4 +91,101 @@ void noModeratorPhasesAtAll_isSilent() { assertTrue(AgentGroupStore.moderatorlessPhaseNames(config(DiscussionStyle.CUSTOM, phases, null)).isEmpty()); } + + // ================================================================= + // I6 β€” HUMAN member save-time matrix + // ================================================================= + + private GroupMember human(String id, String name) { + return new GroupMember(id, name, 1, null, MemberType.HUMAN); + } + + private AgentGroupConfiguration humanConfig(DiscussionStyle style, List phases, GroupMember... members) { + var c = config(style, phases, null); + c.setMembers(List.of(members)); + return c; + } + + @Test + void humanMember_withoutDisplayName_isRejected() { + var problems = AgentGroupStore.humanMemberProblems(humanConfig(DiscussionStyle.CUSTOM, + List.of(phase("Open", "ALL")), human("h-1", " "))); + + assertEquals(1, problems.size()); + assertTrue(problems.get(0).contains("displayName"), problems.toString()); + } + + @Test + void humanMember_inTaskForceGroup_isRejected_presetExpanded() { + // TASK_FORCE stores NO phases β€” the preset expansion is what makes this + // check reach PLAN/EXECUTE/VERIFY at all. + var problems = AgentGroupStore.humanMemberProblems(humanConfig(DiscussionStyle.TASK_FORCE, + null, human("h-1", "Hannah"))); + + assertFalse(problems.isEmpty()); + assertTrue(problems.stream().anyMatch(p -> p.contains("task-force")), problems.toString()); + } + + @Test + void humanMember_inTargetEachPeerPhase_isRejected() { + var peerPhase = new DiscussionPhase("Critique", PhaseType.CRITIQUE, "ALL", TurnOrder.SEQUENTIAL, + ContextScope.FULL, true, null, 1, false); + var problems = AgentGroupStore.humanMemberProblems(humanConfig(DiscussionStyle.CUSTOM, + List.of(peerPhase), human("h-1", "Hannah"))); + + assertFalse(problems.isEmpty()); + assertTrue(problems.stream().anyMatch(p -> p.contains("targetEachPeer")), problems.toString()); + } + + @Test + void humanMember_inPlainSequentialGroup_isAccepted() { + var config = humanConfig(DiscussionStyle.CUSTOM, List.of(phase("Open", "ALL")), + human("h-1", "Hannah"), new GroupMember("a-1", "Agent", 2, null)); + config.setHumanMemberConfig(new HumanMemberConfig("PT4H", OnHumanTimeout.SKIP_TURN)); + + assertTrue(AgentGroupStore.humanMemberProblems(config).isEmpty()); + } + + @Test + void humanTimeout_notIso8601_isRejected() { + var config = humanConfig(DiscussionStyle.CUSTOM, List.of(phase("Open", "ALL")), human("h-1", "Hannah")); + config.setHumanMemberConfig(new HumanMemberConfig("4 hours", null)); + + var problems = AgentGroupStore.humanMemberProblems(config); + + assertEquals(1, problems.size()); + assertTrue(problems.get(0).contains("ISO-8601"), problems.toString()); + } + + @Test + void humanTimeout_zeroOrNegative_isRejected() { + // Duration.parse accepts both; armed, they would fire effectively + // immediately and silently skip every human turn. + for (String bad : new String[]{"PT0S", "PT-4H"}) { + var config = humanConfig(DiscussionStyle.CUSTOM, List.of(phase("Open", "ALL")), human("h-1", "Hannah")); + config.setHumanMemberConfig(new HumanMemberConfig(bad, null)); + + var problems = AgentGroupStore.humanMemberProblems(config); + + assertEquals(1, problems.size(), bad); + assertTrue(problems.get(0).contains("positive"), problems.toString()); + } + } + + @Test + void humanValidation_nullMembersList_neverNPEs() { + var config = config(DiscussionStyle.CUSTOM, List.of(phase("Open", "ALL")), "mod"); + config.setMembers(null); + + assertTrue(AgentGroupStore.humanMemberProblems(config).isEmpty()); + assertFalse(AgentGroupStore.hasHumanMembers(config)); + } + + @Test + void agentOnlyGroups_produceNoHumanProblems() { + assertTrue(AgentGroupStore.humanMemberProblems(config(DiscussionStyle.TASK_FORCE, null, null)).isEmpty(), + "the whole matrix only applies when a HUMAN member exists"); + assertFalse(AgentGroupStore.hasHumanMembers(config(DiscussionStyle.CUSTOM, null, null))); + assertTrue(AgentGroupStore.hasHumanMembers(humanConfig(DiscussionStyle.CUSTOM, null, human("h", "H")))); + } } diff --git a/src/test/java/ai/labs/eddi/engine/hitl/HitlAccessGuardTest.java b/src/test/java/ai/labs/eddi/engine/hitl/HitlAccessGuardTest.java index be2fe7589..bab7a404c 100644 --- a/src/test/java/ai/labs/eddi/engine/hitl/HitlAccessGuardTest.java +++ b/src/test/java/ai/labs/eddi/engine/hitl/HitlAccessGuardTest.java @@ -227,4 +227,112 @@ void requireGroupConversationHitlAccess_denied_throwsForbidden() throws Exceptio assertThrows(ForbiddenException.class, () -> guard.requireGroupConversationHitlAccess("g1", "gc1")); } + + // ================================================================= + // I6 β€” requireGroupHumanInputAccess (speaking β‰  approving) + // ================================================================= + + private GroupConversation humanPausedGc() throws Exception { + GroupConversation gc = new GroupConversation(); + gc.setId("gc1"); + gc.setGroupId("g1"); + gc.setUserId("owner1"); + when(groupConversationService.readGroupConversation("gc1")).thenReturn(gc); + return gc; + } + + @Test + void humanInputAccess_thePendingMemberThemselves_allowed() throws Exception { + when(ownershipValidator.isAuthEnabled()).thenReturn(true); + humanPausedGc(); + callerNamed("hannah"); + + guard.requireGroupHumanInputAccess("g1", "gc1", "hannah"); + } + + @Test + void humanInputAccess_adminBreakGlass_allowed() throws Exception { + when(ownershipValidator.isAuthEnabled()).thenReturn(true); + when(ownershipValidator.isAdmin(identity)).thenReturn(true); + humanPausedGc(); + callerNamed("some-admin"); + + guard.requireGroupHumanInputAccess("g1", "gc1", "hannah"); + } + + @Test + void humanInputAccess_ownerAndApprover_forbidden_speakingIsNotApproving() throws Exception { + when(ownershipValidator.isAuthEnabled()).thenReturn(true); + when(ownershipValidator.isApprover(identity)).thenReturn(true); + humanPausedGc(); + // The conversation OWNER, who is also an approver β€” may decide approvals, + // but must never speak AS another human. + callerNamed("owner1"); + + assertThrows(ForbiddenException.class, + () -> guard.requireGroupHumanInputAccess("g1", "gc1", "hannah")); + } + + @Test + void humanInputAccess_wrongGroupPath_404sWithoutLeaking() throws Exception { + when(ownershipValidator.isAuthEnabled()).thenReturn(true); + humanPausedGc(); + callerNamed("hannah"); + + assertThrows(jakarta.ws.rs.NotFoundException.class, + () -> guard.requireGroupHumanInputAccess("other-group", "gc1", "hannah")); + } + + @Test + void humanInputAccess_authDisabled_noOp() { + when(ownershipValidator.isAuthEnabled()).thenReturn(false); + + guard.requireGroupHumanInputAccess("g1", "gc1", "anyone"); + + verify(ownershipValidator, never()).isAdmin(any()); + } + + @Test + void readAccess_pendingMemberMayReadTheStatusOfTheirTurn() throws Exception { + var gc = humanPausedGc(); + gc.setPendingHumanInput(new GroupConversation.PendingHumanInput("hannah", "Hannah", 0, 0, 1, + "OPINION", "the prompt", "SKIP_TURN", java.time.Instant.now())); + callerNamed("hannah"); + // Not owner, not admin, not approver β€” the strict guard would refuse. + doThrow(new ForbiddenException("no")) + .when(ownershipValidator).requireOwnerAdminOrApprover(any(), any(), any()); + + guard.requireGroupConversationReadAccess("g1", "gc1"); + } + + @Test + void readAccess_strangerStillRefused_andWrongGroup404s() throws Exception { + var gc = humanPausedGc(); + gc.setPendingHumanInput(new GroupConversation.PendingHumanInput("hannah", "Hannah", 0, 0, 1, + "OPINION", "the prompt", "SKIP_TURN", java.time.Instant.now())); + callerNamed("mallory"); + doThrow(new ForbiddenException("no")) + .when(ownershipValidator).requireOwnerAdminOrApprover(any(), any(), any()); + + assertThrows(ForbiddenException.class, () -> guard.requireGroupConversationReadAccess("g1", "gc1")); + assertThrows(jakarta.ws.rs.NotFoundException.class, + () -> guard.requireGroupConversationReadAccess("other-group", "gc1")); + } + + @Test + void groupInbox_pendingMemberSeesTheirTurn_withoutOwningTheConversation() throws Exception { + when(ownershipValidator.isAdmin(identity)).thenReturn(false); + when(ownershipValidator.isApprover(identity)).thenReturn(false); + callerNamed("hannah"); + var ownTurn = new PendingApprovalSummary("gc1", null, "owner1", Instant.now(), "waiting", null); + ownTurn.setPendingMemberId("hannah"); + var someoneElses = new PendingApprovalSummary("gc2", null, "owner2", Instant.now(), "waiting", null); + someoneElses.setPendingMemberId("bob"); + when(groupConversationService.listGroupPendingApprovals(null, 50)).thenReturn(List.of(ownTurn, someoneElses)); + + List result = guard.listScopedGroupPendingApprovals(null, 50); + + assertEquals(1, result.size()); + assertEquals("gc1", result.get(0).getConversationId()); + } } diff --git a/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java b/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java index b98e46685..d380499bf 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java @@ -144,6 +144,86 @@ private void stubAgent(String agentId, String response) throws Exception { // startAndDiscussAsync // ========================================================= + /** + * Review finding (final pass): a human turn pauses MID-repeat, after other + * speakers already appended this repeat's entries β€” a resumed leg recomputing + * "transcript size at top of repeat" sliced only the post-pause entries, so the + * convergence check (and every later consumer of the repeat slice) silently + * lost the pre-pause contributions. + */ + @Nested + class HumanPauseRepeatSlice { + + private AgentGroupConfiguration humanConfig() { + var c = new AgentGroupConfiguration(); + c.setName("Hybrid"); + c.setStyle(DiscussionStyle.CUSTOM); + c.setMembers(List.of(new GroupMember("a1", "Alice", 1, null), + new GroupMember("h1", "Hannah", 2, null, AgentGroupConfiguration.MemberType.HUMAN), + new GroupMember("a2", "Bob", 3, null))); + c.setPhases(List.of(new AgentGroupConfiguration.DiscussionPhase("Discuss", PhaseType.OPINION, "ALL", + TurnOrder.SEQUENTIAL, ContextScope.FULL, false, null, 1, false))); + c.setProtocol(new ProtocolConfig(60, ProtocolConfig.MemberFailurePolicy.SKIP, 2, + ProtocolConfig.MemberUnavailablePolicy.SKIP)); + return c; + } + + @Test + void midRepeatHumanPause_persistsTheRepeatSliceBase() throws Exception { + setupStore(humanConfig()); + stubAgent("a1", "Opinion A"); + // The resumed leg re-reads the DOCUMENT β€” the in-memory instance below + // proves nothing about what a crashed-and-recovered pod would see + // (review finding: a captor would hold the same mutable instance, so + // the value is recorded AT persist time instead). + var basesAtPersistTime = new java.util.ArrayList(); + doAnswer(inv -> { + basesAtPersistTime.add(((GroupConversation) inv.getArgument(0)).getPausedRepeatSliceBase()); + return null; + }).when(conversationStore).update(any()); + + GroupConversation gc = service.discuss(GROUP_ID, QUESTION, USER_ID, 0); + + assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, gc.getState()); + assertEquals(2, gc.getTranscript().size(), "the QUESTION entry plus a1's pre-pause opinion"); + assertEquals(1, gc.getPausedRepeatSliceBase(), + "the repeat began AFTER the question entry β€” the resumed leg must slice from there, " + + "not from the pause point (which would lose a1's contribution)"); + assertFalse(basesAtPersistTime.isEmpty(), "the pause must persist the conversation"); + assertEquals(1, basesAtPersistTime.get(basesAtPersistTime.size() - 1), + "the slice base must be IN the persisted pause record, not only in memory"); + } + + @Test + void resumedLeg_consumesThePersistedBase_exactlyOnce() throws Exception { + var config = humanConfig(); + setupStore(config); + stubAgent("a2", "Opinion B"); + // The resumed leg: a1 and the human already answered before the pause; + // the bookmark stands past the human, and the persisted base points at + // the top of the repeat. + var gc = new GroupConversation(); + gc.setId("gc-resume"); + gc.setGroupId(GROUP_ID); + gc.setUserId(USER_ID); + gc.setState(GroupConversationState.IN_PROGRESS); + gc.setOriginalQuestion(QUESTION); + gc.getTranscript().add(new TranscriptEntry("a1", "Alice", "Opinion A", 0, "Discuss", + TranscriptEntryType.OPINION, java.time.Instant.now(), null, null)); + gc.getTranscript().add(new TranscriptEntry("h1", "Hannah", "Human view", 0, "Discuss", + TranscriptEntryType.OPINION, java.time.Instant.now(), null, null)); + gc.setResumePoint(new GroupConversation.ResumePoint(0, 0, 2, GroupConversation.RESUME_KIND_HUMAN_TURN)); + gc.setPausedRepeatSliceBase(0); + + service.executeDiscussion(gc, config, service.resolvePhases(config), QUESTION, null, 0); + + assertEquals(-1, gc.getPausedRepeatSliceBase(), + "the base is consumed exactly once by the resumed repeat β€” a stale base must never bleed forward"); + assertEquals(GroupConversationState.COMPLETED, gc.getState()); + assertEquals(3, gc.getTranscript().size(), "only a2 ran on the resumed leg"); + } + } + @Nested class AsyncDiscussion { diff --git a/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java b/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java index a44f8c115..24929d62a 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java @@ -302,6 +302,26 @@ void moderator_withValidModerator_returnsSingleModeratorMember() throws Exceptio assertEquals("Moderator", result.get(0).displayName()); } + @Test + void moderator_whoIsAHumanMember_keepsHumanTypeAndName() throws Exception { + var phase = new DiscussionPhase("Synth", PhaseType.SYNTHESIS, "MODERATOR", + AgentGroupConfiguration.TurnOrder.SEQUENTIAL, AgentGroupConfiguration.ContextScope.FULL, + false, null, 1); + var members = List.of( + new GroupMember("a1", "Alice", 1, "MEMBER"), + new GroupMember("h-1", "Hannah", 2, null, AgentGroupConfiguration.MemberType.HUMAN)); + + List result = invoke(phase, members, "h-1"); + + assertEquals(1, result.size()); + assertEquals("h-1", result.get(0).agentId()); + // I6: the 4-arg ctor synthesized a fresh AGENT-typed moderator, silently + // demoting a HUMAN β€” their synthesis turn then went to a (nonexistent) + // LLM agent instead of pausing for their input. + assertEquals(AgentGroupConfiguration.MemberType.HUMAN, result.get(0).memberType()); + assertEquals("Hannah", result.get(0).displayName()); + } + @Test void moderator_withNullModerator_picksOneDeterministicSynthesizer() throws Exception { var phase = new DiscussionPhase("Synth", PhaseType.SYNTHESIS, "MODERATOR", diff --git a/src/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.java b/src/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.java index 59cdc02df..12b84f50a 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/HitlTimeoutHandlerTest.java @@ -225,4 +225,55 @@ void unknownPolicy_noCallsNoException() { verifyNoInteractions(groupConversationService); } } + + // ========================================================================= + // I6 β€” human-turn timeouts (surface group-human, OnHumanTimeout policies) + // ========================================================================= + + @Nested + @DisplayName("group-human surface (I6)") + class HumanTurnTimeouts { + + @Test + @DisplayName("SKIP_TURN β†’ skipHumanTurnOnTimeout, never the approval resume path") + void skipTurn_routesToSkipHumanTurn() { + var metadata = Map.of( + "policy", "SKIP_TURN", + "surface", "group-human", + "conversationId", "gc-1"); + + handler.handleTimeout(metadata); + + verify(groupConversationService).skipHumanTurnOnTimeout("gc-1"); + verifyNoMoreInteractions(groupConversationService); + verifyNoInteractions(conversationService); + } + + @Test + @DisplayName("ABORT β†’ graceful cancel of the discussion") + void abort_cancelsDiscussion() throws Exception { + var metadata = Map.of( + "policy", "ABORT", + "surface", "group-human", + "conversationId", "gc-1"); + + handler.handleTimeout(metadata); + + verify(groupConversationService).cancelDiscussion("gc-1", ControlSignal.CANCEL_GRACEFUL); + verifyNoMoreInteractions(groupConversationService); + } + + @Test + @DisplayName("an unknown human policy degrades to SKIP_TURN β€” a lost turn beats a stuck discussion") + void unknownHumanPolicy_degradesToSkip() { + var metadata = Map.of( + "policy", "SOMETHING_NEW", + "surface", "group-human", + "conversationId", "gc-1"); + + assertDoesNotThrow(() -> handler.handleTimeout(metadata)); + + verify(groupConversationService).skipHumanTurnOnTimeout("gc-1"); + } + } } diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationHitlTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationHitlTest.java index 558bcb019..f9ba54b12 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationHitlTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationHitlTest.java @@ -432,6 +432,23 @@ void approverFullWhilePaused() throws Exception { assertSame(gc, response.getEntity(), "Full view should return the conversation"); } + @Test + @DisplayName("Approver gets 403 for detail=full during a HUMAN-TURN pause β€” nothing awaits their decision") + void approverFullDeniedDuringHumanTurnPause() throws Exception { + // Review finding: the gate used the shared `paused` predicate, which + // also covers AWAITING_HUMAN_INPUT β€” an approver could read the full + // transcript of a discussion merely waiting on a human member's turn. + asApprover(ATTACKER_ID); + var gc = makeGc(OWNER_ID); + gc.setState(GroupConversationState.AWAITING_HUMAN_INPUT); + when(groupService.readGroupConversation(GC_ID)).thenReturn(gc); + + Response response = restGroupConversation.getGroupApprovalStatus(GROUP_ID, GC_ID, "full"); + + assertEquals(403, response.getStatus(), + "The approver full-view window is the APPROVAL pause only"); + } + @Test @DisplayName("Approver gets 403 for detail=full on a non-paused conversation") void approverFullDeniedWhenNotPaused() throws Exception { diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java index 0a01ae853..fe326fbca 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupHitlCoordinatorTest.java @@ -6,24 +6,40 @@ import ai.labs.eddi.configs.groups.IAgentGroupStore; import ai.labs.eddi.configs.groups.IGroupConversationStore; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextScope; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.DiscussionPhase; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.GroupMember; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.MemberType; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.OnHumanTimeout; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.PhaseType; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.TurnOrder; import ai.labs.eddi.configs.groups.model.GroupConversation; import ai.labs.eddi.configs.groups.model.GroupConversation.GroupConversationState; +import ai.labs.eddi.configs.groups.model.GroupConversation.HitlPauseType; +import ai.labs.eddi.configs.groups.model.GroupConversation.PendingHumanInput; +import ai.labs.eddi.configs.groups.model.GroupConversation.TranscriptEntryType; import ai.labs.eddi.configs.groups.model.SharedTaskList; import ai.labs.eddi.configs.hitl.HitlTimeoutPolicy; import ai.labs.eddi.configs.groups.model.SharedTaskList.TaskItem; import ai.labs.eddi.datastore.IResourceStore; import ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionEventListener; import ai.labs.eddi.engine.audit.AuditLedgerService; +import ai.labs.eddi.engine.hitl.HitlSchedules; import ai.labs.eddi.engine.internal.GroupConversationService; import ai.labs.eddi.engine.lifecycle.GroupConversationEventSink; import ai.labs.eddi.engine.lifecycle.model.ControlSignal; import ai.labs.eddi.engine.lifecycle.model.DiscussionControlToken; import ai.labs.eddi.engine.schedule.IScheduleStore; +import ai.labs.eddi.engine.schedule.model.ScheduleConfiguration; import ai.labs.eddi.engine.security.CallerIdentityContext; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mockito; +import java.time.Instant; +import java.util.List; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; @@ -55,15 +71,19 @@ class GroupHitlCoordinatorTest { private IGroupConversationStore conversationStore; private IAgentGroupStore groupStore; private IScheduleStore scheduleStore; + private ExecutorService executorService; + private GroupConversationService groupConversationService; private GroupHitlCoordinator coordinator() { conversationStore = mock(IGroupConversationStore.class); groupStore = mock(IAgentGroupStore.class); scheduleStore = mock(IScheduleStore.class); + executorService = Mockito.mock(ExecutorService.class); + groupConversationService = Mockito.mock(GroupConversationService.class); return new GroupHitlCoordinator(groupStore, conversationStore, scheduleStore, mock(AuditLedgerService.class), new GroupSigningGuard(null, null, null, "default"), - new ConcurrentHashMap<>(), Mockito.mock(ExecutorService.class), new CallerIdentityContext(null, null), - Mockito.mock(GroupConversationService.class), + new ConcurrentHashMap<>(), executorService, new CallerIdentityContext(null, null), + groupConversationService, new SimpleMeterRegistry().counter("test.hitl.pause"), new SimpleMeterRegistry().counter("test.hitl.resume"), new SimpleMeterRegistry().counter("test.group.failure")); @@ -280,4 +300,218 @@ void convertPauseToCancelIfSignalled_commitLosesRace_revertsToAwaitingApproval() void constructedWithMockedCollaborators_doesNotThrow() { assertDoesNotThrow(this::coordinator); } + + // ================================================================= + // I6 β€” human member turns + // ================================================================= + + private static final GroupMember HUMAN = new GroupMember("h-1", "Hannah", 1, null, MemberType.HUMAN); + + private DiscussionPhase opinionPhase() { + return new DiscussionPhase("Discuss", PhaseType.OPINION, "ALL", TurnOrder.SEQUENTIAL, ContextScope.FULL, + false, null, 1, false); + } + + private AgentGroupConfiguration humanGroupConfig(String turnTimeout) { + var config = new AgentGroupConfiguration(); + config.setName("G"); + config.setMembers(List.of(HUMAN)); + config.setPhases(List.of(opinionPhase())); + if (turnTimeout != null) { + config.setHumanMemberConfig(new AgentGroupConfiguration.HumanMemberConfig(turnTimeout, OnHumanTimeout.SKIP_TURN)); + } + return config; + } + + private GroupConversation humanPausedGc() { + var gc = gc(GroupConversationState.AWAITING_HUMAN_INPUT); + gc.setOriginalQuestion("Q?"); + gc.setPausedAt(Instant.now()); + gc.setPausedAtPhaseIndex(0); + gc.setPausedPhaseName("Discuss"); + gc.setHitlPauseType(HitlPauseType.HUMAN_TURN); + gc.setPendingHumanInput(new PendingHumanInput("h-1", "Hannah", 0, 0, 1, "OPINION", "the prompt", + "SKIP_TURN", Instant.now())); + gc.setResumePoint(new GroupConversation.ResumePoint(0, 0, 1, GroupConversation.RESUME_KIND_HUMAN_TURN)); + return gc; + } + + @Test + void commitHumanTurnPause_persistsPauseBookmarkPendingAndSchedule() throws Exception { + var coordinator = coordinator(); + var gc = gc(GroupConversationState.IN_PROGRESS); + var turn = new PhaseExecutionEngine.HumanTurnRequired(HUMAN, 2, "the prompt", false); + var listener = mock(GroupDiscussionEventListener.class); + + coordinator.commitHumanTurnPause(gc, 1, opinionPhase(), 0, turn, 5, "OPINION", listener, humanGroupConfig("PT4H")); + + assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, gc.getState()); + assertEquals(HitlPauseType.HUMAN_TURN, gc.getHitlPauseType()); + assertEquals(5, gc.getPausedTurnCount(), "the human's turn is spent when it resolves β€” counted at the pause"); + assertEquals("PT4H", gc.getHitlApprovalTimeout()); + var pending = gc.getPendingHumanInput(); + assertEquals("h-1", pending.memberId()); + assertEquals("the prompt", pending.renderedPrompt()); + assertEquals("OPINION", pending.entryType()); + assertEquals("SKIP_TURN", pending.onTimeout()); + var bookmark = gc.getResumePoint(); + assertEquals(1, bookmark.phaseIdx()); + assertEquals(2, bookmark.speakerIdx()); + assertEquals(GroupConversation.RESUME_KIND_HUMAN_TURN, bookmark.pauseKind()); + verify(conversationStore).update(gc); + var scheduleCaptor = ArgumentCaptor.forClass(ScheduleConfiguration.class); + verify(scheduleStore).createSchedule(scheduleCaptor.capture()); + assertEquals(HitlSchedules.SURFACE_GROUP_HUMAN, + scheduleCaptor.getValue().getMetadata().get(HitlSchedules.METADATA_SURFACE_KEY)); + assertEquals("SKIP_TURN", scheduleCaptor.getValue().getMetadata().get(HitlSchedules.METADATA_POLICY_KEY)); + verify(listener).onHumanInputRequested(any(GroupConversationEventSink.HumanInputRequestedEvent.class)); + } + + @Test + void commitHumanTurnPause_parallelKind_andNoTimeout_waitsIndefinitely() throws Exception { + var coordinator = coordinator(); + var gc = gc(GroupConversationState.IN_PROGRESS); + var turn = new PhaseExecutionEngine.HumanTurnRequired(HUMAN, 0, "p", true); + + coordinator.commitHumanTurnPause(gc, 0, opinionPhase(), 0, turn, 1, "OPINION", null, humanGroupConfig(null)); + + assertEquals(GroupConversation.RESUME_KIND_HUMAN_TURN_PARALLEL, gc.getResumePoint().pauseKind()); + verify(scheduleStore, never()).createSchedule(any()); + } + + @Test + void submitHumanInput_recordsEntryAdvancesBookmarkAndResumes() 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.resolvePhases(config)).thenReturn(List.of(opinionPhase())); + var runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + + var result = coordinator.submitHumanInput(GC_ID, "h-1", "My considered answer.", "h-1", null); + + assertEquals(GroupConversationState.IN_PROGRESS, result.getState()); + assertNull(result.getPendingHumanInput()); + assertEquals(1, result.getTranscript().size()); + var entry = result.getTranscript().get(0); + assertEquals("h-1", entry.speakerAgentId()); + assertEquals("My considered answer.", entry.content()); + assertEquals(TranscriptEntryType.OPINION, entry.type(), "the phase's NATURAL type, captured at pause time"); + assertEquals(2, result.getResumePoint().speakerIdx(), "advanced past the answered speaker"); + verify(conversationStore).updateIfState(gc, GroupConversationState.AWAITING_HUMAN_INPUT); + verify(scheduleStore).deleteSchedulesByName(anyString()); + // The resume runs on the executor β€” capture and run it, then verify the + // discussion re-entered at the paused phase. + verify(executorService).submit(runnableCaptor.capture()); + runnableCaptor.getValue().run(); + verify(groupConversationService).executeDiscussion(eq(gc), eq(config), anyList(), eq("Q?"), isNull(), eq(0)); + } + + @Test + void submitHumanInput_wrongMember_rejectsBeforeAnyMutation() throws Exception { + var coordinator = coordinator(); + var gc = humanPausedGc(); + when(conversationStore.read(GC_ID)).thenReturn(gc); + + assertThrows(IllegalArgumentException.class, + () -> coordinator.submitHumanInput(GC_ID, "someone-else", "text", "someone-else", null)); + + assertTrue(gc.getTranscript().isEmpty()); + assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, gc.getState()); + verify(conversationStore, never()).updateIfState(any(), any()); + } + + @Test + void submitHumanInput_wrongState_conflicts() throws Exception { + var coordinator = coordinator(); + when(conversationStore.read(GC_ID)).thenReturn(gc(GroupConversationState.COMPLETED)); + + assertThrows(ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionException.class, + () -> coordinator.submitHumanInput(GC_ID, "h-1", "text", "h-1", null)); + } + + @Test + void submitHumanInput_blankOrOversizeContent_rejected() throws Exception { + var coordinator = coordinator(); + when(conversationStore.read(GC_ID)).thenReturn(humanPausedGc()); + + assertThrows(IllegalArgumentException.class, + () -> coordinator.submitHumanInput(GC_ID, "h-1", " ", "h-1", null)); + when(conversationStore.read(GC_ID)).thenReturn(humanPausedGc()); + assertThrows(IllegalArgumentException.class, + () -> coordinator.submitHumanInput(GC_ID, "h-1", + "x".repeat(GroupHitlCoordinator.MAX_HUMAN_INPUT_LENGTH + 1), "h-1", null)); + } + + @Test + void submitHumanInput_configDrift_refusesBeforeMutation() throws Exception { + var coordinator = coordinator(); + var gc = humanPausedGc(); + gc.setPausedPhaseName("Old Phase Name"); + 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(null); + when(groupStore.read(GROUP_ID, 1)).thenReturn(config); + when(groupConversationService.resolvePhases(config)).thenReturn(List.of(opinionPhase())); + + assertThrows(ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionException.class, + () -> coordinator.submitHumanInput(GC_ID, "h-1", "text", "h-1", null)); + + assertTrue(gc.getTranscript().isEmpty(), "drift refuses BEFORE any mutation β€” no rollback needed"); + assertEquals(GroupConversationState.AWAITING_HUMAN_INPUT, gc.getState()); + } + + @Test + void skipHumanTurnOnTimeout_writesSkippedEntryAndResumes() throws Exception { + var coordinator = coordinator(); + var gc = humanPausedGc(); + gc.setHitlApprovalTimeout("PT1H"); + 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("PT1H"); + when(groupStore.read(GROUP_ID, 1)).thenReturn(config); + when(groupConversationService.resolvePhases(config)).thenReturn(List.of(opinionPhase())); + + coordinator.skipHumanTurnOnTimeout(GC_ID); + + assertEquals(GroupConversationState.IN_PROGRESS, gc.getState()); + assertEquals(1, gc.getTranscript().size()); + var entry = gc.getTranscript().get(0); + assertEquals(TranscriptEntryType.SKIPPED, entry.type()); + assertTrue(entry.errorReason().contains("Hannah"), "the skip names WHO did not respond: " + entry.errorReason()); + assertTrue(entry.errorReason().contains("PT1H"), "and within WHAT window: " + entry.errorReason()); + assertEquals(2, gc.getResumePoint().speakerIdx(), "a skipped turn still advances past the speaker"); + } + + @Test + void skipHumanTurnOnTimeout_alreadyResolved_noOp() throws Exception { + var coordinator = coordinator(); + when(conversationStore.read(GC_ID)).thenReturn(gc(GroupConversationState.COMPLETED)); + + assertDoesNotThrow(() -> coordinator.skipHumanTurnOnTimeout(GC_ID)); + + verify(conversationStore, never()).updateIfState(any(), any()); + } + + @Test + void cancelDiscussion_awaitingHumanInput_cancelsAndClearsPending() throws Exception { + var coordinator = coordinator(); + var gc = humanPausedGc(); + when(conversationStore.read(GC_ID)).thenReturn(gc); + + assertTrue(coordinator.cancelDiscussion(GC_ID, ControlSignal.CANCEL_GRACEFUL)); + + assertEquals(GroupConversationState.CANCELLED, gc.getState()); + assertNull(gc.getPendingHumanInput(), "a cancelled turn is no longer owed"); + verify(conversationStore).updateIfState(gc, GroupConversationState.AWAITING_HUMAN_INPUT); + verify(scheduleStore).deleteSchedulesByName(anyString()); + } } diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java index 0f9964eb8..2bf19e366 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java @@ -272,4 +272,27 @@ void executeGroupMemberTurn_rollsUpChildDiscussionCost() throws Exception { assertEquals(0.42, gc.getMemberCosts().get("sub-group-1")); assertEquals(0.42, gc.getTotalCost()); } + + /** + * I6 defense in depth: the phase loops intercept HUMAN speakers BEFORE this + * method and pause the discussion β€” any caller reaching it with a human + * (convergence judge, dissent round, task-force wave, nested group) is an + * automated context that cannot pause, and gets a SKIPPED entry instead of an + * attempted LLM call against a person. + */ + @Test + void executeAgentTurn_humanMember_skippedInAutomatedContexts() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + gc.setGroupId("group-1"); + var human = new GroupMember("h-1", "Hannah", 1, null, MemberType.HUMAN); + + var entry = executor().executeAgentTurn(human, gc, "input", protocol(MemberFailurePolicy.SKIP), 0, + phase(PhaseType.OPINION), null, null); + + assertEquals(TranscriptEntryType.SKIPPED, entry.type()); + assertEquals("h-1", entry.speakerAgentId()); + assertEquals("Hannah", entry.speakerDisplayName()); + assertTrue(entry.errorReason().contains("Human member"), entry.errorReason()); + } } diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java index 07c4f1bd9..7bafd2aa9 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java @@ -165,6 +165,113 @@ void sequentialPhase_startSpeakerIdxAtOrBeyondSize_clampsToNoTurns() throws Exce verifyNoInteractions(memberTurnExecutor); } + // ================================================================= + // I6 β€” human member turns + // ================================================================= + + private GroupMember human(String id) { + return new GroupMember(id, "Human " + id, 5, null, AgentGroupConfiguration.MemberType.HUMAN); + } + + @Test + void sequentialPhase_humanSpeaker_pausesWithRenderedPromptAndAbsoluteIndex() throws Exception { + var engine = engine(); + when(memberTurnExecutor.executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any())) + .thenAnswer(inv -> opinionEntry(((GroupMember) inv.getArgument(0)).agentId())); + var speakers = List.of(member("a"), human("h"), member("c")); + var gc = gc(); + var turnCounter = new AtomicInteger(0); + + var pause = assertThrows(PhaseExecutionEngine.HumanTurnRequired.class, + () -> engine.executeSequentialPhase(gc, new AgentGroupConfiguration(), speakers, phase(TurnOrder.SEQUENTIAL), + protocol(), "Q?", 0, null, turnCounter, 10)); + + assertEquals("h", pause.member().agentId()); + assertEquals(1, pause.speakerIdx(), "the index into the phase's resolved speaker list β€” the resume bookmark"); + assertEquals("rendered-input", pause.renderedPrompt(), "the human sees exactly what an agent speaker would"); + assertFalse(pause.parallel()); + assertEquals(1, gc.getTranscript().size(), "the agent BEFORE the human spoke"); + assertEquals(1, turnCounter.get(), "the human's own turn is accounted by the pause commit, not the loop"); + // The speaker after the human never ran β€” the pause ends the leg. + verify(memberTurnExecutor, times(1)).executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any()); + } + + @Test + void sequentialPhase_turnBudgetExhaustedBeforeHuman_noPause() throws Exception { + var engine = engine(); + when(memberTurnExecutor.executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any())) + .thenAnswer(inv -> opinionEntry(((GroupMember) inv.getArgument(0)).agentId())); + var turnCounter = new AtomicInteger(0); + + // Budget of 1: the agent takes it; the human's turn is no longer owed β€” + // an exhausted budget must not park the discussion on a human. + engine.executeSequentialPhase(gc(), new AgentGroupConfiguration(), List.of(member("a"), human("h")), + phase(TurnOrder.SEQUENTIAL), protocol(), "Q?", 0, null, turnCounter, 1); + + assertEquals(1, turnCounter.get()); + } + + @Test + @SuppressWarnings("unchecked") + void parallelPhase_humansPromptedAfterFanOut_blindToTheBatch() throws Exception { + var engine = engine(); + when(memberTurnExecutor.executeAgentTurn(any(), any(), any(), any(), anyInt(), any(), any(), any(), any())) + .thenAnswer(inv -> opinionEntry(((GroupMember) inv.getArgument(0)).agentId())); + var speakers = List.of(member("a"), human("h"), member("b")); + var gc = gc(); + var turnCounter = new AtomicInteger(0); + + var pause = assertThrows(PhaseExecutionEngine.HumanTurnRequired.class, + () -> engine.executeParallelPhase(gc, new AgentGroupConfiguration(), speakers, phase(TurnOrder.PARALLEL), + protocol(), "Q?", 0, null, turnCounter, 10)); + + assertTrue(pause.parallel()); + assertEquals(0, pause.speakerIdx(), "the index into the phase's HUMAN-ONLY sublist"); + assertEquals(2, gc.getTranscript().size(), "both agents fanned out and completed first"); + assertEquals(2, turnCounter.get(), "agent turns are counted; the human's comes with the pause commit"); + // Blindness: the human's prompt renders from the PRE-fan-out snapshot. + var transcriptCaptor = org.mockito.ArgumentCaptor.forClass(List.class); + verify(contextBuilder).buildPhaseInput(any(), argThat(m -> "h".equals(m.agentId())), any(), + transcriptCaptor.capture(), anyInt(), any(), any()); + assertTrue(transcriptCaptor.getValue().isEmpty(), + "a 'parallel' (independent) round must stay independent β€” the human must not read the batch's answers"); + } + + @Test + void parallelPhase_resumeHumanTail_allAnswered_completesWithoutReRunningFanOut() throws Exception { + var engine = engine(); + var gc = gc(); + gc.getTranscript().add(opinionEntry("a")); // the fan-out's pre-pause output + + engine.executeParallelPhase(gc, new AgentGroupConfiguration(), List.of(member("a"), human("h")), + phase(TurnOrder.PARALLEL), protocol(), "Q?", 0, null, new AtomicInteger(2), 10, 1); + + verifyNoInteractions(memberTurnExecutor); + assertEquals(1, gc.getTranscript().size(), "no re-run, no duplicate agent entries"); + } + + @Test + @SuppressWarnings("unchecked") + void parallelPhase_resumeHumanTail_nextHumanPauses_blindToThePhase() throws Exception { + var engine = engine(); + var gc = gc(); + gc.getTranscript().add(opinionEntry("a")); // phaseIndex 0 β€” this phase's entry + + var pause = assertThrows(PhaseExecutionEngine.HumanTurnRequired.class, + () -> engine.executeParallelPhase(gc, new AgentGroupConfiguration(), + List.of(member("a"), human("h1"), human("h2")), + phase(TurnOrder.PARALLEL), protocol(), "Q?", 0, null, new AtomicInteger(2), 10, 1)); + + assertEquals("h2", pause.member().agentId()); + assertEquals(1, pause.speakerIdx()); + verifyNoInteractions(memberTurnExecutor); + var transcriptCaptor = org.mockito.ArgumentCaptor.forClass(List.class); + verify(contextBuilder).buildPhaseInput(any(), argThat(m -> "h2".equals(m.agentId())), any(), + transcriptCaptor.capture(), anyInt(), any(), any()); + assertTrue(transcriptCaptor.getValue().isEmpty(), + "the resumed human prompt excludes this phase's entries entirely β€” the blindness bound survives the pause"); + } + // ================================================================= // I1 β€” cost ceiling gates // ================================================================= diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsCoverageTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsCoverageTest.java index 27d192a6d..ba3defbfa 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsCoverageTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsCoverageTest.java @@ -309,8 +309,10 @@ void getGroupApprovalStatus_blankConversationId_returnsBadRequest() { @Test void getGroupApprovalStatus_forbidden_returnsForbidden() { + // I6: the status endpoint now uses the READ guard (which additionally + // admits the pending human member) β€” not the strict HITL guard. doThrow(new ForbiddenException("no")) - .when(guard).requireGroupConversationHitlAccess("g1", "gc1"); + .when(guard).requireGroupConversationReadAccess("g1", "gc1"); String out = tools.getGroupApprovalStatus("g1", "gc1", "summary"); assertTrue(out.contains("\"errorCode\":\"FORBIDDEN\""), out); } diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.java index a0981c235..d302e84b0 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpHitlToolsTest.java @@ -277,6 +277,21 @@ void getGroupApprovalStatus_detailFull_nonOwnerApproverNotPaused_returnsForbidde assertTrue(out.contains("\"errorCode\":\"FORBIDDEN\""), out); } + @Test + void getGroupApprovalStatus_detailFull_duringHumanTurnPause_approverForbidden() throws Exception { + // Review finding: `paused` also covers AWAITING_HUMAN_INPUT β€” an approver + // must not read the transcript of a discussion waiting on a human's turn. + GroupConversation gc = mock(GroupConversation.class); + when(gc.getState()).thenReturn(GroupConversation.GroupConversationState.AWAITING_HUMAN_INPUT); + when(gc.getUserId()).thenReturn("someone-else"); + when(groupConversationService.readGroupConversation("gc1")).thenReturn(gc); + when(ownershipValidator.isApprover(any())).thenReturn(true); + + String out = tools.getGroupApprovalStatus("g1", "gc1", "full"); + + assertTrue(out.contains("\"errorCode\":\"FORBIDDEN\""), out); + } + @Test void getGroupApprovalStatus_detailFull_whilePaused_returnsFullConversation() throws Exception { GroupConversation gc = mock(GroupConversation.class); @@ -284,8 +299,26 @@ void getGroupApprovalStatus_detailFull_whilePaused_returnsFullConversation() thr when(gc.getUserId()).thenReturn("someone-else"); when(groupConversationService.readGroupConversation("gc1")).thenReturn(gc); when(json.serialize(any())).thenReturn("{\"full\":true}"); + // I6: the full view while paused is for APPROVERS β€” the read guard now + // also admits the pending human member, who must NOT see the transcript, + // so the gate checks the role explicitly instead of inferring it from + // having passed the guard. + when(ownershipValidator.isApprover(any())).thenReturn(true); String out = tools.getGroupApprovalStatus("g1", "gc1", "full"); assertTrue(out.contains("full"), out); assertFalse(out.contains("FORBIDDEN"), out); } + + @Test + void getGroupApprovalStatus_detailFull_pendingMemberWithoutRole_refused() throws Exception { + GroupConversation gc = mock(GroupConversation.class); + when(gc.getState()).thenReturn(GroupConversation.GroupConversationState.AWAITING_HUMAN_INPUT); + when(gc.getUserId()).thenReturn("someone-else"); + when(groupConversationService.readGroupConversation("gc1")).thenReturn(gc); + + String out = tools.getGroupApprovalStatus("g1", "gc1", "full"); + + assertTrue(out.contains("FORBIDDEN"), + "the pending member reads their prompt from the SUMMARY, never the transcript: " + out); + } }