Skip to content

Commit 3d39258

Browse files
authored
Python: [BREAKING] Align workflow state attribute lookup (microsoft#8893)
* Align workflow state attribute lookup * Use regex for workflow state attribute validation
1 parent 2a7470a commit 3d39258

7 files changed

Lines changed: 326 additions & 15 deletions

File tree

‎python/packages/declarative/AGENTS.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ YAML/JSON-based declarative agent and workflow definitions.
66

77
- **`AgentFactory`** - Creates agents from declarative definitions
88
- **`WorkflowFactory`** - Creates workflows from declarative definitions
9-
- **`WorkflowState`** - State management for declarative workflows
9+
- **`WorkflowState`** - Standalone state management and expression evaluation; factory-created workflows use the internal, checkpoint-backed `DeclarativeWorkflowState` instead
1010
- **`ProviderTypeMapping`** - Maps provider types to implementations
1111
- **`HttpRequestHandler`** / **`DefaultHttpRequestHandler`** - Pluggable HTTP transport for the `HttpRequestAction` declarative action (configured via `WorkflowFactory(http_request_handler=...)`)
1212
- **`MCPToolHandler`** / **`DefaultMCPToolHandler`** - Pluggable MCP transport for the `InvokeMcpTool` declarative action (configured via `WorkflowFactory(mcp_tool_handler=...)`)
1313
- **`DeclarativeLoaderError`** / **`ProviderLookupError`** / **`DeclarativeWorkflowError`** / **`DeclarativeActionError`** - Error types
1414

15+
Both state classes validate object attribute names but leave dictionary keys unchanged.
16+
1517
## MCP Handler Lifetimes
1618

1719
`DefaultMCPToolHandler` caches/coalesces sessions only without a `client_provider`.

‎python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747

4848
from ._errors import DeclarativeWorkflowError
4949
from ._powerfx_limits import _PowerFxStateBudget, _validate_powerfx_state # pyright: ignore[reportPrivateUsage]
50+
from ._state_path import _is_safe_path_segment # pyright: ignore[reportPrivateUsage]
5051

5152
try:
5253
from powerfx import Engine
@@ -66,9 +67,6 @@
6667

6768
_ENV_REFERENCE_RE = re.compile(r"\bEnv\.([A-Za-z_][A-Za-z0-9_]*)")
6869

69-
# Allowed identifier shape for object-attribute steps in declarative state paths
70-
_SAFE_PATH_SEGMENT_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_]*$")
71-
7270

