Skip to content

Commit 748365c

Browse files
jpalvarezlCopilot
jpalvarezl
and
Copilot
committed
Python: Respect quoted identifiers and comments in MessageText scanning
Share quoted-token and comment handling between formula scanning and call-boundary matching. Cover identifier escapes and comment boundaries with preprocessing and real-PowerFx regressions, and replace the test append-return lambda with a typed helper for mypy. Co-authored-by: Copilot <[email protected]>
1 parent d3bb137 commit 748365c

2 files changed

Lines changed: 135 additions & 29 deletions

File tree

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

Lines changed: 37 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -670,6 +670,7 @@ def _preprocess_custom_functions(self, formula: str, temp_writes: list[tuple[str
670670
Results are stored in temporary state variables so untrusted message text is
671671
passed to PowerFx as data rather than inserted into formula source.
672672
Temporary names avoid existing Local keys and references in the original formula.
673+
Quoted strings, quoted identifiers, and comments are left untouched.
673674
674675
Args:
675676
formula: The PowerFx formula to pre-process
@@ -682,6 +683,30 @@ def _preprocess_custom_functions(self, formula: str, temp_writes: list[tuple[str
682683
Returns:
683684
The rewritten formula.
684685
"""
686+
687+
def skip_opaque_token(start: int) -> int:
688+
"""Return the end of a quoted token or comment, or start if neither."""
689+
quote = formula[start]
690+
if quote in ('"', "'"):
691+
pos = start + 1
692+
while pos < len(formula):
693+
if formula[pos] == quote:
694+
if pos + 1 < len(formula) and formula[pos + 1] == quote:
695+
pos += 2
696+
continue
697+
return pos + 1
698+
pos += 1
699+
return pos
700+
if formula.startswith("//", start):
701+
pos = start + 2
702+
while pos < len(formula) and formula[pos] not in "\r\n":
703+
pos += 1
704+
return pos
705+
if formula.startswith("/*", start):
706+
end = formula.find("*/", start + 2)
707+
return len(formula) if end == -1 else end + 2
708+
return start
709+
685710
temp_var_counter = 0
686711
reserved_names = {name.casefold() for name in self.get_state_data().get("Local", {})}
687712
# Reserve formula references too, so previously undefined names stay undefined.
@@ -691,19 +716,10 @@ def _preprocess_custom_functions(self, formula: str, temp_writes: list[tuple[str
691716
cursor = 0
692717

693718
while cursor < len(formula):
694-
if formula[cursor] == '"':
695-
string_start = cursor
696-
cursor += 1
697-
while cursor < len(formula):
698-
if formula[cursor] != '"':
699-
cursor += 1
700-
continue
701-
if cursor + 1 < len(formula) and formula[cursor + 1] == '"':
702-
cursor += 2
703-
continue
704-
cursor += 1
705-
break
706-
result.append(formula[string_start:cursor])
719+
token_end = skip_opaque_token(cursor)
720+
if token_end != cursor:
721+
result.append(formula[cursor:token_end])
722+
cursor = token_end
707723
continue
708724

709725
call_prefix = f"{function_name}("
@@ -715,19 +731,16 @@ def _preprocess_custom_functions(self, formula: str, temp_writes: list[tuple[str
715731
paren_start = cursor + len(function_name)
716732
depth = 1
717733
pos = paren_start + 1
718-
in_string = False
719734
while pos < len(formula) and depth > 0:
735+
token_end = skip_opaque_token(pos)
736+
if token_end != pos:
737+
pos = token_end
738+
continue
720739
char = formula[pos]
721-
if char == '"':
722-
if in_string and pos + 1 < len(formula) and formula[pos + 1] == '"':
723-
pos += 2
724-
continue
725-
in_string = not in_string
726-
elif not in_string:
727-
if char == "(":
728-
depth += 1
729-
elif char == ")":
730-
depth -= 1
740+
if char == "(":
741+
depth += 1
742+
elif char == ")":
743+
depth -= 1
731744
pos += 1
732745

733746
if depth != 0:

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

Lines changed: 98 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,11 +330,12 @@ def test_message_text_inside_string_literal_is_not_preprocessed(
330330
monkeypatch: pytest.MonkeyPatch,
331331
) -> None:
332332
calls: list[str] = []
333-
monkeypatch.setattr(
334-
state,
335-
"_eval_and_replace_message_text",
336-
lambda inner_expr: calls.append(inner_expr) or "hello",
337-
)
333+
334+
def replace_message_text(inner_expr: str) -> str:
335+
calls.append(inner_expr)
336+
return "hello"
337+
338+
monkeypatch.setattr(state, "_eval_and_replace_message_text", replace_message_text)
338339

339340
formula = state._preprocess_custom_functions(
340341
'Concatenate("MessageText(Local.Secret)", MessageText(Local.Messages))',
@@ -344,6 +345,60 @@ def test_message_text_inside_string_literal_is_not_preprocessed(
344345
assert formula == 'Concatenate("MessageText(Local.Secret)", Local._TempMessageText0)'
345346
assert calls == ["Local.Messages"]
346347

348+
@pytest.mark.parametrize(
349+
"formula",
350+
[
351+
'Upper("Say ""MessageText(ignored)""")',
352+
"Upper(Local.'MessageText(ignored)')",
353+
"Upper(Local.'Archived'' MessageText(ignored)')",
354+
"1 // MessageText(ignored)",
355+
"1 // MessageText(ignored)\n + 2",
356+
"1 // MessageText(ignored)\r + 2",
357+
"1 // MessageText(ignored)\r\n + 2",
358+
"/* MessageText(ignored) */ 1",
359+
"1 /* unmatched ) ' \" MessageText(ignored) */",
360+
],
361+
)
362+
def test_quoted_tokens_and_comments_are_not_preprocessed(
363+
self, state: DeclarativeWorkflowState, monkeypatch: pytest.MonkeyPatch, formula: str
364+
) -> None:
365+
def unexpected_call(inner_expr: str) -> str:
366+
pytest.fail(f"Processed a call inside a quoted token or comment: {inner_expr}")
367+
368+
monkeypatch.setattr(state, "_eval_and_replace_message_text", unexpected_call)
369+
temp_writes: list[tuple[str, Any]] = []
370+
371+
assert state._preprocess_custom_functions(formula, temp_writes) == formula
372+
assert temp_writes == []
373+
374+
@pytest.mark.parametrize(
375+
"inner_expr",
376+
[
377+
"Local.'Messages) archive'",
378+
"Local.'Messages'' ) archive'",
379+
'If(")""(" = ")""(", Local.Messages, Local.Messages)',
380+
"Local.Messages /* ) ' \" MessageText(ignored) */",
381+
"Local.Messages // ) MessageText(ignored)\n",
382+
"Local.Messages // ) MessageText(ignored)\r",
383+
"Local.Messages // ) MessageText(ignored)\r\n",
384+
],
385+
)
386+
def test_call_boundaries_ignore_quoted_tokens_and_comments(
387+
self, state: DeclarativeWorkflowState, monkeypatch: pytest.MonkeyPatch, inner_expr: str
388+
) -> None:
389+
calls: list[str] = []
390+
391+
def replace_message_text(expression: str) -> str:
392+
calls.append(expression)
393+
return "hello"
394+
395+
monkeypatch.setattr(state, "_eval_and_replace_message_text", replace_message_text)
396+
397+
formula = state._preprocess_custom_functions(f"Upper(MessageText({inner_expr}))", [])
398+
399+
assert formula == "Upper(Local._TempMessageText0)"
400+
assert calls == [inner_expr]
401+
347402
@pytest.mark.parametrize(
348403
("existing_name", "existing_value"),
349404
[("_TempMessageText0", None), ("_tempmessagetext0", "user-value")],
@@ -389,6 +444,44 @@ def test_simple_powerfx_expression_evaluates(self, state: DeclarativeWorkflowSta
389444
state.set("Local.y", 7)
390445
assert state.eval("=Local.x * Local.y") == 42
391446

447+
@pytest.mark.parametrize("name", ["Messages) archive", "Messages' ) archive"])
448+
def test_message_text_argument_with_quoted_identifier(self, state: DeclarativeWorkflowState, name: str) -> None:
449+
state.set(f"Local.{name}", [{"text": "hello", "contents": [{"type": "text", "text": "hello"}]}])
450+
quoted_name = name.replace("'", "''")
451+
original_local = state.get_state_data()["Local"].copy()
452+
453+
assert state.eval(f"=Upper(MessageText(Local.'{quoted_name}'))") == "HELLO"
454+
assert state.get_state_data()["Local"] == original_local
455+
456+
@pytest.mark.parametrize("name", ["MessageText(ignored)", "Archived' MessageText(ignored)"])
457+
def test_quoted_identifier_with_function_like_text(self, state: DeclarativeWorkflowState, name: str) -> None:
458+
state.set(f"Local.{name}", "hello")
459+
quoted_name = name.replace("'", "''")
460+
original_local = state.get_state_data()["Local"].copy()
461+
462+
assert state.eval(f"=Upper(Local.'{quoted_name}')") == "HELLO"
463+
assert state.get_state_data()["Local"] == original_local
464+
465+
@pytest.mark.parametrize(
466+
("formula", "expected"),
467+
[
468+
("1 // MessageText(1 / 0)", 1),
469+
("1 /* MessageText(1 / 0) */", 1),
470+
("/* MessageText(1 / 0) */ Upper(MessageText(Local.Messages))", "HELLO"),
471+
("// MessageText(1 / 0)\n Upper(MessageText(Local.Messages))", "HELLO"),
472+
("Upper(MessageText(Local.Messages /* ) MessageText(1 / 0) */))", "HELLO"),
473+
("Upper(MessageText(Local.Messages // ) MessageText(1 / 0)\n))", "HELLO"),
474+
],
475+
)
476+
def test_message_text_in_comments_is_not_evaluated(
477+
self, state: DeclarativeWorkflowState, formula: str, expected: str | int
478+
) -> None:
479+
state.set("Local.Messages", [{"text": "hello", "contents": [{"type": "text", "text": "hello"}]}])
480+
original_local = state.get_state_data()["Local"].copy()
481+
482+
assert state.eval(f"={formula}") == expected
483+
assert state.get_state_data()["Local"] == original_local
484+
392485
def test_internal_temp_message_text_still_works(self, state: DeclarativeWorkflowState) -> None:
393486
"""Long MessageText() results round-trip and the temp key is removed after eval."""
394487
long_text = "A" * 600

0 commit comments

Comments
 (0)