Skip to content

Commit 5374dd4

Browse files
peibekwepeibekwe
authored
.NET: Fix source generator bug that silently drops base class handler registrations for protocol-only partial executors (#4751)
* Fix source generator bug that silently drops base class handler registrations for protocol-only partial executors * Fixed xml comments and variable naming.
1 parent 29dfcbb commit 5374dd4

5 files changed

Lines changed: 104 additions & 13 deletions

File tree

‎dotnet/samples/03-workflows/Agents/CustomAgentExecutors/Program.cs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ private static async Task Main()
6161
{
6262
Console.WriteLine($"{outputEvent}");
6363
}
64+
65+
if (evt is WorkflowErrorEvent errorEvent)
66+
{
67+
Console.WriteLine($"Workflow error: {errorEvent.Exception?.Message}");
68+
Console.WriteLine($"Details: {errorEvent.Exception}");
69+
}
6470
}
6571
}
6672
}
@@ -175,7 +181,9 @@ internal sealed class FeedbackEvent(FeedbackResult feedbackResult) : WorkflowEve
175181
/// <summary>
176182
/// A custom executor that uses an AI agent to provide feedback on a slogan.
177183
/// </summary>
178-
internal sealed class FeedbackExecutor : Executor<SloganResult>
184+
[SendsMessage(typeof(FeedbackResult))]
185+
[YieldsOutput(typeof(string))]
186+
internal sealed partial class FeedbackExecutor : Executor<SloganResult>
179187
{
180188
private readonly AIAgent _agent;
181189
private AgentSession? _session;

‎dotnet/src/Microsoft.Agents.AI.Workflows.Generators/Analysis/SemanticAnalyzer.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public static MethodAnalysisResult AnalyzeHandlerMethod(
6868
string classKey = GetClassKey(classSymbol);
6969
bool isPartialClass = IsPartialClass(classSymbol, cancellationToken);
7070
bool derivesFromExecutor = DerivesFromExecutor(classSymbol);
71-
bool configureProtocol = HasConfigureProtocolDefined(classSymbol);
71+
bool hasManualConfigureProtocol = HasConfigureProtocolDefined(classSymbol);
7272

7373
// Extract class metadata
7474
string? @namespace = classSymbol.ContainingNamespace?.IsGlobalNamespace == true
@@ -97,7 +97,7 @@ public static MethodAnalysisResult AnalyzeHandlerMethod(
9797
return new MethodAnalysisResult(
9898
classKey, @namespace, className, genericParameters, isNested, containingTypeChain,
9999
baseHasConfigureProtocol, classSendTypes, classYieldTypes,
100-
isPartialClass, derivesFromExecutor, configureProtocol,
100+
isPartialClass, derivesFromExecutor, hasManualConfigureProtocol,
101101
classLocation,
102102
handler,
103103
Diagnostics: new ImmutableEquatableArray<DiagnosticInfo>(methodDiagnostics.ToImmutable()));
@@ -149,7 +149,7 @@ public static AnalysisResult CombineHandlerMethodResults(IEnumerable<MethodAnaly
149149
return AnalysisResult.WithDiagnostics(allDiagnostics.ToImmutable());
150150
}
151151

152-
if (first.HasManualConfigureRoutes)
152+
if (first.HasManualConfigureProtocol)
153153
{
154154
allDiagnostics.Add(Diagnostic.Create(
155155
DiagnosticDescriptors.ConfigureProtocolAlreadyDefined,
@@ -212,6 +212,7 @@ public static ImmutableArray<ClassProtocolInfo> AnalyzeClassProtocolAttribute(
212212
bool isPartialClass = IsPartialClass(classSymbol, cancellationToken);
213213
bool derivesFromExecutor = DerivesFromExecutor(classSymbol);
214214
bool hasManualConfigureProtocol = HasConfigureProtocolDefined(classSymbol);
215+
bool baseHasConfigureProtocol = BaseHasConfigureProtocol(classSymbol);
215216

216217
string? @namespace = classSymbol.ContainingNamespace?.IsGlobalNamespace == true
217218
? null
@@ -241,6 +242,7 @@ public static ImmutableArray<ClassProtocolInfo> AnalyzeClassProtocolAttribute(
241242
isPartialClass,
242243
derivesFromExecutor,
243244
hasManualConfigureProtocol,
245+
baseHasConfigureProtocol,
244246
classLocation,
245247
typeName,
246248
attributeKind));
@@ -321,7 +323,7 @@ public static AnalysisResult CombineOutputOnlyResults(IEnumerable<ClassProtocolI
321323
first.GenericParameters,
322324
first.IsNested,
323325
first.ContainingTypeChain,
324-
BaseHasConfigureProtocol: false, // Not relevant for protocol-only
326+
first.BaseHasConfigureProtocol,
325327
Handlers: ImmutableEquatableArray<HandlerInfo>.Empty,
326328
ClassSendTypes: new ImmutableEquatableArray<string>(sendTypes.ToImmutable()),
327329
ClassYieldTypes: new ImmutableEquatableArray<string>(yieldTypes.ToImmutable()));

‎dotnet/src/Microsoft.Agents.AI.Workflows.Generators/Models/ClassProtocolInfo.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
55
/// <summary>
66
/// Represents protocol type information extracted from class-level [SendsMessage] or [YieldsOutput] attributes.
77
/// Used by the incremental generator pipeline to capture classes that declare protocol types
8-
/// but may not have [MessageHandler] methods (e.g., when ConfigureRoutes is manually implemented).
8+
/// but may not have [MessageHandler] methods (e.g., when ConfigureProtocol is manually implemented).
99
/// </summary>
1010
/// <param name="ClassKey">Unique identifier for the class (fully qualified name).</param>
1111
/// <param name="Namespace">The namespace of the class.</param>
@@ -15,7 +15,8 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
1515
/// <param name="ContainingTypeChain">The chain of containing types for nested classes. Empty if not nested.</param>
1616
/// <param name="IsPartialClass">Whether the class is declared as partial.</param>
1717
/// <param name="DerivesFromExecutor">Whether the class derives from Executor.</param>
18-
/// <param name="HasManualConfigureRoutes">Whether the class has a manually defined ConfigureRoutes method.</param>
18+
/// <param name="HasManualConfigureProtocol">Whether the class has a manually defined ConfigureProtocol method.</param>
19+
/// <param name="BaseHasConfigureProtocol">Whether a base class already overrides ConfigureProtocol.</param>
1920
/// <param name="ClassLocation">Location info for diagnostics.</param>
2021
/// <param name="TypeName">The fully qualified type name from the attribute.</param>
2122
/// <param name="AttributeKind">Whether this is from a SendsMessage or YieldsOutput attribute.</param>
@@ -28,7 +29,8 @@ internal sealed record ClassProtocolInfo(
2829
string ContainingTypeChain,
2930
bool IsPartialClass,
3031
bool DerivesFromExecutor,
31-
bool HasManualConfigureRoutes,
32+
bool HasManualConfigureProtocol,
33+
bool BaseHasConfigureProtocol,
3234
DiagnosticLocationInfo? ClassLocation,
3335
string TypeName,
3436
ProtocolAttributeKind AttributeKind)
@@ -38,5 +40,5 @@ internal sealed record ClassProtocolInfo(
3840
/// </summary>
3941
public static ClassProtocolInfo Empty { get; } = new(
4042
string.Empty, null, string.Empty, null, false, string.Empty,
41-
false, false, false, null, string.Empty, ProtocolAttributeKind.Send);
43+
false, false, false, false, null, string.Empty, ProtocolAttributeKind.Send);
4244
}

‎dotnet/src/Microsoft.Agents.AI.Workflows.Generators/Models/MethodAnalysisResult.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
88
/// Uses value-equatable types to support incremental generator caching.
99
/// </summary>
1010
/// <remarks>
11-
/// Class-level validation (IsPartialClass, DerivesFromExecutor, HasManualConfigureRoutes)
11+
/// Class-level validation (IsPartialClass, DerivesFromExecutor, HasManualConfigureProtocol)
1212
/// is extracted here but validated once per class in CombineMethodResults to avoid
1313
/// redundant validation work when a class has multiple handlers.
1414
/// </remarks>
@@ -29,7 +29,7 @@ internal sealed record MethodAnalysisResult(
2929
// Class-level facts (used for validation in CombineMethodResults)
3030
bool IsPartialClass,
3131
bool DerivesFromExecutor,
32-
bool HasManualConfigureRoutes,
32+
bool HasManualConfigureProtocol,
3333

3434
// Class location for diagnostics (value-equatable)
3535
DiagnosticLocationInfo? ClassLocation,

‎dotnet/tests/Microsoft.Agents.AI.Workflows.Generators.UnitTests/ExecutorRouteGeneratorTests.cs‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,7 @@ private void HandleFromFile2(int message, IWorkflowContext context) { }
651651
}
652652

653653
[Fact]
654-
public void PartialClass_SendsYieldsInBothFiles_GeneratesAlOverrides()
654+
public void PartialClass_SendsYieldsInBothFiles_GeneratesAllOverrides()
655655
{
656656
// File 1: Partial with one handler
657657
var file1 = """
@@ -700,7 +700,7 @@ private void HandleFromFile2(int message, IWorkflowContext context) { }
700700
generated.Should().RegisterSentMessageType("string")
701701
.And.RegisterSentMessageType("int")
702702
.And.RegisterYieldedOutputType("string")
703-
.And.RegisterYieldedOutputType("string");
703+
.And.RegisterYieldedOutputType("int");
704704
}
705705

706706
#endregion
@@ -1046,6 +1046,85 @@ public GenericExecutor() : base("generic") { }
10461046
.And.RegisterSentMessageType("global::TestNamespace.BroadcastMessage");
10471047
}
10481048

1049+
[Fact]
1050+
public void ProtocolOnly_DerivesFromExecutorOfT_GeneratesBaseCall()
1051+
{
1052+
// A protocol-only partial executor deriving from Executor<T>
1053+
// has a base class that already overrides ConfigureProtocol. The generator must emit
1054+
// "return base.ConfigureProtocol(protocolBuilder)" so inherited handler registrations
1055+
// are preserved — not "return protocolBuilder" which silently drops them.
1056+
var source = """
1057+
using System;
1058+
using System.Threading;
1059+
using System.Threading.Tasks;
1060+
using Microsoft.Agents.AI.Workflows;
1061+
1062+
namespace TestNamespace;
1063+
1064+
public class FeedbackResult { }
1065+
1066+
[SendsMessage(typeof(FeedbackResult))]
1067+
[YieldsOutput(typeof(string))]
1068+
public partial class FeedbackExecutor : Executor<string>
1069+
{
1070+
public FeedbackExecutor() : base("feedback") { }
1071+
1072+
public override System.Threading.Tasks.ValueTask HandleAsync(string message, IWorkflowContext context, System.Threading.CancellationToken cancellationToken = default)
1073+
=> default;
1074+
}
1075+
""";
1076+
1077+
var result = GeneratorTestHelper.RunGenerator(source);
1078+
1079+
result.RunResult.GeneratedTrees.Should().HaveCount(1);
1080+
result.RunResult.Diagnostics.Should().BeEmpty();
1081+
1082+
var generated = result.RunResult.GeneratedTrees[0].ToString();
1083+
1084+
// Base class Executor<T> overrides ConfigureProtocol, so the generated override
1085+
// must chain to base to preserve the inherited handler registration.
1086+
generated.Should().Contain("return base.ConfigureProtocol(protocolBuilder)",
1087+
because: "Executor<T> overrides ConfigureProtocol, so base must be called to preserve its handler registration");
1088+
generated.Should().Contain(".SendsMessage<global::TestNamespace.FeedbackResult>()");
1089+
generated.Should().Contain(".YieldsOutput<string>()");
1090+
}
1091+
1092+
[Fact]
1093+
public void ProtocolOnly_DerivesDirectlyFromExecutor_DoesNotGenerateBaseCall()
1094+
{
1095+
// A protocol-only partial executor deriving directly from Executor (abstract base
1096+
// with no non-abstract ConfigureProtocol override) should generate "return protocolBuilder"
1097+
// rather than "return base.ConfigureProtocol(protocolBuilder)".
1098+
var source = """
1099+
using System;
1100+
using System.Threading;
1101+
using System.Threading.Tasks;
1102+
using Microsoft.Agents.AI.Workflows;
1103+
1104+
namespace TestNamespace;
1105+
1106+
public class BroadcastMessage { }
1107+
1108+
[SendsMessage(typeof(BroadcastMessage))]
1109+
public partial class BroadcastExecutor : Executor
1110+
{
1111+
public BroadcastExecutor() : base("broadcast") { }
1112+
}
1113+
""";
1114+
1115+
var result = GeneratorTestHelper.RunGenerator(source);
1116+
1117+
result.RunResult.GeneratedTrees.Should().HaveCount(1);
1118+
result.RunResult.Diagnostics.Should().BeEmpty();
1119+
1120+
var generated = result.RunResult.GeneratedTrees[0].ToString();
1121+
1122+
// Executor's ConfigureProtocol is abstract — no base call needed.
1123+
generated.Should().Contain("return protocolBuilder",
1124+
because: "Executor base class has no non-abstract ConfigureProtocol, so no base call is needed");
1125+
generated.Should().NotContain("base.ConfigureProtocol");
1126+
}
1127+
10491128
#endregion
10501129

10511130
#region Generic Executor Tests

0 commit comments

Comments
 (0)