add criteria - #1382
Merged
Merged
Conversation
iceljc
marked this pull request as draft
July 17, 2026 16:56
Contributor
PR Summary by QodoAdd pluggable rule criteria evaluators with code + LLM fallback
AI Description
Diagram
High-Level Assessment
Files changed (22)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+27
to
+46
| var settings = context.Options.GetData<CodeCriteriaSettings>() ?? new(); | ||
| var provider = settings.CodeProcessor ?? BuiltInCodeProcessor.PyInterpreter; | ||
| var processor = _services.GetServices<ICodeProcessor>().FirstOrDefault(x => x.Provider.IsEqualTo(provider)); | ||
| if (processor == null) | ||
| { | ||
| _logger.LogWarning($"Unable to find code processor: {provider}."); | ||
| return true; | ||
| } | ||
|
|
||
| var agentService = _services.GetRequiredService<IAgentService>(); | ||
| var scriptName = settings.CodeScriptName ?? $"{trigger.Name}_rule.py"; | ||
| var codeScript = await agentService.GetAgentCodeScript(agent.Id, scriptName, scriptType: AgentCodeScriptType.Src); | ||
|
|
||
| var msg = $"rule trigger ({trigger.Name}) code script ({scriptName}) in agent ({agent.Name}) => args: {settings.ArgumentContent?.RootElement.GetRawText()}."; | ||
|
|
||
| if (codeScript == null || string.IsNullOrWhiteSpace(codeScript.Content)) | ||
| { | ||
| _logger.LogWarning($"Unable to find {msg}."); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
1. codecriteriaevaluator fails open 📘 Rule violation ☼ Reliability
CodeCriteriaEvaluator.EvaluateAsync returns true when it cannot evaluate criteria (missing code processor or missing script), causing rules to execute even though criteria validation failed. This violates the requirement to validate inputs and fail safely at integration boundaries.
Agent Prompt
## Issue description
`CodeCriteriaEvaluator.EvaluateAsync` currently returns `true` when the evaluator cannot run (e.g., no `ICodeProcessor` found or no script content). This is a fail-open behavior that can trigger rules when criteria evaluation is effectively unavailable.
## Issue Context
Compliance requires validating boundary inputs/dependencies and providing safe failure behavior when required data/services are missing or invalid.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Code/CodeCriteriaEvaluator.cs[27-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+46
to
+61
| var agentService = _services.GetRequiredService<IAgentService>(); | ||
| var innerAgent = await agentService.GetAgent(agentId); | ||
| if (innerAgent == null) | ||
| { | ||
| _logger.LogWarning($"Unable to find agent for {msg}"); | ||
| return true; | ||
| } | ||
|
|
||
| // Render the template as the system instruction, exposing the request states. | ||
| var render = _services.GetRequiredService<ITemplateRender>(); | ||
| var template = innerAgent.Templates.FirstOrDefault(x => x.Name.IsEqualTo(templateName)); | ||
| if (template == null || string.IsNullOrWhiteSpace(template.Content)) | ||
| { | ||
| _logger.LogWarning($"Unable to find agent template for {msg}"); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
2. llmcriteriaevaluator fails open 📘 Rule violation ☼ Reliability
LlmCriteriaEvaluator.EvaluateAsync claims to fail closed but returns true when it cannot evaluate criteria due to a missing criteria agent or missing/empty template, allowing rules to execute despite invalid or missing inputs. This violates the requirement to validate inputs and fail safely at integration boundaries, making LLM-based criteria unreliable under configuration drift.
Agent Prompt
## Issue description
`LlmCriteriaEvaluator.EvaluateAsync` currently returns `true` (pass) when required prerequisites for LLM criteria evaluation are missing (e.g., criteria agent not found, template missing/empty), which is a fail-open behavior. This contradicts the documented “fails closed” contract and violates compliance expectations for safe failure at integration boundaries.
## Issue Context
Compliance requires explicit failure when required inputs/dependencies are missing or invalid so rule execution does not proceed as if criteria passed. The evaluator already behaves fail-closed for other error paths (e.g., missing completion provider, empty LLM response, exceptions), so the missing-agent/template branches are inconsistent and should be aligned; warning messages can also be updated to clearly state evaluation failed and the rule will not execute.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[6-61]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[46-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
44
| // Resolve the criteria evaluator | ||
| IRuleCriteriaEvaluator? criteriaEvaluator = null; | ||
| if (options?.Criteria != null) | ||
| { | ||
| criteriaEvaluator = _services.GetServices<IRuleCriteriaEvaluator>() | ||
| .FirstOrDefault(x => x.Type.IsEqualTo(options.Criteria.Type)); | ||
| if (criteriaEvaluator == null) | ||
| { | ||
| _logger.LogWarning($"Unable to find rule criteria evaluator for type ({options.Criteria.Type})."); | ||
| } | ||
| } | ||
|
|
||
| // Trigger agents | ||
| var filteredAgents = agents.Items.Where(x => x.Rules.Exists(r => r.TriggerName.IsEqualTo(trigger.Name) && !x.Disabled)).ToList(); | ||
| foreach (var agent in filteredAgents) |
Contributor
There was a problem hiding this comment.
3. Missing criteria fail-safe in engine 📘 Rule violation ☼ Reliability
When options.Criteria is provided but no matching IRuleCriteriaEvaluator is registered for options.Criteria.Type, the engine only logs a warning and still triggers agents without applying criteria filtering, causing criteria enforcement to fail-open. This violates the requirement to safely fail when boundary configuration is invalid or misconfigured.
Agent Prompt
## Issue description
When `RuleTriggerOptions.Criteria` is specified but the corresponding `IRuleCriteriaEvaluator` cannot be resolved (e.g., `options.Criteria.Type` does not match any registered evaluator), `RuleEngine` currently logs a warning and proceeds to trigger agents without applying criteria filtering. This is a fail-open behavior that can bypass criteria enforcement and violates the requirement to fail safely on invalid boundary configuration.
## Issue Context
Compliance requires validating boundary configuration and behaving safely when inputs/configuration are invalid. `RuleEngine.Triggered` resolves `criteriaEvaluator` once; if it is null, the later `if (criteriaEvaluator != null)` gate skips criteria evaluation entirely, allowing unconditional triggering.
Desired behavior: when `options.Criteria` is present but no evaluator exists, do not trigger rules (fail-closed). Keep the warning log, but ensure misconfiguration is surfaced and cannot be used to bypass criteria.
Implementation options (choose one consistent with the system’s error-handling conventions):
- Return an empty result immediately.
- Treat all agents as not triggered (skip loop).
- Throw a controlled exception to surface misconfiguration.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[30-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+68
to
70
| var convId = await SendMessageToAgent(agent, trigger, text, states); | ||
| newConversationIds.Add(convId); | ||
| } |
Contributor
There was a problem hiding this comment.
4. Rule flow path removed 🐞 Bug ≡ Correctness
RuleEngine.Triggered no longer executes rule-flow graphs and instead always calls SendMessageToAgent, breaking topology-based rule execution and skipping graph traversal/validation. This is compounded by removing TopologyName from RuleConfig, leaving no per-rule way to select a topology.
Agent Prompt
### Issue description
`RuleEngine.Triggered` previously supported executing rule-flow topologies (graphs) but now always sends a message directly to the agent. This breaks any rules relying on `IRuleFlow<T>`-based topology execution and removes schema/graph traversal behavior.
### Issue Context
The repository still defines rule-flow abstractions (`IRuleFlow<T>`) and flow options (`RuleFlowOptions` with `topology_name`), but `AgentRule.RuleConfig` no longer contains topology configuration. If flow execution is still a supported feature, the engine needs a way to choose and execute the topology.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[16-129]
- src/Infrastructure/BotSharp.Abstraction/Agents/Models/AgentRule.cs[16-20]
- src/Infrastructure/BotSharp.Abstraction/Rules/Options/RuleTriggerOptions.cs[7-18]
- src/Plugins/BotSharp.Plugin.MongoStorage/Models/AgentRuleMongoElement.cs[33-62]
### What to change
- Decide the supported configuration source for topology selection (e.g., reintroduce `RuleConfig.TopologyName`, or add `RuleTriggerOptions.Flow` back, or both).
- Reintroduce (and update) the graph execution branch in `RuleEngine.Triggered`:
- If a topology name is present, load the graph via `IRuleFlow<FlowGraph>` and execute it.
- Otherwise fall back to `SendMessageToAgent`.
- If flow execution is intentionally deprecated, remove/retire the unused flow abstractions/options and storage fields explicitly and update docs/contracts accordingly (don’t leave large commented-out blocks).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+32
to
+45
| public async Task<bool> EvaluateAsync(Agent agent, IRuleTrigger trigger, RuleCriteriaContext context) | ||
| { | ||
| var settings = context.Options.GetData<LlmCriteriaSettings>() ?? new(); | ||
| var rule = agent.Rules.FirstOrDefault(x => x.TriggerName.IsEqualTo(trigger.Name)); | ||
|
|
||
| // The Rules agent hosts the criteria-check template by default. | ||
| var agentId = !string.IsNullOrWhiteSpace(settings.AgentId) ? settings.AgentId! : BuiltInAgentId.RulesInterpreter; | ||
| var templateName = !string.IsNullOrWhiteSpace(settings.TemplateName) ? settings.TemplateName! : DefaultTemplateName; | ||
|
|
||
| var input = BuildInput(rule?.Config, settings); | ||
| var msg = $"rule trigger ({trigger.Name}) llm criteria (agent {agentId}, template {templateName})."; | ||
|
|
||
| try | ||
| { |
Contributor
There was a problem hiding this comment.
5. Criteria deserialization uncaught 🐞 Bug ☼ Reliability
CriteriaOptions.GetData<T>() can throw during JSON deserialization, and both LlmCriteriaEvaluator and CodeCriteriaEvaluator call it before their try/catch blocks, allowing the exception to escape and abort RuleEngine.Triggered. A malformed or type-incompatible Criteria.Data can therefore fail the entire trigger operation.
Agent Prompt
### Issue description
`CriteriaOptions.GetData<T>()` deserializes raw JSON without error handling. Evaluators call it outside their try/catch, so deserialization failures escape and can abort the whole trigger request.
### Issue Context
- `CriteriaOptions.GetData<T>()` uses `JsonElement.Deserialize<T>()`, which can throw (e.g., when JSON types don’t match the target settings model).
- `LlmCriteriaEvaluator` and `CodeCriteriaEvaluator` call `GetData<...>()` before their `try` blocks.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/Rules/Options/RuleTriggerOptions.cs[39-47]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[32-45]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Code/CodeCriteriaEvaluator.cs[25-28]
### What to change
- Make deserialization safe:
- Option A (preferred): wrap `Data.Value.Deserialize<T>` in a try/catch inside `GetData<T>()` and return `default` on failure (optionally log via a passed-in logger or rethrow a controlled domain exception).
- Option B: move `GetData<...>()` calls inside each evaluator’s existing try/catch and treat deserialization failure as a fail-closed result (`false`).
- Ensure behavior is consistent with your intended fail-open/fail-closed semantics for criteria evaluation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
added 9 commits
July 17, 2026 16:48
…atures/add-rule-criteria
…atures/add-rule-criteria
iceljc
marked this pull request as ready for review
August 5, 2026 14:47
Contributor
Code Review by Qodo
1. LLM criteria fails open
|
Comment on lines
+50
to
+53
| if (innerAgent == null) | ||
| { | ||
| _logger.LogWarning($"Unable to find agent for {msg}"); | ||
| return true; |
Contributor
There was a problem hiding this comment.
1. Llm criteria fails open 🐞 Bug ≡ Correctness
LlmCriteriaEvaluator returns true when the criteria agent or template is missing, causing rules to execute even though criteria was not evaluated (contradicts its own fail-closed contract). This can unintentionally bypass rule gating for any trigger using LLM criteria or LLM fallback.
Agent Prompt
### Issue description
`LlmCriteriaEvaluator` is documented to fail closed (return `false`) but it currently returns `true` when it cannot find the inner criteria agent or the requested template. This makes the rule engine treat the criteria as satisfied and execute the rule.
### Issue Context
The rule engine uses the evaluator's boolean result to decide whether to skip or execute the agent rule. Returning `true` on missing dependencies effectively bypasses criteria enforcement.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[12-15]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[48-63]
### Proposed fix
- Change both `return true;` branches (missing agent, missing template/content) to `return false;` (or `return null;` if you want the engine to treat it as “could not evaluate”, but since this is the fallback evaluator, `false` is the safer/consistent option).
- Keep behavior consistent with the class doc: other error paths already return `false`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+46
| return default; | ||
| } | ||
|
|
||
| return Data.Value.Deserialize<T>(options ?? _webJsonOptions); |
Contributor
There was a problem hiding this comment.
2. Criteria data can throw 🐞 Bug ☼ Reliability
CriteriaOptions.GetData<T>() deserializes JsonElement without handling JsonException; malformed or non-convertible Criteria.Data can throw and abort the Triggered request. This exception occurs before evaluator try/catch blocks (e.g., CodeCriteriaEvaluator/LlmCriteriaEvaluator) so it can propagate out of the rule engine.
Agent Prompt
### Issue description
`CriteriaOptions.GetData<T>()` calls `JsonElement.Deserialize<T>()` directly. If `Criteria.Data` is malformed or incompatible with `T`, deserialization can throw and fail the entire rule-trigger request.
### Issue Context
Both `CodeCriteriaEvaluator` and `LlmCriteriaEvaluator` call `context.Options.GetData<...>()` before their `try {}` blocks, and `RuleEngine.EvaluateCriteria` does not guard `EvaluateAsync`, so exceptions from `GetData<T>()` can propagate.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Abstraction/Rules/Options/RuleTriggerOptions.cs[39-47]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Code/CodeCriteriaEvaluator.cs[28-33]
- src/Infrastructure/BotSharp.Core.Rules/Criteria/Llm/LlmCriteriaEvaluator.cs[33-36]
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[86-96]
### Proposed fix
- Wrap `Data.Value.Deserialize<T>(...)` in a `try/catch (JsonException)` (and optionally `NotSupportedException`) inside `GetData<T>()` and return `default` on failure.
- Optionally add a safe log point (if you don't want logging in Abstraction): catch in evaluators or `RuleEngine.EvaluateCriteria` and treat it as `null`/`false` so criteria enforcement doesn't crash the request.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+37
| criteriaEvaluator = ResolveCriteriaEvaluator(options.Criteria.Type); | ||
| if (criteriaEvaluator == null) | ||
| { | ||
| _logger.LogWarning($"Unable to find rule criteria evaluator for type ({options.Criteria.Type})."); |
Contributor
There was a problem hiding this comment.
3. Unknown type bypasses criteria 🐞 Bug ⛨ Security
If options.Criteria.Type doesn't match any registered IRuleCriteriaEvaluator, RuleEngine only logs a warning and continues without evaluating criteria, so rules execute unconditionally. This can unintentionally bypass criteria enforcement on misconfiguration/typos.
Agent Prompt
### Issue description
When `options.Criteria` is provided but `ResolveCriteriaEvaluator(type)` returns null, the engine logs a warning and proceeds to send messages to agents without any criteria check.
### Issue Context
The criteria check is only performed under `if (criteriaEvaluator != null)`. A misspelled/unsupported type therefore disables criteria gating.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[30-69]
### Proposed fix
Implement fail-closed behavior when criteria is explicitly requested but cannot be evaluated:
- Option A (recommended): if evaluator not found, skip triggering (return empty list / continue) and log warning.
- Option B: fall back to the built-in LLM evaluator when the requested evaluator type is unknown (similar to the existing null-result fallback path).
Make sure the engine does not execute rules when a criteria type is specified but no evaluator exists.
ⓘ 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.