You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(groups): SERVICE convergence judge - one direct model call per check - #1224
convergence.judge: SERVICE used to be a name only: with no model behind it the check silently fell back to the moderator agent, so a group with no moderator could never stop a repeating phase early, and a group with an expensive moderator paid a full agent turn per convergence check. This PR gives SERVICE its own model: one direct LLM call per check, no agent, no pipeline, no moderator required.
What changed
Backend
ServiceJudgeConfig is a new convergence.service block (provider, model, parameters, optional per-1M prices, timeoutMs). It is the 6th component of ConvergenceConfig; the old 5-arg constructor is kept so existing code and stored configs behave as before.
ServiceConvergenceJudge calls SummarizationService.summarizeWithUsage with a fixed system instruction (member text is declared to be material, not instructions); the judge prompt travels as the user message, as data. The timeout is forced from timeoutMs (capped at 60 s), overriding any timeout in parameters.
Spend is booked on the discussion ledger as system:judge:<phaseIndex>:<repeat> plus a cost_updated event, so it shows in memberCosts / totalCost. A reply that cannot be read was still paid for and is still booked.
The judge never ends a phase on doubt: provider error, timeout or unparseable reply all mean "not converged". The provider error text is not copied into the transcript.
JudgeCallPricing is the single pricing seam; it uses TokenPricing.cost for now. The F1 cost-accounting work will re-point it (that PR is not open yet; this one does not depend on it).
ServiceJudgeValidation runs at save time: it rejects plaintext credentials in parameters (400 naming the field, never the value), rejects ${connection:...} references, and rejects a provider/model pair with only one half set. It warns on SERVICE with no model (falls back to the moderator) and on an unknown judge value.
Timer eddi.group.convergence.judge.duration, tags judge=service, mode, outcome; charted as panel id 320 in the Full Metrics Reference (k8s copies re-synced) and listed in docs/metrics.md.
Manager: the phase editor shows the service fields (provider, model, prices, timeout) when Judge is set to Service; en plus the ten other locales.
Docs: new docs/group-service-judge.md, a docs/group-conversations.md pointer, a docs/SUMMARY.md line, changelog fragment, and planning/operator-followups/group-service-judge.md.
Off by default: nothing changes unless a phase opts into judge: SERVICEand names a model. MODERATOR stays the default judge, and SERVICE without a model still runs the moderator exactly as before.
What this breaks
Nothing for stored configs: the new service field is optional and absent on every existing document. A config saved earlier with judge: SERVICE and no model behaves as before and now logs a warning on re-save instead of failing. No REST path, MCP tool or A2A change; the group configuration gains one optional nested object.
Rolling upgrade: configs are only affected once a designer adds convergence.service; until every node runs this version, do not add the block (a node without the field cannot honour it).
Look hardest at
PhaseExecutionEngine.runJudge (the dispatch between the two judges and the budget/maxTurns gates both share).
ServiceJudgeValidation: the credential classifier and the ${connection:...} refusal, since this is the one place a group configuration could become a plaintext key store.
Verification
Compile (Checkstyle + formatter:validate) clean. New unit tests: ServiceConvergenceJudgeTest, ServiceJudgeValidationTest.
Docker was down on the build machine, so no integration tests ran locally; the store round-trip was tested through the production serializer. CI is the first run of the ITs.
Merge order and conflicts
Independent of the sibling group PRs. Expect small textual conflicts with them in docs/SUMMARY.md (each adds a line under Group Conversations, keep all), GroupConversationService (constructor end), PhaseExecutionEngine, AgentGroupStore imports, the locale JSON files and group-phase-editor.tsx.
Follow-ups
Platform Operator prompt and allow-list updates are deliberately not in this PR; they are tracked in planning/operator-followups/group-service-judge.md for a combined integration PR.
Summary by CodeRabbit
New Features
Group conversation phases can use a configured service model to judge convergence, with settings for provider, model, timeout, and token pricing.
When the service model is not configured, judging falls back to the moderator. Service-call costs are recorded, and call duration and outcomes are available in monitoring.
The group editor now supports service-judge settings, and configuration checks flag incomplete settings and unsafe credential values.
Documentation
Added guidance on service-based judging, fallback behavior, configuration, and monitoring metrics.
A phase with judge SERVICE and a convergence.service block (provider, model, reference-only parameters, prices, timeout) now asks that model directly through SummarizationService instead of running a moderator-agent turn. It runs without a moderator, books its spend on the ledger as system:judge:<phase>:<repeat>, and never ends a phase on a failure or an unreadable reply. SERVICE without the block keeps the moderator fallback. Save time refuses plaintext credentials and connection references in the parameters.
…e editor
Judge = Service now shows provider, model, an API key reference, both prices and the timeout, and says when it falls back to the moderator. normalizeConvergence carries the service block through instead of dropping it. Strings live in a new groupServiceJudge namespace in all 11 locales; the stale 'not yet wired' note is removed.
New page group-service-judge.md, linked from SUMMARY.md and from the convergence paragraph of group-conversations.md, which no longer calls SERVICE unwired. Changelog fragment and the deferred operator/MCP follow-up note.
Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).
The PR adds a SERVICE convergence judge that can call a configured model directly. It adds service settings, save-time validation, runtime fallback and cost accounting, Manager controls, and documentation and metrics updates.
Changes: Service Convergence Judge
Layer / File(s)
Summary
Service configuration and save-time validation src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java, src/main/java/ai/labs/eddi/configs/groups/model/ServiceJudgeConfig.java, src/main/java/ai/labs/eddi/configs/groups/validation/ServiceJudgeValidation.java, src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java, src/test/java/ai/labs/eddi/configs/groups/validation/ServiceJudgeValidationTest.java
Convergence configuration adds an optional service block for provider, model, parameters, prices, and timeout. Save-time validation reports incomplete provider/model pairs and credential parameters that do not use accepted references.
The phase engine tries the service judge before the moderator judge. The service judge calls the summarization service with one attempt, handles failures and unreadable replies as non-converged results, records priced calls, and records duration and outcome metrics.
The Manager adds provider, model, API-key reference, timeout, and price fields for SERVICE judging. Configuration normalization preserves the service block, and tests cover saving settings and removing an API key. Locale files add matching labels and behavior notes.
Documentation and monitoring docs/group-service-judge.md, docs/group-conversations.md, docs/metrics.md, docs/monitoring/eddi-full-metrics-dashboard.json, docs/changelog.d/2026-11-02-group-service-judge.md, docs/SUMMARY.md, planning/operator-followups/group-service-judge.md
Documentation describes SERVICE judging, configuration, validation, fallback, accounting, and failure handling. The metrics reference and dashboard add judge-call duration information.
sequenceDiagram
participant PhaseExecutionEngine
participant ServiceConvergenceJudge
participant SummarizationService
participant GroupDiscussionEventListener
PhaseExecutionEngine->>ServiceConvergenceJudge: request a verdict for a phase repeat
ServiceConvergenceJudge->>SummarizationService: call with configured provider, model, parameters, and retry policy
SummarizationService-->>ServiceConvergenceJudge: return reply and usage
ServiceConvergenceJudge->>GroupDiscussionEventListener: announce cost for a priced completed call
ServiceConvergenceJudge-->>PhaseExecutionEngine: return verdict or non-converged result
Loading
Security architecture risk: High · up to 7b3b1
Editable configuration gains a new way to use stored credentials, but equivalent permission enforcement has not been established. Destination restrictions and bounded execution reduce exposure without resolving the potential unauthorized credential use.
Retained concerns
High · security · inferred: The new group-owned SERVICE path reaches credential resolution without demonstrated equivalent enforcement of the deployed-agent secret grants. Save-time validation accepts vault and variable references, while the resolver and vault provider resolve named credentials without checking caller or agent entitlement. An editor able to save and execute a configured group could therefore potentially use a non-reserved credential outside its allowed-agent grants, including a tenant-qualified reference. Destination restrictions limit where credentials can be used but do not establish authorization to use them. Complete outer save-policy coverage remains unresolved.
Security review details
Security Blast Radius
inferred — The authorization concern requires configuration-editing authority and the ability to execute an affected group. Its potential scope is not limited to that discussion's ledger: a known tenant-qualified, non-reserved credential reachable by the shared vault provider could be used for external requests, subject to destination restrictions. Raw secret disclosure and unrestricted cross-tenant compromise were not verified.
Security Findings and Attack Paths
inferred — The material attack path is an authorized group editor supplying a credential reference, running SERVICE convergence, and causing the shared resolver to obtain that credential for provider construction without the usual deployed-agent grant decision. The brief contains no retained Security findings; this is a newly identified architecture concern with incomplete outer-policy coverage.
Trust Boundaries and Controls
observed — Existing controls reject unresolved vault references, refuse reserved internal secret namespaces, check vault credentials against configured endpoint hosts, and validate provider endpoints. Save-time errors omit credential values, and SERVICE failures publish a fixed failure summary rather than provider exception text. These controls constrain egress and leakage but are not substitutes for credential-use authorization.
Resilience and Maintainability Implications
observed — SERVICE requests one outer attempt and overwrites parameter-supplied timeout values with a defaulted, capped timeout of at most 60 seconds. Construction and secret resolution precede the retry-wrapped chat operation. Immediate cancellation signals the discussion and cancels its active future, but the direct judge receives no cancellation token; prompt interruption of an in-flight provider call is not established.
Hardening Proposals
proposed — Define an explicit authorization principal for group-owned provider credentials and enforce its tenant and secret-use permissions before resolution or cached-client reuse. Include references reached through variables, while retaining independent destination restrictions.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the feature (a SERVICE convergence judge) and its key behavior (one direct model call per check), which matches the changeset: ServiceConvergenceJudge, ServiceJudgeConfig, ServiceJudge…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the feature (a SERVICE convergence judge) and its key behavior (one direct model call per check), which matches the changeset: ServiceConvergenceJudge, ServiceJudgeConfig, ServiceJudgeValidation, the PhaseExecutionEngine wiring, the manager UI fields, and the docs. It does not mention the validation, metrics, or UI parts, but it does not need to cover every detail. The title is specific and uses the conventional feat(scope) form.
Full details: Docstring Coverage
Explanation
Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 17 functions across 17 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
Your included review limit is currently reached under our Fair Usage Limits Policy. Your current included review allowance is based on your included PR review attempts over the past 7 days. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 34 minutes.
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
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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
convergence.judge: SERVICEused to be a name only: with no model behind it the check silently fell back to the moderator agent, so a group with no moderator could never stop a repeating phase early, and a group with an expensive moderator paid a full agent turn per convergence check. This PR givesSERVICEits own model: one direct LLM call per check, no agent, no pipeline, no moderator required.What changed
Backend
ServiceJudgeConfigis a newconvergence.serviceblock (provider, model, parameters, optional per-1M prices,timeoutMs). It is the 6th component ofConvergenceConfig; the old 5-arg constructor is kept so existing code and stored configs behave as before.ServiceConvergenceJudgecallsSummarizationService.summarizeWithUsagewith a fixed system instruction (member text is declared to be material, not instructions); the judge prompt travels as the user message, as data. The timeout is forced fromtimeoutMs(capped at 60 s), overriding anytimeoutinparameters.system:judge:<phaseIndex>:<repeat>plus acost_updatedevent, so it shows inmemberCosts/totalCost. A reply that cannot be read was still paid for and is still booked.JudgeCallPricingis the single pricing seam; it usesTokenPricing.costfor now. The F1 cost-accounting work will re-point it (that PR is not open yet; this one does not depend on it).ServiceJudgeValidationruns at save time: it rejects plaintext credentials inparameters(400 naming the field, never the value), rejects${connection:...}references, and rejects a provider/model pair with only one half set. It warns onSERVICEwith no model (falls back to the moderator) and on an unknownjudgevalue.eddi.group.convergence.judge.duration, tagsjudge=service,mode,outcome; charted as panel id 320 in the Full Metrics Reference (k8s copies re-synced) and listed indocs/metrics.md.Manager: the phase editor shows the service fields (provider, model, prices, timeout) when Judge is set to Service; en plus the ten other locales.
Docs: new
docs/group-service-judge.md, adocs/group-conversations.mdpointer, adocs/SUMMARY.mdline, changelog fragment, andplanning/operator-followups/group-service-judge.md.Config surface
Off by default: nothing changes unless a phase opts into
judge: SERVICEand names a model.MODERATORstays the default judge, andSERVICEwithout a model still runs the moderator exactly as before.What this breaks
Nothing for stored configs: the new
servicefield is optional and absent on every existing document. A config saved earlier withjudge: SERVICEand no model behaves as before and now logs a warning on re-save instead of failing. No REST path, MCP tool or A2A change; the group configuration gains one optional nested object.Rolling upgrade: configs are only affected once a designer adds
convergence.service; until every node runs this version, do not add the block (a node without the field cannot honour it).Look hardest at
PhaseExecutionEngine.runJudge(the dispatch between the two judges and the budget/maxTurnsgates both share).ServiceJudgeValidation: the credential classifier and the${connection:...}refusal, since this is the one place a group configuration could become a plaintext key store.Verification
formatter:validate) clean. New unit tests:ServiceConvergenceJudgeTest,ServiceJudgeValidationTest.ImportStyleTest,BuildQualityGatesTest,ChangelogRotationTest,ChangelogFragmentTest,Documentation*Tests,DocumentedRestPathsTest,DeploymentManifestsTest,MetricsDashboardCoverageTest,DescriptorTypeConsistencyTest,ConfigurationReferenceCoverageTest.Merge order and conflicts
Independent of the sibling group PRs. Expect small textual conflicts with them in
docs/SUMMARY.md(each adds a line under Group Conversations, keep all),GroupConversationService(constructor end),PhaseExecutionEngine,AgentGroupStoreimports, the locale JSON files andgroup-phase-editor.tsx.Follow-ups
Platform Operator prompt and allow-list updates are deliberately not in this PR; they are tracked in
planning/operator-followups/group-service-judge.mdfor a combined integration PR.Summary by CodeRabbit