fix(llm): dynamicAgentConfig guardrails failed open on any turn that did not re-inject them - #655
Closed
ginccc wants to merge 2 commits into
Closed
fix(llm): dynamicAgentConfig guardrails failed open on any turn that did not re-inject them#655ginccc wants to merge 2 commits into
ginccc wants to merge 2 commits into
Conversation
…did not re-inject them dynamicAgentConfig is the only typed POJO the system injects into conversation context, and it gates the tools that deploy real agents to production. resolveDynamicAgentConfig defeated MemberTurnExecutor's deliberate defense twice: - it read the CURRENT step only, so an HITL resume, crash recovery or the group follow-up path saw no config at all; - it required a live DynamicAgentConfig, but conversation memory rebuilds a stored context as new Context(type, map), so after any reload the instanceof failed even when the key was present. Both landed on the fully permissive standalone default. Now: earlier steps are consulted (the shape every sibling resolver already uses for the resume case), the map form is coerced whole-object, and a group turn with unresolvable guardrails fails CLOSED. GroupLifecycleOps.followUp never injected the config at all — fixed. contribute() now consumes the once-resolved ctx.dynamicAgentConfig() instead of re-resolving, making AgentOrchestrator's Javadoc true. Withheld group tools are logged so an off-node discussion is diagnosable. +7 tests, one nest per observation point: injecting turn, after store reload, no fresh injection, genuinely standalone.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
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 (6)
📝 WalkthroughWalkthroughThe change resolves dynamic-agent configuration from current and earlier context steps, restores persisted maps, fails closed for unresolved group configuration, injects configuration into group follow-ups, logs withheld tools, and adds boundary tests. ChangesDynamic agent context handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConversationMemory
participant DynamicAgentToolsProvider
participant GroupLifecycleOps
participant DynamicAgentConfig
ConversationMemory->>DynamicAgentToolsProvider: Read current and earlier context
DynamicAgentToolsProvider->>DynamicAgentConfig: Restore or resolve configuration
DynamicAgentConfig-->>DynamicAgentToolsProvider: Return configuration or disabled group default
GroupLifecycleOps->>DynamicAgentToolsProvider: Invoke follow-up with dynamicAgentConfig
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}
CodeQL log-injection alerts 490-491. Both values are caller-supplied — the group conversation id arrives as a context variable and the conversation id from memory — which is exactly why these lines exist, so they must be sanitized before they reach the log.
aisabella-ai
self-requested a review
August 10, 2026 17:35
aisabella-ai
approved these changes
Aug 10, 2026
Member
Author
|
Superseded — closing without merging. While this branch was in review, I re-checked every finding in this PR against current
No work is lost; the review threads here remain readable for the reasoning. |
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
3 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.
A systematic sweep of the Context serialization boundary: for every context the system injects, what type does it hold (a) on the injecting turn, (b) after a store reload, (c) after crash recovery, (d) on another node?
Nine context keys. Eight are strings, lists or maps and survive unchanged.
dynamicAgentConfigis the only typed POJO the system injects — and it is the one that gates the tools that deploy real agents to production.groupId,groupConversationId,groupDepth,delegationDepthgroupTranscript,schedule,attachment_Nmcp,slack,lang,channelIntentdynamicAgentConfig1. The resolver failed open in two independent ways
MemberTurnExecutorinjects the group's guardrails on every member turn, and deliberately injects an explicitly disabled config when the group configured none. Its comment says exactly why: a member turn that finds no config falls back to the STANDALONE default, which is fully permissive — creation, recruitment and delegation all on.resolveDynamicAgentConfigdefeated that defense twice:ConversationMemoryStoreandPostgresConversationMemoryStoreboth rebuild a stored context asnew Context(type, map.get("value")). After any reload the value is aMap, so theinstanceof DynamicAgentConfigfailed even when the key was present.Both paths landed on the permissive default.
This was the only sibling resolver that did not already handle the resume case —
ContextualToolsProvider#resolveGroupIdsandresolveDelegationDepthboth fall back to earlier steps, with comments explaining that a resumed turn re-enters without the original context map. Fixed the same way, plus whole-object Jackson coercion of the map form (not field-by-field — a guardrail added later must not silently read back as its permissive Java default).2. Unresolvable guardrails on a group turn now fail CLOSED
A conversation that demonstrably belongs to a group but whose config cannot be resolved returns a disabled config instead of the permissive default — the discipline
GroupTaskToolsProviderandArtifactToolsProvideralready state.The group probe requires a context entry that actually carries a value, not merely a key. "The key exists" is too weak a claim to strip a standalone agent of tools its designer explicitly whitelisted.
3.
GroupLifecycleOps.followUpnever injected the guardrails at allIt injects
groupTranscript/groupId/groupConversationIdbut notdynamicAgentConfig— so the defenseMemberTurnExecutordocuments was bypassed on the entire follow-up path, with no crash or resume needed. Now injected identically.4. Two disagreeing defaults, and a Javadoc that was not true
ToolAssemblyContext's compact constructor normalizes a null config to a disabled one.DynamicAgentToolsProviderused a permissive one. Andcontribute()ignoredctx.dynamicAgentConfig()entirely, re-resolving from memory — despiteAgentOrchestrator#toolAssemblyContextdocumenting that the value is resolved once "so two providers resolving it independently could [not] disagree". The inconsistency the Javadoc warns about was already in the code. The provider now consumes the resolved value.5. Withheld group tools are logged
GroupTaskToolsProvider/ArtifactToolsProviderreturned an empty contribution silently. Withholding is correct for a forged or finished discussion id — but it is also what happens if the discussion runs on another node, and that failure looked like "the model just lost its tools" with nothing to diagnose from.Verified, not changed: the node-affinity invariant
LiveDiscussionRegistry's correctness rests on "a member turn always runs in-process". Re-checked against the implementation rather than the changelog note:NatsConversationCoordinator.publishAndExecutepublishes onlyconversationId.getBytes()as an ordering marker and then runs the callable throughruntime.submitCallablelocally. No JetStream consumer deserializes or executes callables, and the payload could not carry one. A member turn cannot be routed off-node. Invariant holds.Testing
377 tests across
DynamicAgentToolsProvider/AgentOrchestrator/GroupLifecycleOps/MemberTurnExecutorstay green. +7 new inDynamicAgentConfigContextBoundaryTest, one nest per observation point (a)–(d).Summary by CodeRabbit
Bug Fixes
Documentation