.NET: Preserve table state across declarative EditTable operations - #7353
.NET: Preserve table state across declarative EditTable operations #7353peibekwe wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET declarative EditTable / EditTableV2 executors so table-edit operations (Remove, Clear, TakeFirst, TakeLast) preserve the original table variable as a TableValue, enabling subsequent table edits and improving checkpoint/resume compatibility. It also expands unit test coverage to validate table preservation and portable (checkpoint) round-trips.
Changes:
- Update
EditTableExecutorandEditTableV2Executorto keepitemsVariablebound to a (mutated) table afterClear/Remove/TakeFirst/TakeLast, with taken items routed viaResultVariable. - Add/adjust unit tests to validate preserved table schema/shape and editability after portable restore and checkpoint/resume.
- Refine portable table restoration logic for legacy shapes and add tests for legacy portability expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/ObjectModel/EditTableExecutor.cs | Preserves table variables across edits and routes operation results via ResultVariable. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/ObjectModel/EditTableV2Executor.cs | Aligns V2 executor behavior with table preservation and supports ResultVariable for take operations. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Extensions/PortableValueExtensions.cs | Updates portable → formula table reconstruction to better preserve table shapes/types. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/ObjectModel/EditTableExecutorTest.cs | Adds/updates tests for table preservation, portable restore, and checkpoint/resume scenarios. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/ObjectModel/EditTableV2ExecutorTest.cs | Adds/updates V2 tests ensuring operations preserve table state and set results as expected. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/Extensions/PortableValueExtensionsTests.cs | Adds tests asserting legacy portable shapes and append behavior after restoration. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 89%
✓ Correctness
The PR correctly preserves table state across EditTable operations (Remove, Clear, TakeFirst, TakeLast) by keeping the table in itemsVariable and writing taken/removed results to resultVariable. The PortableValueExtensions changes ensure restored tables are mutable (support AppendAsync) by materializing records into arrays. The special-case handling for empty+schemaless tables in the Add operation gracefully handles the schema loss that occurs during checkpoint round-trips of empty tables. AssignAsync safely handles null PropertyPath by short-circuiting. The changes are well-covered by comprehensive tests including checkpoint/restore scenarios.
✓ Security Reliability
The PR preserves table state across declarative EditTable operations by keeping
itemsVariableas a table after Remove, Clear, TakeFirst, and TakeLast, and routing extracted values toresultVariable. The changes are well-structured and well-tested.AssignAsyncsafely handles nullPropertyPath?via early return (DeclarativeActionExecutor.cs:129-131), so the nullablethis.Model.ResultVariable?.Pathcalls are safe. The empty-table schema-loss during checkpoint serialization (viaRecordType.Empty()inToTablefor 0-length arrays) is correctly compensated in the Add path by inferring schema from the new value. No security, injection, resource leak, or reliability issues found.
✓ Test Coverage
Test coverage for this PR is comprehensive. All key behavioral changes — table preservation across Remove/Clear/TakeFirst/TakeLast, resultVariable assignment, checkpoint/restore roundtrips, and legacy table mutability — are well-covered with meaningful assertions. Both V1 (EditTableExecutor) and V2 (EditTableV2Executor) have thorough test suites, with composite operation tests (e.g., clear→restore→add, takeFirst→add) validating the core fix. Two minor symmetric gaps exist but are low risk.
✓ Failure Modes
The PR correctly preserves table state across EditTable operations by keeping
itemsVariableas a table after Remove, Clear, TakeFirst, and TakeLast, routing extracted/result values toresultVariableinstead. ThePortableValueExtensions.ToTablechange (.ToArray()) ensures restored tables are mutable and support AppendAsync. TheAssignAsyncbase method safely handles nullResultVariable?.Pathby returning early. The empty-schema-table Add path correctly detects schema-less tables via!recordType.FieldNames.Any() && !tableValue.Rows.Any()and rebuilds from the record value. Both V1 and V2 executors are updated consistently. No concrete new failure modes were identified.
✓ Design Approach
The table-preservation approach looks sound for record tables and most primitive tables, but it still leaves a compatibility gap for date-only primitive tables restored from checkpoints. The new PortableValue[] -> TableValue path infers DateTime for restored date-like columns even though the same portable scalar path round-trips midnight DateTime values as FormulaType.Date, so the PR does not fully satisfy its stated downgrade/editability goal for that input.
Automated review by peibekwe's agents
Motivation & Context
Keeps
itemsVariableas a table afterRemove,Clear,TakeFirst, andTakeLastso later edits continue to work. Results useresultVariable, and checkpointed tables remain downgrade-compatible and editable after resume.Fixes #7351
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.