From 11d0812fb72012c2fb1d2dead0fd91b38dbee51c Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Fri, 7 Aug 2026 17:44:49 +0200 Subject: [PATCH 01/38] fix(llm): price ordinary model calls via task-level inputPricePer1M/outputPricePer1M (N1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AUDIT_COST summed cascadeCostUsd + toolCostUsd only, so a plain model call contributed $0 โ€” I1's group cost ceiling could never trip for ordinary members and REST served $0.00 as authoritative. Task-level prices (null = unpriced, $0 as before) now price non-cascade calls; the arithmetic lives once in TokenPricing, shared with the cascade path. Cascade turns keep pricing themselves โ€” key-presence discrimination prevents double counting. Negative prices fail at deployment. --- docs/changelog.md | 20 ++++++ docs/langchain.md | 1 + .../internal/groups/GroupCostLedger.java | 18 ++--- .../llm/impl/CascadeConfigValidator.java | 22 ++++-- .../llm/impl/CascadingModelExecutor.java | 14 +--- .../labs/eddi/modules/llm/impl/LlmTask.java | 25 ++++--- .../eddi/modules/llm/impl/TokenPricing.java | 53 ++++++++++++++ .../modules/llm/model/LlmConfiguration.java | 31 +++++++++ .../llm/impl/CascadeConfigValidatorTest.java | 30 ++++++++ .../llm/impl/LlmTaskAuditLedgerTest.java | 69 +++++++++++++++++++ 10 files changed, 245 insertions(+), 38 deletions(-) create mode 100644 src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java diff --git a/docs/changelog.md b/docs/changelog.md index fd9d4ffc60..dc769fada4 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,26 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## ๐Ÿ’ฐ fix(llm): N1 โ€” price ordinary model calls so the ledger's common case is no longer $0 (2026-08-07) + +**Repo:** EDDI (`fix/group-pre-feature-defects`) + +First of the three pre-feature defects from `planning/group-collaboration-NEXT.md` ยง2. `AUDIT_COST` was written from `cascadeCostUsd + toolCostUsd` only, so a plain model call โ€” no cascade, no priced tool โ€” contributed **$0.00**: I1's `maxCostPerDiscussion` ceiling could never trip for an ordinary group, and `memberCosts`/`totalCost` were served over REST as if authoritative. + +- **`LlmConfiguration.Task` gains `inputPricePer1M`/`outputPricePer1M`** โ€” same names and nullable semantics as the cascade fields (null = unpriced, contributes $0), so nothing changes for anyone not setting prices. Config-driven per Golden Rule 1: no hardcoded provider price table โ€” it would be wrong within weeks. +- **The pricing arithmetic now lives once, in `TokenPricing.cost()`.** `CascadingModelExecutor.computeCost` (stepโ†’cascade price resolution) and `LlmTask.accumulateAuditEvidence` (task-level prices for plain calls) both delegate โ€” the formula cannot drift between paths (ยง4.7 unification rule). +- **Precedence is explicit, not accidental:** `accumulateAuditEvidence` discriminates on the presence of the `cascadeCostUsd` metadata key, which the cascade branch always writes. A cascade run is priced by its steps alone; task-level prices apply only to non-cascade calls โ€” cascade steps may target entirely different models, so inheriting the task price would price the wrong model. (This matches the precedence concern pre-filed in `docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md`.) Pinned by a test with deliberately absurd task prices on a cascade turn. +- **Validation:** negative task-level prices fail deployment (`CascadeConfigValidator`, same new-field hard-error rationale as cascade pricing; the validator now also runs its task-level block for cascade-less tasks). +- **`GroupCostLedger`'s "Known gap (V1)" Javadoc** rewritten โ€” the gap is closed; `totalCost` remains a lower bound only for members whose configs carry no prices. + +**Tests** (`LlmTaskAuditLedgerTest`, `CascadeConfigValidatorTest`): plain priced legacy call accumulates the expected dollars; priced agent-mode turn sums token + tool cost; unpriced call still writes no cost key; cascade with absurd task prices set is priced by the cascade alone; negative task price throws at deploy. **Mutation-checked:** reverting the pricing line to `0.0` fails exactly the two new priced-plain-call tests and nothing else. + +The group-side chain (AUDIT_COST โ†’ `GroupCostLedger` โ†’ ceiling policies ABORT/SYNTHESIZE_NOW) was already pinned end-to-end by `GroupConversationServiceCostCeilingTest` with stubbed member cost; what could not exist before this fix โ€” a plain call producing nonzero AUDIT_COST โ€” is now the pinned link. + +**Files:** `LlmConfiguration.java`, `TokenPricing.java` (new), `CascadingModelExecutor.java`, `LlmTask.java`, `CascadeConfigValidator.java`, `GroupCostLedger.java`, `docs/langchain.md`, tests. + --- ## ๐Ÿ”€ merge: bring `origin/main` (PR #627 HITL request pinning) into the branch (2026-08-07) diff --git a/docs/langchain.md b/docs/langchain.md index 28817ccd36..c136982548 100644 --- a/docs/langchain.md +++ b/docs/langchain.md @@ -519,6 +519,7 @@ Both stored shapes therefore keep working: a config that sets only `streamingTim | `maxBudgetPerConversation` | number | Ceiling on accumulated **tool** cost per conversation, in USD. Records cost; only refuses calls when `enforceBudget` is on | (unlimited) | | `enforceBudget` | boolean | Refuse tool calls once `maxBudgetPerConversation` is passed. **Opt-in** โ€” a ceiling without it is report-only, and is named in a startup WARN | false (`eddi.tools.budget.enforce-by-default`) | | `toolPricing` | map | Per-call tool prices in USD. Keyed on the built-in slug (`{"websearch": 0.005}`) or on a single dispatch name (`{"searchNews": 0.01}`), which takes precedence โ€” so one operation can be priced apart from its siblings | (built-in defaults) | +| `inputPricePer1M` / `outputPricePer1M` | number | Token prices in USD per 1M tokens for this task's **model calls**, feeding the conversation's tracked cost (and any group cost ceiling). Applies to non-cascade calls; a [model cascade](model-cascade.md) prices its steps via its own fields of the same name. Negative values fail deployment | (unpriced โ€” $0) | | `enableToolCaching` | boolean | Cache tool results to reduce API calls | true | | `toolCacheScopes` | map | Per-tool cache partition: `user`/`conversation`/`global` | (all `user`) | | `defaultToolCacheScope` | string | Cache partition for tools without an override | `user` | diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java index cece3a8405..9a727ad3f6 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java @@ -22,17 +22,13 @@ * {@code memberCosts}/{@code totalCost} (Wave 0, F5), and enforces * {@code ProtocolConfig#maxCostPerDiscussion} against it (I1). *

- * Known gap (V1, confirmed): {@link MemoryKeys#AUDIT_COST} โ€” the value - * this class reads โ€” is written by {@code LlmTask.accumulateCost} from - * {@code cascadeCostUsd + toolCostUsd} only. There is no dollar price table for - * a non-cascade model completion anywhere in this codebase today - * ({@code LlmTask}'s own Javadoc says so directly), so a member's own turn - * contributes {@code $0} here unless the group's LLM config uses the - * multi-model cascade (priced) or the turn called a priced tool. This is a - * real, known undercount for the common case, not a bug in this class โ€” I1 is - * where model-call cost recording gets added, reusing whatever price source it - * settles on. Until then, {@code totalCost} is a lower bound, not the true - * total. + * Coverage note: {@link MemoryKeys#AUDIT_COST} โ€” the value this class + * reads โ€” sums the member conversation's LLM token spend (cascade step pricing, + * or the task-level {@code inputPricePer1M}/{@code outputPricePer1M} for plain + * calls) and its tracked tool cost. Pricing is config-driven with no built-in + * provider price table, so a member whose LLM config carries no prices still + * contributes {@code $0} of token cost here โ€” for such members + * {@code totalCost} remains a lower bound. *

* Both {@code memberCosts} and the recomputed {@code totalCost} are set, never * incremented by a delta โ€” each call replaces the member's entry with its diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java b/src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java index 8cb62ccd00..e937b2f236 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java @@ -17,9 +17,9 @@ import java.util.Map; /** - * Deploy-time validation for {@link ModelCascadeConfig}. Runs from - * {@code LlmTask.configure()} so misconfigurations surface at agent deployment - * rather than per-turn mid-conversation. + * Deploy-time validation for {@link ModelCascadeConfig} and the task-level + * pricing fields. Runs from {@code LlmTask.configure()} so misconfigurations + * surface at agent deployment rather than per-turn mid-conversation. *

* Backward compatibility: conditions that older releases tolerated at * load (they still failed/degraded at runtime exactly as before โ€” unknown @@ -53,13 +53,19 @@ static void validate(LlmConfiguration config) throws WorkflowConfigurationExcept } private static void validateTask(LlmConfiguration.Task task) throws WorkflowConfigurationException { + String taskId = task.getId() != null ? task.getId() : ""; + + // Task-level pricing is NEW (same rationale as the cascade pricing below: + // no stored config predating it can carry the field) โ€” hard error on a + // negative value, cascade or not. + requireNonNegativeTaskPrice(taskId, "inputPricePer1M", task.getInputPricePer1M()); + requireNonNegativeTaskPrice(taskId, "outputPricePer1M", task.getOutputPricePer1M()); + ModelCascadeConfig cascade = task.getModelCascade(); if (cascade == null || !cascade.isEnabled()) { return; } - String taskId = task.getId() != null ? task.getId() : ""; - List steps = cascade.getSteps(); if (steps == null || steps.isEmpty()) { // The executor throws at runtime for empty steps (as before) โ€” warn, don't @@ -160,6 +166,12 @@ private static void requireNonNegativePrice(String taskId, String what, Double p } } + private static void requireNonNegativeTaskPrice(String taskId, String what, Double price) throws WorkflowConfigurationException { + if (price != null && price < 0) { + throw new WorkflowConfigurationException("Invalid LLM task '" + taskId + "': " + what + " must be >= 0"); + } + } + private static void warn(String taskId, String message) { LOGGER.warnf("LLM task '%s': modelCascade โ€” %s.", taskId, message); } diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java b/src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java index da17494ab9..0c548c9b73 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java @@ -717,19 +717,7 @@ static double stepToolCost(Map responseMetadata) { static double computeCost(CascadeStep step, ModelCascadeConfig cascade, Map tokenUsage) { Double inPrice = step.getInputPricePer1M() != null ? step.getInputPricePer1M() : cascade.getInputPricePer1M(); Double outPrice = step.getOutputPricePer1M() != null ? step.getOutputPricePer1M() : cascade.getOutputPricePer1M(); - if (tokenUsage == null || (inPrice == null && outPrice == null)) { - return 0.0; - } - long inputTokens = asLong(tokenUsage.get("inputTokens")); - long outputTokens = asLong(tokenUsage.get("outputTokens")); - double cost = 0.0; - if (inPrice != null) { - cost += inputTokens / 1_000_000.0 * inPrice; - } - if (outPrice != null) { - cost += outputTokens / 1_000_000.0 * outPrice; - } - return cost; + return TokenPricing.cost(inPrice, outPrice, tokenUsage); } private static long asLong(Object value) { diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java b/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java index e13c39d71d..3a0765cab1 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java @@ -697,7 +697,7 @@ private void executeTask(IConversationMemory memory, Task task, IWritableConvers var modelNameData = dataFactory.createData(MemoryKeys.AUDIT_MODEL_NAME, modelName); currentStep.storeData(modelNameData); - accumulateAuditEvidence(currentStep, responseMetadata, toolTrace, task.getId()); + accumulateAuditEvidence(currentStep, responseMetadata, toolTrace, task); } // Store tool trace if available @@ -785,19 +785,26 @@ private void executeTask(IConversationMemory memory, Task task, IWritableConvers * plus every escalated cascade step and tool-loop iteration inside each. The * ledger has to report the turn's total, but {@code getLatestData} is * last-write-wins, so each contributor read-modify-writes rather than - * overwriting. Cost is the sum of the two dollar signals that actually exist: - * configured cascade LLM pricing and tracked tool cost. There is no token price - * table for non-cascade tasks, so those contribute tool cost only. + * overwriting. Cost sums the LLM token spend and the tracked tool cost. A + * cascade run prices its own steps and reports the total as + * {@code cascadeCostUsd}; a plain call is priced here from the task-level + * {@code inputPricePer1M}/{@code outputPricePer1M} โ€” the key presence + * discriminates, so cascade tokens are never priced twice. Unpriced configs + * contribute $0 exactly as before. */ private void accumulateAuditEvidence(IWritableConversationStep currentStep, Map responseMetadata, - List> toolTrace, String llmTaskId) { + List> toolTrace, Task task) { if (responseMetadata != null) { - if (responseMetadata.get("tokenUsage") instanceof Map tokenUsage) { + Map tokenUsage = responseMetadata.get("tokenUsage") instanceof Map tu ? tu : null; + if (tokenUsage != null) { accumulateTokenUsage(currentStep, tokenUsage); } - accumulateCost(currentStep, asDouble(responseMetadata.get("cascadeCostUsd")) + asDouble(responseMetadata.get("toolCostUsd"))); + double llmCostUsd = responseMetadata.containsKey("cascadeCostUsd") + ? asDouble(responseMetadata.get("cascadeCostUsd")) + : TokenPricing.cost(task.getInputPricePer1M(), task.getOutputPricePer1M(), tokenUsage); + accumulateCost(currentStep, llmCostUsd + asDouble(responseMetadata.get("toolCostUsd"))); } - accumulateToolCalls(currentStep, toolTrace, llmTaskId); + accumulateToolCalls(currentStep, toolTrace, task.getId()); } private void accumulateTokenUsage(IWritableConversationStep currentStep, Map delta) { @@ -1087,7 +1094,7 @@ private void executeResume(IConversationMemory memory, LlmConfiguration llmConfi // Known gap: the continuation's tokenUsage covers only the post-resume model // calls (see the comment above) โ€” the pre-pause segment is not recoverable // here, so a paused turn's ledger entry under-reports by that segment. - accumulateAuditEvidence(currentStep, responseMetadata, toolTrace, task.getId()); + accumulateAuditEvidence(currentStep, responseMetadata, toolTrace, task); } // Tool trace (mirror executeTask) diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java b/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java new file mode 100644 index 0000000000..0b20db6d3b --- /dev/null +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java @@ -0,0 +1,53 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.llm.impl; + +import java.util.Map; + +/** + * The single home of the configured-price token-cost arithmetic. Both dollar + * signals that price LLM token spend โ€” cascade steps + * ({@code CascadingModelExecutor.computeCost}) and plain task-level pricing + * ({@code LlmTask.accumulateAuditEvidence}) โ€” delegate here, so the formula + * cannot drift between the two paths. + *

+ * There is deliberately no built-in provider price table: prices are an + * agent-designer concern and belong in configuration, where they can track + * provider changes without a code release. + */ +final class TokenPricing { + + private TokenPricing() { + } + + /** + * Dollar cost of one call from its reported token usage and configured + * per-1M-token prices. A null price means "unpriced" and contributes 0; with + * both prices null (or no usage reported) the call is free. + * + * @param tokenUsage + * the {@code tokenUsage} metadata map carrying + * {@code inputTokens}/{@code outputTokens} as numbers + */ + static double cost(Double inputPricePer1M, Double outputPricePer1M, Map tokenUsage) { + if (tokenUsage == null || (inputPricePer1M == null && outputPricePer1M == null)) { + return 0.0; + } + long inputTokens = asLong(tokenUsage.get("inputTokens")); + long outputTokens = asLong(tokenUsage.get("outputTokens")); + double cost = 0.0; + if (inputPricePer1M != null) { + cost += inputTokens / 1_000_000.0 * inputPricePer1M; + } + if (outputPricePer1M != null) { + cost += outputTokens / 1_000_000.0 * outputPricePer1M; + } + return cost; + } + + private static long asLong(Object value) { + return value instanceof Number n ? n.longValue() : 0L; + } +} diff --git a/src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java b/src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java index 2f1548c7db..356f7ba229 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java +++ b/src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java @@ -314,6 +314,21 @@ public static class Task { */ private Map toolPricing; + /** + * Input price per 1M tokens in USD for this task's model calls. Null = + * unpriced, contributing $0 to the tracked conversation cost. Applies to + * non-cascade calls; a cascade run prices its steps via {@code modelCascade}'s + * own {@code inputPricePer1M}/{@code outputPricePer1M} (steps may target + * different models, so task-level prices are not inherited). + */ + private Double inputPricePer1M; + + /** + * Output price per 1M tokens in USD. Same semantics as + * {@link #inputPricePer1M}. + */ + private Double outputPricePer1M; + /** Enable cost tracking */ private Boolean enableCostTracking = true; @@ -775,6 +790,22 @@ public void setToolPricing(Map toolPricing) { this.toolPricing = toolPricing; } + public Double getInputPricePer1M() { + return inputPricePer1M; + } + + public void setInputPricePer1M(Double inputPricePer1M) { + this.inputPricePer1M = inputPricePer1M; + } + + public Double getOutputPricePer1M() { + return outputPricePer1M; + } + + public void setOutputPricePer1M(Double outputPricePer1M) { + this.outputPricePer1M = outputPricePer1M; + } + public Boolean getEnableCostTracking() { return enableCostTracking; } diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java index ac69ff61ff..02612147ca 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java @@ -50,6 +50,36 @@ private static ModelCascadeConfig enabled(List steps) { return c; } + @Test + @DisplayName("task-level pricing: a negative price is a hard error, even with no cascade at all") + void negativeTaskLevelPrice_throws() { + var task = new LlmConfiguration.Task(); + task.setId("t1"); + task.setType("openai"); + task.setInputPricePer1M(-1.0); + var ex = assertThrows(WorkflowConfigurationException.class, + () -> CascadeConfigValidator.validate(new LlmConfiguration(List.of(task)))); + assertTrue(ex.getMessage().contains("inputPricePer1M")); + + var task2 = new LlmConfiguration.Task(); + task2.setId("t1"); + task2.setType("openai"); + task2.setOutputPricePer1M(-0.5); + assertThrows(WorkflowConfigurationException.class, + () -> CascadeConfigValidator.validate(new LlmConfiguration(List.of(task2)))); + } + + @Test + @DisplayName("task-level pricing: valid or absent prices load fine without a cascade") + void taskLevelPrice_ok() { + var task = new LlmConfiguration.Task(); + task.setId("t1"); + task.setType("openai"); + task.setInputPricePer1M(1.25); + task.setOutputPricePer1M(0.0); + assertDoesNotThrow(() -> CascadeConfigValidator.validate(new LlmConfiguration(List.of(task)))); + } + @Test @DisplayName("disabled cascade โ€” never validated") void disabledCascade_ok() { diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java index 673e68644c..104971010e 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java @@ -312,6 +312,48 @@ void freeTurnWritesNoCost() throws Exception { assertNull(stored.get(MemoryKeys.AUDIT_COST)); } + @Test + @DisplayName("a plain priced model call accumulates its token cost โ€” the common case is no longer $0") + void plainPricedCallAccumulatesTokenCost() throws Exception { + when(agentOrchestrator.executeIfToolsEnabled(any(), any(), any(), any(), any(), any(), anyInt(), anyInt(), any())).thenReturn(null); + when(chatModel.chat(anyList())).thenReturn(response("answer", 1000, 500, 1500)); + + var t = task("taskA"); + t.setInputPricePer1M(1.0); + t.setOutputPricePer1M(2.0); + llmTask.execute(memory, new LlmConfiguration(List.of(t))); + + // 1000/1M * $1.00 + 500/1M * $2.00 + assertEquals(0.002, auditCost(), 1e-9, + "an ordinary model call with configured prices must reach the ledger โ€” I1's group ceiling reads this"); + } + + @Test + @DisplayName("a priced agent-mode turn sums its token cost with the tracked tool spend") + void plainPricedAgentModeSumsTokenAndToolCost() throws Exception { + agentReturns("done", new ArrayList<>(), + Map.of("tokenUsage", Map.of("inputTokens", 1000, "outputTokens", 500, "totalTokens", 1500), + "toolCostUsd", 0.005)); + + var t = task("taskA"); + t.setInputPricePer1M(1.0); + t.setOutputPricePer1M(2.0); + llmTask.execute(memory, new LlmConfiguration(List.of(t))); + + assertEquals(0.007, auditCost(), 1e-9, "token cost and tool cost are both real spend and must both be counted"); + } + + @Test + @DisplayName("an unpriced plain call still contributes exactly $0 โ€” no behaviour change without prices") + void unpricedPlainCallStaysFree() throws Exception { + when(agentOrchestrator.executeIfToolsEnabled(any(), any(), any(), any(), any(), any(), anyInt(), anyInt(), any())).thenReturn(null); + when(chatModel.chat(anyList())).thenReturn(response("answer", 1000, 500, 1500)); + + llmTask.execute(memory, new LlmConfiguration(List.of(task("taskA")))); + + assertNull(stored.get(MemoryKeys.AUDIT_COST), "null prices mean unpriced โ€” the cost key must stay absent"); + } + // ==================== cascade ==================== @Test @@ -348,6 +390,33 @@ void cascadeRunCostAndConfidenceRecorded() throws Exception { assertEquals(1000L, tokenUsage.get("inputTokens")); } + @Test + @DisplayName("a cascade run with task-level prices set is priced by the cascade alone โ€” never twice") + void cascadeIgnoresTaskLevelPricesSoTokensAreNeverPricedTwice() throws Exception { + var cascade = new ModelCascadeConfig(); + cascade.setEnabled(true); + cascade.setEvaluationStrategy("none"); + cascade.setInputPricePer1M(1.0); + cascade.setOutputPricePer1M(2.0); + var step = new CascadeStep(); + step.setType("openai"); + step.setTimeoutMs(5000L); + cascade.setSteps(List.of(step)); + + when(chatModel.chat(anyList())).thenReturn(response("cascade answer", 1000, 500, 1500)); + + var t = task("taskA"); + // Deliberately absurd task-level prices: if the plain-call pricing path also + // ran for a cascade turn, the assertion below would be off by dollars. + t.setInputPricePer1M(1000.0); + t.setOutputPricePer1M(1000.0); + t.setModelCascade(cascade); + llmTask.execute(memory, new LlmConfiguration(List.of(t))); + + assertEquals(0.002, auditCost(), 1e-9, + "cascade steps price themselves; task-level prices apply to plain calls only"); + } + /** * The cascade branch builds a FRESH response-metadata map and hand-copies * selected keys out of the winning step's metadata. It copied the validation From 89f2b15b4ef33dc0571755cc34057b495fcaff97 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Fri, 7 Aug 2026 17:50:54 +0200 Subject: [PATCH 02/38] fix(groups): sum nested-group cost per child discussion instead of overwriting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit accumulateNestedGroupCost keyed memberCosts by the GROUP member's agentId, but each turn spawns a fresh child discussion starting at totalCost 0 and the map records by replacement โ€” so only the last child's spend survived. Key by agentId:childConversationId: replacement stays idempotent per conversation and multiple children of one member sum. --- docs/changelog.md | 14 +++++++++++++ .../internal/groups/GroupCostLedger.java | 13 +++++++++++- .../internal/groups/GroupCostLedgerTest.java | 20 +++++++++++++++++++ .../groups/MemberTurnExecutorTest.java | 4 +++- 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index dc769fada4..5ceccc10c6 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,20 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## ๐Ÿงฎ fix(groups): nested-group cost is summed per child discussion, not overwritten (2026-08-07) + +**Repo:** EDDI (`fix/group-pre-feature-defects`) + +The N1 fold-in flagged in `planning/group-collaboration-NEXT.md` ยง4. `GroupCostLedger.accumulateNestedGroupCost` keyed `memberCosts` by the GROUP member's `agentId`, but every turn of a GROUP member spawns a **fresh child discussion** whose `totalCost` starts at 0 โ€” and the map records by replacement (idempotency invariant: one key = one conversation's cumulative cost). So child N's total replaced child Nโˆ’1's, only the last child's spend survived the re-sum, and the parent's ceiling checks ran against an undercount. + +Fix keeps the replacement invariant instead of breaking it with deltas: the attribution key is now `agentId:childConversationId` โ€” one key per child conversation, replacement stays idempotent, multiple children of the same member sum. A child without an id falls back to the plain agentId (old behaviour). No production reader indexes `memberCosts` by agentId โ€” the map is serialized whole. + +`MemberTurnExecutorTest.executeGroupMemberTurn_rollsUpChildDiscussionCost` pinned the old single-key shape and was updated to the new contract. New `GroupCostLedgerTest` case: two children ($1.00, $0.50) of one member total $1.50, not $0.50. **Mutation-checked:** reverting to the agentId key fails exactly that new test. + +**Files:** `GroupCostLedger.java`, `GroupCostLedgerTest.java`, `MemberTurnExecutorTest.java`. + --- ## ๐Ÿ’ฐ fix(llm): N1 โ€” price ordinary model calls so the ledger's common case is no longer $0 (2026-08-07) diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java index 9a727ad3f6..9e4d4b1454 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java @@ -87,12 +87,23 @@ public static void accumulateMemberCost(GroupConversation gc, String attribution * this group's attribution, whole โ€” the child's own {@code totalCost} already * reflects everything its own members accumulated (recursively, via this same * method for individual members). + *

+ * Keyed per child discussion, not per member: {@code memberCosts} + * records by replacement, which is only idempotent when one key means one + * conversation's cumulative cost. Every turn of a GROUP member spawns a fresh + * child discussion whose {@code totalCost} starts at 0, so keying by agentId + * alone replaced child Nโˆ’1's whole spend with child N's and only the last child + * survived the sum. The suffix restores the map's invariant; a child without an + * id (not yet persisted) falls back to the plain agentId. */ public static void accumulateNestedGroupCost(GroupConversation gc, GroupMember member, GroupConversation subConversation) { if (subConversation == null) { return; } - recordAndReSum(gc, member.agentId(), subConversation.getTotalCost()); + String attributionKey = subConversation.getId() != null + ? member.agentId() + ":" + subConversation.getId() + : member.agentId(); + recordAndReSum(gc, attributionKey, subConversation.getTotalCost()); } /** diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.java index c50b0b681a..5f1e76d90c 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.java @@ -172,6 +172,26 @@ void accumulateNestedGroupCost_rollsUpChildTotal() { assertEquals(1.23, gc.getTotalCost()); } + @Test + void accumulateNestedGroupCost_multipleChildDiscussions_sumRatherThanOverwrite() { + var gc = gc(); + // Two turns of the same GROUP member = two FRESH child discussions, each + // starting its own totalCost at 0. Keyed by agentId alone, child 2's $0.50 + // would replace child 1's $1.00 and the group would under-report by half. + var firstChild = gc(); + firstChild.setId("child-1"); + firstChild.setTotalCost(1.00); + var secondChild = gc(); + secondChild.setId("child-2"); + secondChild.setTotalCost(0.50); + + GroupCostLedger.accumulateNestedGroupCost(gc, AGENT_A, firstChild); + GroupCostLedger.accumulateNestedGroupCost(gc, AGENT_A, secondChild); + + assertEquals(1.50, gc.getTotalCost(), 1e-9, + "each child discussion is separate spend โ€” a member's later child must not erase its earlier one"); + } + @Test void accumulateNestedGroupCost_mixesWithIndividualMembers() { var gc = gc(); 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 0f9964eb85..7561193b0a 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 @@ -269,7 +269,9 @@ void executeGroupMemberTurn_rollsUpChildDiscussionCost() throws Exception { null, null); assertEquals("Nested answer", entry.content()); - assertEquals(0.42, gc.getMemberCosts().get("sub-group-1")); + // Attribution is keyed per child discussion (agentId:childId) so a member's + // later children add to โ€” rather than replace โ€” its earlier ones. + assertEquals(0.42, gc.getMemberCosts().get("sub-group-1:sub-gc-1")); assertEquals(0.42, gc.getTotalCost()); } } From 65b00b60003cff5dacd551783c8c98643eef3c0a Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Fri, 7 Aug 2026 18:06:27 +0200 Subject: [PATCH 03/38] fix(schema): default schemaVersion to a legacy sentinel, stamp current at creation (N3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A key-less stored document (every pre-F6 document in production) deserialized claiming CURRENT_SCHEMA_VERSION, so prepareForResume's migration ladder ran zero iterations on exactly the documents it exists for. The initialiser is now LEGACY_SCHEMA_VERSION (1) and the single creation point stamps CURRENT โ€” Jackson runs the no-arg constructor either way, so only the creation stamp can distinguish absent from current. Same split applied to ConversationMemorySnapshot, which was correct only by coincidence (CURRENT == floor == 1). Stale Javadoc in both migration registries corrected. --- docs/changelog.md | 19 ++++++++++ .../groups/model/GroupConversation.java | 15 +++++++- .../ConversationSchemaMigrations.java | 7 +++- .../internal/GroupConversationService.java | 3 ++ .../GroupConversationSchemaMigrations.java | 17 +++++---- .../memory/ConversationMemoryUtilities.java | 4 ++ .../model/ConversationMemorySnapshot.java | 15 +++++++- .../ConversationSchemaMigrationsTest.java | 18 +++++++++ .../GroupConversationServiceExtendedTest.java | 23 +++++++++++ ...GroupConversationSchemaMigrationsTest.java | 38 ++++++++++++++++--- .../ConversationMemoryUtilitiesTest.java | 16 ++++++++ 11 files changed, 159 insertions(+), 16 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 5ceccc10c6..80bfff626d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,25 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## ๐Ÿชœ fix(schema): N3 โ€” legacy documents now enter the migration ladder at the bottom (2026-08-07) + +**Repo:** EDDI (`fix/group-pre-feature-defects`) + +Second pre-feature defect from `planning/group-collaboration-NEXT.md` ยง2, filed by the round-5 review of #626. `GroupConversation.schemaVersion` was initialised to `CURRENT_SCHEMA_VERSION` (3). Every pre-F6 production document has no `schemaVersion` key, Jackson leaves the initialiser standing, so those documents loaded **claiming schema 3 while being version-1-shaped** โ€” and `prepareForResume`'s ladder ran `for (v = 3; v < 3; ...)`: zero iterations on exactly the documents it exists for. Fixed now, while it is free: no released build has ever written a versioned document, so nothing in production carries a wrong claim to migrate away from. + +- **Test first, watched it fail:** `documentWithoutVersionKey_claimsLegacyVersionNotCurrent` deserialises `{}` and asserts version 1 โ€” failed with `expected: <1> but was: <3>` before the fix, exactly the round-5 finding. This is the case none of the four previous review rounds' tests covered (they all *set* a version). +- **The fix is a split, not a re-initialisation:** new `LEGACY_SCHEMA_VERSION = 1` is the field initialiser (the version a key-less document claims โ€” it never moves), and the single creation point (`GroupConversationService.createGroupConversation`) stamps `CURRENT_SCHEMA_VERSION` explicitly. The initialiser alone cannot distinguish "absent" from "current" โ€” Jackson runs the no-arg constructor either way โ€” so the stamp must live at creation. +- **`ConversationMemorySnapshot` got the identical split** even though its `CURRENT` is still 1 (correct only by coincidence โ€” first bump to 2 would have silently re-created the group side's bug). Its stamp lives in `ConversationMemoryUtilities.getMemorySnapshot`, the one place snapshots are built from live memory. A pinning test asserts `LEGACY_SCHEMA_VERSION` stays 1 when `CURRENT` bumps. +- **(b) Stale Javadoc fixed:** `GroupConversationSchemaMigrations.MIGRATIONS` claimed `CURRENT_SCHEMA_VERSION` is 1 and "there is nothing yet to migrate from" โ€” it is 3, and the identity-default path has already been exercised twice. Rewritten (plus the single-conversation mirror and the stale test-class Javadoc). + +**Mutation-checked:** removing the creation stamp fails exactly `discuss_stampsCurrentSchemaVersionOnTheCreatedDocument` (a fresh document would persist claiming legacy). Deserialisation side proven by the redโ†’green cycle above. 133 tests across the six touched classes green. + +โš ๏ธ **Surefire note for the next session:** `-Dtest=ClassName` prints `Tests run: 0` for the parent of `@Nested` tests while the nested groups report under their `@DisplayName` โ€” read the run TOTAL, not the class line, before concluding nothing ran. + +**Files:** `GroupConversation.java`, `GroupConversationService.java`, `ConversationMemorySnapshot.java`, `ConversationMemoryUtilities.java`, both `*SchemaMigrations.java`, tests. + --- ## ๐Ÿงฎ fix(groups): nested-group cost is summed per child discussion, not overwritten (2026-08-07) 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 7e25568aff..29b2cf5d44 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 @@ -32,6 +32,19 @@ public class GroupConversation { * migration in {@code GroupConversationSchemaMigrations}. */ public static final int CURRENT_SCHEMA_VERSION = 3; + /** + * The version a stored document claims when its JSON carries no + * {@code schemaVersion} key โ€” i.e. it was written before F6 existed, which is + * every pre-F6 production document. Jackson runs the no-arg constructor and + * leaves the field initialiser standing either way, so the initialiser cannot + * distinguish "absent" from "current": it MUST be this legacy floor, and the + * single creation point ({@code GroupConversationService}) stamps + * {@link #CURRENT_SCHEMA_VERSION} explicitly. Initialising the field to CURRENT + * instead made key-less documents claim the current version and + * {@code prepareForResume}'s ladder run zero iterations on exactly the + * documents it exists for. + */ + public static final int LEGACY_SCHEMA_VERSION = 1; /** * The shape this specific document was last written in. Checked before a * resume: newer than {@link #CURRENT_SCHEMA_VERSION} refuses (this deployment @@ -41,7 +54,7 @@ public class GroupConversation { * per-resume config-drift check (bookmarked phase vs. current config) that * guards a different axis. See {@code GroupConversationSchemaMigrations}. */ - private int schemaVersion = CURRENT_SCHEMA_VERSION; + private int schemaVersion = LEGACY_SCHEMA_VERSION; private String id; private String groupId; private String userId; diff --git a/src/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.java b/src/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.java index 4ec900fc99..e6105b2d60 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.java +++ b/src/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.java @@ -33,8 +33,11 @@ private ConversationSchemaMigrations() { /** * Migration functions, keyed by the version they upgrade from. Empty - * today for the same reason as the group side's registry: version {@code 1} is - * the first that has ever existed. See + * today: on this surface version {@code 1} really is the only version that has + * ever existed. When bumping + * {@link ConversationMemorySnapshot#CURRENT_SCHEMA_VERSION} for the first time, + * note that key-less stored documents claim + * {@link ConversationMemorySnapshot#LEGACY_SCHEMA_VERSION} โ€” see * {@code GroupConversationSchemaMigrations#MIGRATIONS}'s Javadoc. */ private static final Map> MIGRATIONS = Map.of(); 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 a999ec46e0..a33deef131 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java @@ -1690,6 +1690,9 @@ private GroupConversation createGroupConversation(String groupId, String questio throws IResourceStore.ResourceStoreException { GroupConversation gc = new GroupConversation(); + // The field initialiser is the LEGACY sentinel so key-less stored documents + // read as legacy โ€” a genuinely new document must claim current explicitly. + gc.setSchemaVersion(GroupConversation.CURRENT_SCHEMA_VERSION); gc.setGroupId(groupId); gc.setUserId(userId); gc.setState(GroupConversationState.IN_PROGRESS); diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java index feddd2669e..4920431d58 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java @@ -28,13 +28,16 @@ private GroupConversationSchemaMigrations() { /** * Migration functions, keyed by the version they upgrade from (so * entry {@code N} takes a version-{@code N} document to version {@code N+1}). - * Empty today โ€” {@link GroupConversation#CURRENT_SCHEMA_VERSION} is {@code 1}, - * the first version that has ever existed, so there is nothing yet to migrate - * from. Every future Wave item that adds a resume-relevant field bumps - * {@code CURRENT_SCHEMA_VERSION} and registers its own entry here. A version - * hop with no registered entry defaults to identity (see - * {@link #prepareForResume}) โ€” the documented, common case for a bump whose new - * fields default correctly via Jackson without any transformation. + * Empty today: every bump so far ({@code 1โ†’2โ†’3}) was additive โ€” the new fields + * default correctly via Jackson, so those hops ride the identity fallback below + * rather than a registered entry. That fallback path has therefore already been + * exercised; do not read the empty map as "no version has ever changed". Every + * future Wave item that adds a resume-relevant field bumps + * {@link GroupConversation#CURRENT_SCHEMA_VERSION} and registers an entry here + * only when the hop actually needs a transformation. Documents stored without a + * {@code schemaVersion} key deserialize claiming + * {@link GroupConversation#LEGACY_SCHEMA_VERSION} and enter the ladder at the + * bottom. */ private static final Map> MIGRATIONS = Map.of(); diff --git a/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java b/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java index 7718105910..3d7eb174fc 100644 --- a/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java +++ b/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java @@ -64,6 +64,10 @@ public static ConversationMemorySnapshot convertConversationMemory(IConversation private static ConversationMemorySnapshot getMemorySnapshot(IConversationMemory conversationMemory) { var snapshot = new ConversationMemorySnapshot(); + // The field initialiser is the LEGACY sentinel so key-less stored documents + // read as legacy โ€” a snapshot built from live memory is current by + // definition and must say so explicitly. + snapshot.setSchemaVersion(ConversationMemorySnapshot.CURRENT_SCHEMA_VERSION); if (conversationMemory.getUserId() != null) { snapshot.setUserId(conversationMemory.getUserId()); diff --git a/src/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.java b/src/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.java index be5a772a0c..ff2488f715 100644 --- a/src/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.java +++ b/src/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.java @@ -26,13 +26,26 @@ public class ConversationMemorySnapshot { * resume path. */ public static final int CURRENT_SCHEMA_VERSION = 1; + /** + * The version a stored document claims when its JSON carries no + * {@code schemaVersion} key. Mirrors + * {@code GroupConversation#LEGACY_SCHEMA_VERSION} and exists for the same + * reason: Jackson leaves the field initialiser standing for key-less documents, + * so the initialiser must be the legacy floor and the creation path + * ({@code ConversationMemoryUtilities}) stamps {@link #CURRENT_SCHEMA_VERSION} + * explicitly. While {@code CURRENT} is also {@code 1} this is indistinguishable + * from initialising to CURRENT โ€” but only by coincidence, and the first bump to + * {@code 2} would silently re-create the group side's zero-iteration migration + * bug without this split. + */ + public static final int LEGACY_SCHEMA_VERSION = 1; /** * The shape this specific document was last written in. Checked before a * resume: newer than {@link #CURRENT_SCHEMA_VERSION} refuses (this deployment * predates the document), older runs registered migrations forward. See * {@code ConversationSchemaMigrations}. */ - private int schemaVersion = CURRENT_SCHEMA_VERSION; + private int schemaVersion = LEGACY_SCHEMA_VERSION; private String conversationId; private String agentId; private Integer agentVersion; diff --git a/src/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.java b/src/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.java index 128cc15275..9ed8f070f8 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.java @@ -5,6 +5,7 @@ package ai.labs.eddi.engine.internal; import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot; +import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*; @@ -26,6 +27,23 @@ private ConversationMemorySnapshot snapshot(int schemaVersion) { return snapshot; } + /** + * Pins the legacy sentinel under deserialization. Today {@code LEGACY} and + * {@code CURRENT} are both {@code 1}, so this cannot fail for the group side's + * reason yet โ€” it exists so the first bump of {@code CURRENT} to {@code 2} + * fails THIS test if someone re-couples the field initialiser to + * {@code CURRENT}, instead of silently re-creating the zero-iteration migration + * bug the group side shipped. + */ + @Test + void documentWithoutVersionKey_claimsLegacyVersion() throws Exception { + var legacy = new ObjectMapper().readValue("{}", ConversationMemorySnapshot.class); + + assertEquals(ConversationMemorySnapshot.LEGACY_SCHEMA_VERSION, legacy.getSchemaVersion()); + assertEquals(1, ConversationMemorySnapshot.LEGACY_SCHEMA_VERSION, + "the legacy floor is the first version that ever existed โ€” it never moves, even when CURRENT bumps"); + } + @Test void prepareForResume_currentVersion_noOp() { var snapshot = snapshot(ConversationMemorySnapshot.CURRENT_SCHEMA_VERSION); 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 b98e466852..44e44918ae 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java @@ -296,6 +296,29 @@ void discuss_failureWithListener_emitsGroupError() throws Exception { verify(listener).onGroupError(any(GroupConversationEventSink.GroupErrorEvent.class)); } + /** + * F6/N3: the field initialiser is the LEGACY sentinel (so key-less stored + * documents read as legacy), which means a freshly created document only claims + * the current schema because the creation path stamps it. Drop the stamp and + * every NEW document would persist claiming legacy shape. + */ + @Test + void discuss_stampsCurrentSchemaVersionOnTheCreatedDocument() throws Exception { + var cfg = config(DiscussionStyle.ROUND_TABLE, 1, + new GroupMember("a1", "Alice", 1, null)); + cfg.setModeratorAgentId("mod"); + setupStore(cfg); + stubAgent("a1", "Opinion"); + stubAgent("mod", "Synthesis"); + + service.discuss(GROUP_ID, QUESTION, USER_ID, 0, null); + + var created = ArgumentCaptor.forClass(GroupConversation.class); + verify(conversationStore).create(created.capture()); + assertEquals(GroupConversation.CURRENT_SCHEMA_VERSION, created.getValue().getSchemaVersion(), + "a new document must be stamped current at creation โ€” the initialiser deliberately is not"); + } + @Test void discuss_withNullListener_doesNotThrow() throws Exception { var cfg = config(DiscussionStyle.ROUND_TABLE, 1, diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.java index 0f77710050..5e7b2e072b 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.java @@ -6,16 +6,17 @@ import ai.labs.eddi.configs.groups.model.GroupConversation; import ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionException; +import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*; /** - * Tests for {@link GroupConversationSchemaMigrations} (Wave 0, F6). Per the - * plan's own testing instruction, fixture documents are constructed at - * artificial version numbers โ€” there is no real "older version" today, since - * {@link GroupConversation#CURRENT_SCHEMA_VERSION} ({@code 1}) is the first - * version that has ever existed. + * Tests for {@link GroupConversationSchemaMigrations} (Wave 0, F6). Fixture + * documents are constructed at explicit version numbers; the key-less fixtures + * go through Jackson because the field-initialiser semantics under + * deserialization are exactly what is under test โ€” every pre-F6 production + * document has no {@code schemaVersion} key. * * @author tests */ @@ -28,6 +29,33 @@ private GroupConversation gc(int schemaVersion) { return gc; } + /** + * The case that is every document in production: written before F6 existed, so + * the stored JSON has no {@code schemaVersion} key at all. Jackson runs the + * no-arg constructor and leaves the field initialiser standing โ€” so the + * initialiser IS the version such a document claims. It must be the legacy + * floor ({@code 1}), not {@code CURRENT_SCHEMA_VERSION}: a key-less document + * claiming current makes {@code prepareForResume}'s ladder run zero iterations + * on exactly the documents it exists for. + */ + @Test + void documentWithoutVersionKey_claimsLegacyVersionNotCurrent() throws Exception { + var legacy = new ObjectMapper().readValue("{}", GroupConversation.class); + + assertEquals(1, legacy.getSchemaVersion(), + "a stored document with no schemaVersion key predates F6 and must claim the legacy floor, " + + "so the migration ladder walks every hop instead of zero"); + } + + @Test + void documentWithoutVersionKey_isMigratedForwardOnResume() throws Exception { + var legacy = new ObjectMapper().readValue("{}", GroupConversation.class); + + var result = GroupConversationSchemaMigrations.prepareForResume(legacy); + + assertEquals(GroupConversation.CURRENT_SCHEMA_VERSION, result.getSchemaVersion()); + } + @Test void prepareForResume_currentVersion_noOpReturnsSameVersion() throws Exception { var gc = gc(GroupConversation.CURRENT_SCHEMA_VERSION); diff --git a/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.java b/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.java index 4d82508474..763b706be8 100644 --- a/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.java +++ b/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.java @@ -127,6 +127,22 @@ void preservesMetadata() { assertEquals(5, restored.getAgentVersion()); assertEquals("user-42", restored.getUserId()); } + + /** + * F6/N3: the snapshot field initialiser is the LEGACY sentinel so key-less + * stored documents read as legacy โ€” a snapshot built from live memory is + * current-shaped by definition and must be stamped so explicitly here. + */ + @Test + @DisplayName("a snapshot built from live memory is stamped with the current schema version") + void stampsCurrentSchemaVersion() { + var memory = new ConversationMemory("conv-id", "agent-1", 5, "user-42"); + + var snapshot = ConversationMemoryUtilities.convertConversationMemory(memory); + + assertEquals(ConversationMemorySnapshot.CURRENT_SCHEMA_VERSION, snapshot.getSchemaVersion(), + "a freshly built snapshot must claim current โ€” the initialiser deliberately claims legacy"); + } } // โ”€โ”€โ”€ convertSimpleConversationMemory โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ From b6c0eb4c4813ba120b9283e9835e3245b219e606 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Fri, 7 Aug 2026 18:26:56 +0200 Subject: [PATCH 04/38] feat(groups): transcript windowing for rendered member context (N2/I9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FULL/ANONYMOUS-scope phases re-fed the whole transcript to every member every turn (~quadratic prompt cost). contextWindow config bounds the rendered context: beyond maxRecentEntries the older entries collapse into a rolling summary โ€” extended incrementally at phase boundaries via the shared SummarizationService โ€” or a plain truncation marker when summarization is off, unconfigured, or failing (never blocks). ANONYMOUS keeps its own summary built from Anonymous-labelled input so it can never de-anonymize. Summarizer spend is attributed to the discussion's I1 cost ledger when priced. The stored transcript is never modified. --- docs/changelog.md | 18 ++ docs/group-conversations.md | 37 +++ planning/group-collaboration-NEXT.md | 17 +- .../groups/model/AgentGroupConfiguration.java | 72 +++++ .../groups/model/GroupConversation.java | 57 ++++ .../configs/groups/mongo/AgentGroupStore.java | 21 ++ .../internal/GroupConversationService.java | 17 + .../internal/groups/GroupContextBuilder.java | 259 +++++++++++++-- .../internal/groups/GroupCostLedger.java | 14 + .../internal/groups/PhaseExecutionEngine.java | 6 +- .../eddi/modules/llm/impl/TokenPricing.java | 8 +- .../GroupContextBuilderWindowingTest.java | 296 ++++++++++++++++++ .../groups/PhaseExecutionEngineTest.java | 11 +- 13 files changed, 796 insertions(+), 37 deletions(-) create mode 100644 src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.java diff --git a/docs/changelog.md b/docs/changelog.md index 80bfff626d..60bec4283a 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,24 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## ๐ŸชŸ feat(groups): N2/I9 โ€” transcript windowing for rendered member context (2026-08-07) + +**Repo:** EDDI (`fix/group-pre-feature-defects`) + +Third and last pre-feature item from `planning/group-collaboration-NEXT.md` ยง2 โ€” a live cost bug, not a new collaboration mode: FULL-scope phases re-fed the entire transcript to every member every turn (~quadratic prompt cost), and every queued Wave 2/3 item makes transcripts longer. Landed in `GroupContextBuilder` per the plan (`### I9`, plan line ~337). + +- **Config:** `AgentGroupConfiguration.ContextWindowConfig` (`contextWindow`) โ€” `enabled=false`, `maxRecentEntries=30`, `summarizeOverflow=true`, plus `llmProvider`/`llmModel` for the summarizer and optional `inputPricePer1M`/`outputPricePer1M` (I1 attribution, reusing N1's shared `TokenPricing`). Boolean-not-boolean for `summarizeOverflow` so an omitted JSON key means true; compact-constructor normalization in the `GroupTaskConfig` style. Save-time warn in `AgentGroupStore` when summarization is on but no model is named. +- **Rendering:** windowed `filterByScope` overload โ€” when the FULL/ANONYMOUS scope-filtered context exceeds the cap, older entries collapse into one leading "System" pseudo-entry: the rolling summary when it covers the omitted range, else the `[n earlier entries omitted]` truncation marker (the `summarizeOverflow=false` path and the failure fallback). The verbatim/summary split is the summary's **raw-transcript boundary**, so there is never a gap or duplication; between boundaries the tail may grow a few entries past the cap and the next boundary re-tightens. The stored transcript is never modified; signing verifies raw entries as before. +- **Summaries:** extended **incrementally at phase boundaries only** (`updateWindowSummary`, called from the discussion loop before each repeat โ€” never per member turn), previous summary + new slice, mirroring `ConversationSummarizer`'s self-correcting algorithm via the shared `SummarizationService` (unification rule). Failure or empty answer leaves stored state untouched: WARN, truncation fallback, next boundary catches up with a larger batch. Summarizer spend lands on the discussion ledger via `GroupCostLedger.recordSystemCost` keyed `system:summarizer:{variant}:{boundary}` (idempotent per extension, distinct extensions sum). +- **One deliberate deviation from the plan text:** the plan named a single `transcriptSummary`/`summaryUpToIndex` pair *and* required ANONYMOUS summarizer input to use "Anonymous" labels. One shared summary cannot satisfy both โ€” a FULL-built summary carries real names and would de-anonymize an ANONYMOUS phase through the back door. `GroupConversation` therefore carries a second, lazily-built pair (`anonymousTranscriptSummary`/`anonymousSummaryUpToIndex`); a group that never uses ANONYMOUS never pays for it. Fields are additive with correct defaults โ€” no `CURRENT_SCHEMA_VERSION` bump (a legacy document simply starts summarizing at its next boundary). +- **Wiring:** `PhaseExecutionEngine`'s three `buildPhaseInput` call sites pass `config.getContextWindow()` + gc; `SummarizationService` reaches the facade by the established field-injection pattern (null in direct-construction unit tests โ†’ truncation fallback). The I2 convergence judge's input is untouched (already bounded to two rounds); SYNTHESIS deliberately keeps the full picture. + +**Tests** (`GroupContextBuilderWindowingTest`, 13): boundary at exactly the cap (must exceed, not meet); truncation marker; summary + boundary-split tail (no gap/duplication); ANONYMOUS uses the anonymous summary and labels (the named FULL summary must not surface); incremental extension (second call sees previous summary + only the new slice); failure/empty-answer state untouched + rendering falls back; priced summarization reaches `totalCost`; no summarizer call outside its remit (wrong phase type/scope, below cap, summarization off); config normalization. **Mutation-checked:** re-feeding the whole prefix instead of the new slice fails exactly the incremental test. `PhaseExecutionEngineTest`'s input stub updated to the new 9-arg overload (same reasoning as its own comment: a shorter stub silently nulls every input). Full `engine.internal` suite: 1438 green; checkstyle clean. + +**Files:** `AgentGroupConfiguration.java`, `GroupConversation.java`, `GroupContextBuilder.java`, `PhaseExecutionEngine.java`, `GroupConversationService.java`, `GroupCostLedger.java`, `AgentGroupStore.java`, `TokenPricing.java` (now public), `docs/group-conversations.md`, `planning/group-collaboration-NEXT.md` (all three ยง2 items marked done), tests. + --- ## ๐Ÿชœ fix(schema): N3 โ€” legacy documents now enter the migration ladder at the bottom (2026-08-07) diff --git a/docs/group-conversations.md b/docs/group-conversations.md index 8f440db3bf..8c0a290181 100644 --- a/docs/group-conversations.md +++ b/docs/group-conversations.md @@ -155,6 +155,43 @@ tools. a second writer racing it would corrupt the state machine that decides what runs next. Filing is the only agent-side write. +## Transcript windowing + +A `FULL`- or `ANONYMOUS`-scope discussion phase re-feeds the transcript to every +member every turn, so a long discussion grows roughly quadratically in prompt +tokens. `contextWindow` bounds what each turn *renders* โ€” the stored transcript +is never modified, and signing verification still runs on the raw entries: + +```json +"contextWindow": { + "enabled": true, + "maxRecentEntries": 30, + "summarizeOverflow": true, + "llmProvider": "openai", + "llmModel": "gpt-4o-mini", + "inputPricePer1M": 0.15, + "outputPricePer1M": 0.60 +} +``` + +When the scope-filtered context exceeds `maxRecentEntries`, the older entries +collapse into a single leading block: a rolling **summary** (extended +incrementally at phase boundaries via the shared summarization service โ€” never +per member turn), or, with `summarizeOverflow: false`, a plain +`[n earlier entries omitted]` marker that costs no LLM call. A summarization +failure never blocks the discussion โ€” the turn renders with the truncation +marker and the next boundary catches up. + +- Applies to `FULL` and `ANONYMOUS` scopes only. The `ANONYMOUS` variant keeps + its own summary, built from the same `"Anonymous"` labels the scope filter + produces โ€” the summary can never de-anonymize a peer. +- `llmProvider`/`llmModel` name the summarizer; without them, overflow falls + back to the truncation marker (a save-time warning says so). +- The optional prices attribute the summarizer's own spend to the discussion's + cost ledger, where `maxCostPerDiscussion` sees it. +- The convergence judge's input is not windowed โ€” it is already bounded to the + last two rounds. + Both caps are enforced independently: `maxPerTurn` bounds a runaway single turn, `maxAgentAddedTasksPerDiscussion` bounds slow drift across a long discussion. The discussion cap counts only agent-filed tasks, so a large planned backlog does not diff --git a/planning/group-collaboration-NEXT.md b/planning/group-collaboration-NEXT.md index 3ed9ecf3f1..ced5e1b62e 100644 --- a/planning/group-collaboration-NEXT.md +++ b/planning/group-collaboration-NEXT.md @@ -5,7 +5,7 @@ > holds the **design** for each item and does not repeat status โ€” go there only for the one > item you are about to build, using the section pointers below. > -> **Last updated:** 2026-08-04 ยท after PR [#626](https://github.com/labsai/EDDI/pull/626) (`refactor/group-service-split`) +> **Last updated:** 2026-08-07 ยท after the `fix/group-pre-feature-defects` branch (N1 + N2/I9 + N3, ยง2 below โ€” all three done) --- @@ -21,7 +21,8 @@ builds on (F1โ€“F6), and shipped six feature items. | **Wave 0** โ€” F1 `LiveDiscussionRegistry`, F2 speaker-level `ResumePoint`, F3 `DecisionRecord`, F4 entry types + visibility, F5 cost ledger, F6 schema versioning | โœ… Done | | **Wave 1** โ€” I1 cost ceiling, I2 convergence + early exit, I3 structured verdicts, I4 abstention + minority report | โœ… Done | | **Wave 2** โ€” I5 agent-filed tasks, I7 recruitment + delegation timeout | โœ… Done | -| **Wave 2** โ€” I6, I8, I9, I14, I17 | โฌœ Queued (see ยง3) | +| **Pre-feature defects** โ€” N1 plain-call pricing, N2/I9 transcript windowing, N3 schema sentinel (ยง2) | โœ… Done (2026-08-07, `fix/group-pre-feature-defects`) | +| **Wave 2** โ€” I6, I8, I14, I17 | โฌœ Queued (see ยง3) | | **Wave 3** โ€” I10, I11, I12, I13, I18 | โฌœ Queued (see ยง3) | | **Excluded โ€” do not start** โ€” I15 cross-team process DAGs, I16 A2A remote members | โŒ Out of scope | @@ -32,7 +33,12 @@ tool surfaces โ†’ a `ToolSourceProvider`. --- -## 2. Do these three first +## 2. Do these three first โ€” โœ… ALL DONE (2026-08-07, branch `fix/group-pre-feature-defects`) + +> N1, N2 and N3 below all shipped on that branch, each mutation-checked; the ยง4 +> nested-group cost gap was folded into N1 as planned. The section is kept for +> the reasoning record โ€” do not redo any of it. Details in `docs/changelog.md` +> (entries of 2026-08-07). **Next up: ยง3's queue, starting with I17 / I14 / I8.** These are not new features. They are **defects in what already shipped**, and later items depend on them. Do them as one small PR before starting any Wave 2 feature. @@ -171,9 +177,8 @@ Recorded so they are not rediscovered. Fix opportunistically, or fold into a rel - **`decision_reached` never fires.** I3 writes `DecisionRecord`, but no producer calls the sink event, so verdicts reach REST/MCP and never SSE or Slack. `SlackGroupDiscussionListener.onDecisionReached` is dead code today. *(Natural fold-in: I14.)* -- **Nested-group cost is overwritten, not accumulated.** `accumulateNestedGroupCost` keys by - `agentId`, and each nested turn is a fresh child discussion starting at 0, so only the last - child's spend survives. *(Natural fold-in: N1.)* +- ~~**Nested-group cost is overwritten, not accumulated.**~~ โœ… Fixed with N1 (2026-08-07): + attribution is keyed per child discussion (`agentId:childId`), so children sum. - **`allowAbstention` on a SYNTHESIS phase** yields a `COMPLETED` discussion with no answer. - **Parallel-phase late entries are lost.** After the batch deadline a member finishing milliseconds late has its real entry replaced by `SKIPPED`. **Both obvious fixes were tried 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 40da7c9d0e..2e21acfb52 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,78 @@ public void setTaskListConfig(GroupTaskConfig taskListConfig) { this.taskListConfig = taskListConfig; } + /** + * Transcript windowing for rendered member context (I9). {@code null} means + * windowing is off โ€” every FULL/ANONYMOUS-scope turn renders the whole + * scope-filtered transcript, exactly as before the feature existed. + */ + private ContextWindowConfig contextWindow; + + public ContextWindowConfig getContextWindow() { + return contextWindow; + } + + public void setContextWindow(ContextWindowConfig contextWindow) { + this.contextWindow = contextWindow; + } + + /** + * Bounds what a FULL/ANONYMOUS-scope member turn renders from the transcript + * (I9). Without it, every such turn re-feeds the entire transcript to every + * member โ€” ~quadratic cost as the discussion grows. When the scope-filtered + * context exceeds {@link #maxRecentEntries}, the rendered context becomes + * {@code [summary of the older entries] + [the recent entries verbatim]} โ€” a + * derived view only; the stored transcript is never modified and signing + * verification still operates on the raw entries. + * + * @param enabled + * master switch, default off โ€” a config that does not opt in renders + * exactly as before + * @param maxRecentEntries + * how many of the newest scope-filtered entries stay verbatim. + * Non-positive falls back to {@link #DEFAULT_MAX_RECENT_ENTRIES} (an + * accidental 0 must not blank the whole context) + * @param summarizeOverflow + * {@code true} (default) compresses the older entries into a rolling + * summary via the shared summarization service; {@code false} + * replaces them with a plain "[n earlier entries omitted]" marker + * and never calls an LLM + * @param llmProvider + * provider for the summarization calls (e.g. "openai"). Required for + * summarization โ€” without it (or {@code llmModel}) the overflow + * falls back to the truncation marker with a warning + * @param llmModel + * model name for the summarization calls + * @param inputPricePer1M + * optional USD price per 1M input tokens for the summarizer's own + * calls, so their cost counts toward the discussion's I1 ledger. + * Null or negative = unpriced ($0), same semantics as the LLM task + * pricing fields + * @param outputPricePer1M + * optional USD price per 1M output tokens, same semantics + */ + public record ContextWindowConfig(boolean enabled, int maxRecentEntries, Boolean summarizeOverflow, + String llmProvider, String llmModel, + Double inputPricePer1M, Double outputPricePer1M) { + + public static final int DEFAULT_MAX_RECENT_ENTRIES = 30; + + /** + * Normalizes at the one choke point every reader passes through โ€” same shape as + * {@link GroupTaskConfig}. {@code summarizeOverflow} is a Boolean rather than a + * boolean so an omitted JSON key defaults to {@code true} instead of silently + * disabling summarization. + */ + public ContextWindowConfig { + if (maxRecentEntries <= 0) { + maxRecentEntries = DEFAULT_MAX_RECENT_ENTRIES; + } + summarizeOverflow = summarizeOverflow == null || summarizeOverflow; + inputPricePer1M = inputPricePer1M == null || inputPricePer1M < 0 ? null : inputPricePer1M; + outputPricePer1M = outputPricePer1M == null || outputPricePer1M < 0 ? null : outputPricePer1M; + } + } + /** * 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 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 29b2cf5d44..eeccc1a894 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 @@ -145,6 +145,63 @@ public int getRoundStartTranscriptIndex() { public void setRoundStartTranscriptIndex(int roundStartTranscriptIndex) { this.roundStartTranscriptIndex = roundStartTranscriptIndex; } + + /** + * Rolling summary of transcript entries {@code [0, summaryUpToIndex)} for + * FULL-scope windowed rendering (I9), extended incrementally at phase + * boundaries by {@code GroupContextBuilder.updateWindowSummary}. A derived view + * only โ€” the transcript itself is never modified. {@code null} until windowing + * first summarizes; legacy documents deserialize to exactly that state and + * simply start summarizing at the next boundary, so this is additive and needs + * no {@code CURRENT_SCHEMA_VERSION} bump. + */ + private String transcriptSummary; + /** Exclusive raw-transcript index {@link #transcriptSummary} covers through. */ + private int summaryUpToIndex; + /** + * The ANONYMOUS-scope twin of {@link #transcriptSummary}, built from + * "Anonymous"-labelled input so an ANONYMOUS phase's summary can never leak + * attribution (no de-anonymization). Kept separately rather than reusing the + * FULL summary, which contains real speaker names. + */ + private String anonymousTranscriptSummary; + /** + * Exclusive raw-transcript index {@link #anonymousTranscriptSummary} covers + * through. + */ + private int anonymousSummaryUpToIndex; + + public String getTranscriptSummary() { + return transcriptSummary; + } + + public void setTranscriptSummary(String transcriptSummary) { + this.transcriptSummary = transcriptSummary; + } + + public int getSummaryUpToIndex() { + return summaryUpToIndex; + } + + public void setSummaryUpToIndex(int summaryUpToIndex) { + this.summaryUpToIndex = summaryUpToIndex; + } + + public String getAnonymousTranscriptSummary() { + return anonymousTranscriptSummary; + } + + public void setAnonymousTranscriptSummary(String anonymousTranscriptSummary) { + this.anonymousTranscriptSummary = anonymousTranscriptSummary; + } + + public int getAnonymousSummaryUpToIndex() { + return anonymousSummaryUpToIndex; + } + + public void setAnonymousSummaryUpToIndex(int anonymousSummaryUpToIndex) { + this.anonymousSummaryUpToIndex = anonymousSummaryUpToIndex; + } private SharedTaskList taskList; /** Agents dynamically added during the discussion (recruited or created). */ private List dynamicMembers = new CopyOnWriteArrayList<>(); 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 04ff7972be..2a7b51d5ef 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 @@ -43,6 +43,7 @@ public IResourceStore.IResourceId create(AgentGroupConfiguration groupConfigurat HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); + warnOnSummarizerlessWindow(groupConfiguration); return super.create(groupConfiguration); } @@ -54,6 +55,7 @@ public Integer update(String id, Integer version, AgentGroupConfiguration groupC HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); + warnOnSummarizerlessWindow(groupConfiguration); return super.update(id, version, groupConfiguration); } @@ -103,6 +105,25 @@ static List moderatorlessPhaseNames(AgentGroupConfiguration groupConfigu .toList(); } + /** + * I9: a window that asks for summarization but names no summarizer model will + * silently degrade to the plain truncation marker at discussion time. Worth + * telling the author at save time, when they can still add + * {@code llmProvider}/{@code llmModel}. A warning, not a rejection โ€” the + * truncation fallback is well-defined behaviour, same warn-not-reject shape as + * {@link #warnOnModeratorlessPhases}. + */ + private void warnOnSummarizerlessWindow(AgentGroupConfiguration groupConfiguration) { + var window = groupConfiguration.getContextWindow(); + if (window == null || !window.enabled() || !Boolean.TRUE.equals(window.summarizeOverflow())) { + return; + } + if (window.llmProvider() == null || window.llmProvider().isBlank() || window.llmModel() == null || window.llmModel().isBlank()) { + LOGGER.warnf("Group '%s' enables contextWindow summarization but names no llmProvider/llmModel โ€” " + + "overflow will fall back to a plain truncation marker", groupConfiguration.getName()); + } + } + /** * I1: a {@code maxCostPerDiscussion} of zero or less would stop the very first * turn of every discussion this group ever runs โ€” almost certainly a mistake (a 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 a33deef131..58afc3a6fc 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java @@ -51,6 +51,7 @@ import ai.labs.eddi.engine.model.Deployment.Environment; import ai.labs.eddi.engine.runtime.IAgentFactory; import ai.labs.eddi.engine.runtime.internal.GracefulShutdownService; +import ai.labs.eddi.modules.llm.impl.SummarizationService; import ai.labs.eddi.modules.templating.ITemplatingEngine; import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.MeterRegistry; @@ -208,6 +209,15 @@ public class GroupConversationService implements IGroupConversationService { @Inject LiveDiscussionRegistry liveDiscussionRegistry; + /** + * Shared LLM summarization for I9 transcript windowing. Same field-injection + * pattern and null-safety as {@link #attachmentStore} โ€” {@code null} in the + * direct-construction unit tests, where an enabled window falls back to the + * plain truncation marker instead of summarizing. + */ + @Inject + SummarizationService summarizationService; + // In-node fast-fail guard for concurrent post-discussion operations // (follow-up, continue, close) on the same conversation: a second // operation on the same gcId is rejected rather than queued. The Set is @@ -726,6 +736,13 @@ public GroupConversation executeDiscussion(GroupConversation gc, AgentGroupConfi } } + // I9: extend the rolling window summary this repeat's FULL/ + // ANONYMOUS-scope turns will render with. At the boundary, never + // per member turn โ€” that per-turn re-feeding is the quadratic + // cost the window exists to stop. A no-op unless the window is + // enabled and the transcript outgrew it since the last extension. + contextBuilder.updateWindowSummary(gc, phase, config.getContextWindow(), summarizationService); + // 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. diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java index a418acdc4f..2c703b158a 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java @@ -5,17 +5,22 @@ package ai.labs.eddi.engine.internal.groups; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextScope; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextWindowConfig; 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.PhaseType; import ai.labs.eddi.configs.groups.model.DiscussionStylePresets; +import ai.labs.eddi.configs.groups.model.GroupConversation; import ai.labs.eddi.configs.groups.model.GroupConversation.TranscriptEntry; import ai.labs.eddi.configs.groups.model.GroupConversation.TranscriptEntryType; import ai.labs.eddi.engine.memory.ConversationOutputExtractor; import ai.labs.eddi.engine.memory.model.SimpleConversationMemorySnapshot; +import ai.labs.eddi.modules.llm.impl.SummarizationService; +import ai.labs.eddi.modules.llm.impl.TokenPricing; import ai.labs.eddi.modules.templating.ITemplatingEngine; import org.jboss.logging.Logger; +import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; @@ -51,6 +56,11 @@ public String buildPhaseInput(DiscussionPhase phase, GroupMember speaker, String return buildPhaseInput(phase, speaker, question, transcript, phaseIdx, target, null); } + public String buildPhaseInput(DiscussionPhase phase, GroupMember speaker, String question, List transcript, int phaseIdx, + GroupMember target, List allMembers) { + return buildPhaseInput(phase, speaker, question, transcript, phaseIdx, target, allMembers, null, null); + } + /** * @param allMembers * the full group roster, used only by {@code ARGUE}/{@code @@ -60,9 +70,16 @@ public String buildPhaseInput(DiscussionPhase phase, GroupMember speaker, String * the not-me filter, which is only correct for a 2-member debate โ€” * existing callers that never reach those two phase types are * unaffected either way. + * @param window + * I9 transcript windowing, applied to the FULL/ANONYMOUS + * scope-filtered context only. {@code null} disables windowing + * (every pre-I9 caller). + * @param gc + * the discussion carrying the rolling window summaries; only read + * when {@code window} is enabled */ public String buildPhaseInput(DiscussionPhase phase, GroupMember speaker, String question, List transcript, int phaseIdx, - GroupMember target, List allMembers) { + GroupMember target, List allMembers, ContextWindowConfig window, GroupConversation gc) { String template = phase.inputTemplate() != null ? phase.inputTemplate() @@ -79,7 +96,7 @@ public String buildPhaseInput(DiscussionPhase phase, GroupMember speaker, String // Phase-type specific variables switch (phase.type()) { case OPINION -> { - List> prev = filterByScope(transcript, phase.contextScope(), phaseIdx, speaker); + List> prev = filterByScope(transcript, phase.contextScope(), phaseIdx, speaker, window, gc); data.put("previousResponses", prev); } case CRITIQUE -> { @@ -299,29 +316,229 @@ private static Set debatingRoles(List allMembers) { } public List> filterByScope(List transcript, ContextScope scope, int currentPhaseIdx, GroupMember speaker) { + return filterByScope(transcript, scope, currentPhaseIdx, speaker, null, null); + } + + /** + * As {@link #filterByScope(List, ContextScope, int, GroupMember)}, windowed + * (I9): when {@code window} is enabled, the scope is {@code FULL} or + * {@code ANONYMOUS}, and the filtered context exceeds {@code maxRecentEntries}, + * the older entries collapse into a single leading pseudo-entry โ€” the rolling + * summary {@code gc} carries when one covers the omitted range, otherwise a + * plain "[n earlier entries omitted]" marker (the truncation fallback for + * {@code summarizeOverflow=false} and for summarizer failure). The recent + * entries render verbatim; the stored transcript is never modified. + */ + public List> filterByScope(List transcript, ContextScope scope, int currentPhaseIdx, GroupMember speaker, + ContextWindowConfig window, GroupConversation gc) { if (scope == null || scope == ContextScope.NONE) { return List.of(); } - return transcript.stream().filter(e -> e.content() != null && e.type() != TranscriptEntryType.ERROR && e.type() != TranscriptEntryType.SKIPPED - && e.type() != TranscriptEntryType.QUESTION).filter(e -> isVisibleToPeers(e, currentPhaseIdx)).filter(e -> switch (scope) { - case FULL -> true; - case LAST_PHASE -> e.phaseIndex() >= currentPhaseIdx - 1; - case ANONYMOUS -> true; // Content included, attribution stripped - case OWN_FEEDBACK -> speaker.agentId().equals(e.targetAgentId()); - case NONE -> false; - case TASK_ONLY, TASK_WITH_DEPS -> false; // Handled by task-specific logic - }).map(e -> { - Map entry = new LinkedHashMap<>(); - if (scope == ContextScope.ANONYMOUS) { - entry.put("speaker", "Anonymous"); - } else { - entry.put("speaker", e.speakerDisplayName()); - } - entry.put("content", e.content()); - entry.put("phaseName", e.phaseName() != null ? e.phaseName() : ""); - return entry; - }).collect(Collectors.toList()); + // Raw transcript indices ride along because the rolling summary's + // summaryUpToIndex is a raw-transcript boundary, not a filtered-list one. + List rawIndices = new ArrayList<>(); + List filtered = new ArrayList<>(); + for (int i = 0; i < transcript.size(); i++) { + TranscriptEntry e = transcript.get(i); + boolean included = e.content() != null && e.type() != TranscriptEntryType.ERROR && e.type() != TranscriptEntryType.SKIPPED + && e.type() != TranscriptEntryType.QUESTION && isVisibleToPeers(e, currentPhaseIdx) && switch (scope) { + case FULL -> true; + case LAST_PHASE -> e.phaseIndex() >= currentPhaseIdx - 1; + case ANONYMOUS -> true; // Content included, attribution stripped + case OWN_FEEDBACK -> speaker.agentId().equals(e.targetAgentId()); + case NONE -> false; + case TASK_ONLY, TASK_WITH_DEPS -> false; // Handled by task-specific logic + }; + if (included) { + rawIndices.add(i); + filtered.add(e); + } + } + + boolean windowed = window != null && window.enabled() && gc != null + && (scope == ContextScope.FULL || scope == ContextScope.ANONYMOUS) + && filtered.size() > window.maxRecentEntries(); + if (!windowed) { + return renderEntries(filtered, scope); + } + + String summary = scope == ContextScope.ANONYMOUS ? gc.getAnonymousTranscriptSummary() : gc.getTranscriptSummary(); + int summaryUpTo = scope == ContextScope.ANONYMOUS ? gc.getAnonymousSummaryUpToIndex() : gc.getSummaryUpToIndex(); + + if (summary != null && !summary.isBlank() && summaryUpTo > 0) { + // Verbatim tail = everything the summary does not cover. Split on the raw + // boundary rather than "last maxRecentEntries" so there is never a gap + // (entries neither summarized nor shown) and never duplication (entries + // both summarized and shown). Between phase boundaries the tail can grow + // a few entries past the cap; the next boundary's summary extension + // re-tightens it. + List verbatim = new ArrayList<>(); + int omitted = 0; + for (int i = 0; i < filtered.size(); i++) { + if (rawIndices.get(i) >= summaryUpTo) { + verbatim.add(filtered.get(i)); + } else { + omitted++; + } + } + if (omitted == 0) { + return renderEntries(filtered, scope); + } + List> result = new ArrayList<>(); + result.add(syntheticEntry("Summary of the earlier discussion:\n" + summary)); + result.addAll(renderEntries(verbatim, scope)); + return result; + } + + // Truncation fallback โ€” summarizeOverflow=false, no summarizer configured, + // or every summarization attempt so far failed. + int omitted = filtered.size() - window.maxRecentEntries(); + List> result = new ArrayList<>(); + result.add(syntheticEntry("[%d earlier entries omitted]".formatted(omitted))); + result.addAll(renderEntries(filtered.subList(omitted, filtered.size()), scope)); + return result; + } + + private static List> renderEntries(List entries, ContextScope scope) { + return entries.stream().map(e -> { + Map entry = new LinkedHashMap<>(); + if (scope == ContextScope.ANONYMOUS) { + entry.put("speaker", "Anonymous"); + } else { + entry.put("speaker", e.speakerDisplayName()); + } + entry.put("content", e.content()); + entry.put("phaseName", e.phaseName() != null ? e.phaseName() : ""); + return entry; + }).collect(Collectors.toList()); + } + + /** + * The windowing pseudo-entry. "System" as the speaker, never a member name โ€” + * under ANONYMOUS scope it must not read as an attribution. + */ + private static Map syntheticEntry(String content) { + Map entry = new LinkedHashMap<>(); + entry.put("speaker", "System"); + entry.put("content", content); + entry.put("phaseName", "(earlier discussion)"); + return entry; + } + + /** + * Summarization instructions for the I9 window summary. Tag-style constraint + * header rather than instruction prose, per the plan's research-adopted + * template note. + */ + static final String WINDOW_SUMMARY_INSTRUCTIONS = """ + Summarize the group discussion entries below for a participant who must continue the discussion. + Preserve: each speaker's position and key arguments, decisions reached, open disagreements, and task outcomes. + [format: bullet points] [limit: 500 tokens]"""; + + /** + * Extends the discussion's rolling window summary (I9), called from the + * discussion loop at phase boundaries only โ€” never per member turn, which is + * exactly the quadratic re-feeding this feature exists to stop. + *

+ * A no-op unless the upcoming phase is an OPINION phase with FULL or ANONYMOUS + * scope (the only context {@link #filterByScope} windows), the window is + * enabled with {@code summarizeOverflow=true}, and the transcript has outgrown + * {@code maxRecentEntries} beyond what the summary already covers. The + * extension is incremental: the summarizer is handed the previous summary plus + * only the new slice, mirroring {@code ConversationSummarizer}'s algorithm โ€” + * and like it, a failed call leaves the stored state untouched (WARN, never + * blocks the discussion), so rendering falls back to the truncation marker and + * the next boundary catches up with a larger batch. + *

+ * The ANONYMOUS variant is summarized from "Anonymous"-labelled input โ€” the + * same labels the scope filter itself produces โ€” so the stored anonymous + * summary can never de-anonymize a peer. Summarizer spend is attributed to the + * discussion's cost ledger (I1) when the window config carries prices. + */ + public void updateWindowSummary(GroupConversation gc, DiscussionPhase phase, ContextWindowConfig window, + SummarizationService summarizationService) { + if (window == null || !window.enabled() || !Boolean.TRUE.equals(window.summarizeOverflow()) + || gc == null || phase == null || phase.type() != PhaseType.OPINION) { + return; + } + ContextScope scope = phase.contextScope(); + if (scope != ContextScope.FULL && scope != ContextScope.ANONYMOUS) { + return; + } + boolean anonymous = scope == ContextScope.ANONYMOUS; + + List transcript; + synchronized (gc.getTranscript()) { + transcript = List.copyOf(gc.getTranscript()); + } + int coverThrough = transcript.size() - window.maxRecentEntries(); + int alreadyCovered = anonymous ? gc.getAnonymousSummaryUpToIndex() : gc.getSummaryUpToIndex(); + if (coverThrough <= alreadyCovered) { + return; + } + if (summarizationService == null || window.llmProvider() == null || window.llmModel() == null) { + LOGGER.warnf("Group %s: contextWindow.summarizeOverflow is on but no summarizer %s โ€” falling back to plain truncation", + gc.getId(), summarizationService == null ? "service is available" : "llmProvider/llmModel is configured"); + return; + } + + String newSlice = renderForSummarizer(transcript.subList(alreadyCovered, coverThrough), anonymous); + if (newSlice.isBlank()) { + return; + } + String previousSummary = anonymous ? gc.getAnonymousTranscriptSummary() : gc.getTranscriptSummary(); + String content = previousSummary != null && !previousSummary.isBlank() + ? "## Summary of entries 1-" + alreadyCovered + ":\n" + previousSummary + "\n\n## New entries:\n" + newSlice + : newSlice; + + try { + var result = summarizationService.summarizeWithUsage(content, WINDOW_SUMMARY_INSTRUCTIONS, + window.llmProvider(), window.llmModel()); + if (result.summary().isBlank()) { + LOGGER.warnf("Group %s: window summarization returned empty โ€” keeping previous state, will retry next boundary", gc.getId()); + return; + } + if (anonymous) { + gc.setAnonymousTranscriptSummary(result.summary()); + gc.setAnonymousSummaryUpToIndex(coverThrough); + } else { + gc.setTranscriptSummary(result.summary()); + gc.setSummaryUpToIndex(coverThrough); + } + // I1: the summarizer's own spend is discussion spend. Keyed per variant + // and boundary so a re-run of the same extension replaces (idempotent) + // while distinct extensions sum. + double cost = TokenPricing.cost(window.inputPricePer1M(), window.outputPricePer1M(), + Map.of("inputTokens", result.inputTokens(), "outputTokens", result.outputTokens())); + GroupCostLedger.recordSystemCost(gc, "system:summarizer:" + (anonymous ? "anon" : "full") + ":" + coverThrough, cost); + } catch (Exception e) { + LOGGER.warnf("Group %s: window summarization failed (%s) โ€” falling back to plain truncation for this window", gc.getId(), + e.getMessage()); + } + } + + /** + * Renders transcript entries as summarizer input. Applies the same content + * exclusions the peer-visibility matrix applies to rendered context โ€” + * bookkeeping a member never sees must not leak into a summary a member reads. + */ + private static String renderForSummarizer(List entries, boolean anonymous) { + var sb = new StringBuilder(); + for (TranscriptEntry e : entries) { + if (e.content() == null || e.type() == TranscriptEntryType.ERROR || e.type() == TranscriptEntryType.SKIPPED + || e.type() == TranscriptEntryType.QUESTION || e.type() == TranscriptEntryType.ABSTAINED + || e.type() == TranscriptEntryType.CONVERGENCE || e.type() == TranscriptEntryType.FACILITATION) { + continue; + } + String label = anonymous ? "Anonymous" : e.speakerDisplayName(); + sb.append("- ").append(label); + if (e.phaseName() != null) { + sb.append(" (").append(e.phaseName()).append(')'); + } + sb.append(": ").append(e.content()).append('\n'); + } + return sb.toString(); } /** diff --git a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java index 9e4d4b1454..b0c203c963 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java +++ b/src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java @@ -106,6 +106,20 @@ public static void accumulateNestedGroupCost(GroupConversation gc, GroupMember m recordAndReSum(gc, attributionKey, subConversation.getTotalCost()); } + /** + * Attribution for spend the discussion's own machinery incurs โ€” today the I9 + * transcript summarizer. {@code key} must be unique per priced operation (e.g. + * {@code "system:summarizer:full:42"}, suffixed with the boundary it covered + * through): the map records by replacement, so a re-run of the same operation + * is idempotent while distinct operations sum. + */ + public static void recordSystemCost(GroupConversation gc, String key, double cost) { + if (key == null || cost <= 0.0) { + return; + } + recordAndReSum(gc, key, cost); + } + /** * The pre-turn/pre-wave ceiling check (I1). If * {@code protocol.maxCostPerDiscussion()} is set and {@code gc.getTotalCost()} 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 b974d2e71d..6ca0d0bf58 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 @@ -511,7 +511,7 @@ public void executeSequentialPhase(GroupConversation gc, AgentGroupConfiguration new GroupConversationEventSink.SpeakerStartEvent(speaker.agentId(), speaker.displayName(), phaseIdx, phase.name())); } String input = contextBuilder.buildPhaseInput(phase, speaker, question, gc.getTranscript(), phaseIdx, null, - GroupConversationService.rosterWithRecruits(config, gc)); + GroupConversationService.rosterWithRecruits(config, gc), config.getContextWindow(), gc); TranscriptEntry entry = memberTurnExecutor.executeAgentTurn(speaker, gc, input, protocol, phaseIdx, phase, null, listener); gc.getTranscript().add(entry); if (listener != null) { @@ -586,7 +586,7 @@ public void executeParallelPhase(GroupConversation gc, AgentGroupConfiguration c .map(speaker -> CompletableFuture.supplyAsync(callerIdentityContext.withIdentitySupplying(phaseCaller, () -> { try { String input = contextBuilder.buildPhaseInput(phase, speaker, question, snapshotTranscript, phaseIdx, null, - GroupConversationService.rosterWithRecruits(config, gc)); + GroupConversationService.rosterWithRecruits(config, gc), config.getContextWindow(), gc); return memberTurnExecutor.executeAgentTurn(speaker, gc, input, protocol, phaseIdx, phase, null, listener, cancellation); } catch (MemberTurnCancelledException e) { // The orchestrator stopped waiting for this batch โ€” surface the @@ -715,7 +715,7 @@ public void executePeerTargetedPhase(GroupConversation gc, AgentGroupConfigurati new GroupConversationEventSink.SpeakerStartEvent(speaker.agentId(), speaker.displayName(), phaseIdx, phase.name())); } String input = contextBuilder.buildPhaseInput(phase, speaker, question, gc.getTranscript(), phaseIdx, target, - GroupConversationService.rosterWithRecruits(config, gc)); + GroupConversationService.rosterWithRecruits(config, gc), config.getContextWindow(), gc); TranscriptEntry entry = memberTurnExecutor.executeAgentTurn(speaker, gc, input, protocol, phaseIdx, phase, target.agentId(), listener); gc.getTranscript().add(entry); diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java b/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java index 0b20db6d3b..ffd1efe627 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java @@ -16,8 +16,12 @@ * There is deliberately no built-in provider price table: prices are an * agent-designer concern and belong in configuration, where they can track * provider changes without a code release. + *

+ * Public because the group transcript summarizer (I9, + * {@code GroupContextBuilder}) prices its own summarization calls with the same + * formula. */ -final class TokenPricing { +public final class TokenPricing { private TokenPricing() { } @@ -31,7 +35,7 @@ private TokenPricing() { * the {@code tokenUsage} metadata map carrying * {@code inputTokens}/{@code outputTokens} as numbers */ - static double cost(Double inputPricePer1M, Double outputPricePer1M, Map tokenUsage) { + public static double cost(Double inputPricePer1M, Double outputPricePer1M, Map tokenUsage) { if (tokenUsage == null || (inputPricePer1M == null && outputPricePer1M == null)) { return 0.0; } diff --git a/src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.java b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.java new file mode 100644 index 0000000000..e4e99987aa --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.java @@ -0,0 +1,296 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.internal.groups; + +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextScope; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ContextWindowConfig; +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.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.TranscriptEntry; +import ai.labs.eddi.configs.groups.model.GroupConversation.TranscriptEntryType; +import ai.labs.eddi.modules.llm.impl.SummarizationService; +import ai.labs.eddi.modules.llm.impl.SummarizationService.SummarizationResult; +import ai.labs.eddi.modules.templating.ITemplatingEngine; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import java.time.Instant; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * I9 โ€” transcript windowing in {@link GroupContextBuilder}: the windowed + * {@code filterByScope} rendering (summary pseudo-entry, truncation fallback, + * ANONYMOUS label preservation) and the phase-boundary + * {@code updateWindowSummary} extension (incremental slices, failure fallback, + * I1 cost attribution). + * + * @author tests + */ +class GroupContextBuilderWindowingTest { + + @Mock + private ITemplatingEngine templatingEngine; + @Mock + private SummarizationService summarizationService; + + private GroupContextBuilder builder; + + @BeforeEach + void setUp() { + MockitoAnnotations.openMocks(this); + builder = new GroupContextBuilder(templatingEngine); + } + + private GroupMember member() { + return new GroupMember("agent-a", "Agent A", 1, null); + } + + private DiscussionPhase opinionPhase(ContextScope scope) { + return new DiscussionPhase("Discussion", PhaseType.OPINION, "ALL", TurnOrder.SEQUENTIAL, scope, false, null, 1, false); + } + + private TranscriptEntry entry(String name, String content) { + return new TranscriptEntry("id-" + name, name, content, 0, "P", TranscriptEntryType.OPINION, Instant.now(), null, null); + } + + /** enabled, cap 3, summarize on, summarizer model configured, unpriced. */ + private ContextWindowConfig window() { + return new ContextWindowConfig(true, 3, true, "openai", "gpt-4o-mini", null, null); + } + + private GroupConversation gcWith(TranscriptEntry... entries) { + var gc = new GroupConversation(); + gc.setId("gc-1"); + for (var e : entries) { + gc.getTranscript().add(e); + } + return gc; + } + + // ================================================================= + // windowed filterByScope โ€” rendering + // ================================================================= + + @Test + @DisplayName("at exactly maxRecentEntries the context is untouched โ€” the cap must be exceeded, not met") + void atExactlyTheCap_noWindowing() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3")); + + var result = builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), window(), gc); + + assertEquals(3, result.size()); + assertTrue(result.stream().noneMatch(e -> "System".equals(e.get("speaker"))), "no synthetic entry at the boundary"); + } + + @Test + @DisplayName("over the cap with no summary yet: truncation marker + the recent entries verbatim") + void overTheCap_noSummary_truncationMarker() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + + var result = builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), window(), gc); + + assertEquals(4, result.size(), "marker + the 3 recent entries"); + assertEquals("System", result.get(0).get("speaker")); + assertEquals("[2 earlier entries omitted]", result.get(0).get("content")); + assertEquals("c3", result.get(1).get("content")); + assertEquals("c5", result.get(3).get("content")); + } + + @Test + @DisplayName("over the cap with a summary: summary pseudo-entry + everything past its boundary verbatim") + void overTheCap_withSummary_summaryPlusTail() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + gc.setTranscriptSummary("A and B agreed on X"); + gc.setSummaryUpToIndex(2); + + var result = builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), window(), gc); + + assertEquals(4, result.size(), "summary + the 3 entries past its boundary"); + assertEquals("System", result.get(0).get("speaker")); + assertTrue(result.get(0).get("content").toString().contains("A and B agreed on X")); + assertEquals("c3", result.get(1).get("content"), "the split is the summary's raw boundary โ€” no gap, no duplication"); + assertTrue(result.stream().skip(1).noneMatch(e -> "c1".equals(e.get("content")) || "c2".equals(e.get("content"))), + "summarized entries must not also render verbatim"); + } + + @Test + @DisplayName("ANONYMOUS scope windows with the anonymous summary and keeps Anonymous labels") + void anonymousScope_usesAnonymousSummaryAndLabels() { + var gc = gcWith(entry("Alice", "c1"), entry("Bob", "c2"), entry("Carol", "c3"), entry("Dave", "c4"), entry("Eve", "c5")); + gc.setTranscriptSummary("Alice said c1, Bob said c2"); // named โ€” must NOT surface + gc.setSummaryUpToIndex(2); + gc.setAnonymousTranscriptSummary("Two participants proposed X"); + gc.setAnonymousSummaryUpToIndex(2); + + var result = builder.filterByScope(gc.getTranscript(), ContextScope.ANONYMOUS, 1, member(), window(), gc); + + assertTrue(result.get(0).get("content").toString().contains("Two participants proposed X")); + assertFalse(result.get(0).get("content").toString().contains("Alice"), + "an ANONYMOUS phase must never render the named FULL summary โ€” that would de-anonymize"); + assertTrue(result.stream().skip(1).allMatch(e -> "Anonymous".equals(e.get("speaker")))); + } + + @Test + @DisplayName("a null or disabled window renders exactly as before the feature existed") + void disabledWindow_fullList() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + + assertEquals(5, builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), null, gc).size()); + var disabled = new ContextWindowConfig(false, 3, true, null, null, null, null); + assertEquals(5, builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), disabled, gc).size()); + } + + // ================================================================= + // updateWindowSummary โ€” the phase-boundary extension + // ================================================================= + + @Test + @DisplayName("first boundary summarizes exactly the overflow slice; the next extends with only the new slice") + void incrementalExtension_summarizerSeesOnlyTheNewSlice() { + var gc = gcWith(entry("A", "aaa-1"), entry("B", "bbb-2"), entry("C", "ccc-3"), entry("D", "ddd-4"), entry("E", "eee-5")); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), eq("openai"), eq("gpt-4o-mini"))) + .thenReturn(new SummarizationResult("SUMMARY-1", 10, 5)); + + builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), window(), summarizationService); + + var content = ArgumentCaptor.forClass(String.class); + verify(summarizationService).summarizeWithUsage(content.capture(), anyString(), eq("openai"), eq("gpt-4o-mini")); + assertTrue(content.getValue().contains("aaa-1") && content.getValue().contains("bbb-2"), + "the overflow slice [0, size-cap) is what gets summarized"); + assertFalse(content.getValue().contains("ccc-3"), "entries inside the recent window must not be summarized"); + assertEquals("SUMMARY-1", gc.getTranscriptSummary()); + assertEquals(2, gc.getSummaryUpToIndex()); + + // Discussion grows; the next boundary extends incrementally. + gc.getTranscript().add(entry("F", "fff-6")); + gc.getTranscript().add(entry("G", "ggg-7")); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), eq("openai"), eq("gpt-4o-mini"))) + .thenReturn(new SummarizationResult("SUMMARY-2", 10, 5)); + + builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), window(), summarizationService); + + verify(summarizationService, times(2)).summarizeWithUsage(content.capture(), anyString(), eq("openai"), eq("gpt-4o-mini")); + String secondContent = content.getValue(); + assertTrue(secondContent.contains("SUMMARY-1"), "the previous summary is the compressed form of the old slice"); + assertTrue(secondContent.contains("ccc-3") && secondContent.contains("ddd-4"), "only the new slice rides along verbatim"); + assertFalse(secondContent.contains("aaa-1"), "already-summarized entries must not be re-fed โ€” that is the incremental contract"); + assertEquals("SUMMARY-2", gc.getTranscriptSummary()); + assertEquals(4, gc.getSummaryUpToIndex()); + } + + @Test + @DisplayName("a summarizer failure leaves the stored state untouched โ€” rendering falls back to truncation") + void summarizerFailure_stateUntouched() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), anyString(), anyString())) + .thenThrow(new RuntimeException("provider down")); + + assertDoesNotThrow(() -> builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), window(), summarizationService), + "a summarization failure must never block the discussion"); + assertNull(gc.getTranscriptSummary()); + assertEquals(0, gc.getSummaryUpToIndex()); + + // And rendering degrades to the plain truncation marker, not an error. + var rendered = builder.filterByScope(gc.getTranscript(), ContextScope.FULL, 1, member(), window(), gc); + assertEquals("[2 earlier entries omitted]", rendered.get(0).get("content")); + } + + @Test + @DisplayName("an empty summarizer answer is not adopted โ€” the boundary index must not advance past unsummarized entries") + void emptySummary_notAdopted() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), anyString(), anyString())) + .thenReturn(new SummarizationResult("", 10, 5)); + + builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), window(), summarizationService); + + assertNull(gc.getTranscriptSummary()); + assertEquals(0, gc.getSummaryUpToIndex()); + } + + @Test + @DisplayName("ANONYMOUS boundary: the summarizer input carries Anonymous labels, never real names") + void anonymousBoundary_inputIsAnonymized() { + var gc = gcWith(entry("Alice", "c1"), entry("Bob", "c2"), entry("Carol", "c3"), entry("Dave", "c4"), entry("Eve", "c5")); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), anyString(), anyString())) + .thenReturn(new SummarizationResult("anon summary", 10, 5)); + + builder.updateWindowSummary(gc, opinionPhase(ContextScope.ANONYMOUS), window(), summarizationService); + + var content = ArgumentCaptor.forClass(String.class); + verify(summarizationService).summarizeWithUsage(content.capture(), anyString(), anyString(), anyString()); + assertFalse(content.getValue().contains("Alice") || content.getValue().contains("Bob"), + "no de-anonymization: the summarizer must not even SEE the names"); + assertTrue(content.getValue().contains("Anonymous")); + assertEquals("anon summary", gc.getAnonymousTranscriptSummary()); + assertNull(gc.getTranscriptSummary(), "the FULL variant is a separate summary and must stay untouched"); + } + + @Test + @DisplayName("a priced window attributes the summarizer's own spend to the discussion's I1 ledger") + void pricedSummarization_costReachesTheLedger() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + var priced = new ContextWindowConfig(true, 3, true, "openai", "gpt-4o-mini", 1.0, 2.0); + when(summarizationService.summarizeWithUsage(anyString(), anyString(), anyString(), anyString())) + .thenReturn(new SummarizationResult("S", 1000, 500)); + + builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), priced, summarizationService); + + // 1000/1M * $1.00 + 500/1M * $2.00 + assertEquals(0.002, gc.getTotalCost(), 1e-9, "summarization cost counts toward the discussion (I1)"); + } + + @Test + @DisplayName("no boundary work outside its remit: wrong phase type, wrong scope, below cap, or summarization off") + void noCallOutsideItsRemit() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + + var critique = new DiscussionPhase("Critique", PhaseType.CRITIQUE, "ALL", TurnOrder.SEQUENTIAL, ContextScope.FULL, false, null, 1, false); + builder.updateWindowSummary(gc, critique, window(), summarizationService); + builder.updateWindowSummary(gc, opinionPhase(ContextScope.LAST_PHASE), window(), summarizationService); + var noSummarize = new ContextWindowConfig(true, 3, false, "openai", "gpt-4o-mini", null, null); + builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), noSummarize, summarizationService); + var smallGc = gcWith(entry("A", "c1"), entry("B", "c2")); + builder.updateWindowSummary(smallGc, opinionPhase(ContextScope.FULL), window(), summarizationService); + + verifyNoInteractions(summarizationService); + } + + @Test + @DisplayName("no summarizer service (unit-test wiring) degrades to truncation instead of throwing") + void nullService_noThrow() { + var gc = gcWith(entry("A", "c1"), entry("B", "c2"), entry("C", "c3"), entry("D", "c4"), entry("E", "c5")); + + assertDoesNotThrow(() -> builder.updateWindowSummary(gc, opinionPhase(ContextScope.FULL), window(), null)); + assertNull(gc.getTranscriptSummary()); + } + + // ================================================================= + // config normalization + // ================================================================= + + @Test + @DisplayName("config normalization: non-positive cap falls back to the default, omitted summarizeOverflow means true") + void configNormalization() { + var window = new ContextWindowConfig(true, 0, null, null, null, -1.0, null); + + assertEquals(ContextWindowConfig.DEFAULT_MAX_RECENT_ENTRIES, window.maxRecentEntries(), + "an accidental 0 must not blank the whole context"); + assertEquals(Boolean.TRUE, window.summarizeOverflow(), "summarization is the default overflow behaviour"); + assertNull(window.inputPricePer1M(), "negative prices normalize to unpriced"); + } +} 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 07c4f1bd93..e5e535713a 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 @@ -53,11 +53,12 @@ class PhaseExecutionEngineTest { private PhaseExecutionEngine engine() { memberTurnExecutor = mock(MemberTurnExecutor.class); contextBuilder = mock(GroupContextBuilder.class); - // The 7-ARG overload โ€” the only one PhaseExecutionEngine calls. Stubbing the - // 6-arg one left every turn running with a null input, so a regression that - // dropped the phase rendering entirely and passed the raw question through - // would not have failed a single test here. - when(contextBuilder.buildPhaseInput(any(), any(), any(), any(), anyInt(), any(), any())) + // The 9-ARG overload (I9 added window + gc) โ€” the only one + // PhaseExecutionEngine calls. Stubbing a shorter one left every turn running + // with a null input, so a regression that dropped the phase rendering + // entirely and passed the raw question through would not have failed a + // single test here. + when(contextBuilder.buildPhaseInput(any(), any(), any(), any(), anyInt(), any(), any(), any(), any())) .thenReturn("rendered-input"); return new PhaseExecutionEngine(memberTurnExecutor, contextBuilder, Executors.newVirtualThreadPerTaskExecutor(), new CallerIdentityContext(null, null)); From 6aeba139374661c15b1e2459616bf88a5c70abfe Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Sat, 8 Aug 2026 00:46:45 +0200 Subject: [PATCH 05/38] feat(groups): shared artifacts - typed co-edited documents with CAS and validators (I17) Members co-edit typed documents (TEXT/MARKDOWN/JSON) through four tools gated by a new artifactConfig, instead of re-parsing each other's prose. Own collection (never embedded - the loop's stale-snapshot persists would clobber it); deterministic version CAS with a re-read-and-merge retry sentence, via a new numeric storeIfFieldEquals overload (Mongo's typed BSON equality never matches a number against a string); declarative validators (JSON Schema/regex/length) hard-checked at config save; writes announced as artifact_updated via a change queue the turn executor drains (tools have no listener reference); close/delete cascade + user-keyed GDPR erasure via a stamped ownerUserId. --- docs/changelog.md | 21 + docs/group-conversations.md | 40 ++ pom.xml | 7 + .../configs/groups/ArtifactValidators.java | 180 ++++++++ .../configs/groups/ISharedArtifactStore.java | 80 ++++ .../groups/model/AgentGroupConfiguration.java | 65 +++ .../groups/model/GroupConversation.java | 60 ++- .../configs/groups/model/SharedArtifact.java | 207 +++++++++ .../configs/groups/mongo/AgentGroupStore.java | 3 + .../groups/mongo/SharedArtifactStore.java | 252 +++++++++++ .../labs/eddi/datastore/IResourceStorage.java | 18 + .../datastore/mongo/MongoResourceStorage.java | 22 + .../postgres/PostgresResourceStorage.java | 10 + .../engine/api/IGroupConversationService.java | 2 + .../engine/api/IRestGroupConversation.java | 8 +- .../engine/gdpr/GdprComplianceService.java | 26 +- .../internal/GroupConversationService.java | 35 +- .../internal/RestGroupConversation.java | 6 + .../internal/groups/GroupLifecycleOps.java | 35 +- .../internal/groups/MemberTurnExecutor.java | 30 ++ .../lifecycle/GroupConversationEventSink.java | 20 + .../slack/SlackGroupDiscussionListener.java | 22 + .../modules/llm/impl/AgentOrchestrator.java | 9 + .../llm/impl/ArtifactToolsProvider.java | 105 +++++ .../modules/llm/tools/impl/ArtifactTools.java | 310 ++++++++++++++ .../groups/ArtifactValidatorsTest.java | 148 +++++++ .../groups/mongo/SharedArtifactStoreTest.java | 254 ++++++++++++ .../gdpr/GdprComplianceServiceTest.java | 44 +- .../groups/GroupLifecycleOpsTest.java | 55 +++ .../groups/MemberTurnExecutorTest.java | 43 ++ .../SlackGroupDiscussionListenerTest.java | 36 ++ .../llm/impl/ArtifactToolsProviderTest.java | 176 ++++++++ .../llm/tools/impl/ArtifactToolsTest.java | 392 ++++++++++++++++++ 33 files changed, 2710 insertions(+), 11 deletions(-) create mode 100644 src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java create mode 100644 src/main/java/ai/labs/eddi/configs/groups/ISharedArtifactStore.java create mode 100644 src/main/java/ai/labs/eddi/configs/groups/model/SharedArtifact.java create mode 100644 src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java create mode 100644 src/main/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProvider.java create mode 100644 src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java create mode 100644 src/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.java create mode 100644 src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java create mode 100644 src/test/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProviderTest.java create mode 100644 src/test/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactToolsTest.java diff --git a/docs/changelog.md b/docs/changelog.md index fd9d4ffc60..842a73df08 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,27 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## ๐Ÿ“„ feat(groups): I17 โ€” shared artifacts (blackboard-lite) (2026-08-08) + +**Repo:** EDDI (`feat/group-i17-shared-artifacts`) + +First Wave 2 queue item from `planning/group-collaboration-NEXT.md` ยง3. Agents can now **co-edit typed documents** instead of only talking: four member tools โ€” `createArtifact`, `readArtifact`, `proposeArtifactUpdate`, `listArtifacts` โ€” gated by a new `artifactConfig` on the group config. + +**Design decisions, per the plan (and the plan's own rejections honored):** + +- **Own collection, never embedded.** `SharedArtifact` + `ISharedArtifactStore`/`SharedArtifactStore` follow `GroupConversationStore`'s single-version runtime-document pattern. The discussion loop's whole-document stale-snapshot persists cannot clobber artifact writes, which is also why โ€” unlike I5's task tools โ€” the artifact tools write **through the store directly**. The live registry is still consulted: membership at assembly (`getForMember`, the caller-supplied-id IDOR guard), liveness at write time, and accepted writes ride a new transient change queue on the live `GroupConversation`. +- **Deterministic CAS-and-retry, explicitly not an LLM fusion arbiter.** The version CAS needed a storage primitive that doesn't exist for numbers: `storeIfFieldEquals(String)` text-compares, which "works" on Postgres (`data->>` renders JSON numbers as text) and **silently never matches on Mongo** (typed BSON equality). New `storeIfFieldEquals(โ€ฆ, long)` overload on `IResourceStorage` + both backends, same no-silent-degrade contract (the default throws). Stale writers get the plan's sentence: *"artifact changed since you read it (now vN); re-read and merge your change."* +- **Declarative validators only.** `JSON_SCHEMA` (new dependency `com.networknt:json-schema-validator` โ€” the victools libraries only *generate* schemas), `REGEX`, `MAX_LENGTH`. Specs hard-fail the config save (`ArtifactValidators.requireValidSpecs` from `AgentGroupStore`, `HitlConfigValidation`'s contract); write-time failures are rejection sentences and the gate fails closed on a broken spec. Content โ‰ค 256 KB. +- **Events without a listener reference:** tools can't fire SSE/Slack events (`ToolAssemblyContext` carries no listener โ€” the structural gap that left I5's planned `task_added_by_agent` unfired). Accepted writes queue an `ArtifactChange` on the live instance; `MemberTurnExecutor` drains the queue in a `finally` after every turn and fires the new `artifact_updated` event (sink constant + record + SSE forward + Slack line + OpenAPI description lists). Drained even with a null listener so the queue cannot grow unbounded. +- **Lifecycle:** artifacts are attached to the discussion status payload at read time in the service (so REST *and* MCP `read_group_conversation` carry them โ€” `availableActions` idiom, `READ_ONLY`, never trusted back from storage); close/delete cascade removes them (`GroupLifecycleOps`, warn-and-continue so a broken artifact store can't make discussions undeletable); GDPR erasure sweeps them **user-keyed** via a stamped `ownerUserId` (page/exact-recheck/fail-loud contract copied from the group store) as a new `GdprComplianceService` cascade step. +- **Caps:** `maxArtifactsPerDiscussion` (default 5) counted inside a `synchronized (liveInstance)` block โ€” creation is check-then-act and PARALLEL phases genuinely race; updates need no lock, the CAS decides. + +**Tests (148 across 8 classes, all green):** tools against a real in-memory CAS store (stale-version retry sentence with the CURRENT version, concurrent same-version writers โ†’ exactly one winner, FINAL freeze, foreign-discussion ids don't resolve, validator chain, refusals leave no side effect); provider gate matrix (every uncertainty โ†’ contribute nothing, membership not existence, `enableBuiltInTools` still applies); store CAS through the numeric overload with `verify(never()).store(โ€ฆ)`; anchored+escaped filters with Java exact-recheck; erasure paging/fail-loud; lifecycle cascade ordering (`inOrder` artifact-delete before document-delete) + cascade-failure-still-deletes; GDPR step + not-resolvable skip + failure-continues; turn-executor drain (exactly once, null-listener drain); Slack lines incl. degenerate-payload skip. **Mutation notes:** degrading the store CAS to an unconditional store does not even compile (the gone-document catch becomes unreachable) โ€” the CAS call is structurally load-bearing; the Mockito-verified negatives (`never().store`, `specs().isEmpty()`) pin the rest. + +**Files:** `SharedArtifact`, `ISharedArtifactStore`, `SharedArtifactStore`, `ArtifactValidators`, `ArtifactTools`, `ArtifactToolsProvider` (+ `AgentOrchestrator` phase-1 wiring), `AgentGroupConfiguration` (`ArtifactConfig`/`ArtifactValidator`/`ValidatorKind`), `GroupConversation` (change queue + read-time `artifacts`), `IResourceStorage` + Mongo/Postgres (numeric CAS), `GroupConversationEventSink`/listener/SSE/Slack, `GroupLifecycleOps`, `GroupConversationService`, `GdprComplianceService`, `AgentGroupStore`, `pom.xml`, `docs/group-conversations.md`, 8 test classes. + --- ## ๐Ÿ”€ 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 8f440db3bf..e013008747 100644 --- a/docs/group-conversations.md +++ b/docs/group-conversations.md @@ -160,6 +160,46 @@ 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. +## Shared artifacts (blackboard-lite) + +Without artifacts, the transcript is the only medium โ€” every structured thing an +agent produces is prose the next agent re-parses. `artifactConfig` gives members +four tools to **create together**: `createArtifact(name, type, content)`, +`readArtifact(nameOrId)`, `proposeArtifactUpdate(nameOrId, content, +expectedVersion, markFinal?)` and `listArtifacts()`. Artifacts are typed +documents (`TEXT`, `MARKDOWN`, `JSON`) in their own collection, listed on the +discussion's REST/MCP status payload as `artifacts`, and announced over SSE and +Slack as `artifact_updated` events. + +```json +"artifactConfig": { + "allowArtifactTools": true, + "maxArtifactsPerDiscussion": 5, + "validators": [ + { "kind": "JSON_SCHEMA", "spec": "{\"type\":\"object\",\"required\":[\"title\"]}" }, + { "kind": "MAX_LENGTH", "spec": "20000" } + ] +} +``` + +**Concurrency is deterministic compare-and-set, not an LLM merge.** Every update +presents the version it read; a stale writer is told *"artifact changed since +you read it (now v3); re-read and merge your change"* and retries against fresh +content. The failure mode is a retry, never a silent bad merge. + +**Validators are declarative only** โ€” `JSON_SCHEMA`, `REGEX` (content must +contain a match), `MAX_LENGTH` (characters) โ€” never code. Specs are checked at +config save time; at write time a failing validator refuses the write with its +message and stores nothing. Content is additionally capped at 256 KB per +artifact. + +Off by default with the same absence discipline as the task tools: no opt-in +means the tools are never assembled. The member agent's own +`enableBuiltInTools` switch still applies. `markFinal: true` freezes an +artifact โ€” FINAL artifacts accept no further updates. Artifacts are deleted +with their discussion (close/delete cascade) and by GDPR erasure; the durable +trace of the work is the transcript. + ## 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/pom.xml b/pom.xml index 08b6955d8c..6700ce946c 100644 --- a/pom.xml +++ b/pom.xml @@ -333,6 +333,13 @@ jsonschema-module-jackson 4.38.0 + + + com.networknt + json-schema-validator + 1.5.4 + jakarta.annotation jakarta.annotation-api diff --git a/src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java b/src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java new file mode 100644 index 0000000000..a87003de8c --- /dev/null +++ b/src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java @@ -0,0 +1,180 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups; + +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactConfig; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactValidator; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.networknt.schema.JsonSchema; +import com.networknt.schema.JsonSchemaFactory; +import com.networknt.schema.SpecVersion; +import com.networknt.schema.ValidationMessage; + +import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; +import java.util.stream.Collectors; + +/** + * The declarative artifact validation chain (I17) โ€” JSON Schema, regex and + * length checks a group config can gate artifact writes behind. Declarative + * only: there is deliberately no way to configure code execution here. + *

+ * Two entry points, one per failure audience: {@link #requireValidSpecs} runs + * at config save time and throws so a typo'd spec fails the save (the author's + * problem, at the moment they can fix it); {@link #firstRejection} runs at + * write time and returns a rejection sentence for the model (the write + * is refused, nothing is stored). + * + * @author ginccc + */ +public final class ArtifactValidators { + + /** Bound on schema violation messages quoted back to the model. */ + private static final int MAX_QUOTED_VIOLATIONS = 3; + + /** + * Validation-only mapper: parses candidate content and schema documents, never + * persists anything. + */ + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static final JsonSchemaFactory SCHEMA_FACTORY = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); + + private ArtifactValidators() { + } + + /** + * Save-time spec check: every validator must carry a kind and a spec its kind + * can actually use. Throws {@link IllegalArgumentException} (the same contract + * as {@code HitlConfigValidation}) so the config save fails with an actionable + * message rather than every future artifact write failing at runtime. + */ + public static void requireValidSpecs(ArtifactConfig config) { + if (config == null || config.validators().isEmpty()) { + return; + } + List validators = config.validators(); + for (int i = 0; i < validators.size(); i++) { + ArtifactValidator validator = validators.get(i); + String path = "artifactConfig.validators[" + i + "]"; + if (validator == null || validator.kind() == null) { + throw new IllegalArgumentException(path + " must name a kind (JSON_SCHEMA, REGEX or MAX_LENGTH)"); + } + String spec = validator.spec(); + if (spec == null || spec.isBlank()) { + throw new IllegalArgumentException(path + " (" + validator.kind() + ") needs a spec"); + } + switch (validator.kind()) { + case JSON_SCHEMA -> { + try { + SCHEMA_FACTORY.getSchema(spec); + } catch (Exception e) { + throw new IllegalArgumentException(path + " is not a valid JSON schema: " + e.getMessage()); + } + } + case REGEX -> { + try { + Pattern.compile(spec); + } catch (PatternSyntaxException e) { + throw new IllegalArgumentException(path + " is not a valid regex: " + e.getMessage()); + } + } + case MAX_LENGTH -> { + int max; + try { + max = Integer.parseInt(spec.trim()); + } catch (NumberFormatException e) { + throw new IllegalArgumentException(path + " (MAX_LENGTH) spec must be a positive integer, not '" + spec + "'"); + } + if (max <= 0) { + throw new IllegalArgumentException(path + " (MAX_LENGTH) must be > 0"); + } + } + } + } + } + + /** + * Write-time gate: runs the chain in config order and returns the first failure + * as a rejection sentence for the model, or {@code null} when every validator + * passes. A broken spec that slipped past save-time validation (hand-edited + * storage) rejects the write rather than admitting it โ€” the gate fails closed. + */ + public static String firstRejection(List validators, String content) { + if (validators == null || validators.isEmpty()) { + return null; + } + String candidate = content != null ? content : ""; + for (ArtifactValidator validator : validators) { + if (validator == null || validator.kind() == null || validator.spec() == null) { + return "This discussion's artifact validation is misconfigured; the write was refused."; + } + String rejection = switch (validator.kind()) { + case MAX_LENGTH -> checkMaxLength(validator.spec(), candidate); + case REGEX -> checkRegex(validator.spec(), candidate); + case JSON_SCHEMA -> checkJsonSchema(validator.spec(), candidate); + }; + if (rejection != null) { + return rejection; + } + } + return null; + } + + private static String checkMaxLength(String spec, String content) { + int max; + try { + max = Integer.parseInt(spec.trim()); + } catch (NumberFormatException e) { + return "This discussion's artifact length validator is misconfigured; the write was refused."; + } + if (content.length() > max) { + return "The content is %d characters, over this discussion's %d-character limit for artifacts. Shorten it." + .formatted(content.length(), max); + } + return null; + } + + private static String checkRegex(String spec, String content) { + try { + if (!Pattern.compile(spec, Pattern.DOTALL).matcher(content).find()) { + return "The content does not match this discussion's required pattern for artifacts. Expected to find a match of: " + spec; + } + } catch (PatternSyntaxException e) { + return "This discussion's artifact pattern validator is misconfigured; the write was refused."; + } + return null; + } + + private static String checkJsonSchema(String spec, String content) { + JsonSchema schema; + try { + schema = SCHEMA_FACTORY.getSchema(spec); + } catch (Exception e) { + return "This discussion's artifact schema validator is misconfigured; the write was refused."; + } + JsonNode node; + try { + node = MAPPER.readTree(content); + } catch (Exception e) { + return "The content must be valid JSON to pass this discussion's artifact schema, and it is not. Fix the JSON and retry."; + } + Set violations = schema.validate(node); + if (!violations.isEmpty()) { + String quoted = violations.stream() + .limit(MAX_QUOTED_VIOLATIONS) + .map(ValidationMessage::getMessage) + .collect(Collectors.joining("; ")); + String suffix = violations.size() > MAX_QUOTED_VIOLATIONS + ? " (and " + (violations.size() - MAX_QUOTED_VIOLATIONS) + " more)" + : ""; + return "The content does not satisfy this discussion's artifact schema: " + quoted + suffix + ". Fix it and retry."; + } + return null; + } +} diff --git a/src/main/java/ai/labs/eddi/configs/groups/ISharedArtifactStore.java b/src/main/java/ai/labs/eddi/configs/groups/ISharedArtifactStore.java new file mode 100644 index 0000000000..2f45189042 --- /dev/null +++ b/src/main/java/ai/labs/eddi/configs/groups/ISharedArtifactStore.java @@ -0,0 +1,80 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups; + +import ai.labs.eddi.configs.groups.model.SharedArtifact; +import ai.labs.eddi.datastore.IResourceStore; + +import java.util.List; + +/** + * Store for {@link SharedArtifact}s (I17) โ€” runtime documents in their own + * collection, one per co-edited artifact of a group discussion. Same + * single-version, DB-agnostic shape as {@link IGroupConversationStore}. + * + * @author ginccc + */ +public interface ISharedArtifactStore { + + /** + * Persists a new artifact and assigns its id. + * + * @return the new artifact's id + */ + String create(SharedArtifact artifact) throws IResourceStore.ResourceStoreException; + + SharedArtifact read(String id) throws IResourceStore.ResourceNotFoundException, IResourceStore.ResourceStoreException; + + /** + * Persists {@code artifact} only if the stored document's {@code version} still + * equals {@code expectedVersion} โ€” the deterministic CAS every accepted edit + * goes through. The caller applies the edit (which bumps the in-memory version) + * and presents the version it read. + * + * @throws IResourceStore.ResourceModifiedException + * lost the race โ€” someone else's edit landed first (retry after a + * fresh read) + * @throws ArtifactGoneException + * the artifact was deleted concurrently + */ + void updateIfVersion(SharedArtifact artifact, long expectedVersion) + throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException; + + void delete(String id) throws IResourceStore.ResourceStoreException; + + /** + * All artifacts of one discussion, oldest first. Bounded by the group config's + * {@code maxArtifactsPerDiscussion}, so no pagination surface. + */ + List listByGroupConversationId(String groupConversationId) throws IResourceStore.ResourceStoreException; + + /** + * Cascade delete for a closing/deleted discussion. + * + * @return how many artifacts were removed + */ + long deleteByGroupConversationId(String groupConversationId) throws IResourceStore.ResourceStoreException; + + /** + * GDPR erasure sweep: permanently removes every artifact whose + * {@code ownerUserId} is {@code userId}. Follows the group-conversation store's + * erasure contract โ€” pages until an empty pass, re-checks ownership by exact + * match in Java, and throws rather than reporting a partial erasure as success. + * + * @return how many artifacts were removed + */ + long deleteAllForUser(String userId) throws IResourceStore.ResourceStoreException; + + /** + * Unchecked "deleted concurrently" โ€” thrown by {@link #updateIfVersion} so CAS + * call sites can distinguish a retryable conflict (409) from a gone document + * (404) without a checked-exception cascade. + */ + class ArtifactGoneException extends RuntimeException { + public ArtifactGoneException(String message, Throwable cause) { + super(message, cause); + } + } +} 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 40da7c9d0e..73ea2a35b1 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 @@ -144,6 +144,71 @@ public GroupTaskConfig() { } } + /** + * Whether members may create and co-edit shared artifacts mid-discussion (I17). + * {@code null} means the artifact tools are absent entirely. + */ + private ArtifactConfig artifactConfig; + + public ArtifactConfig getArtifactConfig() { + return artifactConfig; + } + + public void setArtifactConfig(ArtifactConfig artifactConfig) { + this.artifactConfig = artifactConfig; + } + + /** + * Governs shared artifacts (I17, blackboard-lite): typed documents members + * co-edit through tools instead of re-parsing each other's prose. Same + * opt-in-by-absence discipline as {@link GroupTaskConfig}: off means the tools + * are never assembled, and there is no permissive standalone default. + * + * @param allowArtifactTools + * master switch, default off + * @param maxArtifactsPerDiscussion + * ceiling on artifacts per discussion (default 5). Non-positive + * falls back to the default โ€” 0 must never mean unlimited for an LLM + * write surface + * @param validators + * declarative validation chain every accepted write must pass โ€” + * {@link ValidatorKind#JSON_SCHEMA}, {@link ValidatorKind#REGEX} or + * {@link ValidatorKind#MAX_LENGTH} with a {@code spec}. Declarative + * only, never arbitrary code. Failed validation rejects the + * write with the validator's message; nothing is stored + */ + public record ArtifactConfig(boolean allowArtifactTools, int maxArtifactsPerDiscussion, List validators) { + + public static final int DEFAULT_MAX_ARTIFACTS = 5; + + /** Same normalization choke point as {@link GroupTaskConfig}. */ + public ArtifactConfig { + if (maxArtifactsPerDiscussion <= 0) { + maxArtifactsPerDiscussion = DEFAULT_MAX_ARTIFACTS; + } + validators = validators == null ? List.of() : List.copyOf(validators); + } + + /** Disabled, cap at its default, no validators. */ + public ArtifactConfig() { + this(false, DEFAULT_MAX_ARTIFACTS, List.of()); + } + } + + /** + * One declarative artifact validator (I17): {@code kind} selects the check, + * {@code spec} parameterizes it โ€” a JSON schema document, a regex the content + * must match, or a maximum character count. Specs are validated at save time so + * a typo fails the config save, not a member's turn. + */ + public record ArtifactValidator(ValidatorKind kind, String spec) { + } + + /** The closed set of declarative artifact validators (I17). */ + public enum ValidatorKind { + JSON_SCHEMA, REGEX, MAX_LENGTH + } + /** * A member of the group. Members can be individual agents or nested groups. *

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 7e25568aff..560c88b787 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 @@ -14,10 +14,12 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.Map; +import java.util.Queue; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; /** * Transcript record for a group conversation. Persisted with a single-version @@ -225,6 +227,62 @@ public void setRoundStartTranscriptIndex(int roundStartTranscriptIndex) { @JsonIgnore private transient AgentGroupConfiguration.ProtocolConfig.CostPolicy costCeilingOutcome; + /** + * One accepted artifact write (I17), queued by the artifact tools on the LIVE + * instance and drained by {@code MemberTurnExecutor} after the turn to fire the + * {@code artifact_updated} event. This indirection exists because tools have no + * listener reference โ€” {@code ToolAssemblyContext} carries none โ€” while the + * turn executor does. Transient and concurrent: PARALLEL phases run members + * (and so their tools) concurrently. + */ + public record ArtifactChange(String artifactId, String name, String type, long version, String editorAgentId, + String status, boolean created) { + } + + @JsonIgnore + private final transient Queue pendingArtifactChanges = new ConcurrentLinkedQueue<>(); + + /** Queues an accepted artifact write for the turn executor to announce. */ + @JsonIgnore + public void queueArtifactChange(ArtifactChange change) { + if (change != null) { + pendingArtifactChanges.add(change); + } + } + + /** Drains queued artifact writes โ€” each drained exactly once. */ + @JsonIgnore + public List drainArtifactChanges() { + List drained = new ArrayList<>(); + ArtifactChange change; + while ((change = pendingArtifactChanges.poll()) != null) { + drained.add(change); + } + return drained; + } + + /** + * The discussion's shared artifacts (I17), populated at READ time by + * {@code GroupConversationService.readGroupConversation} from the artifact + * store โ€” never persisted with this document (artifacts have their own + * collection; see {@link SharedArtifact}). Serialized when populated so REST's + * status payload and MCP's {@code read_group_conversation} both carry it; + * {@code READ_ONLY} because a stored copy must never be trusted back. Mirrors + * the {@code availableActions} idiom. + */ + @JsonIgnore + private transient List artifacts; + + @JsonProperty(value = "artifacts", access = JsonProperty.Access.READ_ONLY) + public List getArtifacts() { + return artifacts; + } + + @JsonIgnore + public void setArtifacts(List artifacts) { + this.artifacts = artifacts; + } + /** * A parent discussion's remaining cost budget at the moment it dispatched this * nested one (I1), or {@code null} for a top-level discussion (and for a parent diff --git a/src/main/java/ai/labs/eddi/configs/groups/model/SharedArtifact.java b/src/main/java/ai/labs/eddi/configs/groups/model/SharedArtifact.java new file mode 100644 index 0000000000..627b949943 --- /dev/null +++ b/src/main/java/ai/labs/eddi/configs/groups/model/SharedArtifact.java @@ -0,0 +1,207 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups.model; + +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; + +/** + * A typed document a group discussion's members create and co-edit through the + * artifact tools (I17, blackboard-lite) โ€” state that lives outside the + * dialogue, so a structured thing an agent produces stops being prose the next + * agent re-parses. + *

+ * Persisted in its own collection ({@code sharedartifacts}), never + * embedded in the {@link GroupConversation} document: the discussion loop + * persists that document whole from stale snapshots after each phase, which + * would silently clobber concurrent artifact writes. Because artifacts have + * their own collection, tools write them through the store directly โ€” the F1 + * live-instance rule that governs task-list writes does not apply here. + *

+ * Concurrency is deterministic compare-and-set on {@link #version} (see + * {@code ISharedArtifactStore.updateIfVersion}): a stale writer gets a "re-read + * and merge" rejection and retries โ€” explicitly not an LLM fusion + * arbiter, whose failure mode is a silent bad merge rather than a retry. + * + * @author ginccc + */ +public class SharedArtifact { + + /** + * Hard ceiling on {@link #content}, in UTF-8 bytes. An LLM handed a write tool + * can write in a loop; the cap bounds a single document while staying far above + * any deliberated draft. + */ + public static final int MAX_CONTENT_BYTES = 256 * 1024; + + /** + * How many prior revisions ride on the document. A bounded tail, oldest dropped + * โ€” full history is the transcript's job, this exists so a bad edit one or two + * turns back is recoverable without archaeology. + */ + public static final int HISTORY_CAP = 10; + + /** The artifact's content type โ€” how consumers should read {@link #content}. */ + public enum ArtifactType { + TEXT, MARKDOWN, JSON + } + + /** Editing lifecycle: DRAFT while being worked, FINAL once frozen. */ + public enum ArtifactStatus { + DRAFT, FINAL + } + + /** + * One superseded revision. {@code version} is the version this content carried + * while current. + */ + public record ArtifactRevision(String content, String editorAgentId, long version, Instant at) { + } + + private String id; + private String groupConversationId; + /** + * The owning discussion's {@code userId}, stamped at creation so GDPR erasure + * can sweep artifacts by user exactly like group conversations โ€” independent of + * whether the parent document still exists at erasure time. + */ + private String ownerUserId; + private String name; + private ArtifactType type; + private String content; + /** + * Monotonic edit counter, starting at 1 on creation โ€” the CAS token every + * update must present. Serialized as a JSON number; the store's version CAS + * uses the numeric {@code storeIfFieldEquals} overload for exactly that reason. + */ + private long version; + private String lastEditorAgentId; + private ArtifactStatus status = ArtifactStatus.DRAFT; + private List history = new ArrayList<>(); + private Instant createdAt; + private Instant updatedAt; + + /** + * Applies an accepted edit: archives the current content into {@link #history} + * (capped, oldest dropped), then installs the new content and bumps + * {@link #version}. Pure in-memory mutation โ€” persistence and the CAS happen at + * the store. + */ + public void applyEdit(String newContent, String editorAgentId, Instant at) { + history.add(new ArtifactRevision(content, lastEditorAgentId, version, updatedAt != null ? updatedAt : createdAt)); + while (history.size() > HISTORY_CAP) { + history.remove(0); + } + content = newContent; + lastEditorAgentId = editorAgentId; + version = version + 1; + updatedAt = at; + } + + /** + * UTF-8 byte length of {@code content}, for the {@link #MAX_CONTENT_BYTES} cap. + */ + public static int contentBytes(String content) { + return content == null ? 0 : content.getBytes(StandardCharsets.UTF_8).length; + } + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } + + public String getGroupConversationId() { + return groupConversationId; + } + + public void setGroupConversationId(String groupConversationId) { + this.groupConversationId = groupConversationId; + } + + public String getOwnerUserId() { + return ownerUserId; + } + + public void setOwnerUserId(String ownerUserId) { + this.ownerUserId = ownerUserId; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public ArtifactType getType() { + return type; + } + + public void setType(ArtifactType type) { + this.type = type; + } + + public String getContent() { + return content; + } + + public void setContent(String content) { + this.content = content; + } + + public long getVersion() { + return version; + } + + public void setVersion(long version) { + this.version = version; + } + + public String getLastEditorAgentId() { + return lastEditorAgentId; + } + + public void setLastEditorAgentId(String lastEditorAgentId) { + this.lastEditorAgentId = lastEditorAgentId; + } + + public ArtifactStatus getStatus() { + return status; + } + + public void setStatus(ArtifactStatus status) { + this.status = status; + } + + public List getHistory() { + return history; + } + + public void setHistory(List history) { + this.history = history != null ? history : new ArrayList<>(); + } + + public Instant getCreatedAt() { + return createdAt; + } + + public void setCreatedAt(Instant createdAt) { + this.createdAt = createdAt; + } + + public Instant getUpdatedAt() { + return updatedAt; + } + + public void setUpdatedAt(Instant updatedAt) { + this.updatedAt = updatedAt; + } +} 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 04ff7972be..d67134a3e1 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 @@ -5,6 +5,7 @@ package ai.labs.eddi.configs.groups.mongo; import ai.labs.eddi.configs.hitl.HitlConfigValidation; +import ai.labs.eddi.configs.groups.ArtifactValidators; import ai.labs.eddi.configs.groups.IAgentGroupStore; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.DiscussionPhase; @@ -41,6 +42,7 @@ public AgentGroupStore(IResourceStorageFactory storageFactory, IDocumentBuilder public IResourceStore.IResourceId create(AgentGroupConfiguration groupConfiguration) throws IResourceStore.ResourceStoreException { HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); + ArtifactValidators.requireValidSpecs(groupConfiguration.getArtifactConfig()); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); return super.create(groupConfiguration); @@ -52,6 +54,7 @@ public Integer update(String id, Integer version, AgentGroupConfiguration groupC throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { HitlConfigValidation.validate(groupConfiguration.getHitlConfig()); + ArtifactValidators.requireValidSpecs(groupConfiguration.getArtifactConfig()); normalizeNonPositiveCostCeiling(groupConfiguration); warnOnModeratorlessPhases(groupConfiguration); return super.update(id, version, groupConfiguration); diff --git a/src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java b/src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java new file mode 100644 index 0000000000..e0baf04bdf --- /dev/null +++ b/src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java @@ -0,0 +1,252 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups.mongo; + +import ai.labs.eddi.configs.groups.ISharedArtifactStore; +import ai.labs.eddi.configs.groups.model.SharedArtifact; +import ai.labs.eddi.datastore.IResourceFilter; +import ai.labs.eddi.datastore.IResourceStorage; +import ai.labs.eddi.datastore.IResourceStorageFactory; +import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.datastore.serialization.IDocumentBuilder; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.jboss.logging.Logger; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.regex.Pattern; + +/** + * DB-agnostic store for {@link SharedArtifact}s (I17). Mirrors + * {@link GroupConversationStore}'s single-version runtime-document shape (the + * {@code mongo} package name is historic โ€” this class is backend-neutral via + * {@link IResourceStorageFactory}). + *

+ * The version CAS goes through the numeric + * {@code storeIfFieldEquals(resource, "version", long)} overload: the + * artifact's {@code version} is a JSON number, and MongoDB's typed BSON + * equality would never match it against a string. + * + * @author ginccc + */ +@ApplicationScoped +public class SharedArtifactStore implements ISharedArtifactStore { + + private static final Logger LOGGER = Logger.getLogger(SharedArtifactStore.class); + + private static final int SINGLE_VERSION = 1; + + /** + * Both backends turn a String filter into an UNANCHORED regex โ€” ids must be + * validated before they are interpolated into one. Group-conversation ids are + * Mongo ObjectIds or UUIDs on the two backends, both within this set. + */ + private static final Pattern SAFE_ID = Pattern.compile("[A-Za-z0-9-]+"); + + /** Same spin bound and rationale as {@code GroupConversationStore}. */ + private static final int MAX_ERASURE_PASSES = 1_000; + + private final IResourceStorage storage; + + @Inject + public SharedArtifactStore(IResourceStorageFactory storageFactory, IDocumentBuilder documentBuilder) { + this.storage = storageFactory.create("sharedartifacts", documentBuilder, SharedArtifact.class, + "groupConversationId", "ownerUserId"); + } + + @Override + public String create(SharedArtifact artifact) throws IResourceStore.ResourceStoreException { + try { + IResourceStorage.IResource resource = storage.newResource(artifact); + storage.store(resource); + String id = resource.getId(); + artifact.setId(id); + return id; + } catch (IOException e) { + throw new IResourceStore.ResourceStoreException("Failed to create shared artifact: " + e.getMessage(), e); + } + } + + @Override + public SharedArtifact read(String id) throws IResourceStore.ResourceNotFoundException, IResourceStore.ResourceStoreException { + try { + IResourceStorage.IResource resource = storage.read(id, SINGLE_VERSION); + if (resource == null) { + // Deliberately does not embed the caller-supplied id โ€” reflected-value + // sink, same rule as GroupConversationStore.read. + throw new IResourceStore.ResourceNotFoundException("Shared artifact not found."); + } + SharedArtifact artifact = resource.getData(); + artifact.setId(id); + return artifact; + } catch (IOException e) { + throw new IResourceStore.ResourceStoreException("Failed to read shared artifact: " + e.getMessage(), e); + } + } + + @Override + public void updateIfVersion(SharedArtifact artifact, long expectedVersion) + throws IResourceStore.ResourceStoreException, IResourceStore.ResourceModifiedException { + try { + IResourceStorage.IResource resource = storage.newResource(artifact.getId(), SINGLE_VERSION, artifact); + storage.storeIfFieldEquals(resource, "version", expectedVersion); + } catch (IResourceStore.ResourceNotFoundException e) { + // Unchecked, so CAS call sites can tell "gone" (404) from a genuine + // version conflict (409) โ€” same shape as GroupConversationGoneException. + throw new ArtifactGoneException("Shared artifact no longer exists.", e); + } catch (IOException e) { + throw new IResourceStore.ResourceStoreException("Failed to update shared artifact: " + e.getMessage(), e); + } + } + + @Override + public void delete(String id) throws IResourceStore.ResourceStoreException { + try { + storage.removeAllPermanently(id); + } catch (Exception e) { + throw new IResourceStore.ResourceStoreException("Failed to delete shared artifact: " + e.getMessage(), e); + } + } + + @Override + public List listByGroupConversationId(String groupConversationId) throws IResourceStore.ResourceStoreException { + if (groupConversationId == null || !SAFE_ID.matcher(groupConversationId).matches()) { + return List.of(); + } + try { + var filter = new IResourceFilter.QueryFilters( + List.of(new IResourceFilter.QueryFilter("groupConversationId", "^" + groupConversationId + "$"))); + var resourceIds = storage.findResources( + new IResourceFilter.QueryFilters[]{filter}, "createdAt", 0, IResourceStorage.MAX_RESULT_LIMIT); + List artifacts = new ArrayList<>(); + for (var resourceId : resourceIds) { + try { + var resource = storage.read(resourceId.getId(), SINGLE_VERSION); + if (resource == null) { + continue; + } + SharedArtifact artifact = resource.getData(); + // The anchored regex only narrows; equality decides โ€” same + // narrow-then-recheck rule as every other cross-document filter. + if (!groupConversationId.equals(artifact.getGroupConversationId())) { + continue; + } + artifact.setId(resourceId.getId()); + artifacts.add(artifact); + } catch (IOException e) { + LOGGER.warnf("Skipping unreadable shared artifact %s: %s", resourceId.getId(), e.getMessage()); + } + } + return artifacts; + } catch (Exception e) { + throw new IResourceStore.ResourceStoreException("Failed to list shared artifacts: " + e.getMessage(), e); + } + } + + @Override + public long deleteByGroupConversationId(String groupConversationId) throws IResourceStore.ResourceStoreException { + if (groupConversationId == null || !SAFE_ID.matcher(groupConversationId).matches()) { + return 0; + } + long deleted = 0; + // maxArtifactsPerDiscussion bounds a discussion's artifacts far below one + // page, but the loop stays honest anyway: delete until a pass finds nothing. + for (int pass = 0; pass < MAX_ERASURE_PASSES; pass++) { + List artifacts = listByGroupConversationId(groupConversationId); + if (artifacts.isEmpty()) { + return deleted; + } + for (SharedArtifact artifact : artifacts) { + delete(artifact.getId()); + deleted++; + } + } + return deleted; + } + + @Override + public long deleteAllForUser(String userId) throws IResourceStore.ResourceStoreException { + if (userId == null || userId.isBlank()) { + return 0; + } + long deleted = 0; + var processed = new HashSet(); + try { + var filter = new IResourceFilter.QueryFilters( + List.of(new IResourceFilter.QueryFilter("ownerUserId", "^" + escapeRegex(userId) + "$"))); + + // Page until an empty pass, always from offset 0 (rows are removed as we + // go); fail loudly on an owned row that will not delete. Contract and + // reasoning identical to GroupConversationStore.deleteAllForUser. + for (int pass = 0; pass < MAX_ERASURE_PASSES; pass++) { + var resourceIds = storage.findResources( + new IResourceFilter.QueryFilters[]{filter}, "createdAt", 0, IResourceStorage.MAX_RESULT_LIMIT); + if (resourceIds == null || resourceIds.isEmpty()) { + return deleted; + } + + long newThisPass = 0; + long failedToDelete = 0; + for (var resourceId : resourceIds) { + if (!processed.add(resourceId.getId())) { + continue; + } + newThisPass++; + try { + var resource = storage.read(resourceId.getId(), SINGLE_VERSION); + if (resource == null) { + continue; + } + if (!userId.equals(resource.getData().getOwnerUserId())) { + // regex matched more than it should have โ€” never delete on it + LOGGER.warnf("Skipping shared artifact %s during erasure: ownerUserId is not an exact match", resourceId.getId()); + continue; + } + storage.removeAllPermanently(resourceId.getId()); + deleted++; + } catch (IOException e) { + failedToDelete++; + LOGGER.warnf("Failed to erase shared artifact %s: %s", resourceId.getId(), e.getMessage()); + } + } + + if (failedToDelete > 0) { + throw new IResourceStore.ResourceStoreException( + "Erasure incomplete: " + failedToDelete + " shared artifact(s) belonging to the user could not be deleted after " + + deleted + " successful deletion(s)"); + } + if (newThisPass == 0) { + return deleted; + } + } + LOGGER.warnf("Erasure stopped after %d passes; more shared artifacts may remain", MAX_ERASURE_PASSES); + } catch (IResourceStore.ResourceStoreException e) { + throw e; + } catch (Exception e) { + throw new IResourceStore.ResourceStoreException("Failed to delete shared artifacts for user: " + e.getMessage(), e); + } + return deleted; + } + + /** + * Backslash-escape the regex metacharacters shared by both backends' engines. + * Not {@link Pattern#quote}: its {@code \Q...\E} form is Java-specific and + * PostgreSQL rejects it. + */ + private static String escapeRegex(String value) { + StringBuilder sb = new StringBuilder(value.length() + 8); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if ("\\^$.|?*+()[]{}".indexOf(c) >= 0) { + sb.append('\\'); + } + sb.append(c); + } + return sb.toString(); + } +} diff --git a/src/main/java/ai/labs/eddi/datastore/IResourceStorage.java b/src/main/java/ai/labs/eddi/datastore/IResourceStorage.java index 000d933dcc..148d82c032 100644 --- a/src/main/java/ai/labs/eddi/datastore/IResourceStorage.java +++ b/src/main/java/ai/labs/eddi/datastore/IResourceStorage.java @@ -153,6 +153,24 @@ default void storeIfFieldEquals(IResource newResource, String fieldName, Stri + " โ€” a compare-and-swap must never silently degrade to an unconditional store"); } + /** + * As {@link #storeIfFieldEquals(IResource, String, String)}, but comparing a + * JSON number field. A separate overload because the two backends + * disagree about text-comparing numbers: PostgreSQL's {@code data ->> field} + * renders a JSON number as text so {@code "3"} matches, but MongoDB's typed + * BSON equality never matches an int64 against a string โ€” a string-typed CAS on + * a numeric field would "work" on one backend and silently never match on the + * other. Used by the shared-artifact store's version CAS (I17). + *

+ * Same no-fallback contract as the String overload. + */ + default void storeIfFieldEquals(IResource newResource, String fieldName, long expectedValue) + throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { + throw new UnsupportedOperationException( + "storeIfFieldEquals(long) is not implemented by " + getClass().getName() + + " โ€” a compare-and-swap must never silently degrade to an unconditional store"); + } + /** * Archive {@code history} and apply the version-checked update as ONE unit of * work. diff --git a/src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java b/src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java index b24fbbe88a..240997ead6 100644 --- a/src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java +++ b/src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java @@ -147,6 +147,28 @@ public void storeIfFieldEquals(IResource newResource, String fieldName, Strin } } + @Override + public void storeIfFieldEquals(IResource newResource, String fieldName, long expectedValue) + throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { + Resource resource = checkInternalResource(newResource); + // Typed BSON equality โ€” the String overload's Filters.eq(field, "3") never + // matches an int64 3, which is exactly why this overload exists. + var result = currentCollection.replaceOne( + Filters.and( + Filters.eq(ID_FIELD, new ObjectId(resource.getId())), + Filters.eq(fieldName, expectedValue)), + resource.getMongoDocument()); + if (result.getMatchedCount() == 0) { + long exists = currentCollection.countDocuments(Filters.eq(ID_FIELD, new ObjectId(resource.getId()))); + if (exists == 0) { + throw new IResourceStore.ResourceNotFoundException( + String.format("Resource no longer exists (id=%s)", resource.getId())); + } + throw new IResourceStore.ResourceModifiedException( + String.format("Resource field '%s' was not %d (id=%s)", fieldName, expectedValue, resource.getId())); + } + } + @Override public void createNew(IResource currentResource) { Resource resource = checkInternalResource(currentResource); diff --git a/src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java b/src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java index 5b65615891..69d58e1ba3 100644 --- a/src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java +++ b/src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java @@ -304,6 +304,16 @@ public void storeIfFieldEquals(IResource newResource, String fieldName, Strin } } + @Override + public void storeIfFieldEquals(IResource newResource, String fieldName, long expectedValue) + throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { + // data ->> field renders a JSON number as its canonical text ("3"), so on + // this backend the numeric CAS is a text comparison against the rendered + // number. The overload exists for MongoDB, whose typed BSON equality has + // no such coincidence โ€” see IResourceStorage's Javadoc. + storeIfFieldEquals(newResource, fieldName, String.valueOf(expectedValue)); + } + @Override public void createNew(IResource resource) { Resource pgResource = checkInternalResource(resource); 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 a5a4b77ce2..c6a9924d6d 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java @@ -210,6 +210,8 @@ default void onConvergenceChecked(GroupConversationEventSink.ConvergenceCheckedE } default void onConvergenceReached(GroupConversationEventSink.ConvergenceReachedEvent event) { } + default void onArtifactUpdated(GroupConversationEventSink.ArtifactUpdatedEvent 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 ac078b93b2..453bb2dce0 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java @@ -99,8 +99,8 @@ Response discuss(@PathParam("groupId") String groupId, @NotNull @Operation(summary = "Start a group discussion with SSE streaming", description = "Starts a group discussion asynchronously and streams progress events " + "(group_start, phase_start, speaker_start, speaker_complete, " - + "phase_complete, synthesis_start, group_complete, group_error) " - + "via Server-Sent Events.") + + "phase_complete, synthesis_start, group_complete, group_error, " + + "artifact_updated) via Server-Sent Events.") @APIResponse(responseCode = "200", description = "SSE event stream of discussion progress.") @APIResponse(responseCode = "400", description = "Missing/blank or oversized 'question', or an oversized attachment.") @APIResponse(responseCode = "404", description = "Group not found.") @@ -183,8 +183,8 @@ Response continueDiscussion(@PathParam("groupId") String groupId, description = "Re-run all discussion phases with SSE event streaming for progress. " + "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: " + + "synthesis_start, group_complete, group_error, artifact_updated), plus the HITL " + + "events (awaiting_approval, hitl_resume, cancelled, member_pause_skipped). 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.") diff --git a/src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java b/src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java index e4eae91c5f..8dd591c6c7 100644 --- a/src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java +++ b/src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java @@ -4,6 +4,7 @@ */ package ai.labs.eddi.engine.gdpr; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; import ai.labs.eddi.configs.groups.mongo.GroupConversationStore; import ai.labs.eddi.configs.properties.IUserMemoryStore; import ai.labs.eddi.configs.properties.model.Property; @@ -62,6 +63,7 @@ public class GdprComplianceService { private final IConversationDescriptorStore conversationDescriptorStore; private final IConversationCheckpointStore checkpointStore; private final Instance groupConversationStoreInstance; + private final Instance sharedArtifactStoreInstance; private final IScheduleStore scheduleStore; private final ICache userConversationCache; @@ -77,6 +79,7 @@ public GdprComplianceService(IUserMemoryStore userMemoryStore, IConversationDescriptorStore conversationDescriptorStore, IConversationCheckpointStore checkpointStore, Instance groupConversationStoreInstance, + Instance sharedArtifactStoreInstance, IScheduleStore scheduleStore, ICacheFactory cacheFactory) { this.userMemoryStore = userMemoryStore; @@ -90,6 +93,7 @@ public GdprComplianceService(IUserMemoryStore userMemoryStore, this.conversationDescriptorStore = conversationDescriptorStore; this.checkpointStore = checkpointStore; this.groupConversationStoreInstance = groupConversationStoreInstance; + this.sharedArtifactStoreInstance = sharedArtifactStoreInstance; this.scheduleStore = scheduleStore; this.userConversationCache = cacheFactory.getCache(USER_CONVERSATION_CACHE_NAME); } @@ -306,6 +310,23 @@ public GdprDeletionResult deleteUserData(String userId) { pseudonym); } + // 5c2. Delete shared artifacts (I17). Artifacts carry ownerUserId (the + // owning discussion's user) precisely so this sweep works even when the + // parent discussion document is already gone. + long sharedArtifactsDeleted = 0; + try { + if (sharedArtifactStoreInstance.isResolvable()) { + sharedArtifactsDeleted = sharedArtifactStoreInstance.get().deleteAllForUser(userId); + if (sharedArtifactsDeleted > 0) { + LOGGER.infof("[GDPR] Deleted %d shared artifacts [%s]", + sharedArtifactsDeleted, pseudonym); + } + } + } catch (Exception e) { + LOGGER.errorf(e, "[GDPR] Failed to delete shared artifacts [%s]", + pseudonym); + } + // 5d. Delete schedules owned by the user. Left behind, they keep firing new // conversations under the erased identity โ€” recreating the data forever. long schedulesDeleted = 0; @@ -346,9 +367,9 @@ public GdprDeletionResult deleteUserData(String userId) { LOGGER.infof("[GDPR] Erasure cascade complete [%s]: " + "memories=%d, conversations=%d, checkpoints=%d, mappings=%d, " - + "groupConversations=%d, schedules=%d, logs=%d, audit=%d", + + "groupConversations=%d, sharedArtifacts=%d, schedules=%d, logs=%d, audit=%d", pseudonym, memoriesDeleted, conversationsDeleted, checkpointsDeleted, - mappingsDeleted, groupConversationsDeleted, schedulesDeleted, + mappingsDeleted, groupConversationsDeleted, sharedArtifactsDeleted, schedulesDeleted, logsPseudonymized, auditPseudonymized); // Write compliance event to immutable audit ledger @@ -360,6 +381,7 @@ public GdprDeletionResult deleteUserData(String userId) { auditDetails.put("conversationsDeleted", conversationsDeleted); auditDetails.put("mappingsDeleted", mappingsDeleted); auditDetails.put("groupConversationsDeleted", groupConversationsDeleted); + auditDetails.put("sharedArtifactsDeleted", sharedArtifactsDeleted); auditDetails.put("schedulesDeleted", schedulesDeleted); auditDetails.put("logsPseudonymized", logsPseudonymized); auditDetails.put("auditPseudonymized", auditPseudonymized); 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 a999ec46e0..80d6e26010 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java @@ -14,6 +14,7 @@ import ai.labs.eddi.configs.groups.IAgentGroupStore; import ai.labs.eddi.configs.groups.IGroupConversationStore; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; 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; @@ -208,6 +209,14 @@ public class GroupConversationService implements IGroupConversationService { @Inject LiveDiscussionRegistry liveDiscussionRegistry; + /** + * I17's artifact store. Same field-injection pattern and null-safety as + * {@link #attachmentStore} โ€” {@code null} in direct-construction unit tests, + * where reads simply carry no artifacts and lifecycle cascades no-op. + */ + @Inject + ISharedArtifactStore sharedArtifactStore; + // In-node fast-fail guard for concurrent post-discussion operations // (follow-up, continue, close) on the same conversation: a second // operation on the same gcId is rejected rather than queued. The Set is @@ -1188,7 +1197,29 @@ private void scheduleGroupHitlTimeout(GroupConversation gc) { @Override public GroupConversation readGroupConversation(String groupConversationId) throws IResourceStore.ResourceNotFoundException, IResourceStore.ResourceStoreException { - return lifecycleOps().readGroupConversation(groupConversationId); + GroupConversation gc = lifecycleOps().readGroupConversation(groupConversationId); + populateArtifacts(gc); + return gc; + } + + /** + * I17: attaches the discussion's shared artifacts as a read-time derived field, + * here in the service so REST's status payload and MCP's + * {@code read_group_conversation} both carry them. Best-effort โ€” a status read + * must not fail because the artifact store hiccuped. + */ + private void populateArtifacts(GroupConversation gc) { + if (sharedArtifactStore == null || gc == null || gc.getId() == null) { + return; + } + try { + var artifacts = sharedArtifactStore.listByGroupConversationId(gc.getId()); + if (!artifacts.isEmpty()) { + gc.setArtifacts(artifacts); + } + } catch (Exception e) { + LOGGER.warnf("Could not attach shared artifacts to group conversation %s: %s", gc.getId(), e.getMessage()); + } } @Override @@ -1242,7 +1273,7 @@ private void failConversation(GroupConversation gc) { private GroupLifecycleOps lifecycleOps() { return new GroupLifecycleOps(conversationStore, groupStore, conversationService, agentFactory, agentStore, - deploymentStore, operationsInProgress, activeTokens, this, + deploymentStore, sharedArtifactStore, operationsInProgress, activeTokens, this, counterGroupFollowUp, counterGroupContinue, counterGroupClose, counterGroupFailure); } 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 fb54f43b62..9f6c742496 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java @@ -875,6 +875,12 @@ public void onDecisionReached(GroupConversationEventSink.DecisionReachedEvent ev // debate verdict before a later synthesis phase). sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_DECISION_REACHED, toJson(event)); } + + @Override + public void onArtifactUpdated(GroupConversationEventSink.ArtifactUpdatedEvent event) { + // Not terminal โ€” artifacts are edited throughout the discussion. + sendEvent(eventSink, sse, GroupConversationEventSink.EVENT_ARTIFACT_UPDATED, toJson(event)); + } }; } 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 f028795881..10662bc14b 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 @@ -8,6 +8,7 @@ import ai.labs.eddi.configs.deployment.IDeploymentStore; import ai.labs.eddi.configs.groups.IAgentGroupStore; import ai.labs.eddi.configs.groups.IGroupConversationStore; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.DiscussionPhase; import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.LifecyclePolicy; @@ -83,6 +84,7 @@ public class GroupLifecycleOps { private final IAgentFactory agentFactory; private final IAgentStore agentStore; private final IDeploymentStore deploymentStore; + private final ISharedArtifactStore sharedArtifactStore; private final Set operationsInProgress; private final ConcurrentHashMap activeTokens; private final GroupConversationService groupConversationService; @@ -93,7 +95,7 @@ public class GroupLifecycleOps { public GroupLifecycleOps(IGroupConversationStore conversationStore, IAgentGroupStore groupStore, IConversationService conversationService, IAgentFactory agentFactory, IAgentStore agentStore, - IDeploymentStore deploymentStore, Set operationsInProgress, + IDeploymentStore deploymentStore, ISharedArtifactStore sharedArtifactStore, Set operationsInProgress, ConcurrentHashMap activeTokens, GroupConversationService groupConversationService, Counter counterGroupFollowUp, Counter counterGroupContinue, Counter counterGroupClose, Counter counterGroupFailure) { @@ -103,6 +105,7 @@ public GroupLifecycleOps(IGroupConversationStore conversationStore, IAgentGroupS this.agentFactory = agentFactory; this.agentStore = agentStore; this.deploymentStore = deploymentStore; + this.sharedArtifactStore = sharedArtifactStore; this.operationsInProgress = operationsInProgress; this.activeTokens = activeTokens; this.groupConversationService = groupConversationService; @@ -153,6 +156,10 @@ public void deleteGroupConversation(String groupConversationId) // operations. Delete is terminal, so reclaim any dynamically-created agents // here; otherwise deleting a COMPLETED conversation would orphan them. cleanupEphemeralAgentsForGroup(gc); + // I17: artifacts live in their own collection keyed by this conversation + // โ€” remove them with their discussion, before the document goes (while + // the id is still provably a discussion the caller could delete). + deleteArtifactsForGroupConversation(groupConversationId); conversationStore.delete(groupConversationId); } catch (IResourceStore.ResourceNotFoundException e) { LOGGER.warnf("Group conversation %s not found for deletion", LogSanitizer.sanitize(groupConversationId)); @@ -476,6 +483,11 @@ public GroupConversation closeGroupConversation(String groupConversationId) // Ephemeral agent cleanup (deferred from executeDiscussion) cleanupEphemeralAgentsForGroup(gc); + // I17: close is a lifecycle end โ€” the working artifacts go with it. + // Their durable trace is the transcript (accepted updates are announced + // there and a synthesis quotes what mattered), not the working copies. + deleteArtifactsForGroupConversation(groupConversationId); + LOGGER.infof("Group conversation %s closed โ€” member conversations ended, ephemeral agents cleaned up", LogSanitizer.sanitize(groupConversationId)); @@ -486,6 +498,27 @@ public GroupConversation closeGroupConversation(String groupConversationId) } } + /** + * I17 cascade: removes the discussion's shared artifacts. Warn-and-continue on + * failure โ€” a broken artifact store must not make discussions undeletable; the + * user-keyed GDPR erasure sweep (which fails loudly) is the completeness + * guarantee, this is the tidy path. + */ + private void deleteArtifactsForGroupConversation(String groupConversationId) { + if (sharedArtifactStore == null) { + return; + } + try { + long removed = sharedArtifactStore.deleteByGroupConversationId(groupConversationId); + if (removed > 0) { + LOGGER.infof("Removed %d shared artifact(s) of group conversation %s", removed, LogSanitizer.sanitize(groupConversationId)); + } + } catch (Exception e) { + LOGGER.warnf("Failed to remove shared artifacts of group conversation %s: %s", + LogSanitizer.sanitize(groupConversationId), e.getMessage()); + } + } + public List listGroupPendingApprovals(String groupId, int limit) throws IResourceStore.ResourceStoreException { // Bounded summaries โ€” never hand full transcripts to a listing endpoint. 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 aba17f729a..d7f5929323 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 @@ -152,6 +152,36 @@ public TranscriptEntry executeAgentTurn(GroupMember member, GroupConversation gc DiscussionPhase phase, String targetAgentId, GroupDiscussionEventListener listener, GroupConversationService.MemberTurnCancellation cancellation, String conversationKey) throws GroupDiscussionException { + try { + return doExecuteAgentTurn(member, gc, input, protocol, phaseIdx, phase, targetAgentId, listener, cancellation, conversationKey); + } finally { + // I17: announce artifact writes this turn made. In a finally because an + // accepted write already happened whatever the turn's own outcome โ€” + // a timeout or cancellation after the write must not swallow the event. + // Drained even with a null listener so the queue never grows unbounded. + announceArtifactChanges(gc, listener); + } + } + + /** + * I17: fires {@code artifact_updated} for every write queued during the turn. + */ + private static void announceArtifactChanges(GroupConversation gc, GroupDiscussionEventListener listener) { + List changes = gc.drainArtifactChanges(); + if (listener == null || changes.isEmpty()) { + return; + } + for (GroupConversation.ArtifactChange change : changes) { + listener.onArtifactUpdated(new GroupConversationEventSink.ArtifactUpdatedEvent( + change.artifactId(), change.name(), change.type(), change.version(), + change.editorAgentId(), change.status(), change.created())); + } + } + + private TranscriptEntry doExecuteAgentTurn(GroupMember member, GroupConversation gc, String input, ProtocolConfig protocol, int phaseIdx, + DiscussionPhase phase, String targetAgentId, GroupDiscussionEventListener listener, + GroupConversationService.MemberTurnCancellation cancellation, String conversationKey) + throws GroupDiscussionException { if (cancellation != null && cancellation.isCancelled()) { throw new GroupConversationService.MemberTurnCancelledException(); 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 8182bcebde..aa08a2f255 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,13 @@ private GroupConversationEventSink() { * Always preceded by an {@link #EVENT_CONVERGENCE_CHECKED} for the same repeat. */ public static final String EVENT_CONVERGENCE_REACHED = "convergence_reached"; + /** + * A member created or updated a shared artifact (I17). Fired by the turn + * executor after the turn that made the write โ€” tools have no listener + * reference, so accepted writes ride the live discussion's artifact-change + * queue until the executor drains it. + */ + public static final String EVENT_ARTIFACT_UPDATED = "artifact_updated"; // --- Event payloads --- @@ -159,4 +166,17 @@ public record ConvergenceCheckedEvent(int phaseIndex, String phaseName, int repe */ public record ConvergenceReachedEvent(int phaseIndex, String phaseName, int repeat, int repeatsSkipped, String reason) { } + + /** + * A member created or updated a shared artifact (I17). Carries metadata only, + * never the content โ€” an SSE observer reads the artifact through the REST + * payload, and content can be a quarter megabyte. + * + * @param created + * {@code true} for a fresh artifact (v1), {@code false} for an + * accepted update + */ + public record ArtifactUpdatedEvent(String artifactId, String name, String type, long version, String editorAgentId, + String status, boolean created) { + } } 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 b6e8cee99b..a1c06fc30c 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,28 @@ public void onDecisionReached(GroupConversationEventSink.DecisionReachedEvent ev postSafe(channelId, threadTs, sb.toString().stripTrailing()); } + @Override + public void onArtifactUpdated(GroupConversationEventSink.ArtifactUpdatedEvent event) { + // Degenerate payload โ†’ skip rather than posting noise, same as + // onDecisionReached's NONE guard. + if (event == null || event.name() == null) { + return; + } + String emoji = event.created() ? "๐Ÿ“„" : "โœ๏ธ"; + String verb = event.created() ? "created" : "updated"; + var sb = new StringBuilder(); + sb.append(String.format("%s *Artifact \"%s\"* %s (v%d)", emoji, event.name(), verb, event.version())); + if (event.editorAgentId() != null && !event.editorAgentId().isBlank()) { + sb.append(String.format(" by %s", event.editorAgentId())); + } + if ("FINAL".equals(event.status())) { + sb.append(" โ€” FINAL"); + } + + String threadTs = expandedMode ? null : userThreadTs; + postSafe(channelId, threadTs, sb.toString().stripTrailing()); + } + // โ”€โ”€โ”€ HITL (human-in-the-loop) โ”€โ”€โ”€ @Override diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java index dd1248207b..4bf556be1c 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java @@ -49,6 +49,7 @@ import jakarta.enterprise.context.ApplicationScoped; import ai.labs.eddi.engine.internal.groups.LiveDiscussionRegistry; import ai.labs.eddi.configs.groups.IAgentGroupStore; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; import jakarta.inject.Inject; import org.eclipse.microprofile.config.ConfigProvider; import org.jboss.logging.Logger; @@ -267,6 +268,13 @@ static void warnAboutUnenforcedBudgets(LlmConfiguration.Task task) { @Inject volatile IAgentGroupStore agentGroupStore; + /** + * I17's store, field-injected for the same reason as the two above. + * {@code ArtifactToolsProvider} treats null as "no artifact tools". + */ + @Inject + volatile ISharedArtifactStore sharedArtifactStore; + /** * Test seam for supplying the attachment services to a directly-constructed * orchestrator (CDI populates the fields above in production). Previously this @@ -644,6 +652,7 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) var contextual = contextualToolsProvider(); merger.addAll(List.of(builtinToolsProvider, contextual, dynamicAgentToolsProvider(), new GroupTaskToolsProvider(liveDiscussionRegistry, agentGroupStore), + new ArtifactToolsProvider(liveDiscussionRegistry, agentGroupStore, sharedArtifactStore), new AttachmentToolsProvider(contextual)), ctx); // LAZY registers every built-in's executor but shows the model only diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProvider.java b/src/main/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProvider.java new file mode 100644 index 0000000000..e7d78b3f07 --- /dev/null +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProvider.java @@ -0,0 +1,105 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.llm.impl; + +import ai.labs.eddi.configs.groups.IAgentGroupStore; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactConfig; +import ai.labs.eddi.engine.internal.groups.LiveDiscussionRegistry; +import ai.labs.eddi.modules.llm.tools.impl.ArtifactTools; +import ai.labs.eddi.modules.llm.tools.spi.ToolAssemblyContext; +import ai.labs.eddi.modules.llm.tools.spi.ToolContribution; +import ai.labs.eddi.modules.llm.tools.spi.ToolSourceProvider; +import org.jboss.logging.Logger; + +import java.util.List; +import java.util.Map; + +/** + * Contributes the shared-artifact tools (I17) โ€” {@code createArtifact}, + * {@code readArtifact}, {@code proposeArtifactUpdate}, {@code listArtifacts} โ€” + * when, and only when, the turn belongs to a live group discussion whose config + * sets {@code artifactConfig.allowArtifactTools}. + *

+ * Gate discipline is identical to {@link GroupTaskToolsProvider}, and for the + * same reasons: membership (not existence) via + * {@code LiveDiscussionRegistry#getForMember} because the group conversation id + * is a caller-supplied context variable; the member agent's own + * {@code enableBuiltInTools} switch still applies; and every uncertainty โ€” + * missing config, unreadable group, absent store โ€” resolves to "contribute + * nothing" (fail-closed, gate by absence). + * + * @author ginccc + */ +class ArtifactToolsProvider implements ToolSourceProvider { + + private static final Logger LOGGER = Logger.getLogger(ArtifactToolsProvider.class); + + private final LiveDiscussionRegistry liveDiscussionRegistry; + private final IAgentGroupStore groupStore; + private final ISharedArtifactStore artifactStore; + + ArtifactToolsProvider(LiveDiscussionRegistry liveDiscussionRegistry, IAgentGroupStore groupStore, + ISharedArtifactStore artifactStore) { + this.liveDiscussionRegistry = liveDiscussionRegistry; + this.groupStore = groupStore; + this.artifactStore = artifactStore; + } + + @Override + public String source() { + return "builtin"; + } + + @Override + public ToolContribution contribute(ToolAssemblyContext ctx) { + String groupConversationId = ctx.groupConversationId(); + if (groupConversationId == null || liveDiscussionRegistry == null || groupStore == null || artifactStore == null) { + return ToolContribution.empty(); + } + Boolean enableBuiltInTools = ctx.task() != null ? ctx.task().getEnableBuiltInTools() : null; + if (enableBuiltInTools == null || !enableBuiltInTools) { + return ToolContribution.empty(); + } + String callerConversationId = ctx.memory() != null ? ctx.memory().getConversationId() : null; + var live = liveDiscussionRegistry.getForMember(groupConversationId, callerConversationId); + if (live.isEmpty()) { + return ToolContribution.empty(); + } + AgentGroupConfiguration groupConfiguration = resolveGroup(live.get().getGroupId()); + ArtifactConfig config = groupConfiguration != null ? groupConfiguration.getArtifactConfig() : null; + if (config == null || !config.allowArtifactTools()) { + return ToolContribution.empty(); + } + + var tools = List.of(new ArtifactTools(liveDiscussionRegistry, groupConversationId, config, ctx.agentId(), + artifactStore)); + var reflected = ToolObjectReflector.reflect(tools); + return new ToolContribution(reflected.specs(), reflected.executors(), reflected.toolSources(), Map.of(), + List.of(), reflected.toolCanonicalNames()); + } + + /** + * The group config, or {@code null} if it cannot be read โ€” a store failure + * withholds the write tools (fail-closed), logged so the operator sees why they + * vanished. + */ + private AgentGroupConfiguration resolveGroup(String groupId) { + if (groupId == null) { + return null; + } + try { + var resourceId = groupStore.getCurrentResourceId(groupId); + if (resourceId == null) { + return null; + } + return groupStore.read(groupId, resourceId.getVersion()); + } catch (Exception e) { + LOGGER.warnf("Could not read artifact policy for group '%s' โ€” withholding the artifact tools: %s", groupId, e.getMessage()); + return null; + } + } +} diff --git a/src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java new file mode 100644 index 0000000000..fcd9ddf55c --- /dev/null +++ b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java @@ -0,0 +1,310 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.llm.tools.impl; + +import ai.labs.eddi.configs.groups.ArtifactValidators; +import ai.labs.eddi.configs.groups.ISharedArtifactStore; +import ai.labs.eddi.configs.groups.ISharedArtifactStore.ArtifactGoneException; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactConfig; +import ai.labs.eddi.configs.groups.model.GroupConversation; +import ai.labs.eddi.configs.groups.model.GroupConversation.ArtifactChange; +import ai.labs.eddi.configs.groups.model.SharedArtifact; +import ai.labs.eddi.configs.groups.model.SharedArtifact.ArtifactStatus; +import ai.labs.eddi.configs.groups.model.SharedArtifact.ArtifactType; +import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.engine.internal.groups.LiveDiscussionRegistry; +import dev.langchain4j.agent.tool.P; +import dev.langchain4j.agent.tool.Tool; +import jakarta.enterprise.inject.Vetoed; +import org.jboss.logging.Logger; + +import java.time.Instant; +import java.util.List; +import java.util.Locale; + +/** + * Lets group members create and co-edit shared artifacts (I17, blackboard-lite) + * โ€” typed documents that live outside the dialogue, so structured work + * stops being prose the next agent re-parses. + *

+ * Writes go through the artifact store directly. Artifacts have their + * own collection, so the F1 rule that task-list writes must mutate the live + * {@link GroupConversation} instance (or be clobbered by the loop's next + * whole-document persist) does not apply. The live registry is still consulted + * โ€” a write against a finished or paused discussion is refused, and the + * creation/edit is announced through the live instance's artifact-change queue + * so the discussion loop can fire {@code artifact_updated}. + *

+ * Concurrency is deterministic CAS-and-retry, not an LLM merge. Every + * update presents the version it read; a stale version gets a "re-read and + * merge" sentence and the model retries against fresh content. Creation + * (duplicate-name + count cap) is a check-then-act, so it synchronizes on the + * live discussion instance โ€” PARALLEL phases genuinely run members at once. + *

+ * Constructed per-turn with runtime values โ€” NOT a CDI bean. {@code @Vetoed} is + * load-bearing for the same deployment-failure reason as + * {@link GroupTaskTools}. + * + * @author ginccc + */ +@Vetoed +public class ArtifactTools { + + private static final Logger LOGGER = Logger.getLogger(ArtifactTools.class); + + static final int MAX_NAME_LENGTH = 200; + + private final LiveDiscussionRegistry registry; + private final String groupConversationId; + private final ArtifactConfig config; + private final String agentId; + private final ISharedArtifactStore artifactStore; + + public ArtifactTools(LiveDiscussionRegistry registry, String groupConversationId, ArtifactConfig config, String agentId, + ISharedArtifactStore artifactStore) { + this.registry = registry; + this.groupConversationId = groupConversationId; + this.config = config; + this.agentId = agentId; + this.artifactStore = artifactStore; + } + + @Tool("Create a new shared artifact โ€” a named document the whole team can read and propose changes to. " + + "Use listArtifacts first to avoid duplicating one that already exists.") + public String createArtifact( + @P("Short unique name, how the team refers to this artifact") String name, + @P("Content type: TEXT, MARKDOWN or JSON") String type, + @P("The initial content") String content) { + + GroupConversation gc = liveDiscussion(); + if (gc == null) { + return "This discussion is no longer accepting artifact changes (it has finished or is paused)."; + } + if (name == null || name.isBlank()) { + return "An artifact needs a name."; + } + String trimmedName = name.trim(); + if (trimmedName.length() > MAX_NAME_LENGTH) { + return "The name is too long (%d chars, max %d). Put the detail in the content.".formatted(trimmedName.length(), MAX_NAME_LENGTH); + } + ArtifactType artifactType = parseType(type); + if (artifactType == null) { + return "Unknown artifact type \"%s\". Use TEXT, MARKDOWN or JSON.".formatted(type); + } + String rejection = checkContent(content); + if (rejection != null) { + return rejection; + } + + // Check-then-act (duplicate name + count cap) under one monitor: the live + // instance is the one object every member of this discussion shares in + // this JVM, so it is the natural lock for creation races. Updates need no + // lock โ€” the store's version CAS decides those. + synchronized (gc) { + List existing; + try { + existing = artifactStore.listByGroupConversationId(groupConversationId); + } catch (IResourceStore.ResourceStoreException e) { + LOGGER.warnf("Could not list artifacts for %s โ€” refusing the create: %s", groupConversationId, e.getMessage()); + return "The artifact store is unavailable right now; try again next turn."; + } + if (existing.stream().anyMatch(a -> trimmedName.equalsIgnoreCase(a.getName()))) { + return "An artifact named \"%s\" already exists. Read it with readArtifact and use proposeArtifactUpdate to change it." + .formatted(trimmedName); + } + if (existing.size() >= config.maxArtifactsPerDiscussion()) { + return "This discussion already has %d artifacts, which is the limit. Update an existing one instead of creating more." + .formatted(existing.size()); + } + + var artifact = new SharedArtifact(); + artifact.setGroupConversationId(groupConversationId); + artifact.setOwnerUserId(gc.getUserId()); + artifact.setName(trimmedName); + artifact.setType(artifactType); + artifact.setContent(content); + artifact.setVersion(1); + artifact.setLastEditorAgentId(agentId); + artifact.setStatus(ArtifactStatus.DRAFT); + artifact.setCreatedAt(Instant.now()); + artifact.setUpdatedAt(artifact.getCreatedAt()); + try { + artifactStore.create(artifact); + } catch (IResourceStore.ResourceStoreException e) { + LOGGER.warnf("Could not create artifact '%s' for %s: %s", trimmedName, groupConversationId, e.getMessage()); + return "The artifact store is unavailable right now; try again next turn."; + } + + gc.queueArtifactChange(new ArtifactChange(artifact.getId(), trimmedName, artifactType.name(), 1, agentId, + ArtifactStatus.DRAFT.name(), true)); + LOGGER.infof("Agent '%s' created artifact '%s' (v1) on group conversation %s", agentId, trimmedName, groupConversationId); + return "Created artifact \"%s\" (v1). The team can read it with readArtifact and change it with proposeArtifactUpdate." + .formatted(trimmedName); + } + } + + @Tool("Read a shared artifact's current content and version. You need the version to propose an update.") + public String readArtifact(@P("The artifact's name (or id)") String nameOrId) { + SharedArtifact artifact = resolve(nameOrId); + if (artifact == null) { + return "No artifact named \"%s\" here. Use listArtifacts to see what exists.".formatted(nameOrId != null ? nameOrId.trim() : ""); + } + return "Artifact \"%s\" (%s, %s, v%d, last edited by %s):\n%s".formatted( + artifact.getName(), artifact.getType(), artifact.getStatus(), artifact.getVersion(), + artifact.getLastEditorAgentId() != null ? artifact.getLastEditorAgentId() : "unknown", + artifact.getContent() != null ? artifact.getContent() : ""); + } + + @Tool("Propose new content for a shared artifact. You must pass the version you READ โ€” if someone changed the " + + "artifact since, your update is rejected and you re-read, merge your change into theirs, and retry.") + public String proposeArtifactUpdate( + @P("The artifact's name (or id)") String nameOrId, + @P("The complete new content (it replaces the old content)") String content, + @P("The version you read, from readArtifact") long expectedVersion, + @P(value = "Pass true to freeze the artifact as FINAL after this update. " + + "A FINAL artifact accepts no further updates.", + required = false) Boolean markFinal) { + + GroupConversation gc = liveDiscussion(); + if (gc == null) { + return "This discussion is no longer accepting artifact changes (it has finished or is paused)."; + } + SharedArtifact artifact = resolve(nameOrId); + if (artifact == null) { + return "No artifact named \"%s\" here. Use listArtifacts to see what exists.".formatted(nameOrId != null ? nameOrId.trim() : ""); + } + if (artifact.getStatus() == ArtifactStatus.FINAL) { + return "Artifact \"%s\" is FINAL and accepts no further updates.".formatted(artifact.getName()); + } + String rejection = checkContent(content); + if (rejection != null) { + return rejection; + } + if (artifact.getVersion() != expectedVersion) { + return staleVersion(artifact.getName(), artifact.getVersion()); + } + + artifact.applyEdit(content, agentId, Instant.now()); + if (Boolean.TRUE.equals(markFinal)) { + artifact.setStatus(ArtifactStatus.FINAL); + } + try { + artifactStore.updateIfVersion(artifact, expectedVersion); + } catch (IResourceStore.ResourceModifiedException e) { + // Lost the race after our read โ€” report the CURRENT version so the + // retry is usable. A failed re-read still yields the retry instruction. + long nowVersion = currentVersionOf(artifact.getId()); + return nowVersion > 0 + ? staleVersion(artifact.getName(), nowVersion) + : "Artifact \"%s\" changed since you read it; re-read and merge your change.".formatted(artifact.getName()); + } catch (ArtifactGoneException e) { + return "Artifact \"%s\" no longer exists.".formatted(artifact.getName()); + } catch (IResourceStore.ResourceStoreException e) { + LOGGER.warnf("Could not update artifact '%s' for %s: %s", artifact.getName(), groupConversationId, e.getMessage()); + return "The artifact store is unavailable right now; try again next turn."; + } + + gc.queueArtifactChange(new ArtifactChange(artifact.getId(), artifact.getName(), artifact.getType().name(), + artifact.getVersion(), agentId, artifact.getStatus().name(), false)); + LOGGER.infof("Agent '%s' updated artifact '%s' to v%d on group conversation %s", + agentId, artifact.getName(), artifact.getVersion(), groupConversationId); + return "Updated \"%s\" to v%d.%s".formatted(artifact.getName(), artifact.getVersion(), + artifact.getStatus() == ArtifactStatus.FINAL ? " It is now FINAL." : ""); + } + + @Tool("List this discussion's shared artifacts with their type, status and current version.") + public String listArtifacts() { + List artifacts; + try { + artifacts = artifactStore.listByGroupConversationId(groupConversationId); + } catch (IResourceStore.ResourceStoreException e) { + return "The artifact store is unavailable right now; try again next turn."; + } + if (artifacts.isEmpty()) { + return "No artifacts yet. Create one with createArtifact."; + } + var sb = new StringBuilder("Shared artifacts:\n"); + for (SharedArtifact a : artifacts) { + sb.append("- \"").append(a.getName()).append("\" (").append(a.getType()).append(", ").append(a.getStatus()) + .append(", v").append(a.getVersion()).append(')'); + if (a.getLastEditorAgentId() != null) { + sb.append(" โ€” last edited by ").append(a.getLastEditorAgentId()); + } + sb.append('\n'); + } + return sb.toString().stripTrailing(); + } + + /** The plan-specified stale-CAS sentence, verbatim shape. */ + private static String staleVersion(String name, long currentVersion) { + return "Artifact \"%s\" changed since you read it (now v%d); re-read and merge your change.".formatted(name, currentVersion); + } + + /** + * Resolves an artifact by name or id โ€” but only among THIS discussion's + * artifacts. Never a raw store read of a caller-supplied id: the id is + * model-controlled text, and resolving it globally would read another + * discussion's artifact. + */ + private SharedArtifact resolve(String nameOrId) { + if (nameOrId == null || nameOrId.isBlank()) { + return null; + } + String wanted = nameOrId.trim(); + List artifacts; + try { + artifacts = artifactStore.listByGroupConversationId(groupConversationId); + } catch (IResourceStore.ResourceStoreException e) { + LOGGER.warnf("Could not resolve artifact '%s' for %s: %s", wanted, groupConversationId, e.getMessage()); + return null; + } + return artifacts.stream() + .filter(a -> wanted.equalsIgnoreCase(a.getName()) || wanted.equals(a.getId())) + .findFirst().orElse(null); + } + + /** Size cap first (cheap), then the config's declarative validator chain. */ + private String checkContent(String content) { + if (content == null || content.isBlank()) { + return "An artifact needs content."; + } + int bytes = SharedArtifact.contentBytes(content); + if (bytes > SharedArtifact.MAX_CONTENT_BYTES) { + return "The content is %d KB, over the %d KB limit for a single artifact. Split it or shorten it." + .formatted(bytes / 1024, SharedArtifact.MAX_CONTENT_BYTES / 1024); + } + return ArtifactValidators.firstRejection(config.validators(), content); + } + + private static ArtifactType parseType(String type) { + if (type == null || type.isBlank()) { + return null; + } + try { + return ArtifactType.valueOf(type.trim().toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + return null; + } + } + + /** + * Live-instance lookup for writes. Authorization happened at assembly time (the + * provider resolves membership via {@code getForMember}); this lookup only + * answers "is the discussion still running", so a paused or finished discussion + * refuses instead of accepting a write nobody will announce. + */ + private GroupConversation liveDiscussion() { + return registry.get(groupConversationId).orElse(null); + } + + /** The stored artifact's current version, or 0 when unreadable. */ + private long currentVersionOf(String artifactId) { + try { + return artifactStore.read(artifactId).getVersion(); + } catch (Exception e) { + return 0; + } + } +} diff --git a/src/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.java b/src/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.java new file mode 100644 index 0000000000..a57db78694 --- /dev/null +++ b/src/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.java @@ -0,0 +1,148 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups; + +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactConfig; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ArtifactValidator; +import ai.labs.eddi.configs.groups.model.AgentGroupConfiguration.ValidatorKind; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * I17 โ€” the declarative artifact validation chain: save-time spec checks fail + * the config save with an actionable path, write-time failures come back as + * rejection sentences for the model, and a broken spec fails closed. + * + * @author tests + */ +class ArtifactValidatorsTest { + + private static final String SCHEMA = """ + {"type":"object","required":["title"],"properties":{"title":{"type":"string"}}}"""; + + private static ArtifactConfig config(ArtifactValidator... validators) { + return new ArtifactConfig(true, 5, List.of(validators)); + } + + // ================================================================= + // save-time: requireValidSpecs + // ================================================================= + + @Test + @DisplayName("valid specs of all three kinds pass the save-time check") + void validSpecs_pass() { + assertDoesNotThrow(() -> ArtifactValidators.requireValidSpecs(config( + new ArtifactValidator(ValidatorKind.JSON_SCHEMA, SCHEMA), + new ArtifactValidator(ValidatorKind.REGEX, "^#"), + new ArtifactValidator(ValidatorKind.MAX_LENGTH, "1000")))); + } + + @Test + @DisplayName("null config or empty chain is a no-op") + void absentConfig_noOp() { + assertDoesNotThrow(() -> ArtifactValidators.requireValidSpecs(null)); + assertDoesNotThrow(() -> ArtifactValidators.requireValidSpecs(new ArtifactConfig(true, 5, null))); + } + + @Test + @DisplayName("a broken spec fails the save and names the validator's position") + void brokenSpecs_throwWithPath() { + var badRegex = assertThrows(IllegalArgumentException.class, + () -> ArtifactValidators.requireValidSpecs(config(new ArtifactValidator(ValidatorKind.REGEX, "[unclosed")))); + assertTrue(badRegex.getMessage().contains("validators[0]"), badRegex.getMessage()); + + var badLength = assertThrows(IllegalArgumentException.class, + () -> ArtifactValidators.requireValidSpecs(config( + new ArtifactValidator(ValidatorKind.REGEX, "ok"), + new ArtifactValidator(ValidatorKind.MAX_LENGTH, "lots")))); + assertTrue(badLength.getMessage().contains("validators[1]"), badLength.getMessage()); + + assertThrows(IllegalArgumentException.class, + () -> ArtifactValidators.requireValidSpecs(config(new ArtifactValidator(ValidatorKind.MAX_LENGTH, "0")))); + assertThrows(IllegalArgumentException.class, + () -> ArtifactValidators.requireValidSpecs(config(new ArtifactValidator(null, "x")))); + assertThrows(IllegalArgumentException.class, + () -> ArtifactValidators.requireValidSpecs(config(new ArtifactValidator(ValidatorKind.JSON_SCHEMA, " ")))); + } + + // ================================================================= + // write-time: firstRejection + // ================================================================= + + @Test + @DisplayName("content passing the whole chain yields null") + void passingContent_null() { + var validators = List.of( + new ArtifactValidator(ValidatorKind.MAX_LENGTH, "100"), + new ArtifactValidator(ValidatorKind.REGEX, "title"), + new ArtifactValidator(ValidatorKind.JSON_SCHEMA, SCHEMA)); + + assertNull(ArtifactValidators.firstRejection(validators, "{\"title\":\"ok\"}")); + } + + @Test + @DisplayName("MAX_LENGTH rejection names both counts so the model can act") + void maxLength_rejects() { + String rejection = ArtifactValidators.firstRejection( + List.of(new ArtifactValidator(ValidatorKind.MAX_LENGTH, "5")), "123456"); + + assertNotNull(rejection); + assertTrue(rejection.contains("6") && rejection.contains("5"), rejection); + } + + @Test + @DisplayName("REGEX requires a match somewhere in the content") + void regex_rejectsAndPasses() { + var validators = List.of(new ArtifactValidator(ValidatorKind.REGEX, "^# .+")); + + assertNull(ArtifactValidators.firstRejection(validators, "# Heading\nbody")); + String rejection = ArtifactValidators.firstRejection(validators, "no heading here"); + assertNotNull(rejection); + assertTrue(rejection.contains("pattern"), rejection); + } + + @Test + @DisplayName("JSON_SCHEMA distinguishes 'not JSON' from 'JSON that violates the schema'") + void jsonSchema_rejections() { + var validators = List.of(new ArtifactValidator(ValidatorKind.JSON_SCHEMA, SCHEMA)); + + String notJson = ArtifactValidators.firstRejection(validators, "plain prose"); + assertNotNull(notJson); + assertTrue(notJson.contains("valid JSON"), notJson); + + String violates = ArtifactValidators.firstRejection(validators, "{\"other\":1}"); + assertNotNull(violates); + assertTrue(violates.contains("title"), "the violation message must name what is missing: " + violates); + + assertNull(ArtifactValidators.firstRejection(validators, "{\"title\":\"x\"}")); + } + + @Test + @DisplayName("the chain runs in config order โ€” the first failure wins") + void chainOrder_firstFailureWins() { + var validators = List.of( + new ArtifactValidator(ValidatorKind.MAX_LENGTH, "3"), + new ArtifactValidator(ValidatorKind.REGEX, "nope")); + + String rejection = ArtifactValidators.firstRejection(validators, "12345"); + assertNotNull(rejection); + assertTrue(rejection.contains("character"), "the length failure comes first: " + rejection); + } + + @Test + @DisplayName("a broken spec at write time fails closed โ€” the write is refused, never admitted") + void brokenSpecAtWriteTime_failsClosed() { + assertNotNull(ArtifactValidators.firstRejection( + List.of(new ArtifactValidator(ValidatorKind.REGEX, "[unclosed")), "anything")); + assertNotNull(ArtifactValidators.firstRejection( + List.of(new ArtifactValidator(ValidatorKind.MAX_LENGTH, "many")), "anything")); + assertNotNull(ArtifactValidators.firstRejection( + List.of(new ArtifactValidator(null, null)), "anything")); + } +} diff --git a/src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java b/src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java new file mode 100644 index 0000000000..2ba34911cd --- /dev/null +++ b/src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java @@ -0,0 +1,254 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.groups.mongo; + +import ai.labs.eddi.configs.groups.ISharedArtifactStore.ArtifactGoneException; +import ai.labs.eddi.configs.groups.model.SharedArtifact; +import ai.labs.eddi.datastore.IResourceFilter; +import ai.labs.eddi.datastore.IResourceStorage; +import ai.labs.eddi.datastore.IResourceStorageFactory; +import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.datastore.serialization.IDocumentBuilder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import java.io.IOException; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * I17 โ€” {@link SharedArtifactStore}: the version CAS goes through the numeric + * {@code storeIfFieldEquals} overload (never an unconditional store), filters + * are anchored and re-checked in Java, and the GDPR erasure sweep follows the + * group-conversation store's page/re-check/fail-loud contract. + * + * @author tests + */ +@SuppressWarnings("unchecked") +class SharedArtifactStoreTest { + + private IResourceStorage storage; + private SharedArtifactStore store; + + @BeforeEach + void setUp() { + IResourceStorageFactory storageFactory = mock(IResourceStorageFactory.class); + IDocumentBuilder documentBuilder = mock(IDocumentBuilder.class); + storage = mock(IResourceStorage.class); + // Must mirror the production create() call EXACTLY, index-hint varargs + // included โ€” a mismatch silently leaves the internal storage null. + when(storageFactory.create(eq("sharedartifacts"), eq(documentBuilder), eq(SharedArtifact.class), + eq("groupConversationId"), eq("ownerUserId"))).thenReturn(storage); + store = new SharedArtifactStore(storageFactory, documentBuilder); + } + + private static SharedArtifact artifact(String id, String gcId, String owner, long version) { + var a = new SharedArtifact(); + a.setId(id); + a.setGroupConversationId(gcId); + a.setOwnerUserId(owner); + a.setName("draft"); + a.setVersion(version); + return a; + } + + private IResourceStorage.IResource resource(String id, SharedArtifact data) throws IOException { + IResourceStorage.IResource resource = mock(IResourceStorage.IResource.class); + when(resource.getId()).thenReturn(id); + when(resource.getData()).thenReturn(data); + return resource; + } + + private IResourceStore.IResourceId resourceId(String id) { + return new IResourceStore.IResourceId() { + @Override + public String getId() { + return id; + } + + @Override + public Integer getVersion() { + return 1; + } + }; + } + + // ================================================================= + // CAS + // ================================================================= + + @Test + @DisplayName("updateIfVersion goes through the NUMERIC CAS overload, never an unconditional store") + void updateIfVersion_usesNumericCas() throws Exception { + var a = artifact("a-1", "gc-1", "user-1", 3); + IResourceStorage.IResource writeResource = resource("a-1", a); + when(storage.newResource("a-1", 1, a)).thenReturn(writeResource); + + store.updateIfVersion(a, 2); + + verify(storage).storeIfFieldEquals(writeResource, "version", 2L); + verify(storage, never()).store(any(IResourceStorage.IResource.class)); + verify(storage, never()).storeIfFieldEquals(any(), anyString(), anyString()); + } + + @Test + @DisplayName("a CAS conflict propagates as ResourceModifiedException (retry), a gone document as ArtifactGoneException") + void updateIfVersion_distinguishesConflictFromGone() throws Exception { + var a = artifact("a-1", "gc-1", "user-1", 3); + IResourceStorage.IResource writeResource = resource("a-1", a); + when(storage.newResource("a-1", 1, a)).thenReturn(writeResource); + + doThrow(new IResourceStore.ResourceModifiedException("conflict")) + .when(storage).storeIfFieldEquals(writeResource, "version", 2L); + assertThrows(IResourceStore.ResourceModifiedException.class, () -> store.updateIfVersion(a, 2)); + + doThrow(new IResourceStore.ResourceNotFoundException("gone")) + .when(storage).storeIfFieldEquals(writeResource, "version", 2L); + assertThrows(ArtifactGoneException.class, () -> store.updateIfVersion(a, 2)); + } + + // ================================================================= + // read / create + // ================================================================= + + @Test + @DisplayName("read maps a missing document to a not-found whose message does NOT embed the caller-supplied id") + void readNotFound_messageDoesNotEmbedTheId() { + when(storage.read("attacker