Conversation
…tions The rule engine needs a way to evaluate conditional expressions (when clauses) that go beyond simple pattern matching — e.g. checking environment variables, parsed flags, positional arguments, and path lists. CEL (Common Expression Language) provides a non-Turing-complete, safe expression language ideal for this use case. Add `expr_evaluator` module with: - `ExprContext` struct providing access to env vars, flags, args, and path lists - `evaluate()` function that compiles and executes CEL expressions against the context, returning a boolean result - Support for `env.VAR`, `flags.name`, `args[0]`, `paths.listname` access patterns - Support for `&&`, `||`, `!` logical operators and string methods like `startsWith()` - Proper error mapping to existing `ExprError` variants (Parse, Eval, TypeError) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @fohte, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the rule engine's capabilities by integrating a CEL expression evaluator. This allows Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful feature by integrating a CEL expression evaluator for when clauses. The implementation in src/rules/expr_evaluator.rs is well-structured and accompanied by a comprehensive test suite, which is excellent.
My main feedback is to refactor how variables are added to the CEL context to improve performance and code clarity by avoiding unnecessary data cloning and manual conversions. Using add_variable consistently for all context properties will make the code more efficient and maintainable.
Overall, this is a great addition that significantly enhances the rule engine's capabilities.
| cel_context.add_variable_from_value("env", context.env.clone()); | ||
|
|
||
| // Convert Option<String> flags to CEL-compatible values. | ||
| // None values become null in CEL. | ||
| let flags_value: HashMap<String, cel_interpreter::Value> = context | ||
| .flags | ||
| .iter() | ||
| .map(|(k, v)| { | ||
| let val = match v { | ||
| Some(s) => cel_interpreter::Value::String(s.clone().into()), | ||
| None => cel_interpreter::Value::Null, | ||
| }; | ||
| (k.clone(), val) | ||
| }) | ||
| .collect(); | ||
| cel_context.add_variable_from_value("flags", flags_value); | ||
|
|
||
| cel_context.add_variable_from_value("args", context.args.clone()); | ||
|
|
||
| cel_context | ||
| .add_variable("paths", &context.paths) | ||
| .map_err(|e| ExprError::Eval(e.to_string()))?; |
There was a problem hiding this comment.
The context variables env, flags, and args are being cloned and/or manually converted before being added to the CEL context. This can be simplified and made more efficient.
The cel-interpreter crate's add_variable function works with any type that implements serde::Serialize and takes a reference, avoiding unnecessary clones. HashMap<K, V> and Vec<T> (where K, V, T are serializable) both implement Serialize.
For flags, HashMap<String, Option<String>> can be serialized directly, with None values correctly becoming null in the CEL context, which removes the need for manual conversion.
I suggest using add_variable for all context properties for consistency, better performance, and improved readability. This also makes error handling consistent across all variables.
cel_context
.add_variable("env", &context.env)
.map_err(|e| ExprError::Eval(e.to_string()))?;
cel_context
.add_variable("flags", &context.flags)
.map_err(|e| ExprError::Eval(e.to_string()))?;
cel_context
.add_variable("args", &context.args)
.map_err(|e| ExprError::Eval(e.to_string()))?;
cel_context
.add_variable("paths", &context.paths)
.map_err(|e| ExprError::Eval(e.to_string()))?;1 participant
Why
whenclauses, such as checking environment variables, flags, and argument valuesWhat
whenclauses to evaluate conditional expressions using CEL (Common Expression Language)env.VAR == 'value'flags.name == 'value'args[0].startsWith('prefix')definitions.pathspath lists viapaths.sensitive&&,||,!logical operators