Address re-review findings #1-4, 7-8: explicit-unversioned helper conflict, TaskVersion null-safety, integration test refresh, migration recipe

Closes the second-pass review findings from PR #695:

#1 — Stale tag references in integration tests: VersionedClassSyntaxTestOrchestration.cs and VersionedClassSyntaxIntegrationTests.cs were still using the pre-rename 'microsoft.durabletask.activity.explicit-version' constant. The spoof-protection test in particular was passing for the wrong reason since the SDK no longer recognizes that key. Updated both files to use ActivityVersioning.VersionSourceTagName / ExplicitSource / InheritedSource and added IVT for Worker.Grpc.IntegrationTests in Worker.csproj so the integration tests can reference the SDK constants directly.

#2 — Generator helper conflict check missed explicit-unversioned: ApplyGeneratedVersion (StartOrchestrationOptions / SubOrchestrationOptions) and ApplyGeneratedActivityVersion all used patterns that required '.Version.Length > 0' or '!IsNullOrWhiteSpace', which excluded TaskVersion.Unversioned. A user calling CallProcessPayment_2Async(..., new ActivityOptions { Version = TaskVersion.Unversioned }) silently got v2 instead of the contradicting throw the helper was supposed to enforce. Now the patterns match any non-null Version (including the empty/unversioned case) and the diagnostic message distinguishes 'explicit-unversioned' from a specific version. Generator-output snapshot tests updated.

#3 — TaskVersion null-storage caused null/empty mismatch and GetHashCode crash: the constructor stored 'null' verbatim when given null. Effects: TaskVersion.Unversioned.Equals(new TaskVersion('')) was false, and TaskVersion.Unversioned.GetHashCode() called StringComparer.OrdinalIgnoreCase.GetHashCode(null) which throws — making any user who put TaskVersion in a dictionary key crash at runtime. Constructor now normalizes null/empty to string.Empty; Equals and GetHashCode are null-safe and treat null and empty as identical. (default(TaskVersion) still has Version=null at the field level — that's a struct-default constraint — but Equals/GetHashCode handle it.)

#4 — Whitespace TaskVersion ctor validation: the registry rejected whitespace-only TaskVersion, but new TaskVersion('  ') itself accepted the value, so it could leak into ActivityOptions / StartOrchestrationOptions / SubOrchestrationOptions and route silently to 'no exact match'. The constructor now throws ArgumentException for non-empty whitespace. The redundant ValidateRegistrationVersion method and the explicit whitespace check in DurableTaskVersionAttribute were removed because TaskVersion's constructor handles them centrally.

#7 — Migration recipe: added a new section to the proposal documenting the 'add [DurableTaskVersion] to an existing class' migration path. Recommended recipe is keep the unversioned class registered until in-flight unversioned instances drain or ContinueAsNew to the new version. Reverse migration (removing [DurableTaskVersion]) is documented as unsupported.

#8 — Tag rename pre-release note: added a one-paragraph note in the proposal acknowledging the in-flight rename from 'explicit-version' (boolean) to 'version-source' ('explicit'/'inherited') and recommending pre-release deployments drain before pointing at the new contract since the worker now fail-closes on missing tag for versioned activity requests.

Tests: 84 generator + 127 worker.tests + 135 worker.grpc.tests + 159 abstractions.tests + 8 integration = 513 total, all passing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: torosent

src/Abstractions/DurableTaskRegistry.Orchestrators.cs