7371
def _skip_powerfx_opaque_token(formula: str, start: int) -> int:
7472
"""Return the end of an ordinary quoted token or comment, or start if neither."""
@@ -500,7 +498,7 @@ def get(self, path: str, default: Any = None) -> Any:
500498
return default
501499
else:
502500
# Attribute access is only allowed for safe declarative identifiers.
503-
if not _SAFE_PATH_SEGMENT_RE.match(part):
501+
if not _is_safe_path_segment(part):
504502
logger.warning(
505503
"DeclarativeWorkflowState.get: rejecting attribute segment %r in path %r",
506504
part,

‎python/packages/declarative/agent_framework_declarative/_workflows/_state.py‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from typing import Any, cast
1818

1919
from ._powerfx_limits import _PowerFxStateLimitError, _validate_powerfx_state # pyright: ignore[reportPrivateUsage]
20+
from ._state_path import _is_safe_path_segment # pyright: ignore[reportPrivateUsage]
2021

2122
try:
2223
from powerfx import Engine
@@ -31,7 +32,10 @@
3132

3233

3334
class WorkflowState:
34-
"""Manages variables and state during declarative workflow execution.
35+
"""Manages standalone workflow state and expression evaluation.
36+
37+
Workflows created by ``WorkflowFactory`` use a separate, checkpoint-backed
38+
``DeclarativeWorkflowState`` implementation.
3539
3640
WorkflowState provides a unified interface for:
3741
@@ -161,12 +165,16 @@ def conversation(self) -> dict[str, Any]:
161165
def get(self, path: str, default: Any = None) -> Any:
162166
"""Get a value from the state using a dot-notated path.
163167
168+
Dict-keyed segments may use arbitrary string keys. Segments resolved
169+
through object-attribute access must match ``[A-Za-z][A-Za-z0-9_]*``;
170+
other shapes return ``default`` without accessing the attribute.
171+
164172
Args:
165173
path: Dot-notated path like 'Local.results' or 'Workflow.Inputs.query'
166174
default: Default value if path doesn't exist
167175
168176
Returns:
169-
The value at the path, or default if not found
177+
The value at the path, or default if not found or unreachable
170178
"""
171179
parts = path.split(".")
172180
if not parts:
@@ -206,10 +214,18 @@ def get(self, path: str, default: Any = None) -> Any:
206214
obj = obj_dict.get(part, default)
207215
if obj is default:
208216
return default
209-
elif hasattr(obj, part):
210-
obj = getattr(obj, part)
211217
else:
212-
return default
218+
if not _is_safe_path_segment(part):
219+
logger.warning(
220+
"WorkflowState.get: rejecting attribute segment %r in path %r",
221+
part,
222+
path,
223+
)
224+
return default
225+
if hasattr(obj, part):
226+
obj = getattr(obj, part)
227+
else:
228+
return default
213229

214230
return obj
215231

@@ -372,7 +388,10 @@ def eval(self, expression: str) -> Any:
372388
"""Evaluate a PowerFx expression with the current state.
373389
374390
Expressions starting with '=' are evaluated as PowerFx.
375-
Other strings are returned as-is (after variable interpolation if applicable).
391+
Other strings are returned as-is. If PowerFx is unavailable or raises
392+
an ordinary evaluation error, the simple evaluator is used instead.
393+
Its state references follow :meth:`get`'s object-member rules.
394+
State-budget errors propagate without falling back.
376395
377396
Args:
378397
expression: The expression to evaluate
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# Copyright (c) Microsoft. All rights reserved.
2+
3+
"""Shared identifier policy for object-attribute steps in workflow state paths."""
4+
5+
import regex
6+
7+
_SAFE_PATH_SEGMENT_RE = regex.compile(r"[A-Za-z][A-Za-z0-9_]*")
8+
9+
10+
def _is_safe_path_segment(segment: str) -> bool: # pyright: ignore[reportUnusedFunction]
11+
"""Return whether an object-attribute segment is a declarative identifier."""
12+
return _SAFE_PATH_SEGMENT_RE.fullmatch(segment) is not None

‎python/packages/declarative/tests/test_declarative_state_path_safety.py‎

Lines changed: 130 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@
44
# pyright: reportPrivateUsage=false, reportUnknownVariableType=false
55
# pyright: reportGeneralTypeIssues=false
66

7-
"""Path-segment validation tests for DeclarativeWorkflowState.
7+
"""Path-segment validation tests for workflow state.
88
9-
Path segments handed to ``get``/``set``/``append`` and ``{Variable.Path}``
10-
placeholders in ``interpolate_string`` are subject to three distinct rules
11-
that this module pins:
9+
For DeclarativeWorkflowState, path segments handed to ``get``/``set``/``append``
10+
and ``{Variable.Path}`` placeholders in ``interpolate_string`` are subject to
11+
three distinct rules that this module pins:
1212
1313
- **Empty segments** (e.g. ``""``, ``"Local."``, ``"Local..foo"``) are rejected
1414
by all of ``get``/``set``/``append`` and ``interpolate_string``. ``get`` and
@@ -21,6 +21,9 @@
2121
- **Dict-keyed segments** — segments that resolve via dict lookup because the
2222
parent is a ``dict`` — may use arbitrary non-empty string keys (e.g. UUIDs
2323
or hyphenated identifiers like ``System.conversations.<uuid>.messages``).
24+
25+
Standalone WorkflowState shares the object-member rule, but retains its
26+
existing empty-dictionary-key behavior.
2427
"""
2528

2629
import logging
@@ -29,8 +32,11 @@
2932
from unittest.mock import MagicMock
3033

3134
import pytest
35+
from agent_framework._workflows._state import State
3236

37+
from agent_framework_declarative import WorkflowState
3338
from agent_framework_declarative._workflows import DeclarativeWorkflowState
39+
from agent_framework_declarative._workflows import _declarative_base as base
3440

3541
try:
3642
import powerfx # noqa: F401
@@ -81,6 +87,126 @@ class _PlainObj:
8187
text: str = "hi"
8288

8389

90+
@dataclass
91+
class _MemberRecord(_PlainObj):
92+
_private: str = "private-marker"
93+
94+
95+
class TestStateMemberAccess:
96+
"""Both state implementations distinguish object members from dictionary keys."""
97+
98+
@pytest.fixture(params=["standalone", "modern"])
99+
def member_state(self, request: pytest.FixtureRequest) -> WorkflowState | DeclarativeWorkflowState:
100+
if request.param == "standalone":
101+
return WorkflowState(inputs={"record": _MemberRecord()})
102+
state = DeclarativeWorkflowState(State())
103+
state.initialize({"record": _MemberRecord()})
104+
return state
105+
106+
@pytest.mark.parametrize("name", ["_private", "__marker__", "text-other", "1text", "text other", "text\n"])
107+
def test_rejects_existing_invalid_members(
108+
self, member_state: WorkflowState | DeclarativeWorkflowState, name: str
109+
) -> None:
110+
record = _PlainObj()
111+
setattr(record, name, "member-marker")
112+
member_state.set("Local.record", record)
113+
default = object()
114+
115+
assert member_state.get(f"Local.record.{name}", default) is default
116+
117+
def test_rejects_before_reading_attribute(
118+
self, member_state: WorkflowState | DeclarativeWorkflowState, caplog: pytest.LogCaptureFixture
119+
) -> None:
120+
class Record:
121+
@property
122+
def _private(self) -> str:
123+
raise AssertionError("Rejected attribute was read")
124+
125+
member_state.set("Local.record", Record())
126+
default = object()
127+
with caplog.at_level(logging.WARNING):
128+
assert member_state.get("Local.record._private", default) is default
129+
130+
assert any("rejecting attribute segment" in record.message for record in caplog.records)
131+
132+
@pytest.mark.parametrize(
133+
"namespace", ["Workflow.Inputs", "Workflow.Outputs", "Local", "System", "Agent", "Conversation", "Custom"]
134+
)
135+
def test_member_rule_applies_across_namespaces(
136+
self, member_state: WorkflowState | DeclarativeWorkflowState, namespace: str
137+
) -> None:
138+
if namespace != "Workflow.Inputs":
139+
member_state.set(f"{namespace}.record", _MemberRecord())
140+
141+
assert member_state.get(f"{namespace}.record._private") is None
142+
assert member_state.get(f"{namespace}.record.text") == "hi"
143+
144+
def test_mixed_dictionary_and_attribute_steps(self, member_state: WorkflowState | DeclarativeWorkflowState) -> None:
145+
class Record:
146+
class_value = "class-marker"
147+
_private = "private-marker"
148+
149+
def __init__(self) -> None:
150+
self.public_value = "instance-marker"
151+
self.child = {"nested": _PlainObj()}
152+
153+
member_state.set("Local.bag", {"record": Record()})
154+
member_state.set("Local.nested", {"record": _MemberRecord(_private="nested-marker")})
155+
156+
assert member_state.get("Local.bag.record.class_value") == "class-marker"
157+
assert member_state.get("Local.bag.record.public_value") == "instance-marker"
158+
assert member_state.get("Local.bag.record.child.nested.text") == "hi"
159+
assert member_state.get("Local.bag.record._private") is None
160+
assert member_state.get("Local.nested.record._private") is None
161+
default = object()
162+
assert member_state.get("Local.bag.record.missing", default) is default
163+
164+
@pytest.mark.parametrize("key", ["_private", "__marker__", "text-other", "123", "text other", "text\n"])
165+
def test_dictionary_keys_are_not_member_identifiers(
166+
self, member_state: WorkflowState | DeclarativeWorkflowState, key: str
167+
) -> None:
168+
member_state.set("Local.bag", {key: "dictionary-marker"})
169+
assert member_state.get(f"Local.bag.{key}") == "dictionary-marker"
170+
171+
@pytest.mark.parametrize("mode", ["absent", "error"])
172+
def test_modern_engine_errors_do_not_fall_back(self, monkeypatch: pytest.MonkeyPatch, mode: str) -> None:
173+
state = DeclarativeWorkflowState(State())
174+
state.initialize()
175+
state.set("Local.record", _PlainObj())
176+
get = MagicMock(side_effect=AssertionError("Unexpected member lookup"))
177+
monkeypatch.setattr(state, "get", get)
178+
if mode == "absent":
179+
monkeypatch.setattr(base, "Engine", None)
180+
with pytest.raises(RuntimeError, match="PowerFx is not available"):
181+
state.eval("=Local.record.text")
182+
else:
183+
engine = MagicMock()
184+
engine.eval.side_effect = ValueError("synthetic engine error")
185+
monkeypatch.setattr(base, "Engine", MagicMock(return_value=engine))
186+
with pytest.raises(ValueError, match="synthetic engine error"):
187+
state.eval("=Local.record.text")
188+
get.assert_not_called()
189+
190+
@_requires_powerfx
191+
def test_modern_projection_preserves_instance_data(self) -> None:
192+
class Record:
193+
_class_value = "class-marker"
194+
195+
def __init__(self) -> None:
196+
self._declared = "instance-marker"
197+
self.public_value = "public-marker"
198+
199+
state = DeclarativeWorkflowState(State())
200+
state.initialize()
201+
state.set("Local.record", Record())
202+
203+
assert state.get("Local.record._declared") is None
204+
assert state.get("Local.record._class_value") is None
205+
assert state.eval("=Local.record.public_value") == "public-marker"
206+
assert state.eval("=Local.record._declared") == "instance-marker"
207+
assert state.eval("=Local.record._class_value") is None
208+
209+
84210
# ---------------------------------------------------------------------------
85211
# get(): invalid paths return default
86212
# ---------------------------------------------------------------------------

‎python/packages/declarative/tests/test_graph_executors.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import pytest
99
from agent_framework import WorkflowInvocationKwargs
10+
from agent_framework._workflows._state import State
1011

1112
try:
1213
import powerfx # noqa: F401
@@ -17,11 +18,13 @@
1718

1819
_requires_powerfx = pytest.mark.skipif(not _powerfx_available, reason="PowerFx engine not available")
1920

21+
from agent_framework_declarative import WorkflowFactory, WorkflowState # noqa: E402
2022
from agent_framework_declarative._workflows import ( # noqa: E402
2123
ALL_ACTION_EXECUTORS,
2224
DECLARATIVE_STATE_KEY,
2325
ActionComplete,
2426
ActionTrigger,
27+
DeclarativeActionExecutor,
2528
DeclarativeWorkflowBuilder,
2629
DeclarativeWorkflowState,
2730
ForeachInitExecutor,
@@ -31,6 +34,53 @@
3134
)
3235

3336

37+
class TestFactoryStateMemberRoutes:
38+
"""Factory-created workflows use modern state for expressions and templates."""
39+
40+
@_requires_powerfx
41+
async def test_factory_uses_modern_member_resolution(self, monkeypatch: pytest.MonkeyPatch) -> None:
42+
class Record:
43+
_private = "private-marker"
44+
45+
def __init__(self) -> None:
46+
self.public_value = "public-marker"
47+
48+
forbidden = MagicMock(side_effect=AssertionError("Standalone state used by factory"))
49+
monkeypatch.setattr(WorkflowState, "get", forbidden)
50+
monkeypatch.setattr(WorkflowState, "eval", forbidden)
51+
seen: list[type[DeclarativeWorkflowState]] = []
52+
original = DeclarativeActionExecutor._get_state
53+
54+
def track(executor: DeclarativeActionExecutor, store: State) -> DeclarativeWorkflowState:
55+
state = original(executor, store)
56+
seen.append(type(state))
57+
return state
58+
59+
monkeypatch.setattr(DeclarativeActionExecutor, "_get_state", track)
60+
workflow = WorkflowFactory().create_workflow_from_yaml("""
61+
kind: Workflow
62+
trigger:
63+
kind: OnConversationStart
64+
id: member_routes
65+
actions:
66+
- kind: SendActivity
67+
id: public_expression
68+
activity: =Workflow.Inputs.record.public_value
69+
- kind: SendActivity
70+
id: private_expression
71+
activity: =Workflow.Inputs.record._private
72+
- kind: SendActivity
73+
id: private_template
74+
activity: "template:{Workflow.Inputs.record._private}"
75+
""")
76+
77+
result = await workflow.run({"record": Record()})
78+
79+
assert result.get_outputs() == ["public-marker", "template:"]
80+
assert seen and all(kind is DeclarativeWorkflowState for kind in seen)
81+
forbidden.assert_not_called()
82+
83+
3484
class TestDeclarativeWorkflowState:
3585
"""Tests for DeclarativeWorkflowState."""
3686

0 commit comments

Comments
 (0)