From 421ce9513e4d2e351bbed938273956a764ebfc40 Mon Sep 17 00:00:00 2001 From: digit50 <93092234+digit50@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:16:49 +0300 Subject: [PATCH] fix: resolve state keys before builtins in Condition.safe_expr Condition.safe_expr looked up its allowlisted builtins before state, so a state key named max, sum or any evaluated to the builtin function and gave a different answer than Condition.expr. Names are now read from state first and fall back to the builtin, and every name that is not the target of a call is reported in reads. Closes https://github.com/apache/burr/issues/960 --- burr/core/action.py | 18 ++++++++++-------- tests/core/test_action.py | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/burr/core/action.py b/burr/core/action.py index a69db06c6..6847df290 100644 --- a/burr/core/action.py +++ b/burr/core/action.py @@ -566,11 +566,11 @@ def _eval_Constant(self, node: ast.Constant): return node.value def _eval_Name(self, node: ast.Name): - # Builtins on the allowlist resolve to the builtin; otherwise look up state. - if node.id in _SAFE_EXPR_BUILTINS: - return _SAFE_EXPR_BUILTINS[node.id] + # State wins over an allowlisted builtin of the same name, as in expr(). if node.id in self._names: return self._names[node.id] + if node.id in _SAFE_EXPR_BUILTINS: + return _SAFE_EXPR_BUILTINS[node.id] raise NameError(f"safe_expr: name '{node.id}' is not defined") def _eval_Attribute(self, node: ast.Attribute): @@ -832,16 +832,18 @@ def safe_expr(expr: str) -> "Condition": # disallowed node exists we raise here, before constructing the Condition. _SafeExprValidator().visit(tree) - # Collect Name references for keys, mirroring expr(). - all_builtins = builtins.__dict__ - + # Collect Name references for keys. A called name always resolves to an + # allowlisted builtin; any other name is read from state first. class _NameCollector(ast.NodeVisitor): def __init__(self): self.names = set() + def visit_Call(self, node): + for arg in node.args: + self.visit(arg) + def visit_Name(self, node): - if node.id not in all_builtins: - self.names.add(node.id) + self.names.add(node.id) collector = _NameCollector() collector.visit(tree) diff --git a/tests/core/test_action.py b/tests/core/test_action.py index 0095bdafe..09216937e 100644 --- a/tests/core/test_action.py +++ b/tests/core/test_action.py @@ -436,6 +436,21 @@ def test_safe_expr_all_allowed_builtins(): assert _eval_value(Condition.safe_expr(expr_str), state) == expected, expr_str +def test_safe_expr_state_key_shadows_builtin(): + state = State({"count": 3, "max": 5, "sum": 0, "any": False}) + for expr_str, reads in [ + ("count < max", ["count", "max"]), + ("sum == 0", ["sum"]), + ("any", ["any"]), + ]: + cond = Condition.safe_expr(expr_str) + assert cond.run(state) == Condition.expr(expr_str).run(state), expr_str + assert sorted(cond.reads) == reads, expr_str + call = Condition.safe_expr("max(count, sum) == 3") + assert call.run(state) == {Condition.KEY: True} + assert sorted(call.reads) == ["count", "sum"] + + def test_safe_expr_reads_collected_correctly(): cond = Condition.safe_expr("foo == 'bar' and len(baz) == 3") assert sorted(cond.reads) == ["baz", "foo"]