.NET: fix: Ensure DeclarativeActionCompletedEvent is raised for every action#6780
Open
PratikWayase wants to merge 1 commit into
Open
Conversation
moonbox3
added
.NET
github-actions
Bot
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET declarative workflow graph construction so terminal actions (e.g., GotoAction, loop control, and end/cancel actions) still trigger DeclarativeActionCompletedEvent for non-discrete parent actions whose completion is normally raised in their Post step.
Changes:
- Adds
WorkflowModel.LocateNonDiscreteAncestors<T>()to find non-discrete ancestor executors that require explicit completion emission when control flow bypasses Post. - Introduces completion-step injection for terminal transitions (and updates several terminal-action visitors to use the injected completion chain).
- Adds a regression workflow (
GotoInsideCondition.yaml) and a unit test asserting the parentConditionGroupcompletion event is emitted; updatesCaseInsensitive.yamlexpected completion count.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/WorkflowActionVisitor.cs | Injects ancestor completion steps for terminal transitions and adds helper logic to patch missing completion events. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/WorkflowModel.cs | Adds ancestor-walk helper to locate non-discrete parents that rely on Post-step completion emission. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/DeclarativeWorkflowTest.cs | Adds regression test for #3743 and updates one expected completion count. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/Workflows/GotoInsideCondition.yaml | New workflow fixture reproducing GotoAction inside a ConditionGroup. |
Comment on lines
+642
to
+663
| string currentId = terminalActionId; | ||
|
|
||
| foreach (var ancestor in nonDiscreteAncestors) | ||
| { | ||
| if (stopBeforeParentId is not null && | ||
| string.Equals(ancestor.Id, stopBeforeParentId, StringComparison.Ordinal)) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| string completionId = $"{terminalActionId}_Complete_{ancestor.Id}"; | ||
| var completionStep = new DelegateActionExecutor( | ||
| completionId, | ||
| this._workflowState, | ||
| async (context, _, ct) => await context.RaiseCompletionEventAsync(ancestor.Model, ct).ConfigureAwait(false), | ||
| emitResult: false); | ||
|
|
||
| this._workflowModel.AddNode(completionStep, ancestor.ParentId); | ||
| this._workflowModel.AddLink(currentId, completionId); | ||
|
|
||
| currentId = completionId; | ||
| } |
Comment on lines
+322
to
+323
| this._workflowModel.AddNode(completionStep, ancestor.ParentId); | ||
| this._workflowModel.AddLink(currentId, completionId); |
Comment on lines
+353
to
+354
| this._workflowModel.AddNode(completionStep, ancestor.ParentId); | ||
| this._workflowModel.AddLink(currentId, completionId); |
Comment on lines
+384
to
+385
| this._workflowModel.AddNode(completionStep, ancestor.ParentId); | ||
| this._workflowModel.AddLink(currentId, completionId); |
Comment on lines
+415
to
+416
| this._workflowModel.AddNode(completionStep, ancestor.ParentId); | ||
| this._workflowModel.AddLink(currentId, completionId); |
Comment on lines
+174
to
+183
| Type actionType = nonDiscreteAction.GetType(); | ||
| Type? baseType = actionType.BaseType; | ||
|
|
||
| // ConditionGroupExecutor and ForeachExecutor are non-discrete | ||
| // They have IsDiscreteAction = false and raise completion in their Post step | ||
| bool isNonDiscrete = actionType.Name.Contains("ConditionGroupExecutor") || | ||
| actionType.Name.Contains("ForeachExecutor") || | ||
| (baseType is not null && (baseType.Name.Contains("ConditionGroupExecutor") || | ||
| baseType.Name.Contains("ForeachExecutor"))); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Description
Motivation & Context
Why is this change required? The
DeclarativeActionCompletedEventwas not being raised for parent actions when terminal actions (Goto, EndDialog, etc.) bypassed the normal workflow flow. This broke the contract that every started action must emit a completion event.What problem does it solve? When a
GotoActionor other terminal action is nested inside a non-discrete action (likeConditionGrouporForeach), the parent's completion event was skipped because the terminal action created a direct link to its target, bypassing the parent's Post step where the completion event is raised.What scenario does it contribute to? This is critical for workflow designers and debuggers that rely on completion events to track action execution state. Without this fix, the workflow designer would show incomplete/incorrect action state for workflows using goto/end actions inside conditions or loops.
If it fixes an open issue, please link to the issue below. Fixes .NET Workflows - Ensure
DeclarativeActionCompletedEventis raised for every action #3743Description & Review Guide
What are the major changes?
LocateNonDiscreteAncestors<T>()method toWorkflowModelto find parent actions that need completion eventsInjectParentCompletionEvents()helper method toWorkflowActionVisitorthat inserts intermediate completion steps in the workflow graphVisit()methods forGotoAction,BreakLoop,ContinueLoop,EndDialog,EndConversation,CancelAllDialogs, andCancelDialogto use the new helperGotoInsideCondition_RaisesParentCompletionEventAsyncWhat is the impact of these changes?
GotoActioninside aConditionGroup→ skips parent completion eventGotoActioninside aConditionGroup→ injects completion step → parent completion event is raisedCaseInsensitive.yaml) was updated to expect the now-correct completion countWhat do you want reviewers to focus on?
LocateNonDiscreteAncestorsmethod uses type name matching to identify non-discrete actions. Consider if there's a more robust approach.InjectParentCompletionEventsmethod handles thestopBeforeParentIdparameter for loop control flow (BreakLoop/ContinueLoop) - verify this logic is correctRelated Issue
Fixes #3743
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.