Skip to content

fix: resolve state keys before builtins in Condition.safe_expr - #962

Open
digit50 wants to merge 1 commit into
apache:mainfrom
digit50:fix/safe-expr-state-before-builtins
Open

digit50 wants to merge 1 commit into
apache:mainfrom
digit50:fix/safe-expr-state-before-builtins

Conversation

@digit50

@digit50 digit50 commented Oct 15, 2026

Copy link
Copy Markdown

closes #960

Condition.safe_expr resolved a name to an allowlisted builtin before looking at state, so a state key named max, sum or any evaluated to the builtin function. Condition.expr resolves state first, so the same expression gave a different answer: count < max raised TypeError, and sum == 0 silently returned False and sent a transition down the default edge.

Changes

  • _SafeExprInterpreter._eval_Name now looks up the state names first and falls back to _SAFE_EXPR_BUILTINS, matching expr().
  • The name collector for reads no longer drops every name found in builtins.__dict__. It skips the function name of a call (the validator only admits calls to allowlisted builtins, and the interpreter always resolves those to the builtin) and records every other name, so a state key named max or sum is reported as read and covered by the missing-key check.
  • Calls are unchanged: max(a, b) still calls the builtin even when state has a max key.

How I tested this

Added test_safe_expr_state_key_shadows_builtin in tests/core/test_action.py. With state {"count": 3, "max": 5, "sum": 0, "any": False} it checks that count < max, sum == 0 and any give the same result under safe_expr as under expr and that reads contains the shadowed names, and that max(count, sum) == 3 still calls the builtin with reads == ["count", "sum"]. The test fails on main with TypeError: '<' not supported between instances of 'int' and 'builtin_function_or_method' and passes with the change.

pytest tests/core/test_action.py passes (155 tests). pytest tests/core gives the same result with and without the change apart from the new test; the only failures are the four test_visualize_dot_output cases, which need the Graphviz dot binary that my environment does not have.

Notes

Bare uses of an allowlisted builtin name that is not in state (for example any == 1) are now listed in reads, so they fail the missing-key check instead of comparing against the builtin function. I could not find a meaningful expression that relied on the old behaviour.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

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 apache#960
@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label Oct 15, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Application, State, Graph, Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Condition.safe_expr resolves builtin names before state keys, unlike expr

1 participant