Fix parser comparison mismatches (#4220)

src/Runner.Worker/ActionManifestManagerWrapper.cs

@@ -84,7 +84,8 @@ namespace GitHub.Runner.Worker
                 "EvaluateContainerEnvironment",
                 () => _legacyManager.EvaluateContainerEnvironment(executionContext, token, extraExpressionValues),
                 () => _newManager.EvaluateContainerEnvironment(executionContext, ConvertToNewToken(token) as GitHub.Actions.WorkflowParser.ObjectTemplating.Tokens.MappingToken, ConvertToNewExpressionValues(extraExpressionValues)),
-                (legacyResult, newResult) => {
+                (legacyResult, newResult) =>
+                {
                     var trace = HostContext.GetTrace(nameof(ActionManifestManagerWrapper));
                     return CompareDictionaries(trace, legacyResult, newResult, "ContainerEnvironment");
                 });
@@ -165,9 +166,150 @@ namespace GitHub.Runner.Worker
                 return null;
             }
 
-            // Serialize new steps and deserialize to old steps
-            var json = StringUtil.ConvertToJson(newSteps, Newtonsoft.Json.Formatting.None);
-            return StringUtil.ConvertFromJson<List<GitHub.DistributedTask.Pipelines.ActionStep>>(json);
+            var result = new List<GitHub.DistributedTask.Pipelines.ActionStep>();
+            foreach (var step in newSteps)
+            {
+                var actionStep = new GitHub.DistributedTask.Pipelines.ActionStep
+                {
+                    ContextName = step.Id,
+                };
+
+                if (step is GitHub.Actions.WorkflowParser.RunStep runStep)
+                {
+                    actionStep.Condition = ExtractConditionString(runStep.If);
+                    actionStep.DisplayNameToken = ConvertToLegacyToken<TemplateToken>(runStep.Name);
+                    actionStep.ContinueOnError = ConvertToLegacyToken<TemplateToken>(runStep.ContinueOnError);
+                    actionStep.TimeoutInMinutes = ConvertToLegacyToken<TemplateToken>(runStep.TimeoutMinutes);
+                    actionStep.Environment = ConvertToLegacyToken<TemplateToken>(runStep.Env);
+                    actionStep.Reference = new GitHub.DistributedTask.Pipelines.ScriptReference();
+                    actionStep.Inputs = BuildRunStepInputs(runStep);
+                }
+                else if (step is GitHub.Actions.WorkflowParser.ActionStep usesStep)
+                {
+                    actionStep.Condition = ExtractConditionString(usesStep.If);
+                    actionStep.DisplayNameToken = ConvertToLegacyToken<TemplateToken>(usesStep.Name);
+                    actionStep.ContinueOnError = ConvertToLegacyToken<TemplateToken>(usesStep.ContinueOnError);
+                    actionStep.TimeoutInMinutes = ConvertToLegacyToken<TemplateToken>(usesStep.TimeoutMinutes);
+                    actionStep.Environment = ConvertToLegacyToken<TemplateToken>(usesStep.Env);
+                    actionStep.Reference = ParseActionReference(usesStep.Uses?.Value);
+                    actionStep.Inputs = ConvertToLegacyToken<MappingToken>(usesStep.With);
+                }
+
+                result.Add(actionStep);
+            }
+            return result;
+        }
+
+        private string ExtractConditionString(GitHub.Actions.WorkflowParser.ObjectTemplating.Tokens.BasicExpressionToken ifToken)
+        {
+            if (ifToken == null)
+            {
+                return null;
+            }
+
+            // The Expression property is internal, so we use ToString() which formats as "${{ expr }}"
+            // Then strip the delimiters to get just the expression
+            var str = ifToken.ToString();
+            if (str.StartsWith("${{") && str.EndsWith("}}"))
+            {
+                return str.Substring(3, str.Length - 5).Trim();
+            }
+            return str;
+        }
+
+        private MappingToken BuildRunStepInputs(GitHub.Actions.WorkflowParser.RunStep runStep)
+        {
+            var inputs = new MappingToken(null, null, null);
+
+            // script (from run)
+            if (runStep.Run != null)
+            {
+                inputs.Add(
+                    new StringToken(null, null, null, "script"),
+                    ConvertToLegacyToken<TemplateToken>(runStep.Run));
+            }
+
+            // shell
+            if (runStep.Shell != null)
+            {
+                inputs.Add(
+                    new StringToken(null, null, null, "shell"),
+                    ConvertToLegacyToken<TemplateToken>(runStep.Shell));
+            }
+
+            // working-directory
+            if (runStep.WorkingDirectory != null)
+            {
+                inputs.Add(
+                    new StringToken(null, null, null, "workingDirectory"),
+                    ConvertToLegacyToken<TemplateToken>(runStep.WorkingDirectory));
+            }
+
+            return inputs.Count > 0 ? inputs : null;
+        }
+
+        private GitHub.DistributedTask.Pipelines.ActionStepDefinitionReference ParseActionReference(string uses)
+        {
+            if (string.IsNullOrEmpty(uses))
+            {
+                return null;
+            }
+
+            // Docker reference: docker://image:tag
+            if (uses.StartsWith("docker://", StringComparison.OrdinalIgnoreCase))
+            {
+                return new GitHub.DistributedTask.Pipelines.ContainerRegistryReference
+                {
+                    Image = uses.Substring("docker://".Length)
+                };
+            }
+
+            // Local path reference: ./path/to/action
+            if (uses.StartsWith("./") || uses.StartsWith(".\\"))
+            {
+                return new GitHub.DistributedTask.Pipelines.RepositoryPathReference
+                {
+                    RepositoryType = "self",
+                    Path = uses
+                };
+            }
+
+            // Repository reference: owner/repo@ref or owner/repo/path@ref
+            var atIndex = uses.LastIndexOf('@');
+            string refPart = null;
+            string repoPart = uses;
+
+            if (atIndex > 0)
+            {
+                refPart = uses.Substring(atIndex + 1);
+                repoPart = uses.Substring(0, atIndex);
+            }
+
+            // Split by / to get owner/repo and optional path
+            var parts = repoPart.Split('/');
+            string name;
+            string path = null;
+
+            if (parts.Length >= 2)
+            {
+                name = $"{parts[0]}/{parts[1]}";
+                if (parts.Length > 2)
+                {
+                    path = string.Join("/", parts, 2, parts.Length - 2);
+                }
+            }
+            else
+            {
+                name = repoPart;
+            }
+
+            return new GitHub.DistributedTask.Pipelines.RepositoryPathReference
+            {
+                RepositoryType = "GitHub",
+                Name = name,
+                Ref = refPart,
+                Path = path
+            };
         }
 
         private T ConvertToLegacyToken<T>(GitHub.Actions.WorkflowParser.ObjectTemplating.Tokens.TemplateToken newToken) where T : TemplateToken
