fix: avoid saving routing instruction args as state#1364
Merged
Conversation
Contributor
Code Review by Qodo
1. MergeRoutingRules returns incoming reference
|
Contributor
PR Summary by QodoFix: stop persisting routing instruction arguments into conversation state WalkthroughsDescription• Prevent routing instruction arguments from being stored as conversation state. • Avoid unintended state pollution/leakage across routing/execution loops. Diagramgraph TD
A["RoutingService.InstructLoop"] --> B["HookEmitter (IRoutingHook)"] --> C["RoutingReasoner"] --> D["Executor"] --> E[("ConversationStorage")]
A -. "no longer saves inst.Arguments" .-> F["ConversationStateService"]
subgraph Legend
direction LR
_svc([Service/Component]) ~~~ _db[(Storage)]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Whitelist/sanitize args before persisting
2. Persist under a namespaced, ephemeral scope (TTL)
Recommendation: Disabling persistence of routing instruction arguments is a safe, minimal bug fix if those args are transient and not intended as durable state. If some arguments are legitimately needed later, prefer a whitelist/sanitization approach (or a short-lived namespaced scope) rather than saving the raw argument bag into global conversation state. File ChangesBug fix (1)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Allow required routing rules to stay available for validation and redirects without exposing those fields for router-generated args.
This reverts commit 58e6ab1.
Member
Author
|
/review |
Contributor
|
Code review by qodo was updated up to the latest commit 56d1ee7 |
Comment on lines
+58
to
+78
| private static List<RoutingRule> MergeRoutingRules(List<RoutingRule>? existingRules, List<RoutingRule>? incomingRules) | ||
| { | ||
| if (incomingRules == null) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| foreach (var incomingRule in incomingRules) | ||
| { | ||
| var existingRule = existingRules?.FirstOrDefault(x => | ||
| string.Equals(x.Field, incomingRule.Field, StringComparison.OrdinalIgnoreCase) && | ||
| string.Equals(x.Type, incomingRule.Type, StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (existingRule != null) | ||
| { | ||
| incomingRule.AllowLlmFill = existingRule.AllowLlmFill; | ||
| } | ||
| } | ||
|
|
||
| return incomingRules; | ||
| } |
Contributor
There was a problem hiding this comment.
1. mergeroutingrules returns incoming reference 📘 Rule violation ☼ Reliability
MergeRoutingRules mutates the caller-provided incomingRules list (and its items) and then returns the same list instance, which is assigned to record.RoutingRules. This can leak state across components/requests and allow later mutations of the input list to affect persisted agent state.
Agent Prompt
## Issue description
`MergeRoutingRules` modifies and stores the caller-provided `incomingRules` list by reference (`incomingRule.AllowLlmFill = ...` and `return incomingRules;`). This violates the defensive-copy requirement for caller-provided mutable collections and can cause unintended shared-state mutations.
## Issue Context
The assignment `record.RoutingRules = MergeRoutingRules(record.RoutingRules, agent.RoutingRules);` stores whatever `MergeRoutingRules` returns into a long-lived/persisted record. `incomingRules` originates from the caller (`agent.RoutingRules`) and is mutable.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.UpdateAgent.cs[58-78]
## Implementation notes
- Avoid mutating `incomingRules` directly.
- Create a new `List<RoutingRule>` and clone/copy each incoming rule (e.g., `incomingRule.Clone()`), then apply the merge behavior onto the copies.
- Return the new list and assign that list to `record.RoutingRules`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
No description provided.