Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name> within <d>") 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)
Expand Down
43 changes: 43 additions & 0 deletions docs/group-conversations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
<name> within <window>" — 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <em>discovers</em> while
Expand Down Expand Up @@ -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).
* <p>
* 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.
* <p>
* 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.
* <p>
* 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.
Expand All @@ -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 <name> within <d>") and move on.
*/
SKIP_TURN,
/** Cancel the discussion. */
ABORT
}

// --- Discussion Style ---
Expand Down
Loading
Loading