fix(agents): dynamic-agent guardrails — permissive fallback on resume, per-member caps, duplicate recruits, V7 - #649
Merged
Merged
Conversation
…, per-member caps, duplicate recruits, V7 Wave B of the Agent / Group Agent review. These guard the highest-blast-radius capability in the product -- an LLM deploying agents to production -- and were the weakest-enforced things in the system. 1. CRITICAL: a group's dynamicAgents policy silently reverted to fully permissive on a resumed member turn. resolveDynamicAgentConfig accepted only a TYPED DynamicAgentConfig out of the context value, but a Context whose value round-trips through the conversation store comes back as a raw LinkedHashMap. Any turn against a reloaded step therefore missed the instanceof and fell through to the permissive standalone default -- creation, recruitment and delegation all ON for a group that may have disabled every one. The trigger is ordinary: a member's gated tool call is auto-rejected by tryResolveMemberToolPause, which resumes the member conversation, and Conversation#resume re-enters the same LlmTask at the same index against memory freshly loaded from the store. Resolution is now three-state and fails closed -- absent means standalone, present and readable (typed or map) means the group's policy, present but unreadable means every capability off. 2. maxCreatedAgentsPerDiscussion was enforced per member conversation. seedCreatedAgentIds always read a dynamicCreatedAgentIds context variable for the discussion-wide total, but nothing wrote it, so a 5-member group with the default cap of 5 could deploy 25 agents to production. MemberTurnExecutor now injects gc.getCreatedAgentIds(). 3. RecruitAgentTool could re-recruit a configured member. isAlreadyMember checked memberConversationIds, which holds an agent only once it has SPOKEN, so a member whose first turn had not come up could be recruited as a duplicate -- consuming the cap, writing a misleading transcript entry, and overwriting the operator-chosen display name with a raw agent id. The tool now receives the configured roster, and display-name recording is putIfAbsent. 4. Teardown never freed a creation slot, and a failed delete orphaned the agent. createdAgentIds.remove ran before the delete, and the removal was from a per-turn list rebuilt from every earlier step. Teardown now records into dynamic:torn_down_agent_ids, which the seed subtracts and propagateDynamicAgentTracking applies to the group's tracking. 5. V7 resolved: an omitted builtInToolsWhitelist no longer skips the dynamic-agent tools. docs/langchain.md states twice that omitting the whitelist enables all built-in tools. Deliberately narrower in one respect -- the omitted case is honoured only under a governing group policy, because dynamicAgents is a group-config field and a standalone conversation has no surface on which an operator could have declined. The three tracking keys moved to MemoryKeys so both sides name one constant. Behaviour changes, deliberate: a group whose policy cannot be read now gets no dynamic-agent capabilities instead of all of them; an agent in a group with enableBuiltInTools=true and no whitelist now receives the tools its group policy permits. 869 + 487 existing tests green; 25 new.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDynamic-agent guardrails now resolve group policies with fail-closed behavior, enforce discussion-wide creation limits, prevent duplicate recruitment, preserve configured display names, and track successful teardowns across conversation and group lifecycle operations. ChangesDynamic-agent guardrails
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentOrchestrator
participant DynamicAgentToolsProvider
participant IAgentGroupStore
participant RecruitAgentTool
participant TeardownAgentTool
AgentOrchestrator->>DynamicAgentToolsProvider: provide whitelist and group store
DynamicAgentToolsProvider->>IAgentGroupStore: resolve policy and configured members
IAgentGroupStore-->>DynamicAgentToolsProvider: return policy and roster
DynamicAgentToolsProvider->>RecruitAgentTool: provide roster IDs
RecruitAgentTool-->>DynamicAgentToolsProvider: allow or reject recruitment
DynamicAgentToolsProvider->>TeardownAgentTool: provide lifecycle tracking
TeardownAgentTool-->>DynamicAgentToolsProvider: record successful teardown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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: 8
🧹 Nitpick comments (2)
src/test/java/ai/labs/eddi/modules/llm/tools/DynamicAgentToolsTest.java (1)
944-945: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the new teardown-tracking argument observable.
The
ConcurrentHashMap.newKeySet()at Lines [944-945] is discarded. These tests cannot detect whetherTeardownAgentToolrecords successful teardowns or leaves failed deletions retryable.Store the set as
tornDownAgentIds, pass it to the tool, and assert the set after the successful teardown path and afterteardownAgent_deleteFailure.Proposed fixture adjustment
private List<String> createdAgentIds; private Set<String> retainedAgentIds; + private Set<String> tornDownAgentIds; private TeardownAgentTool tool; ... createdAgentIds = new CopyOnWriteArrayList<>(List.of("created-1", "created-2")); retainedAgentIds = ConcurrentHashMap.newKeySet(); + tornDownAgentIds = ConcurrentHashMap.newKeySet(); tool = new TeardownAgentTool(agentFactory, agentStore, deploymentStore, createdAgentIds, retainedAgentIds, - ConcurrentHashMap.newKeySet()); + tornDownAgentIds);🤖 Prompt for 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. In `@src/test/java/ai/labs/eddi/modules/llm/tools/DynamicAgentToolsTest.java` around lines 944 - 945, Update the DynamicAgentToolsTest fixture around TeardownAgentTool construction to store the ConcurrentHashMap.newKeySet() in a tornDownAgentIds field or local, then pass that same set to the tool. Add assertions verifying the expected contents after the successful teardown path and after teardownAgent_deleteFailure, including that failed deletions remain retryable.src/test/java/ai/labs/eddi/modules/llm/tools/RecruitAgentToolTest.java (1)
294-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test exercise
addMemberDisplayNameIfAbsent.
configuredMemberIds.add(TARGET)makesrecruitAgentreturn at Line [123] before the changed display-name method runs. The test therefore passes with the old unconditionalputand does not verify the new behavior.Use an unconfigured target with a pre-existing
"Alice"mapping, assert that recruitment succeeds, and then assert that the mapping remains"Alice".Proposed test adjustment
- void recruit_aConfiguredMember_doesNotClobberItsDisplayName() { - configuredMemberIds.add(TARGET); + void recruit_withExistingDisplayName_doesNotClobberIt() { gc.addMemberDisplayName(TARGET, "Alice"); - tool().recruitAgent(TARGET, "SecurityReviewer", "we need review"); + String result = tool().recruitAgent(TARGET, "SecurityReviewer", "we need review"); + assertTrue(result.startsWith("Recruited"), result); assertEquals("Alice", gc.getMemberDisplayNames().get(TARGET),🤖 Prompt for 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. In `@src/test/java/ai/labs/eddi/modules/llm/tools/RecruitAgentToolTest.java` around lines 294 - 303, Update recruit_aConfiguredMember_doesNotClobberItsDisplayName to remove configuredMemberIds.add(TARGET), keeping the pre-existing "Alice" display-name mapping. Assert that tool().recruitAgent(TARGET, "SecurityReviewer", "we need review") succeeds, then verify the mapping remains "Alice" so the test exercises addMemberDisplayNameIfAbsent.
🤖 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/GroupConversation.java`:
- Around line 1204-1216: Update setMemberDisplayNames to always install a
ConcurrentHashMap, including when the provided input is null and when copying
non-null entries. Preserve the existing member display names while maintaining
the concurrent-map invariant required by addMemberDisplayNameIfAbsent and
concurrent persistence iteration.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java`:
- Around line 742-746: The created-agent merge in the dynamic ID propagation
logic must be atomic to prevent duplicate IDs from concurrent callbacks. Replace
the check-then-add behavior around gc.getCreatedAgentIds() with an atomic set,
or synchronize both contains/add and corresponding remove operations using the
same group-level lock so teardown reliably removes the ID.
- Around line 742-771: The group conversation tracking in the step-data
propagation logic must prevent stale DYNAMIC_CREATED_AGENT_IDS snapshots from
re-adding agents removed by teardown. Persist group-level teardown tombstones or
apply snapshots using a monotonic turn/version, and make the update thread-safe
so teardown and snapshot processing cannot race. Ensure later created-agent
snapshots ignore IDs already marked torn down while preserving valid agent
tracking.
- Around line 733-771: Update propagateDynamicAgentTracking to persist its
createdAgentIds and retainedAgentIds mutations before terminal cleanup callbacks
run. Add a guarded conversationStore.update or updateIfState using the existing
conversation identity/state safeguards, ensuring timed-out or abandoned-member
paths save tracking changes without overwriting newer conversation state.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`:
- Around line 318-333: Update the dispatch/retry flow around
MemberTurnExecutor’s dynamicCreatedAgentIds context so each attempt reads the
latest group state instead of reusing a stale InputData snapshot. Add a
synchronized or atomic group-level reservation mechanism that reserves creation
capacity before member callbacks execute and releases or reconciles it on
completion, ensuring overlapping turns cannot exceed
maxCreatedAgentsPerDiscussion. Add a concurrency test covering two overlapping
member turns with a cap of one.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/DynamicAgentToolsProvider.java`:
- Around line 127-152: Update configuredMemberIds to return an explicit
unavailable result for missing, null, or unreadable roster data while preserving
a distinct successful empty roster; adjust the recruit_agent construction path
around RecruitAgentTool to omit recruitment whenever the roster is unavailable,
and pass the loaded member IDs only when available.
- Around line 298-300: Update the teardown_agent registration in
DynamicAgentToolsProvider to also require dynamicConfig.isEnabled(), so it is
not added when resolveDynamicAgentConfig applies a disabled or fail-closed
policy. Add an integration test covering an unreadable group policy with an
omitted whitelist and verify TeardownAgentTool is not exposed.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/RecruitAgentTool.java`:
- Around line 83-98: Preserve whether the configured roster was successfully
loaded instead of collapsing missing or failed loads into an empty set: update
DynamicAgentToolsProvider.configuredMemberIds(...) and RecruitAgentTool
construction to carry an explicit unavailable state alongside
configuredMemberIds. In RecruitAgentTool.isAlreadyMember or the recruitment
entry point, refuse recruitment for live groups while the roster is unavailable,
while retaining normal empty-roster behavior after a successful read. Add a test
covering roster-loading failure and verifying recruitment is rejected.
---
Nitpick comments:
In `@src/test/java/ai/labs/eddi/modules/llm/tools/DynamicAgentToolsTest.java`:
- Around line 944-945: Update the DynamicAgentToolsTest fixture around
TeardownAgentTool construction to store the ConcurrentHashMap.newKeySet() in a
tornDownAgentIds field or local, then pass that same set to the tool. Add
assertions verifying the expected contents after the successful teardown path
and after teardownAgent_deleteFailure, including that failed deletions remain
retryable.
In `@src/test/java/ai/labs/eddi/modules/llm/tools/RecruitAgentToolTest.java`:
- Around line 294-303: Update
recruit_aConfiguredMember_doesNotClobberItsDisplayName to remove
configuredMemberIds.add(TARGET), keeping the pre-existing "Alice" display-name
mapping. Assert that tool().recruitAgent(TARGET, "SecurityReviewer", "we need
review") succeeds, then verify the mapping remains "Alice" so the test exercises
addMemberDisplayNameIfAbsent.
🪄 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: e714038a-d179-4933-b59c-df15157a878d
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.javasrc/main/java/ai/labs/eddi/engine/memory/MemoryKeys.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/DynamicAgentToolsProvider.javasrc/main/java/ai/labs/eddi/modules/llm/tools/RecruitAgentTool.javasrc/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.javasrc/test/java/ai/labs/eddi/modules/llm/impl/DynamicAgentGuardrailResolutionTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/DynamicAgentToolsProviderTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/DynamicAgentWhitelistAndCapTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/DynamicAgentToolsTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/RecruitAgentToolTest.java
Comment on lines
+733
to
+771
| // Teardowns are applied AFTER the additions below, not interleaved with them: | ||
| // the same step carries both the cumulative created list (which still names an | ||
| // agent torn down this turn) and the teardown record, and iteration order over | ||
| // step data is not something this should depend on. | ||
| Set<String> tornDown = new LinkedHashSet<>(); | ||
| for (var stepData : lastStep.getConversationStep()) { | ||
| if (stepData == null || stepData.getKey() == null) { | ||
| continue; | ||
| } | ||
| if ("dynamic:created_agent_ids" | ||
| .equals(stepData.getKey()) && stepData.getValue() instanceof java.util.Collection<?> ids) { | ||
| if (MemoryKeys.DYNAMIC_CREATED_AGENT_IDS.equals(stepData.getKey()) && stepData.getValue() instanceof Collection<?> ids) { | ||
| for (Object id : ids) { | ||
| if (id instanceof String agentId && !gc.getCreatedAgentIds().contains(agentId)) { | ||
| gc.getCreatedAgentIds().add(agentId); | ||
| LOGGER.debugf("[DYNAMIC] Propagated created agent '%s' to group conversation", agentId); | ||
| } | ||
| } | ||
| } else if ("dynamic:retained_agent_ids" | ||
| .equals(stepData.getKey()) && stepData.getValue() instanceof java.util.Collection<?> ids) { | ||
| } else if (MemoryKeys.DYNAMIC_RETAINED_AGENT_IDS.equals(stepData.getKey()) && stepData.getValue() instanceof Collection<?> ids) { | ||
| for (Object id : ids) { | ||
| if (id instanceof String agentId) { | ||
| gc.getRetainedAgentIds().add(agentId); | ||
| } | ||
| } | ||
| } else if (MemoryKeys.DYNAMIC_TORN_DOWN_AGENT_IDS.equals(stepData.getKey()) && stepData.getValue() instanceof Collection<?> ids) { | ||
| for (Object id : ids) { | ||
| if (id instanceof String agentId) { | ||
| tornDown.add(agentId); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // A torn-down agent is gone: drop it from the group's tracking so the | ||
| // discussion-wide created total injected into the NEXT member turn does not | ||
| // keep it occupying a maxCreatedAgentsPerDiscussion slot, and so ephemeral | ||
| // cleanup does not try to undeploy and delete something already deleted. | ||
| for (String agentId : tornDown) { | ||
| if (gc.getCreatedAgentIds().remove(agentId)) { | ||
| LOGGER.debugf("[DYNAMIC] Dropped torn-down agent '%s' from group conversation tracking", agentId); | ||
| } | ||
| gc.getRetainedAgentIds().remove(agentId); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'propagateDynamicAgentTracking|cleanupEphemeralAgents|conversationStore\.(update|updateIfState)|executeDiscussion' \
src/main/java/ai/labs/eddi/engine/internal/groups --glob '*.java'Repository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and outline the key files without executing repository code.
fd -a 'GroupLifecycleOps\.java|MemberTurnExecutor\.java|GroupConversationStore.*\.java|GroupLifecycleOps.*\.java' src/main/java | sed 's#^\./##' | sort
echo '---'
ast-grep outline src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java --view expanded | sed -n '1,220p'
echo '---'
ast-grep outline src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java --view expanded | sed -n '1,240p'Repository: labsai/EDDI
Length of output: 3758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect focused slices around dynamic tracking propagation and member turn callback flow.
sed -n '700,790p' src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java | cat -n
echo '======'
sed -n '1,260p' src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java | cat -n
echo '======'
sed -n '260,520p' src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java | cat -nRepository: labsai/EDDI
Length of output: 39899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for usages of GroupConversationService.propagateDynamicAgentTracking and surrounding persistence points.
rg -n -C 18 \
'GroupConversationService\.propagateDynamicAgentTracking|propagateDynamicAgentTracking\(|conversationStore\.(update|updateIfState)' \
src/main/java/ai/labs/eddi/engine/internal/groups --glob '*.java'
echo '==== GroupLifecycleOps execute/fail/cleanup slice ====='
sed -n '530,718p' src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java | cat -n
echo '==== MemberTurnExecutor remaining core slice ====='
sed -n '520,820p' src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java | cat -nRepository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect exactly where propagateDynamicAgentTracking is called and whether the GroupConversation snapshot is written to MongoDB.
rg -n -C 24 \
'GroupConversationService\.propagateDynamicAgentTracking|propagateDynamicAgentTracking\(' src/main/java --glob '*.java'
echo '==== Store update methods and usages containing created/retained/teardown terms maybe ====='
rg -n -C 8 \
'getCreatedAgentIds|setCreatedAgentIds|getRetainedAgentIds|setRetainedAgentIds|TORN_DOWN_DYNAMIC_AGENT|cleanupEphemeralAgents|propagateDynamicAgentTracking' src/main/java --glob '*.java'Repository: labsai/EDDI
Length of output: 50367
Persist dynamic agent tracking before late callbacks.
propagateDynamicAgentTracking updates only the in-memory GroupConversation, and none of the call sites followed by terminal teardown writes those mutations back with conversationStore.update(...) or conversationStore.updateIfState(...). Add a guarded persisted update before terminal cleanup so a timed-out or abandoned-member callback cannot lose createdAgentIds/retainedAgentIds changes.
🤖 Prompt for 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.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java`
around lines 733 - 771, Update propagateDynamicAgentTracking to persist its
createdAgentIds and retainedAgentIds mutations before terminal cleanup callbacks
run. Add a guarded conversationStore.update or updateIfState using the existing
conversation identity/state safeguards, ensuring timed-out or abandoned-member
paths save tracking changes without overwriting newer conversation state.
Comment on lines
+318
to
+333
| // The discussion-wide created-agent total, so maxCreatedAgentsPerDiscussion | ||
| // bounds the DISCUSSION rather than each member conversation independently. | ||
| // | ||
| // DynamicAgentToolsProvider.seedCreatedAgentIds has always read this context | ||
| // key, but nothing wrote it: it could only see the ids in the member's OWN | ||
| // conversation memory, so a 5-member group with the default cap of 5 could | ||
| // deploy 25 agents to production per discussion while both the field name and | ||
| // docs/group-conversations.md promised 5. gc.getCreatedAgentIds() is the real | ||
| // total — propagateDynamicAgentTracking folds every member's creations into it | ||
| // at the end of each turn. | ||
| // | ||
| // Snapshot, not the live CopyOnWriteArrayList: this rides into another | ||
| // conversation's memory and gets serialised there. | ||
| context.put("dynamicCreatedAgentIds", | ||
| new Context(Context.ContextType.object, List.copyOf(gc.getCreatedAgentIds()))); | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reserve discussion-wide creation capacity before dispatch.
List.copyOf(gc.getCreatedAgentIds()) captures only the IDs known at that moment. The same InputData is reused across retries, and other member callbacks can update gc after this snapshot is created. Parallel or late member turns can therefore evaluate maxCreatedAgentsPerDiscussion with a stale count and create too many agents.
Refresh the context for each attempt, and enforce an atomic group-level reservation for in-flight creations. Add a test with two overlapping member turns and a cap of one.
As per coding guidelines, backend code must be thread-safe.
🤖 Prompt for 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.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`
around lines 318 - 333, Update the dispatch/retry flow around
MemberTurnExecutor’s dynamicCreatedAgentIds context so each attempt reads the
latest group state instead of reusing a stale InputData snapshot. Add a
synchronized or atomic group-level reservation mechanism that reserves creation
capacity before member callbacks execute and releases or reconciles it on
completion, ensuring overlapping turns cannot exceed
maxCreatedAgentsPerDiscussion. Add a concurrency test covering two overlapping
member turns with a cap of one.
Source: Coding guidelines
Comment on lines
+83
to
+98
| /** | ||
| * The ids on the group's CONFIGURED roster. Supplied at construction because | ||
| * the tool sees only the {@link DynamicAgentConfig}, never the group config — | ||
| * which is why {@link #isAlreadyMember} could not check the roster and this | ||
| * field exists. | ||
| */ | ||
| private final Set<String> configuredMemberIds; | ||
|
|
||
| public RecruitAgentTool(LiveDiscussionRegistry registry, String groupConversationId, String recruiterAgentId, | ||
| DynamicAgentConfig config, IDeploymentStore deploymentStore) { | ||
| DynamicAgentConfig config, IDeploymentStore deploymentStore, Set<String> configuredMemberIds) { | ||
| this.registry = registry; | ||
| this.groupConversationId = groupConversationId; | ||
| this.recruiterAgentId = recruiterAgentId; | ||
| this.config = config; | ||
| this.deploymentStore = deploymentStore; | ||
| this.configuredMemberIds = configuredMemberIds != null ? Set.copyOf(configuredMemberIds) : Set.of(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not fail open when the configured roster cannot be read.
configuredMemberIds represents only a set. DynamicAgentToolsProvider.configuredMemberIds(...) returns Set.of() when the group resource is missing or roster loading fails. The tool cannot distinguish an empty roster from an unavailable roster.
For a live group, a deployed configured member that has not spoken can then pass isAlreadyMember, consume a recruitment slot, and enter the dynamic roster as a duplicate. Preserve an explicit unavailable state and refuse recruitment until the roster is read successfully. Add a failure-path test.
Also applies to: 179-191
🤖 Prompt for 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.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/RecruitAgentTool.java` around
lines 83 - 98, Preserve whether the configured roster was successfully loaded
instead of collapsing missing or failed loads into an empty set: update
DynamicAgentToolsProvider.configuredMemberIds(...) and RecruitAgentTool
construction to carry an explicit unavailable state alongside
configuredMemberIds. In RecruitAgentTool.isAlreadyMember or the recruitment
entry point, refuse recruitment for live groups while the roster is unavailable,
while retaining normal empty-roster behavior after a successful read. Add a test
covering roster-loading failure and verifying recruitment is rejected.
This was referenced Aug 9, 2026
…down tombstone, atomic merge Six of seven CodeRabbit findings on #649; the seventh is declined with reasoning below. Major, security -- teardown_agent was not gated on the policy. Its assembly branch checked only that the stores were present, and the tool itself takes no DynamicAgentConfig, so a group whose policy is disabled -- or unreadable, which now resolves fail-closed -- could still undeploy and PERMANENTLY DELETE a tracked agent. A hole the V7 change widened, since an omitted whitelist now reaches this branch. Gated on dynamicConfig.isEnabled(). Major -- an unreadable roster failed open. configuredMemberIds returned an empty set for both "no members" and "could not read", so a store hiccup silently restored the duplicate-recruit defect the change exists to prevent. It now returns Optional, and an unavailable roster withholds recruit_agent for that turn -- gate by absence, matching ArtifactToolsProvider. Major -- stale snapshots could resurrect a torn-down agent. Each member's tracking snapshot is one member's view: member B's turn can still name an agent member A tore down in between, and the merge re-added it. GroupConversation now carries a tornDownAgentIds tombstone, written before the merge by recordTeardown and consulted by it, so a teardown is final regardless of arrival order. Major -- the created-agent merge was not atomic. CopyOnWriteArrayList makes each add atomic but not contains()-then-add(), and merges run on one coordinator thread per member turn; two could both append the same id, after which the single remove() a teardown performs leaves a duplicate. The compound operation now runs under the list's monitor, the same pattern RecruitAgentTool uses for recruitedAgentIds. Major -- setMemberDisplayNames installed a LinkedHashMap, so every reload dropped the concurrency guarantee the field declares, on a map written from member-turn threads and iterated by serialization. Now ConcurrentHashMap on both branches. Declined -- atomic reservation of creation capacity before dispatch. Correct that parallel member turns can each read a stale count and collectively overshoot. That is the same accepted-overshoot shape this codebase already documents for the cost ceiling ("an in-flight turn may still push the total past the ceiling; that overshoot is accepted, not prevented" -- GroupCostLedger), and closing it properly needs budget RESERVATION at dispatch, which is a design change rather than a fix. The cap moves from unbounded-per-member to bounded-with-parallel-overshoot; the residue is recorded rather than silently fixed. 1107 unit tests green; 7 new, the tombstone guard mutation-verified.
Member
Author
|
Thanks @coderabbitai — six of seven fixed, one declined with reasoning. Fixed
Declined — atomic reservation of creation capacity before dispatch The analysis is right: parallel member turns each read a stale count and can collectively overshoot. But that is the same accepted-overshoot shape this codebase already documents for the cost ceiling — What the PR does deliver is the change from unbounded per member (5 members × cap 5 = 25 agents against a documented cap of 5) to bounded with a parallel overshoot. The residue is recorded in the changelog rather than silently left. 7 new tests in |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I acknowledge the six fixes. The policy gate and the fail-closed roster behavior close the two security-sensitive paths. The tombstone, synchronized merge, and I accept the scope decision for atomic creation-capacity reservation. The remaining parallel overshoot is bounded, documented, and consistent with the existing The new ᕱ⑅ᕱ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/changelog.md`:
- Around line 24-32: Update the Review round 1 entry so its stated total and
fixed/declined counts match the listed findings: six fixed findings and one
declined finding, for seven total. Keep all listed bullets and the 25-test count
unchanged, and make the summary line consistent with those counts.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java`:
- Around line 765-794: Make the teardown and created/retained tracking merge
atomic by introducing one private tracking mutex in the group conversation
tracking state and using it for tombstone insertion, live-list removals, and
created/retained admissions in the merge flow around recordTeardown and the
created/retained loops. Ensure every read-check-write against these collections
uses the same mutex so stale snapshots cannot re-add torn-down IDs. Add a
latching regression test that concurrently races teardown with stale created and
retained snapshots and verifies the IDs remain removed.
🪄 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: 440eec22-3789-427f-88b5-94f1e86e9fc3
📒 Files selected for processing (6)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/main/java/ai/labs/eddi/modules/llm/impl/DynamicAgentToolsProvider.javasrc/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.javasrc/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.java
- src/main/java/ai/labs/eddi/modules/llm/impl/DynamicAgentToolsProvider.java
…monitor CodeRabbit is right that round 1 did not close the race. Ordering the writes inside recordTeardown is not sufficient on its own: a merge can read the tombstone set, find the id absent, be descheduled while a concurrent teardown records the tombstone and removes the id, and then complete its own add -- putting back an agent that no longer exists. The retained-agent branch had the same shape and sat outside the monitor entirely. Check-tombstone-then-add and record-teardown are now mutually exclusive on a shared dynamicTrackingMutex on GroupConversation (transient + @JsonIgnore, the same pattern as artifactAnnounceMutex), and the merge's synchronized region now covers the retained branch as well as the created one. Two 200-round interleaving tests pin it, both mutation-verified: dropping the synchronized block from recordTeardown fails them. 228 tests green across the affected suites.
Member
Author
|
Round 2 — @coderabbitai you're right, and thanks for re-checking rather than accepting round 1. Fixed: teardown and tracking merges now share one monitor. Ordering the writes inside Both now run under a shared Pinned by two 200-round interleaving tests that race a teardown against a merge on the same id — one for The changelog comment appears to have resolved to an analysis chain with no finding attached — I've made the two test counts in that entry unambiguous (869 for the group-service suites, 228 for the merge-affected suites in this round) in case that was the point. Happy to address it directly if there was something more specific. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java --items all --match 'GroupConversation|recordTeardown|dynamicTrackingMutex' --view expanded
ast-grep outline src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java --items all --match 'dynamic|merge|created|retained|torn' --view expanded
ast-grep outline src/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java --items all --view expanded
printf '\n--- mutex and merge references ---\n'
rg -n -C 8 'dynamicTrackingMutex|recordTeardown|tornDownAgentIds|createdAgentIds|retainedAgentIds' \
src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java \
src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java
printf '\n--- interleaving test references ---\n'
rg -n -C 8 '200|round|interleav|teardown|createdAgentIds|retainedAgentIds|Executor|thread' \
src/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.javaLength of output: 41555
The synchronized merge covers both I also verified both 200-round interleaving tests in The changelog test-count clarification is sufficient. I have no further finding on that entry. ᕱᕱ ✏️ Learnings added
You are interacting with an AI system. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java (1)
155-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen the reload test so it can detect a map-type regression.
The test name states that the map stays concurrent, but the assertions do not verify that. Both assertions pass on a
LinkedHashMap, because uncontendedputIfAbsentbehaves the same. IfsetMemberDisplayNamesregresses toLinkedHashMap, this test stays green — and that regression is the defect the fix at Line 1275 addresses.Add contention so the concurrent-map invariant is actually exercised. The file already uses this latch pattern.
💚 Proposed strengthening
`@Test` `@DisplayName`("member display names survive a store reload as a concurrent map") - void displayNamesStayConcurrentAfterReload() { + void displayNamesStayConcurrentAfterReload() throws Exception { var gc = conversation(); // What deserialization does. gc.setMemberDisplayNames(Map.of("agent-1", "Alice")); gc.addMemberDisplayNameIfAbsent("agent-1", "agent-1"); gc.addMemberDisplayNameIfAbsent("agent-2", "agent-2"); assertEquals("Alice", gc.getMemberDisplayNames().get("agent-1"), "putIfAbsent must be atomic here, and must not overwrite an operator-chosen name"); assertEquals("agent-2", gc.getMemberDisplayNames().get("agent-2")); + + // A LinkedHashMap passes everything above; only contention distinguishes + // the two. Concurrent writers plus an unguarded reader is exactly what + // RecruitAgentTool and serialization do to this map. + int writers = 16; + var start = new CountDownLatch(1); + var done = new CountDownLatch(writers); + for (int i = 0; i < writers; i++) { + int n = i; + Thread.ofVirtual().start(() -> { + try { + start.await(); + gc.addMemberDisplayNameIfAbsent("recruit-" + n, "recruit-" + n); + gc.getMemberDisplayNames().forEach((k, v) -> { + }); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + done.countDown(); + } + }); + } + start.countDown(); + assertTrue(done.await(10, TimeUnit.SECONDS), "display-name writes deadlocked"); + assertEquals(writers + 2, gc.getMemberDisplayNames().size()); + assertEquals("Alice", gc.getMemberDisplayNames().get("agent-1")); }🤖 Prompt for 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. In `@src/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java` around lines 155 - 168, Strengthen displayNamesStayConcurrentAfterReload by adding the file’s existing latch-based concurrent contention pattern around addMemberDisplayNameIfAbsent, using multiple threads and joining them before assertions. Ensure the test would fail or expose a regression where setMemberDisplayNames creates a non-concurrent map, while preserving the assertion that the pre-existing “Alice” value is never overwritten.
🤖 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.
Outside diff comments:
In
`@src/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java`:
- Around line 155-168: Strengthen displayNamesStayConcurrentAfterReload by
adding the file’s existing latch-based concurrent contention pattern around
addMemberDisplayNameIfAbsent, using multiple threads and joining them before
assertions. Ensure the test would fail or expose a regression where
setMemberDisplayNames creates a non-concurrent map, while preserving the
assertion that the pre-existing “Alice” value is never overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d877655-ee77-4993-bbb1-d987cbb7e19a
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/test/java/ai/labs/eddi/engine/internal/groups/DynamicAgentTrackingMergeTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.java
- docs/changelog.md
One documentation-discipline finding from a model-independent second pass: tornDownAgentIds is a persisted, resume-consumed field, and the schema-version comment's own rule says such fields bump the version. It deliberately rides v4 instead, and the comment now records why, so the decision reads as decided rather than missed: - It fails soft in every skew direction. A legacy document defaults to an empty set (identity hop, no migration entry), and an older pod re-saving a paused document drops the tombstones -- after which the worst outcome is a torn-down agent re-occupying a maxCreatedAgentsPerDiscussion slot and terminal cleanup retrying a deletion that 404s harmlessly. - Contrast runtimePhases, where the same skew mis-indexes resume bookmarks: corruption, not conservatism. A version bump signals "an old pod must not touch this document"; this field does not earn that. The second pass also re-verified the V7 wiring end to end: collectEnabledTools has no production caller, the live path is buildToolSetup -> contribute(), and both paths carry the new semantics -- no double-add. 68 tests green incl. GroupConversationSchemaMigrationsTest.
Member
Author
|
Final review pass (independent second-model review of the full diff) — one documentation-discipline finding, fixed in the latest commit, plus two verifications worth recording:
|
aisabella-ai
self-requested a review
August 10, 2026 17:29
aisabella-ai
approved these changes
Aug 10, 2026
…rdrails # Conflicts: # docs/changelog.md
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 10, 2026
…not yet supported" section
Wave E of the Agent / Group Agent review -- the documentation drifts the
review turned up, each one a place where docs and code disagreed.
1. Attachments x groups were entirely undocumented. group-conversations.md
had zero mentions of attachments and attachments-guide.md zero mentions
of groups, while POST /groups/{groupId}/conversations accepts them and
GroupAttachmentBinder is a whole subsystem. Both files now cover the
three input shapes, the first-turn grant, how later phases keep access
(history plus the auto-enabled readAttachment tool), and the two
group-specific bounds: the per-turn cap applies per MEMBER turn, and
anything dropped is reported in that member's attachments:errors, not
in the group transcript.
2. The protocol table did not say the defaults apply to an absent block.
Nothing backfills a stored config, so "no protocol block" is the common
shape -- which is what made the 60-vs-180 drift fixed in labsai#648
invisible.
3. maxCreatedAgentsPerDiscussion now reads "counted across all members,
not per member" -- the behaviour labsai#649 delivers.
4. LAST_PHASE was documented as "only the previous phase's entries", but
the filter is phaseIndex >= currentPhaseIdx - 1, which includes the
running phase. The code is right -- in a sequential phase that is what
lets the second speaker react to the first -- so the doc and the enum
Javadoc were corrected, not the filter.
5. New "Not yet supported" section: member-level tool approval inside a
group, nested pauses, groups over the OpenAI-compatible /v1 adapter,
groups over A2A, and the per-node scope of the live-discussion
registry.
Also: an FQN sweep of CreateSubAgentTool (11 inline fully-qualified names,
against AGENTS.md 4.7) and the orphaned Javadoc in LiveDiscussionRegistry,
where the paragraph documenting get() sat above getForMember() so both
attached to the latter and get() had none.
Scoping: the FQN violation is repo-wide (~130 sites). This sweeps only
files no other open PR touches; the rest is a follow-up once labsai#648-labsai#651
land.
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
2 participants
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.
Wave B of the Agent / Group Agent review (Wave A is #648). These guard the highest-blast-radius capability in the product — an LLM deploying agents to production — and the review found them to be the weakest-enforced things in the system.
1. 🔴 A group's
dynamicAgentspolicy silently reverted to fully permissive on a resumed member turnresolveDynamicAgentConfigaccepted only a typedDynamicAgentConfigout of the context value. It is the only place in the codebase that does a typed cast on aContextvalue — every other consumer (GroupCostLedger,GroupLifecycleOps,AttachmentContextExtractor,OpenAiConversationBridge) handles the deserialized shape.A
Contextwhose value round-trips through the conversation store comes back as a rawLinkedHashMap—ConversationMemoryStorerebuilds it asnew Context(type, map.get("value")). So any turn running against a reloaded step missed theinstanceofand fell through tocreateDefaultDynamicConfig(): creation, recruitment and delegation all on, for a group that may have disabled every one of them.The trigger is an ordinary group path, not an exotic one:
MemberTurnExecutor#tryResolveMemberToolPauseauto-rejects it (system:group) and resumes the member conversation.ConversationHitlServiceloads the snapshot from the store;Conversation#resumere-enters the same LlmTask at the same index (executeLifecycleFromIndex).buildToolListruns again against the reloaded step → permissive default.The orchestrator is still blocked inside that call, so the discussion is also still live in
LiveDiscussionRegistryand the group-gated tools are available on the same leg.Resolution is now three-state and fails closed:
"The operator said something we cannot parse" must never resolve to "the operator said yes to everything".
2.
maxCreatedAgentsPerDiscussionwas enforced per member, not per discussionseedCreatedAgentIdshas always read adynamicCreatedAgentIdscontext variable for the discussion-wide total — its own Javadoc said "Nothing insrc/mainwrites it today". So the cap bounded each member conversation independently: a 5-member group with the default cap of 5 could deploy 25 agents to production, while both the field name anddocs/group-conversations.mdpromise 5.MemberTurnExecutornow injectsgc.getCreatedAgentIds()alongside the policy it already injects per turn. The read side was already written and waiting.3.
RecruitAgentToolcould re-recruit a configured memberThe Javadoc claimed "Configured roster and prior recruits both count", but
isAlreadyMemberchecked onlyrecruitedAgentIds,dynamicMembersandmemberConversationIds— and the last holds an agent only once it has spoken. A member whose first turn had not come up yet (any member, during the opening phase) could be "recruited" as a duplicate.rosterWithRecruitsde-duplicates so nobody spoke twice, but: the recruitment cap was consumed, a misleadingFACILITATIONentry was written, andaddMemberDisplayNameoverwrote the operator-chosen display name with the raw agent id — visible in the UI and infollowUpWithMember's name resolution.The tool now receives the configured roster (resolved the same way
ArtifactToolsProviderresolves its artifact policy), and display-name recording becameputIfAbsent.4. Teardown never freed a creation slot, and a failed delete orphaned the agent
Two problems in
TeardownAgentTool:createdAgentIds.removeran before the delete, so a failed delete left the agent untracked andcleanupEphemeralAgentsnever retried it — config and deployment record orphaned.seedCreatedAgentIdsrebuilds by unioning every earlier step, so the id came straight back and the cap counted a non-existent agent forever — defeating the tool's entire purpose.Teardown now records into
dynamic:torn_down_agent_ids, which the seed subtracts andpropagateDynamicAgentTrackingapplies to the group's own tracking. The tracking removal moved after the successful delete.5. V7 resolved — an omitted whitelist no longer skips the dynamic tools
docs/langchain.mdstates twice that omittingbuiltInToolsWhitelistenables all built-in tools ("(all if not specified)", "OmittingbuiltInToolsWhitelistenables all available built-in tools"), andBuiltinToolsProviderimplements exactly that for the nine plain beans. This provider alone returned early, so an agent withenableBuiltInTools=true, no whitelist anddynamicAgents.enabled=truegot none of them. Tracked as verify-task V7 inplanning/group-collaboration-improvements-plan.md§2, which sanctions fixing it as a separate labeled behaviour-change commit.Deliberately narrower than "all" in one respect: the omitted case is honoured only when a group policy governs the turn.
dynamicAgentsis a field onAgentGroupConfiguration, so a standalone conversation has no configuration surface on which an operator could have declined — handing it unconfigurable, production-deploying capabilities because it omitted a list would be a worse defect than the asymmetry being fixed. Under a group policy the operator has that surface (enabled/allowCreation/allowRecruitment/allowDelegation), which is what makes "all" safe to mean all.The existing
AgentOrchestratorBuiltInToolWiringTestturned out not to pin the no-whitelist case at all, so nothing had to be un-pinned; new coverage pins it in both directions.Also
The three dynamic-agent tracking keys moved to
MemoryKeys— the group layer reads them positionally out of a serialized snapshot, so both sides now name one constant instead of two string literals.Testing
GroupConversationService*,GroupLifecycleOps*,LlmTask*,ConversationHitl*,ConversationToolResume*; 487 across the orchestrator/tool suites.DynamicAgentGuardrailResolutionTestandDynamicAgentWhitelistAndCapTest, plus 3 added toRecruitAgentToolTest.Summary by CodeRabbit
New Features
Bug Fixes
Documentation