Compare commits

..

1 Commits

Author SHA1 Message Date
eric sciple
90cc9f13df Fix ConvertToLegacySteps to use explicit property mapping
The JSON round-trip approach in ConvertToLegacySteps() was broken because
IStep (new parser) and ActionStep (legacy parser) have completely different
JSON structures. This caused all steps to deserialize as null, triggering
ActionManifestMismatch: Load telemetry for every composite action.

This fix replaces the JSON round-trip with explicit property mapping:
- RunStep → ActionStep with ScriptReference and script/shell/workingDirectory inputs
- ActionStep (uses) → ActionStep with RepositoryPathReference and with inputs
- Properly extracts condition string from BasicExpressionToken
- Handles docker://, ./, and owner/repo@ref reference formats

Also updates the test to verify correct conversion instead of expecting nulls.
2026-02-02 18:41:39 +00:00
6 changed files with 676 additions and 4 deletions

View File

@@ -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

View File

@@ -0,0 +1,431 @@
using GitHub.Actions.Expressions.Data;
using GitHub.Actions.WorkflowParser.ObjectTemplating.Tokens;
using GitHub.DistributedTask.WebApi;
using GitHub.Runner.Common;
using GitHub.Runner.Sdk;
using GitHub.Runner.Worker;
using LegacyContextData = GitHub.DistributedTask.Pipelines.ContextData;
using LegacyExpressions = GitHub.DistributedTask.Expressions2;
using Moq;
using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.CompilerServices;
using System.Threading;
using Xunit;
using Xunit.Abstractions;
namespace GitHub.Runner.Common.Tests.Worker
{
/// <summary>
/// Tests to reproduce ActionManifestMismatch: Load telemetry events.
/// These tests compare the legacy and new action manifest parsers to identify
/// differences in how they handle non-standard input fields like 'type' and 'options'.
/// </summary>
public sealed class ActionManifestParserComparisonL0
{
private CancellationTokenSource _ecTokenSource;
private Mock<IExecutionContext> _ec;
private TestHostContext _hc;
private readonly ITestOutputHelper _output;
public ActionManifestParserComparisonL0(ITestOutputHelper output)
{
_output = output;
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_TypeBoolean_JsonDiff()
{
try
{
// Arrange
Setup();
var legacyManager = new ActionManifestManagerLegacy();
var newManager = new ActionManifestManager();
legacyManager.Initialize(_hc);
newManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "action_with_type_boolean.yml");
// Act
var legacyResult = legacyManager.Load(_ec.Object, manifestPath);
var newResult = newManager.Load(_ec.Object, manifestPath);
// Serialize Inputs to JSON (this is how ActionManifestManagerWrapper compares)
var legacyInputsJson = legacyResult.Inputs != null ? StringUtil.ConvertToJson(legacyResult.Inputs) : "null";
var newInputsJson = newResult.Inputs != null ? StringUtil.ConvertToJson(newResult.Inputs) : "null";
// Output for debugging
_output.WriteLine("=== LEGACY PARSER INPUTS JSON ===");
_output.WriteLine(legacyInputsJson);
_output.WriteLine("");
_output.WriteLine("=== NEW PARSER INPUTS JSON ===");
_output.WriteLine(newInputsJson);
_output.WriteLine("");
// This assertion shows the mismatch
var inputsMatch = string.Equals(legacyInputsJson, newInputsJson, StringComparison.Ordinal);
_output.WriteLine($"=== INPUTS MATCH: {inputsMatch} ===");
if (!inputsMatch)
{
_output.WriteLine("MISMATCH FOUND - this reproduces ActionManifestMismatch: Load");
}
// Assert that they match (this will fail if there's a mismatch, showing the repro)
Assert.Equal(legacyInputsJson, newInputsJson);
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_TypeChoice_JsonDiff()
{
try
{
// Arrange
Setup();
var legacyManager = new ActionManifestManagerLegacy();
var newManager = new ActionManifestManager();
legacyManager.Initialize(_hc);
newManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "action_with_type_choice.yml");
// Act
var legacyResult = legacyManager.Load(_ec.Object, manifestPath);
var newResult = newManager.Load(_ec.Object, manifestPath);
// Serialize Inputs to JSON
var legacyInputsJson = legacyResult.Inputs != null ? StringUtil.ConvertToJson(legacyResult.Inputs) : "null";
var newInputsJson = newResult.Inputs != null ? StringUtil.ConvertToJson(newResult.Inputs) : "null";
// Output for debugging
_output.WriteLine("=== LEGACY PARSER INPUTS JSON ===");
_output.WriteLine(legacyInputsJson);
_output.WriteLine("");
_output.WriteLine("=== NEW PARSER INPUTS JSON ===");
_output.WriteLine(newInputsJson);
_output.WriteLine("");
// This assertion shows the mismatch
var inputsMatch = string.Equals(legacyInputsJson, newInputsJson, StringComparison.Ordinal);
_output.WriteLine($"=== INPUTS MATCH: {inputsMatch} ===");
if (!inputsMatch)
{
_output.WriteLine("MISMATCH FOUND - this reproduces ActionManifestMismatch: Load");
}
// Assert that they match (this will fail if there's a mismatch, showing the repro)
Assert.Equal(legacyInputsJson, newInputsJson);
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_StandardAction_ShouldMatch()
{
try
{
// Arrange - use a standard action.yml without custom fields
Setup();
var legacyManager = new ActionManifestManagerLegacy();
var newManager = new ActionManifestManager();
legacyManager.Initialize(_hc);
newManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "dockerfileaction.yml");
// Act
var legacyResult = legacyManager.Load(_ec.Object, manifestPath);
var newResult = newManager.Load(_ec.Object, manifestPath);
// Serialize Inputs to JSON
var legacyInputsJson = legacyResult.Inputs != null ? StringUtil.ConvertToJson(legacyResult.Inputs) : "null";
var newInputsJson = newResult.Inputs != null ? StringUtil.ConvertToJson(newResult.Inputs) : "null";
// Output for debugging
_output.WriteLine("=== LEGACY PARSER INPUTS JSON ===");
_output.WriteLine(legacyInputsJson);
_output.WriteLine("");
_output.WriteLine("=== NEW PARSER INPUTS JSON ===");
_output.WriteLine(newInputsJson);
_output.WriteLine("");
var inputsMatch = string.Equals(legacyInputsJson, newInputsJson, StringComparison.Ordinal);
_output.WriteLine($"=== INPUTS MATCH: {inputsMatch} ===");
// Standard actions should match
Assert.Equal(legacyInputsJson, newInputsJson);
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_ExplicitStrTag_LegacyParserRejectsShortForm()
{
try
{
// Arrange - Test explicit !!str YAML tag
// Legacy parser uses "tag:yaml.org,2002:string" (expects !!string)
// New parser uses "tag:yaml.org,2002:str" (expects !!str)
// This causes the LEGACY parser to FAIL because it doesn't recognize !!str
Setup();
var legacyManager = new ActionManifestManagerLegacy();
legacyManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "action_with_explicit_str_tag.yml");
// Act & Assert - Legacy parser should throw because it doesn't recognize !!str
var ex = Assert.Throws<ArgumentException>(() => legacyManager.Load(_ec.Object, manifestPath));
_output.WriteLine($"Expected exception: {ex.Message}");
_output.WriteLine("Root cause: Different string tag constants in YamlObjectReader");
_output.WriteLine(" Legacy expects: tag:yaml.org,2002:string (!!string)");
_output.WriteLine(" But YAML has: tag:yaml.org,2002:str (!!str)");
Assert.Contains("Failed to load", ex.Message);
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_ExplicitStrTag_NewParserAcceptsShortForm()
{
try
{
// Arrange - New parser should accept !!str
Setup();
var newManager = new ActionManifestManager();
newManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "action_with_explicit_str_tag.yml");
// Act - New parser should succeed
var newResult = newManager.Load(_ec.Object, manifestPath);
// Assert
Assert.NotNull(newResult);
Assert.NotNull(newResult.Inputs);
_output.WriteLine($"New parser loaded successfully with {newResult.Inputs.Count} inputs");
_output.WriteLine("This demonstrates that the new parser handles !!str correctly");
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareInputs_YamlAnchors_LegacyParserRejects()
{
try
{
// Arrange - YAML anchors are handled differently
// Legacy parser rejects anchors entirely
// New parser can optionally support them
Setup();
var legacyManager = new ActionManifestManagerLegacy();
legacyManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "action_with_yaml_anchors.yml");
// Act & Assert - Legacy parser should throw because it rejects anchors
// The InvalidOperationException from the YAML reader gets wrapped in ArgumentException
var ex = Assert.Throws<ArgumentException>(() => legacyManager.Load(_ec.Object, manifestPath));
_output.WriteLine($"Expected exception: {ex.Message}");
_output.WriteLine("Root cause: Legacy parser rejects YAML anchors");
Assert.Contains("Failed to load", ex.Message);
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void CompareCompositeAction_StepsWithExpressions_ShowsMismatch()
{
try
{
// Arrange - Test composite action with expressions that might differ
Setup();
var legacyManager = new ActionManifestManagerLegacy();
var newManager = new ActionManifestManager();
legacyManager.Initialize(_hc);
newManager.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "conditional_composite_action.yml");
// Act
var legacyResult = legacyManager.Load(_ec.Object, manifestPath);
var newResult = newManager.Load(_ec.Object, manifestPath);
// Since execution type is Composite, cast appropriately
Assert.Equal(ActionExecutionType.Composite, legacyResult.Execution.ExecutionType);
Assert.Equal(ActionExecutionType.Composite, newResult.Execution.ExecutionType);
var legacyCompositeExecution = legacyResult.Execution as CompositeActionExecutionData;
var newCompositeExecution = newResult.Execution as CompositeActionExecutionDataNew;
// Serialize and compare steps
var legacyStepsJson = legacyCompositeExecution?.Steps != null ? StringUtil.ConvertToJson(legacyCompositeExecution.Steps) : "null";
var newStepsJson = newCompositeExecution?.Steps != null ? StringUtil.ConvertToJson(newCompositeExecution.Steps) : "null";
_output.WriteLine("=== LEGACY STEPS JSON ===");
_output.WriteLine(legacyStepsJson);
_output.WriteLine("");
_output.WriteLine("=== NEW STEPS JSON ===");
_output.WriteLine(newStepsJson);
var stepsMatch = string.Equals(legacyStepsJson, newStepsJson, StringComparison.Ordinal);
_output.WriteLine($"=== STEPS MATCH: {stepsMatch} ===");
if (!stepsMatch)
{
_output.WriteLine("");
_output.WriteLine("NOTE: Steps JSON differs because legacy and new parsers use different step structures.");
_output.WriteLine("Legacy uses ActionStep with 'type', 'reference', 'contextName', 'inputs', 'condition'");
_output.WriteLine("New uses IStep with 'id', 'if', 'run', 'shell' directly");
_output.WriteLine("This is expected and handled by ConvertToLegacySteps() in ActionManifestManagerWrapper.");
}
// Also compare inputs - these should match
var legacyInputsJson = legacyResult.Inputs != null ? StringUtil.ConvertToJson(legacyResult.Inputs) : "null";
var newInputsJson = newResult.Inputs != null ? StringUtil.ConvertToJson(newResult.Inputs) : "null";
var inputsMatch = string.Equals(legacyInputsJson, newInputsJson, StringComparison.Ordinal);
_output.WriteLine($"=== INPUTS MATCH: {inputsMatch} ===");
// Inputs should still match
Assert.Equal(legacyInputsJson, newInputsJson);
// Steps will NOT match in raw form - this is expected and converted by the wrapper
Assert.False(stepsMatch, "Steps should have different structures before conversion");
}
finally
{
Teardown();
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void ConvertToLegacySteps_ProducesCorrectSteps_WithExplicitPropertyMapping()
{
try
{
// Arrange - Test that ActionManifestManagerWrapper properly converts new steps to legacy format
Setup();
// Enable comparison feature
_ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true");
var wrapper = new ActionManifestManagerWrapper();
wrapper.Initialize(_hc);
var manifestPath = Path.Combine(TestUtil.GetTestDataPath(), "conditional_composite_action.yml");
// Act - Load through the wrapper (which internally converts)
var result = wrapper.Load(_ec.Object, manifestPath);
// Assert
Assert.NotNull(result);
Assert.Equal(ActionExecutionType.Composite, result.Execution.ExecutionType);
var compositeExecution = result.Execution as CompositeActionExecutionData;
Assert.NotNull(compositeExecution);
Assert.NotNull(compositeExecution.Steps);
Assert.Equal(6, compositeExecution.Steps.Count);
// Verify steps are NOT null (this was the bug)
foreach (var step in compositeExecution.Steps)
{
Assert.NotNull(step);
}
_output.WriteLine($"Successfully loaded {compositeExecution.Steps.Count} steps");
// Verify specific step content
var firstStep = compositeExecution.Steps[0];
Assert.Equal("step1", firstStep.ContextName);
Assert.NotNull(firstStep.Reference);
Assert.IsType<GitHub.DistributedTask.Pipelines.ScriptReference>(firstStep.Reference);
_output.WriteLine("");
_output.WriteLine("=== FIX VERIFIED ===");
_output.WriteLine("ConvertToLegacySteps() now uses explicit property mapping");
_output.WriteLine("instead of JSON round-trip, correctly converting IStep to ActionStep.");
}
finally
{
Teardown();
}
}
private void Setup([CallerMemberName] string name = "")
{
_ecTokenSource?.Dispose();
_ecTokenSource = new CancellationTokenSource();
// Test host context.
_hc = new TestHostContext(this, name);
var expressionValues = new LegacyContextData.DictionaryContextData();
var expressionFunctions = new List<LegacyExpressions.IFunctionInfo>();
_ec = new Mock<IExecutionContext>();
_ec.Setup(x => x.Global)
.Returns(new GlobalContext
{
FileTable = new List<String>(),
Variables = new Variables(_hc, new Dictionary<string, VariableValue>()),
WriteDebug = true,
});
_ec.Setup(x => x.CancellationToken).Returns(_ecTokenSource.Token);
_ec.Setup(x => x.ExpressionValues).Returns(expressionValues);
_ec.Setup(x => x.ExpressionFunctions).Returns(expressionFunctions);
_ec.Setup(x => x.Write(It.IsAny<string>(), It.IsAny<string>())).Callback((string tag, string message) => { _hc.GetTrace().Info($"{tag}{message}"); });
_ec.Setup(x => x.AddIssue(It.IsAny<Issue>(), It.IsAny<ExecutionContextLogOptions>())).Callback((Issue issue, ExecutionContextLogOptions logOptions) => { _hc.GetTrace().Info($"[{issue.Type}]{logOptions.LogMessageOverride ?? issue.Message}"); });
}
private void Teardown()
{
_hc?.Dispose();
}
}
}

View File

@@ -0,0 +1,19 @@
name: 'Test Action with Explicit String Tag'
description: 'Tests parsing of explicit !!str YAML tag'
inputs:
my-input:
description: 'Input with explicit string tag'
required: false
default: !!str 'true'
another-input:
description: 'Regular input'
required: false
default: 'hello'
runs:
using: 'composite'
steps:
- name: Test
run: echo "test"
shell: bash

View File

@@ -0,0 +1,25 @@
name: 'Test Action with Type Boolean'
description: 'Tests parsing of type: boolean in inputs'
inputs:
node-version:
description: 'Node.js version to use'
required: false
default: '22.x'
enable-docker-cache:
description: 'Whether to set up Docker layer caching'
required: false
default: 'false'
type: boolean
build-command:
description: 'Command to execute for building'
required: false
default: 'pnpm build'
type: string
runs:
using: 'composite'
steps:
- name: Build
run: echo "Building"
shell: bash

View File

@@ -0,0 +1,34 @@
name: 'Test Action with Type Choice'
description: 'Tests parsing of type: choice with options in inputs'
inputs:
base-component:
required: false
type: string
default: .ocm/base-component.yaml
description: path to base-component-file
version:
required: true
type: string
description: the effective version
post-process:
type: choice
default: none
options:
- component-cli
- none
- callback
description: configures post-processing
outputs:
component-descriptor:
description: the base-component-descriptor
value: ${{ steps.base-component-descriptor.outputs.component-descriptor }}
runs:
using: composite
steps:
- name: base-component-descriptor
id: base-component-descriptor
shell: bash
run: echo "done"

View File

@@ -0,0 +1,21 @@
name: 'Test Action with YAML Anchors'
description: 'Tests parsing of YAML anchors which may cause mismatch'
inputs:
common-setting: &common
description: 'Common settings that get reused'
required: false
default: 'common-value'
setting-a:
<<: *common
default: 'value-a'
setting-b:
<<: *common
default: 'value-b'
runs:
using: 'composite'
steps:
- name: Test
run: echo "test"
shell: bash