From 78125f019a2c016979f6974e9b969de6a83c8c55 Mon Sep 17 00:00:00 2001 From: Jacob Alber Date: Tue, 26 Aug 2025 18:42:07 -0400 Subject: [PATCH] .NET: Make WorkflowBuilder more intuititve (#503) * feat: Make WorkflowBuilder more intutitve Right now Executorish binding has some unintutitive behaviour. When a user adds an eecutor with an id of an executor that already exists, we silently replace it, if the user provides it inside of add_edge. When a user introduces an executor via an unbound id, the user must bind it via BindExecutor, even though the registration is created implicitly when an edge id added. The change will remove the invisible update in favor of a "best efforts" check of type and instance equality. * Expand errors when rebinding to disallowed Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../Microsoft.Agents.Workflows/ExecutorIsh.cs | 11 ++- .../ExecutorRegistration.cs | 4 +- .../WorkflowBuilder.cs | 28 +++++- .../WorkflowBuilderSmokeTests.cs | 94 +++++++++++++++++++ 4 files changed, 133 insertions(+), 4 deletions(-) create mode 100644 dotnet/tests/Microsoft.Agents.Workflows.UnitTests/WorkflowBuilderSmokeTests.cs diff --git a/dotnet/src/Microsoft.Agents.Workflows/ExecutorIsh.cs b/dotnet/src/Microsoft.Agents.Workflows/ExecutorIsh.cs index c303286acb..dd026f541d 100644 --- a/dotnet/src/Microsoft.Agents.Workflows/ExecutorIsh.cs +++ b/dotnet/src/Microsoft.Agents.Workflows/ExecutorIsh.cs @@ -102,13 +102,22 @@ public sealed class ExecutorIsh : _ => throw new InvalidOperationException($"Unknown ExecutorIsh type: {this.ExecutorType}") }; + internal object? RawData => this.ExecutorType switch + { + Type.Unbound => this._idValue, + Type.Executor => this._executorValue, + Type.InputPort => this._inputPortValue, + Type.Agent => this._aiAgentValue, + _ => throw new InvalidOperationException($"Unknown ExecutorIsh type: {this.ExecutorType}") + }; + /// /// Gets the registration details for the current executor. /// /// The returned registration depends on the type of the executor. If the executor is unbound, an /// is thrown. For other executor types, the registration includes the /// appropriate ID, type, and provider based on the executor's configuration. - internal ExecutorRegistration Registration => new(this.Id, this.RuntimeType, this.ExecutorProvider); + internal ExecutorRegistration Registration => new(this.Id, this.RuntimeType, this.ExecutorProvider, this.RawData); private System.Type RuntimeType => this.ExecutorType switch { diff --git a/dotnet/src/Microsoft.Agents.Workflows/ExecutorRegistration.cs b/dotnet/src/Microsoft.Agents.Workflows/ExecutorRegistration.cs index d45a126472..bb1e060107 100644 --- a/dotnet/src/Microsoft.Agents.Workflows/ExecutorRegistration.cs +++ b/dotnet/src/Microsoft.Agents.Workflows/ExecutorRegistration.cs @@ -7,12 +7,14 @@ using ExecutorFactoryF = System.Func; namespace Microsoft.Agents.Workflows; -internal class ExecutorRegistration(string id, Type executorType, ExecutorFactoryF provider) +internal class ExecutorRegistration(string id, Type executorType, ExecutorFactoryF provider, object? rawData) { public string Id { get; } = Throw.IfNullOrEmpty(id); public Type ExecutorType { get; } = Throw.IfNull(executorType); public ExecutorFactoryF Provider { get; } = Throw.IfNull(provider); + internal object? RawExecutorishData { get; } = rawData; + public override string ToString() => $"{this.ExecutorType.Name}({this.Id})"; private Executor CheckId(Executor executor) diff --git a/dotnet/src/Microsoft.Agents.Workflows/WorkflowBuilder.cs b/dotnet/src/Microsoft.Agents.Workflows/WorkflowBuilder.cs index 4e61c0e3a8..1a37415d19 100644 --- a/dotnet/src/Microsoft.Agents.Workflows/WorkflowBuilder.cs +++ b/dotnet/src/Microsoft.Agents.Workflows/WorkflowBuilder.cs @@ -50,8 +50,32 @@ public class WorkflowBuilder } else if (!executorish.IsUnbound) { - // If we already have an executor with this ID, we need to update it (todo: should we throw on double binding?) - this._executors[executorish.Id] = executorish.Registration; + ExecutorRegistration incoming = executorish.Registration; + // If there is already a bound executor with this ID, we need to validate (to best efforts) + // that the two are matching (at least based on type) + if (this._executors.TryGetValue(executorish.Id, out ExecutorRegistration? existing)) + { + if (existing.ExecutorType != incoming.ExecutorType) + { + throw new InvalidOperationException( + $"Cannot bind executor with ID '{executorish.Id}' because an executor with the same ID but a different type ({existing.ExecutorType.Name} vs {incoming.ExecutorType.Name}) is already bound."); + } + + if (existing.RawExecutorishData != null && + !object.ReferenceEquals(existing.RawExecutorishData, incoming.RawExecutorishData)) + { + throw new InvalidOperationException( + $"Cannot bind executor with ID '{executorish.Id}' because an executor with the same ID but different instance is already bound."); + } + } + else + { + this._executors[executorish.Id] = executorish.Registration; + if (this._unboundExecutors.Contains(executorish.Id)) + { + this._unboundExecutors.Remove(executorish.Id); + } + } } if (executorish.ExecutorType == ExecutorIsh.Type.InputPort) diff --git a/dotnet/tests/Microsoft.Agents.Workflows.UnitTests/WorkflowBuilderSmokeTests.cs b/dotnet/tests/Microsoft.Agents.Workflows.UnitTests/WorkflowBuilderSmokeTests.cs new file mode 100644 index 0000000000..3bb33d4411 --- /dev/null +++ b/dotnet/tests/Microsoft.Agents.Workflows.UnitTests/WorkflowBuilderSmokeTests.cs @@ -0,0 +1,94 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using FluentAssertions; + +namespace Microsoft.Agents.Workflows.UnitTests; + +public partial class WorkflowBuilderSmokeTests +{ + private sealed class NoOpExecutor(string? id = null) : Executor(id) + { + protected override RouteBuilder ConfigureRoutes(RouteBuilder routeBuilder) + { + return routeBuilder.AddHandler( + (msg, ctx) => + { + return ctx.SendMessageAsync(msg); + }); + } + } + + private sealed class SomeOtherNoOpExecutor(string? id = null) : Executor(id) + { + protected override RouteBuilder ConfigureRoutes(RouteBuilder routeBuilder) + { + return routeBuilder.AddHandler( + (msg, ctx) => + { + return ctx.SendMessageAsync(msg); + }); + } + } + + [Fact] + public void Test_LateBinding_Executor() + { + Workflow workflow = new WorkflowBuilder("start") + .BindExecutor(new NoOpExecutor("start")) + .Build(); + + workflow.StartExecutorId.Should().Be("start"); + + workflow.Registrations.Should().HaveCount(1); + workflow.Registrations.Should().ContainKey("start"); + workflow.Registrations["start"].ExecutorType.Should().Be(typeof(NoOpExecutor)); + } + + [Fact] + public void Test_LateImplicitBinding_Executor() + { + NoOpExecutor start = new("start"); + Workflow workflow = new WorkflowBuilder("start") + .AddEdge(start, start) + .Build(); + + workflow.StartExecutorId.Should().Be("start"); + + workflow.Registrations.Should().HaveCount(1); + workflow.Registrations.Should().ContainKey("start"); + workflow.Registrations["start"].ExecutorType.Should().Be(typeof(NoOpExecutor)); + } + + [Fact] + public void Test_RebindToDifferent_Disallowed() + { + NoOpExecutor executor1 = new("start"); + SomeOtherNoOpExecutor executor2 = new("start"); + + Func act = () => + { + return new WorkflowBuilder("start") + .AddEdge(executor1, executor2) + .Build(); + }; + + act.Should().Throw(); + } + + [Fact] + public void Test_RebindToSameish_Allowed() + { + NoOpExecutor executor1 = new("start"); + + Workflow workflow = new WorkflowBuilder("start") + .AddEdge(executor1, executor1) + .Build(); + + workflow.StartExecutorId.Should().Be("start"); + + workflow.Registrations.Should().HaveCount(1); + workflow.Registrations.Should().ContainKey("start"); + workflow.Registrations["start"].ExecutorType.Should().Be(typeof(NoOpExecutor)); + } +}