From d215d27363762960bc1963f9010fee1c59e58c42 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Wed, 30 Sep 2026 11:20:12 +0200 Subject: [PATCH 1/2] Align workflow state attribute lookup --- python/packages/declarative/AGENTS.md | 4 +- .../_workflows/_declarative_base.py | 6 +- .../_workflows/_state.py | 31 +++- .../_workflows/_state_path.py | 12 ++ .../test_declarative_state_path_safety.py | 134 +++++++++++++++++- .../declarative/tests/test_graph_executors.py | 50 +++++++ .../declarative/tests/test_workflow_state.py | 104 ++++++++++++++ 7 files changed, 326 insertions(+), 15 deletions(-) create mode 100644 python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py diff --git a/python/packages/declarative/AGENTS.md b/python/packages/declarative/AGENTS.md index 9638b435337..3e3fe5879a3 100644 --- a/python/packages/declarative/AGENTS.md +++ b/python/packages/declarative/AGENTS.md @@ -6,12 +6,14 @@ YAML/JSON-based declarative agent and workflow definitions. - **`AgentFactory`** - Creates agents from declarative definitions - **`WorkflowFactory`** - Creates workflows from declarative definitions -- **`WorkflowState`** - State management for declarative workflows +- **`WorkflowState`** - Standalone state management and expression evaluation; factory-created workflows use the internal, checkpoint-backed `DeclarativeWorkflowState` instead - **`ProviderTypeMapping`** - Maps provider types to implementations - **`HttpRequestHandler`** / **`DefaultHttpRequestHandler`** - Pluggable HTTP transport for the `HttpRequestAction` declarative action (configured via `WorkflowFactory(http_request_handler=...)`) - **`MCPToolHandler`** / **`DefaultMCPToolHandler`** - Pluggable MCP transport for the `InvokeMcpTool` declarative action (configured via `WorkflowFactory(mcp_tool_handler=...)`) - **`DeclarativeLoaderError`** / **`ProviderLookupError`** / **`DeclarativeWorkflowError`** / **`DeclarativeActionError`** - Error types +Both state classes validate object attribute names but leave dictionary keys unchanged. + ## MCP Handler Lifetimes `DefaultMCPToolHandler` caches/coalesces sessions only without a `client_provider`. diff --git a/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py b/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py index a9eb95473d9..42aec8c49bb 100644 --- a/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py +++ b/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py @@ -47,6 +47,7 @@ from ._errors import DeclarativeWorkflowError from ._powerfx_limits import _PowerFxStateBudget, _validate_powerfx_state # pyright: ignore[reportPrivateUsage] +from ._state_path import _is_safe_path_segment # pyright: ignore[reportPrivateUsage] try: from powerfx import Engine @@ -66,9 +67,6 @@ _ENV_REFERENCE_RE = re.compile(r"\bEnv\.([A-Za-z_][A-Za-z0-9_]*)") -# Allowed identifier shape for object-attribute steps in declarative state paths -_SAFE_PATH_SEGMENT_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_]*$") - def _skip_powerfx_opaque_token(formula: str, start: int) -> int: """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: return default else: # Attribute access is only allowed for safe declarative identifiers. - if not _SAFE_PATH_SEGMENT_RE.match(part): + if not _is_safe_path_segment(part): logger.warning( "DeclarativeWorkflowState.get: rejecting attribute segment %r in path %r", part, diff --git a/python/packages/declarative/agent_framework_declarative/_workflows/_state.py b/python/packages/declarative/agent_framework_declarative/_workflows/_state.py index 73f06230ad9..a617f812e66 100644 --- a/python/packages/declarative/agent_framework_declarative/_workflows/_state.py +++ b/python/packages/declarative/agent_framework_declarative/_workflows/_state.py @@ -17,6 +17,7 @@ from typing import Any, cast from ._powerfx_limits import _PowerFxStateLimitError, _validate_powerfx_state # pyright: ignore[reportPrivateUsage] +from ._state_path import _is_safe_path_segment # pyright: ignore[reportPrivateUsage] try: from powerfx import Engine @@ -31,7 +32,10 @@ class WorkflowState: - """Manages variables and state during declarative workflow execution. + """Manages standalone workflow state and expression evaluation. + + Workflows created by ``WorkflowFactory`` use a separate, checkpoint-backed + ``DeclarativeWorkflowState`` implementation. WorkflowState provides a unified interface for: @@ -161,12 +165,16 @@ def conversation(self) -> dict[str, Any]: def get(self, path: str, default: Any = None) -> Any: """Get a value from the state using a dot-notated path. + Dict-keyed segments may use arbitrary string keys. Segments resolved + through object-attribute access must match ``[A-Za-z][A-Za-z0-9_]*``; + other shapes return ``default`` without accessing the attribute. + Args: path: Dot-notated path like 'Local.results' or 'Workflow.Inputs.query' default: Default value if path doesn't exist Returns: - The value at the path, or default if not found + The value at the path, or default if not found or unreachable """ parts = path.split(".") if not parts: @@ -206,10 +214,18 @@ def get(self, path: str, default: Any = None) -> Any: obj = obj_dict.get(part, default) if obj is default: return default - elif hasattr(obj, part): - obj = getattr(obj, part) else: - return default + if not _is_safe_path_segment(part): + logger.warning( + "WorkflowState.get: rejecting attribute segment %r in path %r", + part, + path, + ) + return default + if hasattr(obj, part): + obj = getattr(obj, part) + else: + return default return obj @@ -372,7 +388,10 @@ def eval(self, expression: str) -> Any: """Evaluate a PowerFx expression with the current state. Expressions starting with '=' are evaluated as PowerFx. - Other strings are returned as-is (after variable interpolation if applicable). + Other strings are returned as-is. If PowerFx is unavailable or raises + an ordinary evaluation error, the simple evaluator is used instead. + Its state references follow :meth:`get`'s object-member rules. + State-budget errors propagate without falling back. Args: expression: The expression to evaluate diff --git a/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py b/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py new file mode 100644 index 00000000000..3a23b9488d3 --- /dev/null +++ b/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py @@ -0,0 +1,12 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Shared identifier policy for object-attribute steps in workflow state paths.""" + +import re + +_SAFE_PATH_SEGMENT_RE = re.compile(r"[A-Za-z][A-Za-z0-9_]*") + + +def _is_safe_path_segment(segment: str) -> bool: # pyright: ignore[reportUnusedFunction] + """Return whether an object-attribute segment is a declarative identifier.""" + return _SAFE_PATH_SEGMENT_RE.fullmatch(segment) is not None diff --git a/python/packages/declarative/tests/test_declarative_state_path_safety.py b/python/packages/declarative/tests/test_declarative_state_path_safety.py index 40683e5e2ac..b726dd56708 100644 --- a/python/packages/declarative/tests/test_declarative_state_path_safety.py +++ b/python/packages/declarative/tests/test_declarative_state_path_safety.py @@ -4,11 +4,11 @@ # pyright: reportPrivateUsage=false, reportUnknownVariableType=false # pyright: reportGeneralTypeIssues=false -"""Path-segment validation tests for DeclarativeWorkflowState. +"""Path-segment validation tests for workflow state. -Path segments handed to ``get``/``set``/``append`` and ``{Variable.Path}`` -placeholders in ``interpolate_string`` are subject to three distinct rules -that this module pins: +For DeclarativeWorkflowState, path segments handed to ``get``/``set``/``append`` +and ``{Variable.Path}`` placeholders in ``interpolate_string`` are subject to +three distinct rules that this module pins: - **Empty segments** (e.g. ``""``, ``"Local."``, ``"Local..foo"``) are rejected by all of ``get``/``set``/``append`` and ``interpolate_string``. ``get`` and @@ -21,6 +21,9 @@ - **Dict-keyed segments** — segments that resolve via dict lookup because the parent is a ``dict`` — may use arbitrary non-empty string keys (e.g. UUIDs or hyphenated identifiers like ``System.conversations..messages``). + +Standalone WorkflowState shares the object-member rule, but retains its +existing empty-dictionary-key behavior. """ import logging @@ -29,8 +32,11 @@ from unittest.mock import MagicMock import pytest +from agent_framework._workflows._state import State +from agent_framework_declarative import WorkflowState from agent_framework_declarative._workflows import DeclarativeWorkflowState +from agent_framework_declarative._workflows import _declarative_base as base try: import powerfx # noqa: F401 @@ -81,6 +87,126 @@ class _PlainObj: text: str = "hi" +@dataclass +class _MemberRecord(_PlainObj): + _private: str = "private-marker" + + +class TestStateMemberAccess: + """Both state implementations distinguish object members from dictionary keys.""" + + @pytest.fixture(params=["standalone", "modern"]) + def member_state(self, request: pytest.FixtureRequest) -> WorkflowState | DeclarativeWorkflowState: + if request.param == "standalone": + return WorkflowState(inputs={"record": _MemberRecord()}) + state = DeclarativeWorkflowState(State()) + state.initialize({"record": _MemberRecord()}) + return state + + @pytest.mark.parametrize("name", ["_private", "__marker__", "text-other", "1text", "text other", "text\n"]) + def test_rejects_existing_invalid_members( + self, member_state: WorkflowState | DeclarativeWorkflowState, name: str + ) -> None: + record = _PlainObj() + setattr(record, name, "member-marker") + member_state.set("Local.record", record) + default = object() + + assert member_state.get(f"Local.record.{name}", default) is default + + def test_rejects_before_reading_attribute( + self, member_state: WorkflowState | DeclarativeWorkflowState, caplog: pytest.LogCaptureFixture + ) -> None: + class Record: + @property + def _private(self) -> str: + raise AssertionError("Rejected attribute was read") + + member_state.set("Local.record", Record()) + default = object() + with caplog.at_level(logging.WARNING): + assert member_state.get("Local.record._private", default) is default + + assert any("rejecting attribute segment" in record.message for record in caplog.records) + + @pytest.mark.parametrize( + "namespace", ["Workflow.Inputs", "Workflow.Outputs", "Local", "System", "Agent", "Conversation", "Custom"] + ) + def test_member_rule_applies_across_namespaces( + self, member_state: WorkflowState | DeclarativeWorkflowState, namespace: str + ) -> None: + if namespace != "Workflow.Inputs": + member_state.set(f"{namespace}.record", _MemberRecord()) + + assert member_state.get(f"{namespace}.record._private") is None + assert member_state.get(f"{namespace}.record.text") == "hi" + + def test_mixed_dictionary_and_attribute_steps(self, member_state: WorkflowState | DeclarativeWorkflowState) -> None: + class Record: + class_value = "class-marker" + _private = "private-marker" + + def __init__(self) -> None: + self.public_value = "instance-marker" + self.child = {"nested": _PlainObj()} + + member_state.set("Local.bag", {"record": Record()}) + member_state.set("Local.nested", {"record": _MemberRecord(_private="nested-marker")}) + + assert member_state.get("Local.bag.record.class_value") == "class-marker" + assert member_state.get("Local.bag.record.public_value") == "instance-marker" + assert member_state.get("Local.bag.record.child.nested.text") == "hi" + assert member_state.get("Local.bag.record._private") is None + assert member_state.get("Local.nested.record._private") is None + default = object() + assert member_state.get("Local.bag.record.missing", default) is default + + @pytest.mark.parametrize("key", ["_private", "__marker__", "text-other", "123", "text other", "text\n"]) + def test_dictionary_keys_are_not_member_identifiers( + self, member_state: WorkflowState | DeclarativeWorkflowState, key: str + ) -> None: + member_state.set("Local.bag", {key: "dictionary-marker"}) + assert member_state.get(f"Local.bag.{key}") == "dictionary-marker" + + @pytest.mark.parametrize("mode", ["absent", "error"]) + def test_modern_engine_errors_do_not_fall_back(self, monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + state = DeclarativeWorkflowState(State()) + state.initialize() + state.set("Local.record", _PlainObj()) + get = MagicMock(side_effect=AssertionError("Unexpected member lookup")) + monkeypatch.setattr(state, "get", get) + if mode == "absent": + monkeypatch.setattr(base, "Engine", None) + with pytest.raises(RuntimeError, match="PowerFx is not available"): + state.eval("=Local.record.text") + else: + engine = MagicMock() + engine.eval.side_effect = ValueError("synthetic engine error") + monkeypatch.setattr(base, "Engine", MagicMock(return_value=engine)) + with pytest.raises(ValueError, match="synthetic engine error"): + state.eval("=Local.record.text") + get.assert_not_called() + + @_requires_powerfx + def test_modern_projection_preserves_instance_data(self) -> None: + class Record: + _class_value = "class-marker" + + def __init__(self) -> None: + self._declared = "instance-marker" + self.public_value = "public-marker" + + state = DeclarativeWorkflowState(State()) + state.initialize() + state.set("Local.record", Record()) + + assert state.get("Local.record._declared") is None + assert state.get("Local.record._class_value") is None + assert state.eval("=Local.record.public_value") == "public-marker" + assert state.eval("=Local.record._declared") == "instance-marker" + assert state.eval("=Local.record._class_value") is None + + # --------------------------------------------------------------------------- # get(): invalid paths return default # --------------------------------------------------------------------------- diff --git a/python/packages/declarative/tests/test_graph_executors.py b/python/packages/declarative/tests/test_graph_executors.py index d01ab1c744c..9ab82ef6dcf 100644 --- a/python/packages/declarative/tests/test_graph_executors.py +++ b/python/packages/declarative/tests/test_graph_executors.py @@ -7,6 +7,7 @@ import pytest from agent_framework import WorkflowInvocationKwargs +from agent_framework._workflows._state import State try: import powerfx # noqa: F401 @@ -17,11 +18,13 @@ _requires_powerfx = pytest.mark.skipif(not _powerfx_available, reason="PowerFx engine not available") +from agent_framework_declarative import WorkflowFactory, WorkflowState # noqa: E402 from agent_framework_declarative._workflows import ( # noqa: E402 ALL_ACTION_EXECUTORS, DECLARATIVE_STATE_KEY, ActionComplete, ActionTrigger, + DeclarativeActionExecutor, DeclarativeWorkflowBuilder, DeclarativeWorkflowState, ForeachInitExecutor, @@ -31,6 +34,53 @@ ) +class TestFactoryStateMemberRoutes: + """Factory-created workflows use modern state for expressions and templates.""" + + @_requires_powerfx + async def test_factory_uses_modern_member_resolution(self, monkeypatch: pytest.MonkeyPatch) -> None: + class Record: + _private = "private-marker" + + def __init__(self) -> None: + self.public_value = "public-marker" + + forbidden = MagicMock(side_effect=AssertionError("Standalone state used by factory")) + monkeypatch.setattr(WorkflowState, "get", forbidden) + monkeypatch.setattr(WorkflowState, "eval", forbidden) + seen: list[type[DeclarativeWorkflowState]] = [] + original = DeclarativeActionExecutor._get_state + + def track(executor: DeclarativeActionExecutor, store: State) -> DeclarativeWorkflowState: + state = original(executor, store) + seen.append(type(state)) + return state + + monkeypatch.setattr(DeclarativeActionExecutor, "_get_state", track) + workflow = WorkflowFactory().create_workflow_from_yaml(""" +kind: Workflow +trigger: + kind: OnConversationStart + id: member_routes + actions: + - kind: SendActivity + id: public_expression + activity: =Workflow.Inputs.record.public_value + - kind: SendActivity + id: private_expression + activity: =Workflow.Inputs.record._private + - kind: SendActivity + id: private_template + activity: "template:{Workflow.Inputs.record._private}" +""") + + result = await workflow.run({"record": Record()}) + + assert result.get_outputs() == ["public-marker", "template:"] + assert seen and all(kind is DeclarativeWorkflowState for kind in seen) + forbidden.assert_not_called() + + class TestDeclarativeWorkflowState: """Tests for DeclarativeWorkflowState.""" diff --git a/python/packages/declarative/tests/test_workflow_state.py b/python/packages/declarative/tests/test_workflow_state.py index c72d85d1ab0..531b6c75e5d 100644 --- a/python/packages/declarative/tests/test_workflow_state.py +++ b/python/packages/declarative/tests/test_workflow_state.py @@ -2,8 +2,11 @@ """Unit tests for WorkflowState class.""" +from unittest.mock import MagicMock + import pytest +from agent_framework_declarative._workflows import _state as state_module from agent_framework_declarative._workflows._state import WorkflowState @@ -554,6 +557,107 @@ def test_get_unknown_workflow_subspace(self): assert state.get("Workflow.Unknown.path") is None +class TestWorkflowStateMemberEvaluation: + """Member rules apply to the standalone evaluator's fallback paths.""" + + @pytest.fixture + def member_state(self) -> WorkflowState: + class Record: + _private = "private-marker" + __marker__ = "double-underscore-marker" + + def __init__(self) -> None: + self.public_value = "public-marker" + + state = WorkflowState() + state.set("Local.record", Record()) + state.set("Local.bag", {"_private": "dictionary-marker"}) + return state + + @pytest.mark.parametrize("mode", ["absent", "error", "real"]) + @pytest.mark.parametrize( + ("path", "expected"), + [ + ("Local.record._private", None), + ("Local.record.__marker__", None), + ("Local.record.public_value", "public-marker"), + ("Local.bag._private", "dictionary-marker"), + ], + ) + def test_fallback_member_access( + self, + member_state: WorkflowState, + monkeypatch: pytest.MonkeyPatch, + mode: str, + path: str, + expected: str | None, + ) -> None: + fallback = MagicMock(wraps=member_state._eval_simple) + monkeypatch.setattr(member_state, "_eval_simple", fallback) + if mode == "absent": + engine = None + elif mode == "error": + engine = MagicMock() + engine.eval.side_effect = ValueError("synthetic engine error") + else: + if state_module._powerfx_engine is None: + pytest.skip("PowerFx engine not available") + engine = MagicMock(wraps=state_module._powerfx_engine) + monkeypatch.setattr(state_module, "_powerfx_engine", engine) + + assert member_state.eval(f"={path}") == expected + fallback.assert_called_once_with(path) + if engine is not None: + engine.eval.assert_called_once() + + def test_real_engine_success_does_not_fall_back(self, monkeypatch: pytest.MonkeyPatch) -> None: + if state_module._powerfx_engine is None: + pytest.skip("PowerFx engine not available") + state = WorkflowState() + state.set("Local.bag", {"_private": "dictionary-marker"}) + fallback = MagicMock(side_effect=AssertionError("Unexpected fallback")) + monkeypatch.setattr(state, "_eval_simple", fallback) + + assert state.eval("=Local.bag._private") == "dictionary-marker" + fallback.assert_not_called() + + def test_recursive_expression_values(self, member_state: WorkflowState, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(state_module, "_powerfx_engine", None) + assert member_state.eval_if_expression({ + "values": ["=Local.record._private", "=Local.record.public_value", "=Local.bag._private"] + }) == {"values": [None, "public-marker", "dictionary-marker"]} + + def test_custom_namespace_preserves_unresolved_formula( + self, member_state: WorkflowState, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(state_module, "_powerfx_engine", None) + member_state.set("Custom.record", member_state.get("Local.record")) + + assert member_state.eval("=Custom.record._private") == "Custom.record._private" + assert member_state.eval("=Custom.record.public_value") == "public-marker" + + def test_append_does_not_mutate_rejected_member(self) -> None: + class Record: + def __init__(self) -> None: + self._items = ["original"] + + state = WorkflowState() + record = Record() + state.set("Local.record", record) + + with pytest.raises(TypeError): + state.append("Local.record._items", "new") + assert record._items == ["original"] + + state.append("Local.bag._items", "new") + assert state.get("Local.bag._items") == ["new"] + + def test_empty_dictionary_key_behavior_is_unchanged(self) -> None: + state = WorkflowState() + state.set("Local.bag", {"": "dictionary-marker"}) + assert state.get("Local.bag.") == "dictionary-marker" + + class TestWorkflowStateConversationIdInit: """Tests that WorkflowState generates a real UUID for System.ConversationId.""" From 254f8f9979c6e1d870543dcb6e79356fe269393d Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Wed, 30 Sep 2026 11:57:10 +0200 Subject: [PATCH 2/2] Use regex for workflow state attribute validation --- .../agent_framework_declarative/_workflows/_state_path.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py b/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py index 3a23b9488d3..4bc072f6aca 100644 --- a/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py +++ b/python/packages/declarative/agent_framework_declarative/_workflows/_state_path.py @@ -2,9 +2,9 @@ """Shared identifier policy for object-attribute steps in workflow state paths.""" -import re +import regex -_SAFE_PATH_SEGMENT_RE = re.compile(r"[A-Za-z][A-Za-z0-9_]*") +_SAFE_PATH_SEGMENT_RE = regex.compile(r"[A-Za-z][A-Za-z0-9_]*") def _is_safe_path_segment(segment: str) -> bool: # pyright: ignore[reportUnusedFunction]