refactor: Wave R — decompose GroupConversationService, AgentOrchestrator and ConversationService

docs/changelog.md

@@ -3,6 +3,67 @@
 > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review.
 
 
+---
+
+## 🔍 fix(groups): key-rotation-safe signature verification, plus review-comment cleanup (2026-08-01)
+
+**Repo:** EDDI (`claude/group-collaboration-plan-9bca77`)
+
+Before pushing the three R1 extraction commits, ran a thorough independent review: 5 parallel agents (AGENTS.md compliance, shallow bug scan, git blame/history context, prior-PR-comment context via `gh`, code-comment-vs-code consistency), each reading the diff fresh with no access to the extraction session's own reasoning. Zero regressions from the extraction itself — all three "pure move" claims held up to line-by-line, argument-by-argument scrutiny. Six real findings surfaced; five were doc/comment fixes, one was a genuine pre-existing bug worth fixing in place.
+
+**Real bug fixed — key-rotation-unsafe signature lookups in `GroupSigningGuard`.** Pre-existing on `main` (carried over unchanged by the pure-move extraction; originally flagged by CodeRabbit on PR #494, never fixed). Two call sites resolved a signer's public key via `AgentIdentity.getKeyValidAt(timestamp)` — "whichever key is valid right now" — instead of `getKeyForVersion(exactVersion)`, even though the exact key version used to sign is recorded and available in both cases:
+
+- **Self-verify at signing time** (`signOutgoingMessage`): could self-discard a just-created, perfectly good signature if the signing key's validity window doesn't yet cover "now" by the time self-verify runs.
+- **Peer verify on receipt** (`verifyPriorEntriesIfRequired`): during a rotation overlap window (both old and new key simultaneously valid — the exact scenario `AgentPublicKey`'s own Javadoc says the system is designed to support), every entry was verified against the *newest* valid key regardless of which key actually signed it. Worse, the per-speaker public-key cache was keyed by agent ID alone, so once the first entry from an agent resolved a (possibly wrong) key, every later entry from that same agent — even ones signed with a different key version — silently reused the same cached key without ever consulting its own `signatureKeyVersion`.
+
+Fixed both call sites to use `getKeyForVersion`; changed the verify-side cache key to `agentId#keyVersion` so entries signed with different key versions get independently resolved and cached. Added two regression tests (`GroupSigningGuardTest`) that construct an overlapping-validity two-key identity and assert (via `ArgumentCaptor`) the exact key material passed to `verifyEnvelope` at each call — mutation-checked by temporarily reverting the production fix (`git stash` on just that file) and confirming both new tests fail, and only those two, before restoring it.
+
+**Doc/comment fixes (no behavior change):** an inline fully-qualified name in `GroupSigningGuardTest` (AGENTS.md §4.4 — the only such occurrence across all six new files, everything else in the extraction was already clean); a stale "is now private" comment about `GroupContextBuilder.buildPlainTextFallback`, which is actually `public` (necessarily, for the cross-package delegator call); two Javadoc comments in `GroupConversationService` still naming `lastVerifiedIndex`, a field that moved entirely into `GroupSigningGuard` two commits ago; a `GroupSigningGuardTest` class-Javadoc claim that overstated which characterization suite exercises the signing happy path.
+
+**Two real findings deliberately NOT fixed here** — both pre-existing, both security-relevant, both genuinely delicate rather than mechanical: (1) `verifyPriorEntriesIfRequired` never consults `NonceCacheService` for replay detection, but the sender already calls `validate()` once at signing time (a mutating "mark as seen" op) — naively calling it again on the receive side would make every signature immediately register as "replayed," which is worse than today's gap; needs a non-mutating check method and a decision about what "replay" means on the receive side. (2) `requirePeerVerification=true` is audit-only — a failed verification only logs, the turn proceeds and the receiving agent gets the content anyway; fixing this is a product decision (fail the turn? quarantine the entry? configurable policy?) not a bug fix. Both filed as background follow-up tasks with full context rather than folded into this refactor PR.
+
+Full 15-class group suite green (470 + 12 + 17 + 14, including the 2 new regression tests); clean compile; formatter/Checkstyle clean.
+
+---
+
+## 🧩 refactor(groups): extract GroupSigningGuard from GroupConversationService (2026-08-01)
+
+**Repo:** EDDI (`claude/group-collaboration-plan-9bca77`)
+
+R1 step 3 of `planning/group-collaboration-improvements-plan.md` §3.1: moved the Ed25519 inter-agent signing cluster into a new `ai.labs.eddi.engine.internal.groups.GroupSigningGuard` — `verifyPriorEntriesIfRequired` (receiver-side incremental verification), the signing-creation block that was inline inside `executeAgentTurn` (sign → self-verify → nonce-validate, falling back to unsigned on any failure), and the `lastVerifiedIndex` cursor map that both share.
+
+The signing-creation block previously set four loose local variables (`signature`, `signatureNonce`, `signatureTimestampMs`, `signatureKeyVersion`) that fed straight into a `TranscriptEntry` constructor call; extracted as `signOutgoingMessage(...)` returning a `SigningResult` record (with an `UNSIGNED` singleton for the "not signed, for any reason" case — crypto infra absent, signing not configured, self-verification failed, nonce validation failed), destructured back into the same four constructor args at the call site. `verifyPriorEntriesIfRequired` and the two `lastVerifiedIndex.remove(...)` cleanup call sites (end of a discussion leg; terminal-state cleanup) became one-line delegations.
+
+Repeated the reflection sweep from step 2 before touching anything: `verifyPriorEntriesIfRequired` is reached via `GroupConversationServiceHitlCoverage3Test`'s reflection helper, so it stays a declared delegator on `GroupConversationService` (same pattern as step 2's seven methods). Added a focused `GroupSigningGuardTest` covering the guard-clause branches directly (12 tests) — the full sign → self-verify → nonce-validate happy path needs real Ed25519 key material and stays covered by the untouched characterization suites instead of being re-mocked here.
+
+Full 12-class group suite + all three new focused test classes: 470 + 12 + 17 + 12, all green; clean compile; formatter/Checkstyle clean. `GroupConversationService` is now 4,186 → 3,977 lines; 3 of R1's 10 extraction steps done.
+
+---
+
+## 🧩 refactor(groups): extract GroupContextBuilder from GroupConversationService (2026-08-01)
+
+**Repo:** EDDI (`claude/group-collaboration-plan-9bca77`)
+
+R1 step 2 of `planning/group-collaboration-improvements-plan.md` §3.1: moved the phase-input-construction and scope-filtering cluster (`buildPhaseInput`, `selectDefaultTemplate`, `filterByScope`, `findLatestResponse`, `mapPhaseToEntryType`, `extractResponse`, `buildPlainTextFallback` — ~280 lines) into a new `ai.labs.eddi.engine.internal.groups.GroupContextBuilder`, constructed once in `GroupConversationService`'s constructor (its only dependency, `templatingEngine`, is never reassigned post-construction, unlike the attachments step's field-injected `attachmentStore`).
+
+**A wrinkle this step surfaced that step 1 didn't:** four characterization test classes (`GroupConversationServiceTest`, `GroupConversationServiceHitlCoverage3Test`, `GroupConversationServiceUncoveredBranchTest`) reach several of these methods via `GroupConversationService.class.getDeclaredMethod(...)` reflection, which requires the method to be *declared directly on that class* — a delegator that's merely inlined at call sites doesn't satisfy it. All seven extracted methods are kept as thin private delegators on `GroupConversationService` for this reason (confirmed by an exhaustive grep for every `getDeclaredMethod("...")` / `method("...")` reflection lookup across the test package before deleting anything — one, `findLatestResponse`, is now reachable only via reflection since its one production call site moved into `GroupContextBuilder` too; left as documented dead-from-production-code, not deleted, since removing it would break the pinned characterization test).
+
+Added a new focused `GroupContextBuilderTest` (17 tests, direct construction, no reflection) alongside the untouched characterization suites, per the plan's rule 5. Full 12-class group suite + both new focused test classes: 470 original tests + 12 (step 1) + 17 (step 2), all green; clean compile; formatter/Checkstyle clean.
+
+---
+
+## 🧩 refactor(groups): extract GroupAttachmentBinder from GroupConversationService (2026-08-01)
+
+**Repo:** EDDI (`claude/group-collaboration-plan-9bca77`)
+
+Added `planning/group-collaboration-improvements-plan.md` — the Rev 2.1 implementation plan for group-conversation collaboration features (cost ceilings, convergence detection, voting, negotiation, standing teams, shared artifacts, and more), re-aligned against `main` post-merge of `e20d510a6`. It opens with a **Wave R refactoring workstream**: `GroupConversationService` (4,417 lines), `AgentOrchestrator` (2,725 lines) and `ConversationService` (2,698 lines) are decomposed into focused collaborator classes before any feature work lands, so the ~18 planned items don't pile onto three already-oversized files. Verified the plan's structural claims against the actual repo before starting: line counts, section-banner count (10), and the 12-class/470-test characterization net for `GroupConversationService` all matched exactly.
+
+**This commit is R1 step 1 of that plan** — the first, smallest extraction (the plan's own ordering: static/pure-move clusters first). Moved the attachment-handling cluster (`materializeAttachments`, `rehydrateAttachmentsFromStore`, `grantAndInjectAttachments` — previously ~112 lines inline in `GroupConversationService`) into a new `ai.labs.eddi.engine.internal.groups.GroupAttachmentBinder`, a plain class (not a CDI bean — see the plan's rule 3.0-4: the 12 existing test classes construct `GroupConversationService` directly, and `attachmentStore` is field-injected specifically to keep that compiling, so the extracted collaborator must not force a constructor-signature change). `GroupConversationService` now constructs a `GroupAttachmentBinder(attachmentStore, defaultTenantId)` per call site and delegates — a pure move, no logic changes.
+
+The dedicated `Attachments` nested test class (12 tests) moved from `GroupConversationServiceTest` to a new focused `GroupAttachmentBinderTest`, testing the extracted class directly instead of through the facade. Baselined the full 12-class/470-test `GroupConversationService*Test` suite before touching any code (all green; JaCoCo: 82% instruction / 72% branch on the class) — that is this refactor's regression budget going forward. Re-ran the same suite plus the new test class after the extraction: still all green, 470 tests total (458 in the `GroupConversationService` family + 12 in the new class), `./mvnw clean compile` clean, formatter + Checkstyle clean.
+
+**What's next:** R1 steps 2–10 (extract `GroupContextBuilder`, `GroupSigningGuard`, `MemberTurnExecutor`, `PhaseExecutionEngine`, `TaskForceEngine`, `GroupHitlCoordinator`, `GroupLifecycleOps`, then the graceful-shutdown wiring commit) per `planning/group-collaboration-improvements-plan.md` §3.1, each its own commit. R2 (`AgentOrchestrator` tool-source SPI) and R3 (`ConversationService` HITL split) follow. Full sequencing in the plan's §7 dependency graph.
+
 ---
 
 ## 🔎 test(configs): sweep the config JSON the ITs build inline, not just the files (2026-08-01)