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
Inactive
GitHub Actions
Inactive
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
Inactive
|
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
Inactive
|
Done in af683cb: dict items in a structured content list now check the declared |
GitHub Actions
Inactive
…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
Inactive
| 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
|
Thank you for the contribution. This pull request has had outstanding feedback about preventing direct typed reasoning content from being returned by MessageText for more than three days without recent progress, so we are closing it for now to keep the review queue current. Please reopen it when you are ready to continue and address the outstanding item, and we will be happy to take another look. |
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.