.NET: Fix single-column value unwrap in declarative workflow#6367
.NET: Fix single-column value unwrap in declarative workflow#6367peibekwe wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts how ForeachExecutor materializes loop items when the items expression evaluates to a Power Fx single-column table produced by scalar array literals (e.g. =[1,2,3]), unwrapping the {Value: ...} record shape so the loop variable receives the scalar value as-authored.
Changes:
- Update
ForeachExecutorto unwrap single-columnRecordDataValueitems with the"Value"field into their scalarFormulaValue. - Add a unit test covering the scalar-array-literal table shape to ensure the loop variable is not a
RecordValueand converts to the expected scalar.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/ObjectModel/ForeachExecutor.cs | Unwraps {Value: ...} loop items into scalars when materializing table-backed foreach values. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/ObjectModel/ForeachExecutorTest.cs | Adds regression coverage for single-column Value record unwrapping behavior. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 90%
✓ Correctness
The PR correctly implements unwrapping of Power Fx's single-column {Value: ...} wrapper records in the foreach loop executor. The three-part condition (is RecordDataValue, has exactly one property, that property is named "Value") is precise and does not interfere with multi-field records. The logic is consistent with the existing unwrap pattern in DataValueExtensions.ToObject (lines 210-213). No correctness issues found.
✓ Security Reliability
The change introduces a well-guarded single-column Value-record unwrap in ForeachExecutor that mirrors the existing convention in DataValueExtensions.ToObject. The logic is correct: it only unwraps when the record has exactly one property named "Value", leaving multi-field records and non-record values unchanged. The null safety is sound (TryGetValue returning true guarantees non-null out parameter per .NET contracts). Checkpoint/restore is unaffected since the unwrapped FormulaValue[] is persisted after transformation. No security or reliability issues found.
✓ Test Coverage
The new test
ForeachTakeNextWithSingleColumnValueRecordAsynccorrectly verifies the happy-path unwrapping of single-column{Value: ...}records. The existingForeachTakeNextWithMultiFieldRecordAsynccovers the multi-field pass-through. However, there's no test for the edge case of a single-column record whose key is NOT "Value" (e.g.,{Name: "Alice"}), which exercises a distinct branch in the newToLoopValuelogic (Properties.Count == 1is true butTryGetValue("Value", ...)is false).
✗ Design Approach
The new unwrap helper conflates Power Fx single-column scalar tables with ordinary record tables whose schema is literally
{ Value: ... }. Existing tests already treat{ Value: string }rows as legitimate records, so applying this unconditionally inForeachExecutorchanges the meaning of valid inputs and can breakCurrentValue.Valuelookups inside foreach bodies.
Flagged Issues
-
ForeachExecutornow unwraps every single-fieldValuerecord before publishing the loop item (lines 104-108), but the repo already uses{ Value: ... }as a real record shape (e.g.,EditTableV2ExecutorTestassertions at lines 163-177 and 251-267). A foreach over that same table shape will silently hand the body a scalar instead of a record, breaking expressions likeCurrentValue.Valuein the loop body.
Automated review by peibekwe's agents
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 91%
✓ Correctness
The PR correctly implements single-column Value-record unwrapping for Power Fx scalar array literals in the foreach loop executor. The logic mirrors the existing convention in DataValueExtensions.ToObject (line 210), correctly falls through to value.ToFormula() for multi-field records (count != 1) or records with a single field named something other than "Value", and is null-safe since ToFormula is an extension method on DataValue? that returns blank for null. No correctness issues found.
✓ Security Reliability
The change is sound from a security and reliability standpoint. The new
ToLopValuemethod mirrors the established single-column Value unwrap pattern already present inDataValueExtensions.ToObject(line 210). The guard conditions (exact count check + TryGetValue) are defensive,ToFormulais a null-safe extension method, and no external/untrusted input flows through this path without validation. The previously-raised checkpoint-restore inconsistency concern was already resolved in the review thread.
✓ Test Coverage
The new test
ForeachTakeNextWithSingleColumnValueRecordAsyncadequately covers the primary use case (Power Fx{Value: N}unwrap to scalar), and the existingForeachTakeNextWithMultiFieldRecordAsyncconfirms multi-field records pass through. However, there is a gap: no test verifies that a single-field record whose key is NOT "Value" (e.g.,{name: "Alice"}) remains as aRecordValue. This edge case validates the key-name check inToLoopValueand would catch regressions if the condition were accidentally broadened.
✗ Design Approach
The design change fixes the fresh-execution path, but it still leaves checkpoint restore on the old shape. That means the same foreach can expose a scalar on a new run and a
{Value: ...}record on resume after restore, so I would request changes until the normalization is applied consistently across both entry points.
Flagged Issues
- ForeachExecutor only unwraps single-column
Valuerecords when_valuesis materialized fromModel.Items;OnCheckpointRestoredAsyncstill restores_valueswithsavedValues.Select(value => value.ToFormula())(ForeachExecutor.cs:138), so resumed workflows can still observe wrapped loop values even though fresh executions now return scalars. Please apply the same single-column unwrapping logic to restored values.
Automated review by peibekwe's agents
Motivation and Context
Power Fx wraps scalar array literals like =[1, 2, 3] as Table({Value: 1}, {Value: 2}, {Value: 3}).
This PR preserves the multi-field fix and additionally unwraps the single-column {Value: ...} shape so
Local.LoopValuereads as the scalar the author wrote. Mirrors the convention already applied inDataValueExtensions.ToObject.Contribution Checklist