Skip to content

.NET: fix: Ensure DeclarativeActionCompletedEvent is raised for every action #6780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PratikWayase wants to merge 1 commit into
base: main
Choose a base branch
from
+241 −12
Open

.NET: fix: Ensure DeclarativeActionCompletedEvent is raised for every action #6780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 135 additions & 11 deletions dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/WorkflowActionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ protected override void Visit(GotoAction item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);
// Transition to target action
this._workflowModel.AddLink(action.Id, item.ActionId.Value);
// Transition to target action, injecting completion events for non-discrete parents
this.InjectParentCompletionEvents(action.Id, item.ActionId.Value);
// Define a clean-start to ensure "goto" is not a source for any edge
this.RestartAfter(action.Id, action.ParentId);
}
Expand Down Expand Up @@ -219,8 +219,9 @@ protected override void Visit(BreakLoop item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);
// Transition to post action
this._workflowModel.AddLink(action.Id, Steps.Post(loopAction.Id));
// Transition to post action, injecting completion events for non-discrete parents
// Stop before the Foreach's own Post step since that handles Foreach completion
this.InjectParentCompletionEvents(action.Id, Steps.Post(loopAction.Id), stopBeforeParentId: loopAction.Id);
// Define a clean-start to ensure "break" is not a source for any edge
this.RestartAfter(action.Id, action.ParentId);
}
Expand All @@ -238,8 +239,9 @@ protected override void Visit(ContinueLoop item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);
// Transition to select the next item
this._workflowModel.AddLink(action.Id, ForeachExecutor.Steps.Next(loopAction.Id));
// Transition to select the next item, injecting completion events for non-discrete parents
// Stop before the Foreach's own Next step since that handles Foreach iteration
this.InjectParentCompletionEvents(action.Id, ForeachExecutor.Steps.Next(loopAction.Id), stopBeforeParentId: loopAction.Id);
// Define a clean-start to ensure "continue" is not a source for any edge
this.RestartAfter(action.Id, action.ParentId);
}
Expand Down Expand Up @@ -302,8 +304,28 @@ protected override void Visit(EndDialog item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);

var nonDiscreteAncestors = this._workflowModel
.LocateNonDiscreteAncestors<DeclarativeActionExecutor>(action.Id)
.ToList();

string currentId = action.Id;
foreach (var ancestor in nonDiscreteAncestors)
{
string completionId = $"{action.Id}_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);
Comment on lines +322 to +323
currentId = completionId;
}

// Define a clean-start to ensure "end" is not a source for any edge
this.RestartAfter(item.Id.Value, action.ParentId);
this.RestartAfter(currentId, action.ParentId);
}

protected override void Visit(EndConversation item)
Expand All @@ -313,8 +335,28 @@ protected override void Visit(EndConversation item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);

var nonDiscreteAncestors = this._workflowModel
.LocateNonDiscreteAncestors<DeclarativeActionExecutor>(action.Id)
.ToList();

string currentId = action.Id;
foreach (var ancestor in nonDiscreteAncestors)
{
string completionId = $"{action.Id}_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);
Comment on lines +353 to +354
currentId = completionId;
}

// Define a clean-start to ensure "end" is not a source for any edge
this.RestartAfter(action.Id, action.ParentId);
this.RestartAfter(currentId, action.ParentId);
}

protected override void Visit(CancelAllDialogs item)
Expand All @@ -324,8 +366,28 @@ protected override void Visit(CancelAllDialogs item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);

var nonDiscreteAncestors = this._workflowModel
.LocateNonDiscreteAncestors<DeclarativeActionExecutor>(action.Id)
.ToList();

string currentId = action.Id;
foreach (var ancestor in nonDiscreteAncestors)
{
string completionId = $"{action.Id}_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);
Comment on lines +384 to +385
currentId = completionId;
}

// Define a clean-start to ensure "end" is not a source for any edge
this.RestartAfter(item.Id.Value, action.ParentId);
this.RestartAfter(currentId, action.ParentId);
}

protected override void Visit(CancelDialog item)
Expand All @@ -335,8 +397,27 @@ protected override void Visit(CancelDialog item)
// Represent action with default executor
DefaultActionExecutor action = new(item, this._workflowState);
this.ContinueWith(action);
// Define a clean-start to ensure "end" is not a source for any edge
this.RestartAfter(action.Id, action.ParentId);

var nonDiscreteAncestors = this._workflowModel
.LocateNonDiscreteAncestors<DeclarativeActionExecutor>(action.Id)
.ToList();

string currentId = action.Id;
foreach (var ancestor in nonDiscreteAncestors)
{
string completionId = $"{action.Id}_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);
Comment on lines +415 to +416
currentId = completionId;
}

this.RestartAfter(currentId, action.ParentId);
}

protected override void Visit(CreateConversation item)
Expand Down Expand Up @@ -541,6 +622,49 @@ protected override void Visit(HttpRequestAction item)
this.ContinueWith(new HttpRequestExecutor(item, this._workflowOptions.HttpRequestHandler, this._workflowOptions.AgentProvider, this._workflowState));
}