@@ -633,6 +775,14 @@ namespace GitHub.Runner.Worker
                 return false;
             }
 
+            // Check for known equivalent error patterns (e.g., JSON parse errors)
+            // where both parsers correctly reject invalid input but with different wording
+            if (PipelineTemplateEvaluatorWrapper.HasJsonExceptionType(legacyException) && PipelineTemplateEvaluatorWrapper.HasJsonExceptionType(newException))
+            {
+                trace.Info("CompareExceptions - both exceptions are JSON parse errors, treating as matched");
+                return true;
+            }
+
             // Compare exception messages recursively (including inner exceptions)
             var legacyMessages = GetExceptionMessages(legacyException);
             var newMessages = GetExceptionMessages(newException);
@@ -697,5 +847,6 @@ namespace GitHub.Runner.Worker
 
             return messages;
         }
+
     }
 }

src/Runner.Worker/InternalsVisibleTo.cs

@@ -0,0 +1,3 @@
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("Test")]

src/Runner.Worker/PipelineTemplateEvaluatorWrapper.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using GitHub.Actions.WorkflowParser;
 using GitHub.DistributedTask.Expressions2;
@@ -216,12 +216,15 @@ namespace GitHub.Runner.Worker
             }
         }
 
-        private TLegacy EvaluateAndCompare<TLegacy, TNew>(
+        internal TLegacy EvaluateAndCompare<TLegacy, TNew>(
             string methodName,
             Func<TLegacy> legacyEvaluator,
             Func<TNew> newEvaluator,
             Func<TLegacy, TNew, bool> resultComparer)
         {
+            // Capture cancellation state before evaluation
+            var cancellationRequestedBefore = _context.CancellationToken.IsCancellationRequested;
+
             // Legacy evaluator
             var legacyException = default(Exception);
             var legacyResult = default(TLegacy);
@@ -253,14 +256,18 @@ namespace GitHub.Runner.Worker
                     newException = ex;
                 }
 
+                // Capture cancellation state after evaluation
+                var cancellationRequestedAfter = _context.CancellationToken.IsCancellationRequested;
+
                 // Compare results or exceptions
+                bool hasMismatch = false;
                 if (legacyException != null || newException != null)
                 {
                     // Either one or both threw exceptions - compare them
                     if (!CompareExceptions(legacyException, newException))
                     {
                         _trace.Info($"{methodName} exception mismatch");
-                        RecordMismatch($"{methodName}");
+                        hasMismatch = true;
                     }
                 }
                 else
@@ -269,6 +276,20 @@ namespace GitHub.Runner.Worker
                     if (!resultComparer(legacyResult, newResult))
                     {
                         _trace.Info($"{methodName} mismatch");
+                        hasMismatch = true;
+                    }
+                }
+
+                // Only record mismatch if it wasn't caused by a cancellation race condition
+                if (hasMismatch)
+                {
+                    if (!cancellationRequestedBefore && cancellationRequestedAfter)
+                    {
+                        // Cancellation state changed during evaluation window - skip recording
+                        _trace.Info($"{methodName} mismatch skipped due to cancellation race condition");
+                    }
+                    else
+                    {
                         RecordMismatch($"{methodName}");
                     }
                 }