@@ -54,7 +54,6 @@ public DurableTaskRegistry AddOrchestrator(TaskName name, TaskVersion version, F
     {
         Check.NotDefault(name);
         Check.NotNull(factory);
-        ValidateRegistrationVersion(version);
 
         OrchestratorVersionKey key = new(name, version);
         if (this.Orchestrators.ContainsKey(key))

src/Abstractions/DurableTaskRegistry.cs

@@ -79,7 +79,6 @@ DurableTaskRegistry AddActivity(TaskName name, TaskVersion version, Func<IServic
     {
         Check.NotDefault(name);
         Check.NotNull(factory);
-        ValidateRegistrationVersion(version);
 
         ActivityVersionKey key = new(name, version);
         if (this.Activities.ContainsKey(key))
@@ -93,18 +92,4 @@ DurableTaskRegistry AddActivity(TaskName name, TaskVersion version, Func<IServic
         this.Activities.Add(key, factory);
         return this;
     }
-
-    static void ValidateRegistrationVersion(TaskVersion version)
-    {
-        // An empty/null version is the unversioned/default registration. Whitespace-only is rejected because
-        // it would be silently coerced to "no version" by some code paths and to a non-empty version by others,
-        // making the registration unreachable.
-        string? value = version.Version;
-        if (value is not null && value.Length > 0 && string.IsNullOrWhiteSpace(value))
-        {
-            throw new ArgumentException(
-                "Version must not be whitespace-only. Provide a non-empty version string or pass default(TaskVersion) to register an unversioned task.",
-                nameof(version));
-        }
-    }
 }

src/Abstractions/DurableTaskVersionAttribute.cs

@@ -19,14 +19,9 @@ public sealed class DurableTaskVersionAttribute : Attribute
     /// <param name="version">The version string for the orchestrator or activity.</param>
     public DurableTaskVersionAttribute(string? version = null)
     {
-        if (version is not null && version.Length > 0 && string.IsNullOrWhiteSpace(version))
-        {
-            throw new ArgumentException(
-                "Version must not be whitespace-only. Provide a non-empty version string or omit the attribute argument to declare an unversioned task.",
-                nameof(version));
-        }
-
-        this.Version = string.IsNullOrEmpty(version) ? default : new TaskVersion(version!);
+        // TaskVersion's constructor itself rejects whitespace-only strings and normalizes null/empty to
+        // TaskVersion.Unversioned, so a single delegation here covers all three cases.
+        this.Version = string.IsNullOrEmpty(version) ? TaskVersion.Unversioned : new TaskVersion(version!);
     }
 
     /// <summary>

src/Abstractions/TaskVersion.cs

@@ -9,7 +9,8 @@ namespace Microsoft.DurableTask;
 public readonly struct TaskVersion : IEquatable<TaskVersion>
 {
     /// <summary>
-    /// A sentinel value representing an unversioned task. Equivalent to <c>default(TaskVersion)</c>.
+    /// A sentinel value representing an unversioned task. Equivalent to <c>default(TaskVersion)</c> and
+    /// <c>new TaskVersion(string.Empty)</c>.
     /// </summary>
     /// <remarks>
     /// Use this on <see cref="ActivityOptions.Version"/> to explicitly request the unversioned activity
@@ -21,21 +22,38 @@ namespace Microsoft.DurableTask;
     /// <summary>
     /// Initializes a new instance of the <see cref="TaskVersion"/> struct.
     /// </summary>
-    /// <param name="version">The version of the task. Providing <c>null</c> will result in the default struct.</param>
+    /// <param name="version">The version of the task. <c>null</c> or <see cref="string.Empty"/> produces
+    /// an unversioned <see cref="TaskVersion"/> equal to <see cref="Unversioned"/>.</param>
+    /// <exception cref="ArgumentException">
+    /// Thrown when <paramref name="version"/> is non-empty but contains only whitespace. Pass <c>null</c>,
+    /// <see cref="string.Empty"/>, or use <see cref="Unversioned"/> to represent an unversioned task.
+    /// </exception>
     public TaskVersion(string version)
     {
-        if (version == null)
+        // Normalize null/empty to string.Empty so default(TaskVersion), TaskVersion.Unversioned, and
+        // new TaskVersion("") all compare and hash identically. Without this normalization the struct's
+        // Version field can be null, which makes Equals(null, "") return false and causes
+        // StringComparer.OrdinalIgnoreCase.GetHashCode to throw at runtime when the struct is used as a
+        // dictionary key.
+        if (string.IsNullOrEmpty(version))
         {
-            this.Version = null!;
+            this.Version = string.Empty;
+            return;
         }
-        else
+
+        if (string.IsNullOrWhiteSpace(version))
         {
-            this.Version = version;
+            throw new ArgumentException(
+                "Version must not be whitespace-only. Pass null, an empty string, or use TaskVersion.Unversioned to represent an unversioned task.",
+                nameof(version));
         }
+
+        this.Version = version;
     }
 
     /// <summary>
-    /// Gets the version of a task.
+    /// Gets the version of a task. Returns <see cref="string.Empty"/> for an unversioned task; never
+    /// returns <c>null</c>.
     /// </summary>
     public string Version { get; }
 
@@ -81,7 +99,12 @@ public TaskVersion(string version)
     /// <returns><c>true</c> if the two <see cref="TaskVersion"/> are equal using value semantics; otherwise <c>false</c>.</returns>
     public bool Equals(TaskVersion other)
     {
-        return string.Equals(this.Version, other.Version, StringComparison.OrdinalIgnoreCase);
+        // Treat null and empty Version as the same unversioned identity. Combined with normalization in
+        // the constructor, both default(TaskVersion) and new TaskVersion("") compare equal and hash to
+        // the same value as TaskVersion.Unversioned.
+        string left = this.Version ?? string.Empty;
+        string right = other.Version ?? string.Empty;
+        return string.Equals(left, right, StringComparison.OrdinalIgnoreCase);
     }
 
     /// <summary>
@@ -106,6 +129,9 @@ public override bool Equals(object? obj)
     /// <returns>A 32-bit hash code value.</returns>
     public override int GetHashCode()
     {
-        return StringComparer.OrdinalIgnoreCase.GetHashCode(this.Version);
+        // Null-safe: a default-constructed TaskVersion (or one created via the implicit conversion from
+        // null) must not crash when used as a dictionary key. Treats null and empty as the same key.
+        string value = this.Version ?? string.Empty;
+        return StringComparer.OrdinalIgnoreCase.GetHashCode(value);
     }
 }

src/Generators/DurableTaskSourceGenerator.cs

@@ -941,12 +941,18 @@ static void AddStandaloneGeneratedVersionHelperMethods(
                 sourceBuilder.AppendLine(@"
         static StartOrchestrationOptions? ApplyGeneratedVersion(StartOrchestrationOptions? options, string version)
         {
-            if (options?.Version is { Version: { Length: > 0 } existingVersion })
-            {
-                if (!string.Equals(existingVersion, version, System.StringComparison.OrdinalIgnoreCase))
+            if (options?.Version is TaskVersion existingStart)
+            {
+                // Any non-null Version on the options is an explicit caller selection — including
+                // TaskVersion.Unversioned and the empty-string equivalent. The generated helper bakes a
+                // specific version into its method name, so a disagreement is always a contradiction;
+                // the silently-overridden case is precisely what we are trying to prevent.
+                string existingValue = existingStart.Version ?? string.Empty;
+                if (!string.Equals(existingValue, version, System.StringComparison.OrdinalIgnoreCase))
                 {
+                    string requested = string.IsNullOrEmpty(existingValue) ? ""<unversioned>"" : ""'"" + existingValue + ""'"";
                     throw new System.InvalidOperationException(
-                        $""The generated helper targets version '{version}' but options.Version was set to '{existingVersion}'. Use the unqualified ScheduleNewOrchestrationInstanceAsync overload to schedule a different version."");
+                        $""The generated helper targets version '{version}' but options.Version was set to {requested}. Use the unqualified ScheduleNewOrchestrationInstanceAsync overload to schedule a different version."");
                 }
 
                 return options;
@@ -968,12 +974,14 @@ static void AddStandaloneGeneratedVersionHelperMethods(
 
         static TaskOptions? ApplyGeneratedVersion(TaskOptions? options, string version)
         {
-            if (options is SubOrchestrationOptions { Version: { Version: { Length: > 0 } existingSubVersion } })
+            if (options is SubOrchestrationOptions { Version: TaskVersion existingSub })
             {
-                if (!string.Equals(existingSubVersion, version, System.StringComparison.OrdinalIgnoreCase))
+                string existingValue = existingSub.Version ?? string.Empty;
+                if (!string.Equals(existingValue, version, System.StringComparison.OrdinalIgnoreCase))
                 {
+                    string requested = string.IsNullOrEmpty(existingValue) ? ""<unversioned>"" : ""'"" + existingValue + ""'"";
                     throw new System.InvalidOperationException(
-                        $""The generated sub-orchestrator helper targets version '{version}' but options.Version was set to '{existingSubVersion}'. Use the unqualified CallSubOrchestratorAsync overload to call a different version."");
+                        $""The generated sub-orchestrator helper targets version '{version}' but options.Version was set to {requested}. Use the unqualified CallSubOrchestratorAsync overload to call a different version."");
                 }
 
                 return options;
@@ -1008,13 +1016,17 @@ static void AddStandaloneGeneratedVersionHelperMethods(
         static TaskOptions? ApplyGeneratedActivityVersion(TaskOptions? options, string version)
         {
             if (options is ActivityOptions activityOptions
-                && activityOptions.Version is TaskVersion explicitVersion
-                && !string.IsNullOrWhiteSpace(explicitVersion.Version))
+                && activityOptions.Version is TaskVersion explicitVersion)
             {
-                if (!string.Equals(explicitVersion.Version, version, System.StringComparison.OrdinalIgnoreCase))
+                // Any non-null ActivityOptions.Version is an explicit caller selection — including
+                // TaskVersion.Unversioned and the empty-string equivalent. Disagreement with the helper-
+                // baked version is always a contradiction, so we throw rather than silently override.
+                string explicitValue = explicitVersion.Version ?? string.Empty;
+                if (!string.Equals(explicitValue, version, System.StringComparison.OrdinalIgnoreCase))
                 {
+                    string requested = string.IsNullOrEmpty(explicitValue) ? ""<unversioned>"" : ""'"" + explicitValue + ""'"";
                     throw new System.InvalidOperationException(
-                        $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to '{explicitVersion.Version}'. Use the unqualified CallActivityAsync overload to call a different version."");
+                        $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to {requested}. Use the unqualified CallActivityAsync overload to call a different version."");
                 }
 
                 return options;

src/Worker/Core/Worker.csproj

@@ -21,6 +21,7 @@ The worker is responsible for processing durable task work items.</PackageDescri
 
   <ItemGroup>
     <InternalsVisibleTo Include="Microsoft.DurableTask.Worker.Grpc" Key="$(StrongNamePublicKey)" />
+    <InternalsVisibleTo Include="Microsoft.DurableTask.Grpc.IntegrationTests" Key="$(StrongNamePublicKey)" />
   </ItemGroup>
 
   <ItemGroup>

test/Generators.Tests/VersionedActivityTests.cs

@@ -44,13 +44,17 @@ public static Task<string> CallInvoiceActivityAsync(this TaskOrchestrationContex
 static TaskOptions? ApplyGeneratedActivityVersion(TaskOptions? options, string version)
 {
     if (options is ActivityOptions activityOptions
-        && activityOptions.Version is TaskVersion explicitVersion
-        && !string.IsNullOrWhiteSpace(explicitVersion.Version))
+        && activityOptions.Version is TaskVersion explicitVersion)
     {
-        if (!string.Equals(explicitVersion.Version, version, System.StringComparison.OrdinalIgnoreCase))
+        // Any non-null ActivityOptions.Version is an explicit caller selection — including
+        // TaskVersion.Unversioned and the empty-string equivalent. Disagreement with the helper-
+        // baked version is always a contradiction, so we throw rather than silently override.
+        string explicitValue = explicitVersion.Version ?? string.Empty;
+        if (!string.Equals(explicitValue, version, System.StringComparison.OrdinalIgnoreCase))
         {
+            string requested = string.IsNullOrEmpty(explicitValue) ? ""<unversioned>"" : ""'"" + explicitValue + ""'"";
             throw new System.InvalidOperationException(
-                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to '{explicitVersion.Version}'. Use the unqualified CallActivityAsync overload to call a different version."");
+                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to {requested}. Use the unqualified CallActivityAsync overload to call a different version."");
         }
 
         return options;
@@ -136,13 +140,17 @@ public static Task<string> CallInvoiceActivity_v2Async(this TaskOrchestrationCon
 static TaskOptions? ApplyGeneratedActivityVersion(TaskOptions? options, string version)
 {
     if (options is ActivityOptions activityOptions
-        && activityOptions.Version is TaskVersion explicitVersion
-        && !string.IsNullOrWhiteSpace(explicitVersion.Version))
+        && activityOptions.Version is TaskVersion explicitVersion)
     {
-        if (!string.Equals(explicitVersion.Version, version, System.StringComparison.OrdinalIgnoreCase))
+        // Any non-null ActivityOptions.Version is an explicit caller selection — including
+        // TaskVersion.Unversioned and the empty-string equivalent. Disagreement with the helper-
+        // baked version is always a contradiction, so we throw rather than silently override.
+        string explicitValue = explicitVersion.Version ?? string.Empty;
+        if (!string.Equals(explicitValue, version, System.StringComparison.OrdinalIgnoreCase))
         {
+            string requested = string.IsNullOrEmpty(explicitValue) ? ""<unversioned>"" : ""'"" + explicitValue + ""'"";
             throw new System.InvalidOperationException(
-                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to '{explicitVersion.Version}'. Use the unqualified CallActivityAsync overload to call a different version."");
+                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to {requested}. Use the unqualified CallActivityAsync overload to call a different version."");
         }
 
         return options;
@@ -220,13 +228,17 @@ public static Task<string> CallInvoiceActivityAsync(this TaskOrchestrationContex
 static TaskOptions? ApplyGeneratedActivityVersion(TaskOptions? options, string version)
 {
     if (options is ActivityOptions activityOptions
-        && activityOptions.Version is TaskVersion explicitVersion
-        && !string.IsNullOrWhiteSpace(explicitVersion.Version))
+        && activityOptions.Version is TaskVersion explicitVersion)
     {
-        if (!string.Equals(explicitVersion.Version, version, System.StringComparison.OrdinalIgnoreCase))
+        // Any non-null ActivityOptions.Version is an explicit caller selection — including
+        // TaskVersion.Unversioned and the empty-string equivalent. Disagreement with the helper-
+        // baked version is always a contradiction, so we throw rather than silently override.
+        string explicitValue = explicitVersion.Version ?? string.Empty;
+        if (!string.Equals(explicitValue, version, System.StringComparison.OrdinalIgnoreCase))
         {
+            string requested = string.IsNullOrEmpty(explicitValue) ? ""<unversioned>"" : ""'"" + explicitValue + ""'"";
             throw new System.InvalidOperationException(
-                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to '{explicitVersion.Version}'. Use the unqualified CallActivityAsync overload to call a different version."");
+                $""The generated activity helper targets version '{version}' but ActivityOptions.Version was set to {requested}. Use the unqualified CallActivityAsync overload to call a different version."");
         }
 
         return options;