Responding to PR comments

Author: Evanthx

src/Temporalio/Nexus/ProtoLinkExtensions.cs

@@ -299,7 +299,21 @@ public static Api.Common.V1.Link.Types.WorkflowEvent ToWorkflowEvent(this NexusL
             return evt;
         }
 
-        // Simple query param parser because .NET stdlib doesn't have one in all versions.
+        /// <summary>
+        /// Parse a URI query string into its parameters. Simple hand-rolled parser because the .NET
+        /// stdlib does not have one in all versions we target.
+        /// </summary>
+        /// <param name="uri">URI whose query string to parse.</param>
+        /// <returns>
+        /// The query parameters, with both keys and values percent-decoded. Values are additionally
+        /// form-decoded, i.e. <c>+</c> becomes a space, since that is how a query value is encoded on
+        /// the way out; the replacement is safe to do before percent-decoding because a literal
+        /// <c>+</c> is encoded as <c>%2B</c>. A parameter present with no <c>=</c> maps to an empty
+        /// string, so callers cannot distinguish <c>?reason</c> from <c>?reason=</c>. Keys are looked
+        /// up with the dictionary's default ordinal comparer, so lookups are case-sensitive and
+        /// <c>reason</c> and <c>Reason</c> are different parameters. A query repeating a key throws,
+        /// which callers treat as an invalid link like any other malformed URI.
+        /// </returns>
         private static Dictionary<string, string> ParseQueryParams(Uri uri) =>
             uri.Query.
                 TrimStart('?').

src/Temporalio/Worker/NexusWorker.cs

@@ -389,16 +389,23 @@ private HandlerException ConvertToHandlerException(Exception exc)
             }
             else if (exc is WorkflowQueryFailedException)
             {
-                // A query handler that threw will throw again on every attempt, so this must be a
-                // non-retryable type. Falling through to the Internal default below would make the
-                // server retry the operation until it times out instead of failing it.
-                return new(HandlerErrorType.BadRequest, "Workflow query failed", exc);
+                // The query handler faulted rather than the request being bad, and it will fault the
+                // same way on every attempt, so Internal must be marked non-retryable explicitly.
+                return new(
+                    HandlerErrorType.Internal,
+                    "Workflow query failed",
+                    exc,
+                    HandlerErrorRetryBehavior.NonRetryable);
             }
             else if (exc is WorkflowQueryRejectedException)
             {
-                // Rejection is a property of the workflow's state against the reject condition, so
-                // retrying cannot change the outcome either.
-                return new(HandlerErrorType.BadRequest, "Workflow query rejected", exc);
+                // Rejection follows from the queried workflow's status, so as above this is not a
+                // caller error and retrying cannot change the outcome.
+                return new(
+                    HandlerErrorType.Internal,
+                    "Workflow query rejected",
+                    exc,
+                    HandlerErrorRetryBehavior.NonRetryable);
             }
             else if (exc is ApplicationFailureException appExc && appExc.NonRetryable)
             {

tests/Temporalio.Tests/Nexus/NexusWorkflowUpdateHandleTests.cs

@@ -3,7 +3,6 @@ namespace Temporalio.Tests.Nexus;
 using System.Linq;
 using System.Text;
 using System.Text.Json;
-using Temporalio.Api.Common.V1;
 using Temporalio.Nexus;
 using Xunit;
 
@@ -207,71 +206,4 @@ public void FromToken_ToleratesAbsentRid()
         Assert.Equal(string.Empty, handle.RunId);
         Assert.Equal("u", handle.UpdateId);
     }
-
-    [Fact]
-    public void CommonLink_ToNexusLink_UnsetOneof_ReturnsNull()
-    {
-        // A link whose oneof is unset (neither workflow-event nor workflow) must not dereference a
-        // null variant; it returns null so callers can skip it.
-        var link = new Link().ToNexusLink();
-
-        Assert.Null(link);
-    }
-
-    [Fact]
-    public void WorkflowLink_ToNexusLink_BuildsWorkflowUri()
-    {
-        // A workflow link addresses the execution itself, so there is no "/history" suffix. The
-        // suffix belongs to the workflow-event form, and its absence is what distinguishes the two.
-        var workflow = new Link.Types.Workflow
-        {
-            Namespace = "ns",
-            WorkflowId = "wid",
-            RunId = "rid",
-        };
-        var link = workflow.ToNexusLink();
-
-        Assert.Equal("temporal", link.Uri.Scheme);
-        Assert.Equal("/namespaces/ns/workflows/wid/rid", link.Uri.AbsolutePath);
-        Assert.Equal(Link.Types.Workflow.Descriptor.FullName, link.Type);
-    }
-
-    [Fact]
-    public void CommonLink_ToNexusLink_PrefersWorkflowEvent()
-    {
-        var common = new Link
-        {
-            WorkflowEvent = new()
-            {
-                Namespace = "ns",
-                WorkflowId = "wid",
-                RunId = "rid",
-                EventRef = new() { EventId = 1 },
-            },
-        };
-        var link = common.ToNexusLink();
-
-        Assert.NotNull(link);
-        Assert.Equal(Link.Types.WorkflowEvent.Descriptor.FullName, link.Type);
-    }
-
-    [Fact]
-    public void CommonLink_ToNexusLink_FallsBackToWorkflow()
-    {
-        // No history event (e.g. a rejected update) — falls back to the workflow link.
-        var common = new Link
-        {
-            Workflow = new()
-            {
-                Namespace = "ns",
-                WorkflowId = "wid",
-                RunId = "rid",
-            },
-        };
-        var link = common.ToNexusLink();
-
-        Assert.NotNull(link);
-        Assert.Equal(Link.Types.Workflow.Descriptor.FullName, link.Type);
-        Assert.Equal("/namespaces/ns/workflows/wid/rid", link.Uri.AbsolutePath);
-    }
 }

