.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>

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}")
+    };
+
     /// <summary>
     /// Gets the registration details for the current executor.
     /// </summary>
     /// <remarks>The returned registration depends on the type of the executor. If the executor is unbound, an
     /// <see cref="InvalidOperationException"/> is thrown. For other executor types, the registration  includes the
     /// appropriate ID, type, and provider based on the executor's configuration.</remarks>
-    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
     {

dotnet/src/Microsoft.Agents.Workflows/ExecutorRegistration.cs

@@ -7,12 +7,14 @@ using ExecutorFactoryF = System.Func<Microsoft.Agents.Workflows.Executor>;
 
 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)

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)

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<object>(
+                (msg, ctx) =>
+                {
+                    return ctx.SendMessageAsync(msg);
+                });
+        }
+    }
+
+    private sealed class SomeOtherNoOpExecutor(string? id = null) : Executor(id)
+    {
+        protected override RouteBuilder ConfigureRoutes(RouteBuilder routeBuilder)
+        {
+            return routeBuilder.AddHandler<object>(
+                (msg, ctx) =>
+                {
+                    return ctx.SendMessageAsync(msg);
+                });
+        }
+    }
+
+    [Fact]
+    public void Test_LateBinding_Executor()
+    {
+        Workflow workflow = new WorkflowBuilder("start")
+                                .BindExecutor(new NoOpExecutor("start"))
+                                .Build<object>();
+
+        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<object>();
+
+        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<Workflow> act = () =>
+        {
+            return new WorkflowBuilder("start")
+                       .AddEdge(executor1, executor2)
+                       .Build<object>();
+        };
+
+        act.Should().Throw<InvalidOperationException>();
+    }
+
+    [Fact]
+    public void Test_RebindToSameish_Allowed()
+    {
+        NoOpExecutor executor1 = new("start");
+
+        Workflow workflow = new WorkflowBuilder("start")
+                                .AddEdge(executor1, executor1)
+                                .Build<object>();
+
+        workflow.StartExecutorId.Should().Be("start");
+
+        workflow.Registrations.Should().HaveCount(1);
+        workflow.Registrations.Should().ContainKey("start");
+        workflow.Registrations["start"].ExecutorType.Should().Be(typeof(NoOpExecutor));
+    }
+}