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 eaa97ba079..ae2ed94f44 100644 --- a/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py +++ b/python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py @@ -66,6 +66,7 @@ _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_]*$") + @dataclass(frozen=True) class DeclarativeEnvConfig: """Configuration that populates the PowerFx ``Env`` symbol for a workflow. @@ -408,12 +409,14 @@ class DeclarativeWorkflowState: value: The value to set Raises: - ValueError: If attempting to set Workflow.Inputs (which is read-only). + ValueError: If ``path`` is empty or contains empty segments + (e.g. ``"Local."``, ``"Local..foo"``), or if attempting to set + ``Workflow.Inputs`` (which is read-only). """ state_data = self.get_state_data() parts = path.split(".") - if not parts: - return + if not parts or any(not p for p in parts): + raise ValueError(f"Invalid path {path!r}: empty segments are not allowed") namespace = parts[0] remaining = parts[1:] @@ -469,7 +472,16 @@ class DeclarativeWorkflowState: Args: path: Dot-notated path to a list value: The value to append + + Raises: + ValueError: If ``path`` is empty or contains empty segments + (e.g. ``"Local."``, ``"Local..foo"``), or if the existing + value at ``path`` is not a list. """ + parts = path.split(".") + if not parts or any(not p for p in parts): + raise ValueError(f"Invalid path {path!r}: empty segments are not allowed") + existing = self.get(path) if existing is None: self.set(path, [value]) 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 6760ab47f1..783607a87d 100644 --- a/python/packages/declarative/tests/test_declarative_state_path_safety.py +++ b/python/packages/declarative/tests/test_declarative_state_path_safety.py @@ -7,9 +7,20 @@ """Path-segment validation tests for DeclarativeWorkflowState. Path segments handed to ``get``/``set``/``append`` and ``{Variable.Path}`` -placeholders in ``interpolate_string`` must be valid declarative identifiers -(``[A-Za-z][A-Za-z0-9_]*``). These tests pin that contract and the related -behavior across legitimate and invalid inputs. +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 + ``interpolate_string`` return their default / leave the placeholder literal; + ``set`` and ``append`` raise ``ValueError``. +- **Object-attribute segments** — segments that ``get`` would resolve via + ``getattr`` because the parent is a non-dict object — must match the safe + identifier shape ``[A-Za-z][A-Za-z0-9_]*``. Other shapes are rejected with a + warning log and the default is returned. +- **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``). """ import logging @@ -179,6 +190,43 @@ class TestSetAndAppend: state.set("Workflow.Inputs.x", 1) +# --------------------------------------------------------------------------- +# set() / append(): malformed paths (empty segments) raise ValueError +# --------------------------------------------------------------------------- + + +class TestSetRejectsInvalidPaths: + @pytest.mark.parametrize("bad_path", ["", "Local.", "Local..foo", ".Local"]) + def test_set_rejects_empty_segment(self, state: DeclarativeWorkflowState, bad_path: str) -> None: + with pytest.raises(ValueError, match="empty segments are not allowed"): + state.set(bad_path, "x") + + @pytest.mark.parametrize("bad_path", ["", "Local.", "Local..foo", ".Local"]) + def test_append_rejects_empty_segment(self, state: DeclarativeWorkflowState, bad_path: str) -> None: + with pytest.raises(ValueError, match="empty segments are not allowed"): + state.append(bad_path, "x") + + def test_set_rejection_makes_no_partial_write(self, state: DeclarativeWorkflowState) -> None: + """Rejected set() must not create an unreachable entry in the state.""" + state.set("Local.user_input", "pre") + with pytest.raises(ValueError): + state.set("Local.", "leak") + local = state.get_state_data().get("Local", {}) + assert "" not in local + assert local == {"user_input": "pre"} + assert state.get("Local.") is None + assert state.get("Local.user_input") == "pre" + + def test_append_rejection_makes_no_partial_write(self, state: DeclarativeWorkflowState) -> None: + """Rejected append() must not create an unreachable entry in the state.""" + state.set("Local.items", ["a"]) + with pytest.raises(ValueError): + state.append("Local.", "leak") + local = state.get_state_data().get("Local", {}) + assert "" not in local + assert local == {"items": ["a"]} + + # --------------------------------------------------------------------------- # interpolate_string(): invalid placeholders left intact, valid ones resolved # ---------------------------------------------------------------------------