Repository navigation
Python: fix(declarative): read every message shape in MessageText - #9101
Yufeng He (he-yufeng) wants to merge 8 commits into
Conversation
WorkflowState's fallback producers emit {"role", "content"} while
DeclarativeWorkflowState's custom evaluators emit {"role", "text"}, and
each MessageText implementation read only its own side's key, so a
message crossing eval paths silently extracted "". The .NET record
shape (structured content list) got stringified into the output.
Both readers now share _extract_message_text, which accepts the flat
content shape, the flat text shape, and structured content/contents
lists (lowercase and PascalCase item keys). Producer shapes are
unchanged; the convergence question in microsoft#9041 stays with maintainers.
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
PascalCase top-level content remains unsupported, and non-text .NET content values are incorrectly extracted as text.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Unifies Python declarative message-text extraction across flat and structured message shapes.
Changes:
- Adds a shared message-text extractor.
- Reuses it across both declarative evaluation paths.
- Adds regression tests for supported message shapes.
| File | Description |
|---|---|
_powerfx_functions.py |
Implements shared extraction logic. |
_declarative_base.py |
Delegates extraction to the shared helper. |
test_powerfx_functions.py |
Tests additional message shapes. |
test_powerfx_yaml_compatibility.py |
Tests state-level extraction compatibility. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Iterating the Any-typed parameter left the loop variable and the getattr argument Unknown under strict pyright; bind the list as list[Any] like the dict casts below it.
GitHub Actions
Active
|
Yufeng He (@he-yufeng) The extractor still reads |
…geText Explicitly typed image_url/file records carry a payload reference in value, not displayable text; emitting it could leak the reference into evaluated expressions. Records with no type declaration keep the existing key probe, and the PascalCase Text record stays accepted.
GitHub Actions
Active
|
Done in af683cb: dict items in a structured content list now check the declared |
GitHub Actions
Active
…MessageText
System.Text.Json keeps capitalized property names on a serialized record, so a cross-runtime message like {"Role": "user", "Content": [...]} still extracted as empty: the dict branch only looked at lowercase content/contents/text while item-level keys already tolerated both casings. Probe Content/Contents/Text alongside their lowercase twins, lowercase winning when both are present.
GitHub Actions
Active
| if isinstance(item_dict.get(key), str): | ||
| parts.append(item_dict[key]) | ||
| break | ||
| elif (item_text := getattr(item, "text", None)) is not None: |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) The dict path now skips typed non-text content, but the object path still appends any .text attribute without checking item.type. A Content.from_text_reasoning(...) object therefore leaks reasoning text through MessageText, even though core Message.text includes only type == "text". Please apply the same type gate to object items and add a reasoning-content regression.
There was a problem hiding this comment.
Yufeng He (@he-yufeng) The current fix gates objects only inside structured content lists. Direct supported shapes such as {"content": Content.from_text_reasoning(...)}, an object whose .content is reasoning content, or a direct typed object still return .text without checking type == "text", so reasoning text can still leak through MessageText. Please apply the same gate to those direct object paths and add regressions.
…eText The dict path skipped typed non-text content, but the object path still appended any .text attribute, so a Content.from_text_reasoning object leaked reasoning text through MessageText while core Message.text keeps only type == "text". Apply the same type gate to object items and add a reasoning-content regression test.
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
…e-tolerance # Conflicts: # python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py # python/packages/declarative/agent_framework_declarative/_workflows/_powerfx_functions.py
GitHub Actions
Active
Motivation & Context
#9041: the same declarative formula produces two different message shapes depending on which state class evaluates it.
WorkflowState's fallback producers emit{"role", "content"}whileDeclarativeWorkflowState's custom evaluators emit{"role", "text"}, and eachMessageTextimplementation read only its own side's key, so a message that crossed eval paths silently extracted"". The .NET record shape (a structured content list) fared worse:message_textstringified the content table into the output.Both producer shapes are pinned by existing tests on each side, so picking one winning key is a behavior-breaking call I left to maintainers in the issue. This PR fixes the harmful half that needs no such decision: readers should never lose a message that crosses paths.
Description & Review Guide
_powerfx_functions.pygains_extract_message_text, a single-message extractor that accepts the flatcontentshape, the flattextshape, and structuredcontent/contentsitem lists (lowercase and PascalCase keys).message_textroutes its dict, list, and object branches through it, and_declarative_base._eval_and_replace_message_textdelegates to it instead of its owntext/contents-only logic.""or a stringified table. Lists of message objects that expose only atextattribute (previously skipped) now contribute._extract_message_text(flatcontentstr, then flattextstr, then structured lists), and that_eval_and_replace_message_textkeeps its existing list-only contract.Related Issue
Refs #9041. This PR closes the silent-extraction harm; the producer-convergence question in the issue stays open for a maintainer decision, so this PR intentionally does not use a closing keyword.
AI Assistance
AI assistance details: implementation, tests, and this PR description.
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.