tests/Temporalio.Tests/Nexus/ProtoLinkExtensionsTests.cs

@@ -384,6 +384,31 @@ public void ToWorkflow_ReasonPrefixKeyIgnored()
         Assert.Equal(string.Empty, link.ToWorkflow().Reason);
     }
 
+    [Fact]
+    public void ToWorkflow_ReasonKeyLookupIsCaseSensitive()
+    {
+        // Query params are held in a dictionary with the default ordinal comparer, so "Reason" is a
+        // different param than "reason" and does not populate the field.
+        var link = new NexusLink(
+            new Uri("temporal:///namespaces/ns/workflows/wf-id/run-id?Reason=nope"),
+            Api.Common.V1.Link.Types.Workflow.Descriptor.FullName);
+
+        Assert.Equal(string.Empty, link.ToWorkflow().Reason);
+    }
+
+    [Fact]
+    public void ToWorkflow_RepeatedReasonKey_Throws()
+    {
+        // A repeated key cannot go into the dictionary, so it surfaces as the same ArgumentException
+        // as any other malformed link. Callers converting links catch that and drop the link with a
+        // warning rather than failing the operation.
+        var link = new NexusLink(
+            new Uri("temporal:///namespaces/ns/workflows/wf-id/run-id?reason=a&reason=b"),
+            Api.Common.V1.Link.Types.Workflow.Descriptor.FullName);
+
+        Assert.Throws<ArgumentException>(() => link.ToWorkflow());
+    }
+
     [Fact]
     public void ToWorkflow_LiteralPlusInPathIsPreserved()
     {
@@ -430,6 +455,8 @@ public void ToProtoLink_WorkflowShape_RoundTrips()
     [Fact]
     public void ProtoToNexusLink_WorkflowVariant_Dispatches()
     {
+        // The workflow variant is what a link carries when there is no history event to point at,
+        // e.g. a Query or a rejected update.
         var protoLink = new Api.Common.V1.Link
         {
             Workflow = new() { Namespace = "ns", WorkflowId = "wf", RunId = "run" },
@@ -500,11 +527,12 @@ public void ToWorkflowEvent_FormDecodesQueryValues()
     [Fact]
     public void ToWorkflow_AcceptsEmptyRunId()
     {
-        // Characterization, not a statement of intent. A trailing slash still yields the expected
-        // segment count, so the run ID comes back empty rather than being rejected. The same
-        // leniency exists in the workflow-event, activity, and nexus-operation converters, since
-        // they share this path parsing, so tightening it is a decision about all four rather than
-        // about this converter.
+        // A trailing slash still yields the expected segment count, so the run ID comes back empty
+        // rather than being rejected. The workflow-event, activity, and nexus-operation converters
+        // are equally lenient because all four share this path parsing, so tightening it is a
+        // decision about all four and belongs with that change rather than this one. This test
+        // exists so that whichever way it is decided, the behavior changes visibly instead of
+        // silently.
         var link = new NexusLink(
             new Uri("temporal:///namespaces/ns/workflows/wf-id/"),
             Api.Common.V1.Link.Types.Workflow.Descriptor.FullName);
@@ -515,7 +543,7 @@ public void ToWorkflow_AcceptsEmptyRunId()
     [Fact]
     public void ToWorkflow_AcceptsEmptyNamespace()
     {
-        // Characterization; see ToWorkflow_AcceptsEmptyRunId.
+        // The same shared-path-parsing leniency as ToWorkflow_AcceptsEmptyRunId; see that test.
         var link = new NexusLink(
             new Uri("temporal:///namespaces//workflows/wf-id/run-id"),
             Api.Common.V1.Link.Types.Workflow.Descriptor.FullName);

tests/Temporalio.Tests/Worker/NexusQueryOperationTests.cs

@@ -151,7 +151,9 @@ await RunWithCounterAsync(async (endpoint, taskQueue, counter) =>
         {
             var caller = await RunCallerAsync(
                 taskQueue, endpoint, new(counter.Id, Fail: true));
-            await AssertOperationFailedWithAsync(HandlerErrorType.BadRequest, caller);
+            // The query handler faulted, not the Nexus request, so this is Internal and explicitly
+            // non-retryable rather than BadRequest.
+            await AssertOperationFailedWithAsync(HandlerErrorType.Internal, caller);
         });
     }
 
@@ -180,7 +182,7 @@ await worker.ExecuteAsync(async () =>
 
             var caller = await RunCallerAsync(
                 taskQueue, endpointName, new(counter.Id, RejectNotOpen: true));
-            await AssertOperationFailedWithAsync(HandlerErrorType.BadRequest, caller);
+            await AssertOperationFailedWithAsync(HandlerErrorType.Internal, caller);
         });
     }