Compare updated workflow parser for ActionManifestManager #4111

ericsciple · 2025-11-15T07:38:23Z

Follow-up to:

Compare updated template evaluator #4092

ericsciple · 2025-11-17T22:47:37Z

src/Runner.Common/Constants.cs

                public static readonly string SnapshotPreflightHostedRunnerCheck = "actions_snapshot_preflight_hosted_runner_check";
                public static readonly string SnapshotPreflightImageGenPoolCheck = "actions_snapshot_preflight_image_gen_pool_check";
-                public static readonly string CompareTemplateEvaluator = "actions_runner_compare_template_evaluator";
+                public static readonly string CompareWorkflowParser = "actions_runner_compare_workflow_parser";


Renamed to make it more generic. The action manifest manager also does YAML parsing

do we ever enable/use the actions_runner_compare_template_evaluator ff, or it's never enable, so the rename is safe.

Exactly, it doesn't exist yet

ericsciple · 2025-11-17T22:48:55Z

src/Runner.Worker/ActionManager.cs

                if (File.Exists(manifestFile) || File.Exists(manifestFileYaml))
                {
-                    var manifestManager = HostContext.GetService<IActionManifestManager>();
+                    var manifestManager = HostContext.GetService<IActionManifestManagerWrapper>();


Now uses a wrapper class, which performs the comparison depending on the feature flag state

ericsciple · 2025-11-17T22:49:15Z

src/Runner.Worker/ActionManifestManager.cs

-using Pipelines = GitHub.DistributedTask.Pipelines;
+using GitHub.Runner.Common;
+using GitHub.Runner.Sdk;
+using GitHub.Actions.WorkflowParser;


Switched to the new namespace

ericsciple · 2025-11-17T22:49:55Z

src/Runner.Worker/ActionManifestManager.cs

    public interface IActionManifestManager : IRunnerService
    {
-        ActionDefinitionData Load(IExecutionContext executionContext, string manifestFile);
+        public ActionDefinitionDataNew Load(IExecutionContext executionContext, string manifestFile);


Since this struct contains tokens, I had to return a new struct. The wrapper class ActionManifestManagerWrapper handles comparison.

ericsciple · 2025-11-17T22:53:07Z

src/Runner.Worker/ActionManifestManager.cs

                try
                {
-                    token = TemplateEvaluator.Evaluate(templateContext, "outputs", token, 0, null, omitHeader: true);
+                    token = TemplateEvaluator.Evaluate(templateContext, "outputs", token, 0, null);


omitHeader: true isn't needed an option and isn't needed in new evaluator.

The new evaluator doesn't add a header trace line.

ericsciple · 2025-11-17T22:56:40Z

src/Runner.Worker/ActionManifestManager.cs

+        public ActionDefinitionDataNew Load(IExecutionContext executionContext, string manifestFile);

-        DictionaryContextData EvaluateCompositeOutputs(IExecutionContext executionContext, TemplateToken token, IDictionary<string, PipelineContextData> extraExpressionValues);
+        DictionaryExpressionData EvaluateCompositeOutputs(IExecutionContext executionContext, TemplateToken token, IDictionary<string, ExpressionData> extraExpressionValues);


Most of the changes throughout this file is just switching to the new type names

ericsciple · 2025-11-17T22:57:37Z

src/Runner.Worker/ActionManifestManager.cs

+                // Convert old PipelineContextData to new ExpressionData
+                var json = StringUtil.ConvertToJson(pair.Value, Newtonsoft.Json.Formatting.None);
+                var newValue = StringUtil.ConvertFromJson<GitHub.Actions.Expressions.Data.ExpressionData>(json);
+                result.ExpressionValues[pair.Key] = newValue;


executionContext.ExpressionValues is the old type. So we need to convert to the new types

ericsciple · 2025-11-17T22:58:58Z

src/Runner.Worker/ActionManifestManager.cs

-                result.ExpressionFunctions.Add(item);
+                GitHub.Actions.Expressions.IFunctionInfo newFunc = func.Name switch
+                {
+                    "always" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewAlwaysFunction>(func.Name, func.MinParameters, func.MaxParameters),


These new functions were defined with the last PR:

Compare updated template evaluator #4092

ericsciple · 2025-11-17T22:59:58Z

src/Runner.Worker/ActionManifestManager.cs

        }
    }
+
+    public sealed class ActionDefinitionDataNew


These are temporary. The wrapper class ActionManifestManagerWrapper performs the conversion.

ericsciple · 2025-11-17T23:01:05Z

src/Runner.Worker/ActionManifestManagerLegacy.cs

@@ -0,0 +1,546 @@
+using System;


Existing code

ericsciple · 2025-11-17T23:02:30Z

src/Runner.Worker/ActionManifestManagerLegacy.cs

+namespace GitHub.Runner.Worker
+{
+    [ServiceLocator(Default = typeof(ActionManifestManagerLegacy))]
+    public interface IActionManifestManagerLegacy : IRunnerService


Renamed to Legacy

ericsciple · 2025-11-17T23:04:37Z

src/Runner.Worker/ExecutionContext.cs

        {
            // Create wrapper?
-            if ((context.Global.Variables.GetBoolean(Constants.Runner.Features.CompareTemplateEvaluator) ?? false) || StringUtil.ConvertToBoolean(Environment.GetEnvironmentVariable("ACTIONS_RUNNER_COMPARE_TEMPLATE_EVALUATOR")))
+            if ((context.Global.Variables.GetBoolean(Constants.Runner.Features.CompareWorkflowParser) ?? false) || StringUtil.ConvertToBoolean(Environment.GetEnvironmentVariable("ACTIONS_RUNNER_COMPARE_WORKFLOW_PARSER")))


Again, I renamed this flag to be more generic since the action manifest manager also performs YAML parsing, not just template evaluation

ericsciple · 2025-11-17T23:05:49Z

src/Sdk/Sdk.csproj

    </PropertyGroup>

+    <ItemGroup>
+        <InternalsVisibleTo Include="Test" />


I am trying to remember why I needed this. I think it was for a template token extension method to assert the type

ericsciple · 2025-11-17T23:06:24Z

src/Test/L0/Worker/ActionManifestManagerL0.cs

                Assert.Equal(ActionExecutionType.Container, result.Execution.ExecutionType);

-                var containerAction = result.Execution as ContainerActionExecutionData;
+                var containerAction = result.Execution as ContainerActionExecutionDataNew;


Tests pass!

ericsciple · 2025-11-17T23:16:21Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+            TemplateToken token,
+            IDictionary<string, PipelineContextData> extraExpressionValues)
+        {
+            return EvaluateAndCompare(


Helper method checks the feature flag and conditionally calls the new implementation

Copilot

Pull Request Overview

This PR introduces a comparison mechanism for an updated workflow parser in ActionManifestManager. The implementation creates a wrapper pattern to compare outputs between the legacy implementation and a new workflow parser-based implementation, enabling safe rollout via a feature flag.

Key changes:

Extracted existing ActionManifestManager logic into ActionManifestManagerLegacy
Updated ActionManifestManager to use new GitHub.Actions.WorkflowParser libraries
Created ActionManifestManagerWrapper to compare both implementations and record telemetry on mismatches

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 37 comments.

Show a summary per file

File	Description
ActionManifestManagerWrapper.cs	New wrapper class that invokes both legacy and new implementations, compares results, and logs mismatches
ActionManifestManagerLegacy.cs	Legacy implementation extracted from ActionManifestManager
ActionManifestManager.cs	Updated to use new workflow parser libraries (GitHub.Actions.WorkflowParser, GitHub.Actions.Expressions)
ActionManifestManagerLegacyL0.cs	Test file duplicated from ActionManifestManagerL0 for legacy implementation
ActionManifestManagerL0.cs	Updated tests to use new types and expression data classes
ActionRunnerL0.cs, ActionManagerL0.cs	Test setup updated to initialize all three manifest manager implementations
ContainerActionHandler.cs, CompositeActionHandler.cs, ActionRunner.cs, ActionManager.cs	Updated to use IActionManifestManagerWrapper instead of IActionManifestManager
GlobalContext.cs	Added HasActionManifestMismatch flag for telemetry
ExecutionContext.cs, Constants.cs	Renamed feature flag from CompareTemplateEvaluator to CompareWorkflowParser
Sdk.csproj	Added InternalsVisibleTo for Test assembly

Copilot · 2025-11-17T23:20:45Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+            return messages;
+        }
+    }
+}


The ActionManifestManagerWrapper class lacks test coverage. This wrapper contains critical comparison logic that should be tested to ensure correct behavior of the mismatch detection, result comparison methods, and conversion helpers. Consider adding ActionManifestManagerWrapperL0.cs with tests covering the comparison scenarios.

Copilot · 2025-11-17T23:20:45Z

src/Runner.Worker/ActionManifestManager.cs

                    maxBytes: 10 * 1024 * 1024),
                Schema = _actionManifestSchema,
-                TraceWriter = executionContext.ToTemplateTraceWriter(),
+                // TODO: Switch to real tracewriter for cutover


This TODO comment suggests incomplete implementation. The EmptyTraceWriter may cause debugging difficulties during the comparison phase. Consider either implementing the real trace writer before merging or creating a tracking issue and referencing it in the comment (e.g., 'TODO (#issue-number): Switch to real tracewriter for cutover').

Suggested change

// TODO: Switch to real tracewriter for cutover

// TODO (#12345): Switch to real tracewriter for cutover

Copilot · 2025-11-17T23:20:46Z

src/Runner.Worker/ActionManifestManager.cs

+                GitHub.Actions.Expressions.IFunctionInfo newFunc = func.Name switch
+                {
+                    "always" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewAlwaysFunction>(func.Name, func.MinParameters, func.MaxParameters),
+                    "cancelled" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewCancelledFunction>(func.Name, func.MinParameters, func.MaxParameters),
+                    "failure" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewFailureFunction>(func.Name, func.MinParameters, func.MaxParameters),
+                    "success" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewSuccessFunction>(func.Name, func.MinParameters, func.MaxParameters),
+                    "hashFiles" => new GitHub.Actions.Expressions.FunctionInfo<Expressions.NewHashFilesFunction>(func.Name, func.MinParameters, func.MaxParameters),
+                    _ => throw new NotSupportedException($"Expression function '{func.Name}' is not supported in ActionManifestManager")
+                };


This function conversion logic is duplicated in PipelineTemplateEvaluatorWrapper.cs (lines 326-334). Consider extracting this into a shared helper method to reduce code duplication and ensure consistency between the two conversion points.

src/Runner.Worker/ActionManifestManager.cs

Copilot · 2025-11-17T23:20:46Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+                    foreach (var innerEx in aggregateEx.InnerExceptions)
+                    {
+                        if (innerEx != null && count < 50)
+                        {
+                            toProcess.Enqueue(innerEx);
+                        }
+                    }


This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot · 2025-11-17T23:20:54Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+            if (legacyDict is Dictionary<string, string> legacyTypedDict && newDict is Dictionary<string, string> newTypedDict)
+            {
+                if (!object.Equals(legacyTypedDict.Comparer, newTypedDict.Comparer))
+                {
+                    trace.Info($"CompareDictionaries mismatch - {fieldName} - different comparers (legacy={legacyTypedDict.Comparer.GetType().Name}, new={newTypedDict.Comparer.GetType().Name})");
+                    return false;
+                }


These 'if' statements can be combined.

Suggested change

if (legacyDict is Dictionary<string, string> legacyTypedDict && newDict is Dictionary<string, string> newTypedDict)

{

if (!object.Equals(legacyTypedDict.Comparer, newTypedDict.Comparer))

{

trace.Info($"CompareDictionaries mismatch - {fieldName} - different comparers (legacy={legacyTypedDict.Comparer.GetType().Name}, new={newTypedDict.Comparer.GetType().Name})");

return false;

}

if (legacyDict is Dictionary<string, string> legacyTypedDict && newDict is Dictionary<string, string> newTypedDict

&& !object.Equals(legacyTypedDict.Comparer, newTypedDict.Comparer))

{

trace.Info($"CompareDictionaries mismatch - {fieldName} - different comparers (legacy={legacyTypedDict.Comparer.GetType().Name}, new={newTypedDict.Comparer.GetType().Name})");

return false;

Copilot · 2025-11-17T23:20:55Z

src/Runner.Worker/ActionManifestManagerLegacy.cs

+                }
+            }
+            catch (Exception ex)
+            {


Generic catch clause.

Suggested change

{

{

// Rethrow critical exceptions

if (ex is OutOfMemoryException || ex is StackOverflowException || ex is ThreadAbortException)

{

throw;

}

Copilot · 2025-11-17T23:20:55Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+            catch (Exception ex)
+            {
+                legacyException = ex;
+            }


Generic catch clause.

Copilot · 2025-11-17T23:20:55Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+            catch (Exception ex)
+            {
+                trace.Info($"Comparison failed: {ex.Message}");
+                RecordComparisonError(context, $"{methodName}: {ex.Message}");
+            }


Generic catch clause.

Copilot · 2025-11-17T23:20:55Z

src/Runner.Worker/ActionManifestManagerWrapper.cs

+                catch (Exception ex)
+                {
+                    newException = ex;
+                }


Generic catch clause.

ericsciple force-pushed the users/ericsciple/25-11-manifest branch from 92471cf to 86dcd9a Compare November 15, 2025 07:45

ericsciple changed the title ~~Users/ericsciple/25 11 manifest~~ Compare updated workflow parser for ActionManifestManager Nov 15, 2025

ericsciple commented Nov 17, 2025

View reviewed changes

src/Runner.Worker/ActionManifestManagerLegacy.cs

@@ -0,0 +1,546 @@

using System;

Copy link

Collaborator Author

ericsciple Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing code

ericsciple commented Nov 17, 2025

View reviewed changes

ericsciple marked this pull request as ready for review November 17, 2025 23:13

ericsciple requested a review from a team as a code owner November 17, 2025 23:13

Copilot AI review requested due to automatic review settings November 17, 2025 23:13

Copilot started reviewing on behalf of ericsciple November 17, 2025 23:14 View session

Copilot finished reviewing on behalf of ericsciple November 17, 2025 23:15

ericsciple commented Nov 17, 2025

View reviewed changes

Copilot AI reviewed Nov 17, 2025

View reviewed changes

TingluoHuang approved these changes Nov 18, 2025

View reviewed changes

ericsciple enabled auto-merge (squash) November 18, 2025 01:11

Compare updated workflow parser for ActionManifestManager

d5be076

ericsciple force-pushed the users/ericsciple/25-11-manifest branch from 86dcd9a to d5be076 Compare November 18, 2025 01:11

ericsciple merged commit a54f380 into main Nov 18, 2025
10 checks passed

ericsciple deleted the users/ericsciple/25-11-manifest branch November 18, 2025 01:15

github-actions bot mentioned this pull request Nov 20, 2025

Update GH Runner from 2.329.0 to 2.330.0 PasseiDireto/gh-runner#123

Open

	// TODO: Switch to real tracewriter for cutover
	// TODO (#12345): Switch to real tracewriter for cutover

-            {
+            {
+                // Rethrow critical exceptions
+                if (ex is OutOfMemoryException || ex is StackOverflowException || ex is ThreadAbortException)
+                {
+                    throw;
+                }

Compare updated workflow parser for ActionManifestManager #4111

Compare updated workflow parser for ActionManifestManager #4111

Uh oh!

Conversation

ericsciple commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsciple Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

ericsciple commented Nov 15, 2025 •

edited

Loading

ericsciple Nov 17, 2025 •

edited

Loading

ericsciple Nov 17, 2025 •

edited

Loading

ericsciple Nov 17, 2025 •

edited

Loading

ericsciple Nov 17, 2025 •

edited

Loading

ericsciple Nov 17, 2025 •

edited

Loading