@@ -612,6 +633,13 @@ namespace GitHub.Runner.Worker
                 return false;
             }
 
+            // Check for known equivalent error patterns (e.g., JSON parse errors)
+            // where both parsers correctly reject invalid input but with different wording
+            if (IsKnownEquivalentErrorPattern(legacyException, newException))
+            {
+                return true;
+            }
+
             // Compare exception messages recursively (including inner exceptions)
             var legacyMessages = GetExceptionMessages(legacyException);
             var newMessages = GetExceptionMessages(newException);
@@ -634,6 +662,67 @@ namespace GitHub.Runner.Worker
             return true;
         }
 
+        /// <summary>
+        /// Checks if two exceptions match a known pattern where both parsers correctly reject
+        /// invalid input but with different error messages (e.g., JSON parse errors from fromJSON).
+        /// </summary>
+        private bool IsKnownEquivalentErrorPattern(Exception legacyException, Exception newException)
+        {
+            // fromJSON('') - both parsers fail when parsing empty string as JSON
+            // The error messages differ but both indicate JSON parsing failure.
+            // Legacy throws raw JsonReaderException: "Error reading JToken from JsonReader..."
+            // New wraps it: "Error parsing fromJson" with inner JsonReaderException
+            // Both may be wrapped in TemplateValidationException: "The template is not valid..."
+            if (HasJsonExceptionType(legacyException) && HasJsonExceptionType(newException))
+            {
+                _trace.Info("CompareExceptions - both exceptions are JSON parse errors, treating as matched");
+                return true;
+            }
+
+            return false;
+        }
+
+        /// <summary>
+        /// Checks if the exception chain contains a JSON-related exception type.
+        /// </summary>
+        internal static bool HasJsonExceptionType(Exception ex)
+        {
+            var toProcess = new Queue<Exception>();
+            toProcess.Enqueue(ex);
+            int count = 0;
+
+            while (toProcess.Count > 0 && count < 50)
+            {
+                var current = toProcess.Dequeue();
+                if (current == null) continue;
+
+                count++;
+
+                if (current is Newtonsoft.Json.JsonReaderException ||
+                    current is System.Text.Json.JsonException)
+                {
+                    return true;
+                }
+
+                if (current is AggregateException aggregateEx)
+                {
+                    foreach (var innerEx in aggregateEx.InnerExceptions)
+                    {
+                        if (innerEx != null && count < 50)
+                        {
+                            toProcess.Enqueue(innerEx);
+                        }
+                    }
+                }
+                else if (current.InnerException != null)
+                {
+                    toProcess.Enqueue(current.InnerException);
+                }
+            }
+
+            return false;
+        }
+
         private IList<string> GetExceptionMessages(Exception ex)
         {
             var messages = new List<string>();