fix(groups): pre-feature defects N1-N3 - plain-call pricing, schema sentinel, transcript windowing - #636
Merged
Merged
Conversation
…utputPricePer1M (N1) 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.
…erwriting 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.
…t at creation (N3) 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.
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.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR adds configurable transcript windowing with rolling summaries, corrects legacy schema migration handling, fixes nested-group cost aggregation, and adds task-level token pricing with validation and cascade precedence. ChangesGroup conversation context
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GroupConversationService
participant GroupContextBuilder
participant SummarizationService
participant GroupCostLedger
GroupConversationService->>GroupContextBuilder: updateWindowSummary
GroupContextBuilder->>SummarizationService: summarize uncovered transcript
SummarizationService-->>GroupContextBuilder: return summary and token usage
GroupContextBuilder->>GroupCostLedger: recordSystemCost
GroupContextBuilder-->>GroupConversationService: return windowed context
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java`:
- Around line 156-163: Update the ContextWindowConfig constructor to normalize
blank or whitespace-only summarizer provider and model identifiers to null,
matching AgentGroupStore.warnOnSummarizerlessWindow and
GroupContextBuilder.updateWindowSummary expectations. Preserve nonblank
identifiers and ensure missing values select the documented truncation fallback
instead of invoking SummarizationService.
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 739-744: Move the updateWindowSummary call in the relevant
repeat/phase flow so it runs after the phase has added its transcript entries,
and invoke it after each eligible phase or repeat completes based on FULL or
ANONYMOUS context scope. Ensure a one-repeat OPINION phase can extend the
rolling summary before the following FULL-scope SYNTHESIS phase renders, and add
an integration test covering that overflow sequence.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java`:
- Around line 471-474: Update the transcript snapshot in updateWindowSummary to
use the same thread-safe mechanism as transcript appenders: ensure the
transcript collection is a synchronized list and that all
gc.getTranscript().add(...) operations use its monitor, then copy it while
holding that monitor. Do not rely on synchronizing the read alone when writers
bypass the list monitor.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java`:
- Around line 90-107: Keep GroupConversation.memberCosts keyed exclusively by
member.agentId() so its serialized contract remains member-level. Update
accumulateNestedGroupCost and the associated re-summing state to store per-child
attribution in a separate child-keyed field, then copy the cumulative total for
member.agentId() into memberCosts; ensure multiple null-ID child discussions
accumulate without overwriting.
- Around line 109-120: Update recordSystemCost to reject null keys and
non-finite or negative costs, while allowing zero cost through recordAndReSum so
an existing operation’s spend is replaced and cleared. Preserve the replacement
semantics for valid finite costs and prevent invalid values from reaching
totalCost or ceiling checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e9c88ac-a231-4a6f-8da4-95e0b7e3fce4
📒 Files selected for processing (30)
docs/changelog.mddocs/group-conversations.mddocs/langchain.mdplanning/group-collaboration-NEXT.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.javasrc/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.javasrc/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.javasrc/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.javasrc/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java
There was a problem hiding this comment.
Pull request overview
Fixes group-conversation cost accounting, legacy schema detection, and transcript growth.
Changes:
- Prices ordinary LLM calls and correctly aggregates nested-group costs.
- Distinguishes legacy and current persisted schemas.
- Adds configurable transcript windowing with rolling summaries.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAuditLedgerTest.java |
Tests plain-call and cascade pricing. |
src/test/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidatorTest.java |
Tests task-price validation. |
src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesTest.java |
Tests current snapshot stamping. |
src/test/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngineTest.java |
Updates context-builder mocking. |
src/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java |
Updates nested-cost attribution expectations. |
src/test/java/ai/labs/eddi/engine/internal/groups/GroupCostLedgerTest.java |
Tests multiple child-cost aggregation. |
src/test/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrationsTest.java |
Tests versionless document migration. |
src/test/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilderWindowingTest.java |
Covers window rendering and summarization. |
src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceExtendedTest.java |
Tests new-document schema stamping. |
src/test/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrationsTest.java |
Pins the snapshot legacy sentinel. |
src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java |
Adds task-level token prices. |
src/main/java/ai/labs/eddi/modules/llm/impl/TokenPricing.java |
Centralizes token-cost arithmetic. |
src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java |
Records plain-call token costs. |
src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java |
Reuses centralized pricing. |
src/main/java/ai/labs/eddi/modules/llm/impl/CascadeConfigValidator.java |
Validates task-level prices. |
src/main/java/ai/labs/eddi/engine/memory/model/ConversationMemorySnapshot.java |
Separates legacy/current versions. |
src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java |
Stamps current snapshot versions. |
src/main/java/ai/labs/eddi/engine/internal/groups/PhaseExecutionEngine.java |
Passes window configuration into rendering. |
src/main/java/ai/labs/eddi/engine/internal/groups/GroupCostLedger.java |
Aggregates child and system costs. |
src/main/java/ai/labs/eddi/engine/internal/groups/GroupConversationSchemaMigrations.java |
Clarifies migration behavior. |
src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java |
Implements transcript summaries and truncation. |
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java |
Runs summaries and stamps new conversations. |
src/main/java/ai/labs/eddi/engine/internal/ConversationSchemaMigrations.java |
Documents snapshot migration semantics. |
src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java |
Warns about missing summarizer configuration. |
src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java |
Stores schema and rolling-summary state. |
src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java |
Defines transcript-window configuration. |
planning/group-collaboration-NEXT.md |
Marks prerequisite defects complete. |
docs/langchain.md |
Documents task-level token prices. |
docs/group-conversations.md |
Documents transcript windowing. |
docs/changelog.md |
Records implementation decisions and verification. |
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java:497
- This four-argument overload passes only
modelNametoChatModelRegistry; it supplies noapiKey,baseUrl, or other provider parameters. The existing conversation summarizer explicitly uses the five-argument overload because provider/model alone cannot authenticate, andContextWindowConfigcurrently has no parameters field. Consequently the documented OpenAI example falls back to truncation whenever a summary is attempted. Add a secure provider-parameters source (supporting vault/global references) and pass those parameters to the five-argument overload.
var result = summarizationService.summarizeWithUsage(content, WINDOW_SUMMARY_INSTRUCTIONS,
window.llmProvider(), window.llmModel());
src/main/java/ai/labs/eddi/engine/internal/groups/GroupContextBuilder.java:500
- A successful LLM call can return an empty summary with nonzero token usage, but this early return bypasses cost recording. Because the boundary is not advanced, every later phase boundary retries and all those paid attempts remain invisible to
maxCostPerDiscussion. Record the attempt's token cost before checking the summary text, and ensure retries at the same boundary accumulate as distinct spend rather than replacing one boundary-keyed value.
if (result.summary().isBlank()) {
LOGGER.warnf("Group %s: window summarization returned empty — keeping previous state, will retry next boundary", gc.getId());
return;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+475
to
+476
| int coverThrough = transcript.size() - window.maxRecentEntries(); | ||
| int alreadyCovered = anonymous ? gc.getAnonymousSummaryUpToIndex() : gc.getSummaryUpToIndex(); |
Member
Author
There was a problem hiding this comment.
Fixed in 7d47990 — new summaryBoundary walks back from the tail counting entries that pass isSummarizable (one shared predicate with renderForSummarizer, which is the same exclusion set the scope filter applies minus the phase-dependent VOTE/BID check — at a phase boundary every ballot on the transcript belongs to a completed phase, so no phase check is needed). Pinned by boundaryCountsVisibleEntries_notRawOnes: with bookkeeping rows interleaved in the tail, the three newest visible entries stay verbatim.
| // 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); |
Member
Author
There was a problem hiding this comment.
Fixed in 7d47990 — the boundary call is now gated with GroupCostLedger.wouldExceedCeiling(gc, protocol), matching the convergence judge and dissent round. The pinning test needed two attempts: the first scenario was vacuous because the per-turn gate fired before any boundary could be reached (the mutation survived); the committed test builds a first phase that completes under the per-turn gate while blowing the ceiling cumulatively, so the second phase's boundary is reached with the ceiling blown and a summarizer call otherwise guaranteed. Removing the guard fails exactly that test.
…boundary (PR #636 findings) - accumulateCost/recordSystemCost: !(x > 0) + isFinite instead of x <= 0 - NaN fails every comparison, slipped through, and silently disabled every dollar ceiling (NaN comparisons are all false) - the I9 boundary summarizer is now ceiling-gated via wouldExceedCeiling, like the convergence judge and dissent round (mutation-checked) - summaryBoundary counts VISIBLE entries back from the tail, not raw ones, so bookkeeping rows cannot eat the verbatim window - blank llmProvider/llmModel normalize to null in the config choke point - windowed filterByScope copies the live transcript under its monitor - CodeQL: sanitize gc id + group name in the new WARN sites - memberCosts Javadoc rewritten to the actual one-key-one-conversation invariant (judge/dissent keys predate the nested-child keys)
aisabella-ai
self-requested a review
August 8, 2026 08:03
aisabella-ai
approved these changes
Aug 8, 2026
ginccc
added a commit
that referenced
this pull request
Aug 8, 2026
…ion, tool provenance - AGENTS.md phase-8 row said 60+ MCP tools while every other doc now says 80+ (actual 82) - the opt-in-by-absence convention over-claimed: only artifactConfig and taskListConfig assemble tools; contextWindow/facilitator/humanMemberConfig gate behaviour, not tool assembly - #636 carried the pre-feature defects, not one of the nine items — the changelog and planning header now say #637-#645 with #636 named separately - the MCP table notes which group tools come from the HITL tool set, since they are not declared in McpGroupTools
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 12, 2026
Brings every user-facing doc in line with what shipped in PRs labsai#636-labsai#645. Each claim verified against source, not against the plan. - group-conversations.md: fixed a nonexistent REST endpoint and the stale HUMAN_DECIDES note; completed the REST (+21), MCP (+10), PhaseType, TaskStatus, ProtocolConfig and DynamicAgentConfig tables; new sections for per-phase controls, dissent and the 23 SSE events; documented the RETRO ceilings; fixed the orphaned task-cap paragraph, re-parented bid-based assignment under TASK_FORCE, unglued 8 headings - README: 6 -> 7 styles, ten new capability bullets, MCP 60+ -> 80+, tests 11,000+ -> 14,000+, OpenAI-compatible docs row - AGENTS: 7 styles, new 10c/10d roadmap rows, tests -> 14,000+, HITL row clarified, opt-in-by-absence convention documented in 4.2 - docs/README: 6.0.0 -> 6.2.0, MCP 48+ -> 80+, HITL + Open WebUI entries - SUMMARY: hitl.md and open-webui-integration.md were missing entirely - rag.md: gemini embedding provider and chroma vector store rows - planning: the group-collaboration queue is empty; PRs recorded
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes out the three pre-feature defects from
planning/group-collaboration-NEXT.md§2 (plus the §4 nested-cost gap flagged as N1's natural fold-in), unblocking the Wave 2/3 queue.N1 — Price ordinary model calls (
11d0812fb, fold-in89f2b15b4)AUDIT_COSTwas written fromcascadeCostUsd + toolCostUsdonly, so a plain model call — no cascade, no priced tool — contributed $0.00: I1'smaxCostPerDiscussionceiling could never trip for ordinary members, andmemberCosts/totalCostwere served over REST as if authoritative.LlmConfiguration.TaskgainsinputPricePer1M/outputPricePer1M— same nullable semantics as the cascade fields (null = unpriced → $0; no behaviour change for anyone not setting prices). Config-driven per Golden Rule 1: no hardcoded provider price table.TokenPricing, shared by the cascade path and the new plain-call path (§4.7 unification).cascadeCostUsdprevents double counting; pinned by a test with deliberately absurd task-level prices on a cascade turn.accumulateNestedGroupCostkeyed by agentId, but every GROUP-member turn spawns a fresh child discussion starting at $0, so only the last child's spend survived the re-sum. Attribution is now keyed per child discussion (agentId:childId) — replacement stays idempotent, children sum.N3 — Schema-version legacy sentinel (
65b00b600)Every pre-F6 document has no
schemaVersionkey; Jackson leaves the field initialiser (= CURRENT_SCHEMA_VERSION, i.e. 3) standing, so legacy documents loaded claiming schema 3 while being version-1-shaped andprepareForResume's ladder ran zero iterations on exactly the documents it exists for. Test written first (deserialise{}→ failed withexpected: <1> but was: <3>), then fixed: initialiser is nowLEGACY_SCHEMA_VERSION = 1, and the single creation point stampsCURRENTexplicitly.ConversationMemorySnapshotgot the identical split (correct only by coincidence while its CURRENT == 1). Stale Javadoc in both migration registries corrected. Free only while no released build has written a versioned document — hence before this ships.N2/I9 — Transcript windowing (
b6c0eb4c4)FULL/ANONYMOUS-scope phases re-fed the whole transcript to every member every turn (~quadratic prompt cost, compounding with every queued item). New
contextWindowgroup config: beyondmaxRecentEntries, older entries collapse into a rolling summary — extended incrementally at phase boundaries only via the sharedSummarizationService— or a plain[n earlier entries omitted]marker when summarization is off/unconfigured/failing (WARN, never blocks). ANONYMOUS keeps its own summary built from "Anonymous"-labelled input so it can never de-anonymize (deliberate deviation from the plan's single-field wording, recorded in the changelog). Summarizer spend lands on the I1 ledger when priced. The stored transcript is never modified; signing verifies raw entries.Verification
ai.labs.eddi.engine.internalsuite: 1438 green. LLM-module suites green apart from the known environmental HttpClient/socket failures. Checkstyle clean.Summary by CodeRabbit