/// <summary>
/// Injects completion event steps for all non-discrete parent actions when
/// a terminal action (goto, end, break, etc.) bypasses the normal Post step flow.
/// This ensures DeclarativeActionCompletedEvent is raised for every action.
/// </summary>
private void InjectParentCompletionEvents(string terminalActionId, string targetId, string? stopBeforeParentId = null)
{
var nonDiscreteAncestors = this._workflowModel
.LocateNonDiscreteAncestors<DeclarativeActionExecutor>(terminalActionId)
.ToList();

if (nonDiscreteAncestors.Count == 0)
{
this._workflowModel.AddLink(terminalActionId, targetId);
return;
}

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 +642 to +663

this._workflowModel.AddLink(currentId, targetId);
}

#region Not supported

protected override void Visit(AnswerQuestionWithAI item) => this.NotSupported(item);
Expand Down
51 changes: 51 additions & 0 deletions dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/WorkflowModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,57 @@ private ModelNode DefineNode(IModeledAction action, ModelNode? parentNode = null
return null;
}

/// <summary>
/// Finds all non-discrete parent actions that need completion events raised.
/// These are actions that rely on their Post step to raise completion events
/// (e.g., ConditionGroupExecutor, ForeachExecutor).
/// </summary>
public IEnumerable<TAction> LocateNonDiscreteAncestors<TAction>(string? itemId) where TAction : class, IModeledAction
{
if (string.IsNullOrEmpty(itemId))
{
yield break;
}

// Get the starting node and move to its parent first (don't include the starting node)
if (!this.Nodes.TryGetValue(itemId, out ModelNode? startNode))
{
yield break;
}

string? currentId = startNode.Parent?.Action.Id;

while (currentId is not null)
{
if (!this.Nodes.TryGetValue(currentId, out ModelNode? itemNode))
{
yield break;
}

// Check if this is a non-discrete action type that needs completion events
// (actions that rely on Post step for completion, not HandleAsync finally block)
if (itemNode.Action is TAction nonDiscreteAction)
{
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")));

Comment on lines +174 to +183
if (isNonDiscrete)
{
yield return nonDiscreteAction;
}
}

currentId = itemNode.Parent?.Action.Id;
}
}

private sealed class ModelNode(IModeledAction action, ModelNode? parent = null, Action? completionHandler = null)
{
public IModeledAction Action => action;
Expand Down
21 changes: 20 additions & 1 deletion dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/DeclarativeWorkflowTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,25 @@ public async Task GotoActionAsync()
this.AssertNotExecuted("sendActivity_3");
}

/// <summary>
/// Regression test for issue #3743: DeclarativeActionCompletedEvent must be raised
/// for parent actions when GotoAction bypasses the default terminal edge.
/// </summary>
[Fact]
public async Task GotoInsideCondition_RaisesParentCompletionEventAsync()
{
await this.RunWorkflowAsync("GotoInsideCondition.yaml", 1);

this.AssertExecuted("goto_end");

Assert.Contains(
this.WorkflowEvents.OfType<DeclarativeActionCompletedEvent>(),
e => e.ActionId == "condition_test");

this.AssertNotExecuted("send_activity_normal");
this.AssertNotExecuted("send_activity_after");
}

[Theory]
[InlineData(12)]
[InlineData(37)]
Expand Down Expand Up @@ -180,7 +199,7 @@ public async Task ConditionActionWithFallThroughAsync(int input, int expectedAct
[InlineData("ClearAllVariables.yaml", 1, "clear_all")]
[InlineData("ResetVariable.yaml", 2, "clear_var")]
[InlineData("MixedScopes.yaml", 2, "activity_input")]
[InlineData("CaseInsensitive.yaml", 6, "end_when_match")]
[InlineData("CaseInsensitive.yaml", 7, "end_when_match")]
[InlineData("HttpRequest.yaml", 1, "http_request")]
public async Task ExecuteActionAsync(string workflowFile, int expectedCount, string expectedId)
{
Expand Down
35 changes: 35 additions & 0 deletions ...ts/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/Workflows/GotoInsideCondition.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
kind: Workflow
trigger:

kind: OnConversationStart
id: my_workflow
actions:

- kind: SetVariable
id: set_input
variable: Local.TestValue
value: "=Value(System.LastMessageText)"

- kind: ConditionGroup
id: condition_test
conditions:
- id: condition_goto_branch
condition: "=Local.TestValue = 1"
actions:
- kind: GotoAction
id: goto_end
actionId: end_all

- id: condition_other_branch
condition: "=Local.TestValue <> 1"
actions:
- kind: SendActivity
id: send_activity_normal
activity: NORMAL PATH

- kind: SendActivity
id: send_activity_after
activity: SHOULD NOT SEE THIS

- kind: EndConversation
id: end_all