Python: Preserve MessageText values during formula preprocessing - #8366
Jose Alvarez (jpalvarezl) wants to merge 4 commits into
Conversation
Process only original formula text and bind extracted messages through temporary state instead of embedding message content in formula source. Avoid temporary-name collisions and cover literal handling, nested calls, and cleanup regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
🟡 Changes recommended
The scanner can execute calls inside comments and mishandle valid single-quoted identifiers.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Preserves MessageText results as data during Power Fx preprocessing.
Changes:
- Adds temporary bindings with collision avoidance and cleanup.
- Prevents rescanning text and double-quoted literals.
- Expands regression and Power Fx tests.
File summaries
| File | Description |
|---|---|
_declarative_base.py |
Reworks MessageText preprocessing. |
test_declarative_state_path_safety.py |
Adds preprocessing and cleanup tests. |
test_graph_coverage.py |
Updates short-message expectations. |
Review details
Suppressed comments (1)
python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py:710
- This raw substring match also scans Power Fx line comments. For example,
1 // MessageText(1 / 0)is a valid formula whose comment should be ignored, but preprocessing evaluates the commented expression and can fail before Power Fx evaluates1. Please make comment ranges opaque during scanning so commented-out calls are never executed.
call_prefix = f"{function_name}("
if not formula.startswith(call_prefix, cursor):
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): d3bb137c426f
Model: gpt-5.6-sol-fast
Overview
No publishable findings were identified in the reviewed scope.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
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 <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
|
Pushed 748365c addressing the quoted-identifier feedback and the comment-handling finding from the review summary, plus the CI mypy error. Local validation: 48 focused tests passed without skips, declarative test mypy passed (20 files), and source type-check, lint/format, compilation, and pre-commit hooks passed. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
Assert the rejected sandbox entry is evicted, then drain asynchronous disposal before checking that its output directory is gone. Avoid racing a directory listing against background deletion without changing runtime behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
MAF Automated Review — Iteration 2
Result: No findings
Scope: 53 net-new commit(s): b7deabb8bcf0, dcd1e62715ca, 501bd5274cf1, 1b4513dfa2f4, 8a546111de85, 4b2f3a33d53e, 5be4c7896784, c457acaccf19, d7823b2f3343, b9dbe4f801a8, c37de519ba5c, a6eb2991ddcc, 5a8cef940916, 4d173b24007d, eddccc474c07, 3abfddffc556, e4309e5456c4, e1c849c4ad1a, c744b16af97b, 4e020838d701, c1f33dfca3b4, 595aa8a5f262, 249759787350, 119c795cceef, e0162ab4bea0, c47da07e2294, 14b9f1d7e9a4, a096549746f2, e844e8f5a007, 393fbf261844, f30187e974cb, ebaa4033cee9, ff15faf50578, 3ccb3aff1e86, 3c670707766a, df03d32151dc, 1cd06c5a2058, 926b4ecacdaa, b0bfd61c1383, db418ad75a1e, 567f1695711f, 21f6fe72c6e4, 3347c43ea886, 003ae93fd476, ae2876262194, 7abe2ff92b2f, 4d5c63a46a9e, 216e7616c884, 31bf0112506b, 4b4d849602d3, 748365cd5b6f, 7442753aa3f9, 6d8ca2dd1d54
Model: gpt-5.6-sol-fast
Overview
This incremental range replaces the earlier double-quote-specific scan with a shared opaque-token helper that protects quoted identifiers and comments in both scanning passes. Collision checks, finally-based temporary-state restoration, focused PowerFx regressions, and the Hyperlight registry-drain test provide concrete guards around the changed behavior. No Critical, High, or Medium defect introduced by the reviewed range was established.
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
| # The Python PowerFx wrapper defaults to a 1000 char expression limit (vs 10,000 in .NET). | ||
| # We use 500 to leave room for the rest of the expression around the replaced value. | ||
| MAX_INLINE_LENGTH = 500 | ||
| def skip_opaque_token(start: int) -> int: |
There was a problem hiding this comment.
this probably does not need to be a closure
Evan Mattson (moonbox3)
left a comment
Evan Mattson (moonbox3)
left a comment
There was a problem hiding this comment.
Also looks like there's a file conflict we should resolve.
| def skip_opaque_token(start: int) -> int: | ||
| """Return the end of a quoted token or comment, or start if neither.""" | ||
| quote = formula[start] | ||
| if quote in ('"', "'"): |
There was a problem hiding this comment.
Could skip_opaque_token distinguish Power Fx interpolated strings from ordinary quoted literals? =$"Message: {MessageText(Local.Messages)}" worked previously, but the new scan skips the whole $"...", leaves MessageText for Power Fx, and raises an unsupported-function error. This breaks existing declarative formulas that call MessageText inside interpolation expressions.
Motivation & Context
Nested
MessageText(...)results should remain text when a declarative workflow evaluates the surrounding PowerFx expression. Repeatedly scanning rewritten formula text can process message contents and quoted literals as function calls. Temporary bindings must also preserve the meaning of existing and undefined state references.Description & Review Guide
MessageTextresults through temporary state instead of inserting them into formula source.Localkeys and names in the original formula, while retaining cleanup on success and failure.Related Issue
None.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.