mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
.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.
This commit is contained in:
committed by
GitHub
Unverified
parent
29dfcbb584
commit
5374dd47c5
@@ -61,6 +61,12 @@ public static class Program
|
||||
{
|
||||
Console.WriteLine($"{outputEvent}");
|
||||
}
|
||||
|
||||
if (evt is WorkflowErrorEvent errorEvent)
|
||||
{
|
||||
Console.WriteLine($"Workflow error: {errorEvent.Exception?.Message}");
|
||||
Console.WriteLine($"Details: {errorEvent.Exception}");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -175,7 +181,9 @@ internal sealed class FeedbackEvent(FeedbackResult feedbackResult) : WorkflowEve
|
||||
/// <summary>
|
||||
/// A custom executor that uses an AI agent to provide feedback on a slogan.
|
||||
/// </summary>
|
||||
internal sealed class FeedbackExecutor : Executor<SloganResult>
|
||||
[SendsMessage(typeof(FeedbackResult))]
|
||||
[YieldsOutput(typeof(string))]
|
||||
internal sealed partial class FeedbackExecutor : Executor<SloganResult>
|
||||
{
|
||||
private readonly AIAgent _agent;
|
||||
private AgentSession? _session;
|
||||
|
||||
@@ -68,7 +68,7 @@ internal static class SemanticAnalyzer
|
||||
string classKey = GetClassKey(classSymbol);
|
||||
bool isPartialClass = IsPartialClass(classSymbol, cancellationToken);
|
||||
bool derivesFromExecutor = DerivesFromExecutor(classSymbol);
|
||||
bool configureProtocol = HasConfigureProtocolDefined(classSymbol);
|
||||
bool hasManualConfigureProtocol = HasConfigureProtocolDefined(classSymbol);
|
||||
|
||||
// Extract class metadata
|
||||
string? @namespace = classSymbol.ContainingNamespace?.IsGlobalNamespace == true
|
||||
@@ -97,7 +97,7 @@ internal static class SemanticAnalyzer
|
||||
return new MethodAnalysisResult(
|
||||
classKey, @namespace, className, genericParameters, isNested, containingTypeChain,
|
||||
baseHasConfigureProtocol, classSendTypes, classYieldTypes,
|
||||
isPartialClass, derivesFromExecutor, configureProtocol,
|
||||
isPartialClass, derivesFromExecutor, hasManualConfigureProtocol,
|
||||
classLocation,
|
||||
handler,
|
||||
Diagnostics: new ImmutableEquatableArray<DiagnosticInfo>(methodDiagnostics.ToImmutable()));
|
||||
@@ -149,7 +149,7 @@ internal static class SemanticAnalyzer
|
||||
return AnalysisResult.WithDiagnostics(allDiagnostics.ToImmutable());
|
||||
}
|
||||
|
||||
if (first.HasManualConfigureRoutes)
|
||||
if (first.HasManualConfigureProtocol)
|
||||
{
|
||||
allDiagnostics.Add(Diagnostic.Create(
|
||||
DiagnosticDescriptors.ConfigureProtocolAlreadyDefined,
|
||||
@@ -212,6 +212,7 @@ internal static class SemanticAnalyzer
|
||||
bool isPartialClass = IsPartialClass(classSymbol, cancellationToken);
|
||||
bool derivesFromExecutor = DerivesFromExecutor(classSymbol);
|
||||
bool hasManualConfigureProtocol = HasConfigureProtocolDefined(classSymbol);
|
||||
bool baseHasConfigureProtocol = BaseHasConfigureProtocol(classSymbol);
|
||||
|
||||
string? @namespace = classSymbol.ContainingNamespace?.IsGlobalNamespace == true
|
||||
? null
|
||||
@@ -241,6 +242,7 @@ internal static class SemanticAnalyzer
|
||||
isPartialClass,
|
||||
derivesFromExecutor,
|
||||
hasManualConfigureProtocol,
|
||||
baseHasConfigureProtocol,
|
||||
classLocation,
|
||||
typeName,
|
||||
attributeKind));
|
||||
@@ -321,7 +323,7 @@ internal static class SemanticAnalyzer
|
||||
first.GenericParameters,
|
||||
first.IsNested,
|
||||
first.ContainingTypeChain,
|
||||
BaseHasConfigureProtocol: false, // Not relevant for protocol-only
|
||||
first.BaseHasConfigureProtocol,
|
||||
Handlers: ImmutableEquatableArray<HandlerInfo>.Empty,
|
||||
ClassSendTypes: new ImmutableEquatableArray<string>(sendTypes.ToImmutable()),
|
||||
ClassYieldTypes: new ImmutableEquatableArray<string>(yieldTypes.ToImmutable()));
|
||||
|
||||
@@ -5,7 +5,7 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
|
||||
/// <summary>
|
||||
/// Represents protocol type information extracted from class-level [SendsMessage] or [YieldsOutput] attributes.
|
||||
/// Used by the incremental generator pipeline to capture classes that declare protocol types
|
||||
/// but may not have [MessageHandler] methods (e.g., when ConfigureRoutes is manually implemented).
|
||||
/// but may not have [MessageHandler] methods (e.g., when ConfigureProtocol is manually implemented).
|
||||
/// </summary>
|
||||
/// <param name="ClassKey">Unique identifier for the class (fully qualified name).</param>
|
||||
/// <param name="Namespace">The namespace of the class.</param>
|
||||
@@ -15,7 +15,8 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
|
||||
/// <param name="ContainingTypeChain">The chain of containing types for nested classes. Empty if not nested.</param>
|
||||
/// <param name="IsPartialClass">Whether the class is declared as partial.</param>
|
||||
/// <param name="DerivesFromExecutor">Whether the class derives from Executor.</param>
|
||||
/// <param name="HasManualConfigureRoutes">Whether the class has a manually defined ConfigureRoutes method.</param>
|
||||
/// <param name="HasManualConfigureProtocol">Whether the class has a manually defined ConfigureProtocol method.</param>
|
||||
/// <param name="BaseHasConfigureProtocol">Whether a base class already overrides ConfigureProtocol.</param>
|
||||
/// <param name="ClassLocation">Location info for diagnostics.</param>
|
||||
/// <param name="TypeName">The fully qualified type name from the attribute.</param>
|
||||
/// <param name="AttributeKind">Whether this is from a SendsMessage or YieldsOutput attribute.</param>
|
||||
@@ -28,7 +29,8 @@ internal sealed record ClassProtocolInfo(
|
||||
string ContainingTypeChain,
|
||||
bool IsPartialClass,
|
||||
bool DerivesFromExecutor,
|
||||
bool HasManualConfigureRoutes,
|
||||
bool HasManualConfigureProtocol,
|
||||
bool BaseHasConfigureProtocol,
|
||||
DiagnosticLocationInfo? ClassLocation,
|
||||
string TypeName,
|
||||
ProtocolAttributeKind AttributeKind)
|
||||
@@ -38,5 +40,5 @@ internal sealed record ClassProtocolInfo(
|
||||
/// </summary>
|
||||
public static ClassProtocolInfo Empty { get; } = new(
|
||||
string.Empty, null, string.Empty, null, false, string.Empty,
|
||||
false, false, false, null, string.Empty, ProtocolAttributeKind.Send);
|
||||
false, false, false, false, null, string.Empty, ProtocolAttributeKind.Send);
|
||||
}
|
||||
|
||||
@@ -8,7 +8,7 @@ namespace Microsoft.Agents.AI.Workflows.Generators.Models;
|
||||
/// Uses value-equatable types to support incremental generator caching.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Class-level validation (IsPartialClass, DerivesFromExecutor, HasManualConfigureRoutes)
|
||||
/// Class-level validation (IsPartialClass, DerivesFromExecutor, HasManualConfigureProtocol)
|
||||
/// is extracted here but validated once per class in CombineMethodResults to avoid
|
||||
/// redundant validation work when a class has multiple handlers.
|
||||
/// </remarks>
|
||||
@@ -29,7 +29,7 @@ internal sealed record MethodAnalysisResult(
|
||||
// Class-level facts (used for validation in CombineMethodResults)
|
||||
bool IsPartialClass,
|
||||
bool DerivesFromExecutor,
|
||||
bool HasManualConfigureRoutes,
|
||||
bool HasManualConfigureProtocol,
|
||||
|
||||
// Class location for diagnostics (value-equatable)
|
||||
DiagnosticLocationInfo? ClassLocation,
|
||||
|
||||
+81
-2
@@ -651,7 +651,7 @@ public class ExecutorRouteGeneratorTests
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void PartialClass_SendsYieldsInBothFiles_GeneratesAlOverrides()
|
||||
public void PartialClass_SendsYieldsInBothFiles_GeneratesAllOverrides()
|
||||
{
|
||||
// File 1: Partial with one handler
|
||||
var file1 = """
|
||||
@@ -700,7 +700,7 @@ public class ExecutorRouteGeneratorTests
|
||||
generated.Should().RegisterSentMessageType("string")
|
||||
.And.RegisterSentMessageType("int")
|
||||
.And.RegisterYieldedOutputType("string")
|
||||
.And.RegisterYieldedOutputType("string");
|
||||
.And.RegisterYieldedOutputType("int");
|
||||
}
|
||||
|
||||
#endregion
|
||||
@@ -1046,6 +1046,85 @@ public class ExecutorRouteGeneratorTests
|
||||
.And.RegisterSentMessageType("global::TestNamespace.BroadcastMessage");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ProtocolOnly_DerivesFromExecutorOfT_GeneratesBaseCall()
|
||||
{
|
||||
// A protocol-only partial executor deriving from Executor<T>
|
||||
// has a base class that already overrides ConfigureProtocol. The generator must emit
|
||||
// "return base.ConfigureProtocol(protocolBuilder)" so inherited handler registrations
|
||||
// are preserved — not "return protocolBuilder" which silently drops them.
|
||||
var source = """
|
||||
using System;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Microsoft.Agents.AI.Workflows;
|
||||
|
||||
namespace TestNamespace;
|
||||
|
||||
public class FeedbackResult { }
|
||||
|
||||
[SendsMessage(typeof(FeedbackResult))]
|
||||
[YieldsOutput(typeof(string))]
|
||||
public partial class FeedbackExecutor : Executor<string>
|
||||
{
|
||||
public FeedbackExecutor() : base("feedback") { }
|
||||
|
||||
public override System.Threading.Tasks.ValueTask HandleAsync(string message, IWorkflowContext context, System.Threading.CancellationToken cancellationToken = default)
|
||||
=> default;
|
||||
}
|
||||
""";
|
||||
|
||||
var result = GeneratorTestHelper.RunGenerator(source);
|
||||
|
||||
result.RunResult.GeneratedTrees.Should().HaveCount(1);
|
||||
result.RunResult.Diagnostics.Should().BeEmpty();
|
||||
|
||||
var generated = result.RunResult.GeneratedTrees[0].ToString();
|
||||
|
||||
// Base class Executor<T> overrides ConfigureProtocol, so the generated override
|
||||
// must chain to base to preserve the inherited handler registration.
|
||||
generated.Should().Contain("return base.ConfigureProtocol(protocolBuilder)",
|
||||
because: "Executor<T> overrides ConfigureProtocol, so base must be called to preserve its handler registration");
|
||||
generated.Should().Contain(".SendsMessage<global::TestNamespace.FeedbackResult>()");
|
||||
generated.Should().Contain(".YieldsOutput<string>()");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ProtocolOnly_DerivesDirectlyFromExecutor_DoesNotGenerateBaseCall()
|
||||
{
|
||||
// A protocol-only partial executor deriving directly from Executor (abstract base
|
||||
// with no non-abstract ConfigureProtocol override) should generate "return protocolBuilder"
|
||||
// rather than "return base.ConfigureProtocol(protocolBuilder)".
|
||||
var source = """
|
||||
using System;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Microsoft.Agents.AI.Workflows;
|
||||
|
||||
namespace TestNamespace;
|
||||
|
||||
public class BroadcastMessage { }
|
||||
|
||||
[SendsMessage(typeof(BroadcastMessage))]
|
||||
public partial class BroadcastExecutor : Executor
|
||||
{
|
||||
public BroadcastExecutor() : base("broadcast") { }
|
||||
}
|
||||
""";
|
||||
|
||||
var result = GeneratorTestHelper.RunGenerator(source);
|
||||
|
||||
result.RunResult.GeneratedTrees.Should().HaveCount(1);
|
||||
result.RunResult.Diagnostics.Should().BeEmpty();
|
||||
|
||||
var generated = result.RunResult.GeneratedTrees[0].ToString();
|
||||
|
||||
// Executor's ConfigureProtocol is abstract — no base call needed.
|
||||
generated.Should().Contain("return protocolBuilder",
|
||||
because: "Executor base class has no non-abstract ConfigureProtocol, so no base call is needed");
|
||||
generated.Should().NotContain("base.ConfigureProtocol");
|
||||
}
|
||||
|
||||
#endregion
|
||||
|
||||
#region Generic Executor Tests
|
||||
|
||||
Reference in New Issue
Block a user