From 8a73bccebb88715b3ce5443ec579b4e2d4dae6e4 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Mon, 2 Mar 2026 23:38:16 -0600 Subject: [PATCH] Fix parser comparison mismatches (#4273) --- .../Conversion/WorkflowTemplateConverter.cs | 54 +++- src/Sdk/WorkflowParser/workflow-v1.0.json | 4 +- .../PipelineTemplateEvaluatorWrapperL0.cs | 272 ++++++++++++++++++ 3 files changed, 317 insertions(+), 13 deletions(-) diff --git a/src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs b/src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs index 7c5764cb3..7293ce165 100644 --- a/src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs +++ b/src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs @@ -1079,7 +1079,8 @@ namespace GitHub.Actions.WorkflowParser.Conversion internal static JobContainer ConvertToJobContainer( TemplateContext context, TemplateToken value, - bool isEarlyValidation = false) + bool isEarlyValidation = false, + bool isServiceContainer = false) { var result = new JobContainer(); if (isEarlyValidation && value.Traverse().Any(x => x is ExpressionToken)) @@ -1089,11 +1090,34 @@ namespace GitHub.Actions.WorkflowParser.Conversion if (value is StringToken containerLiteral) { - if (String.IsNullOrEmpty(containerLiteral.Value)) + // Trim "docker://" + var trimmedImage = containerLiteral.Value; + var hasDockerPrefix = containerLiteral.Value.StartsWith(WorkflowTemplateConstants.DockerUriPrefix, StringComparison.Ordinal); + if (hasDockerPrefix) { + trimmedImage = trimmedImage.Substring(WorkflowTemplateConstants.DockerUriPrefix.Length); + } + + // Empty shorthand after trimming "docker://" ? + if (String.IsNullOrEmpty(trimmedImage)) + { + // Error at parse-time for: + // 1. container: 'docker://' + // 2. services.foo: '' + // 3. services.foo: 'docker://' + // + // Do not error for: + // 1. container: '' + if (isEarlyValidation && (hasDockerPrefix || isServiceContainer)) + { + context.Error(value, "Container image cannot be empty"); + } + + // Short-circuit return null; } + // Store original, trimmed further below result.Image = containerLiteral.Value; } else @@ -1152,22 +1176,30 @@ namespace GitHub.Actions.WorkflowParser.Conversion } } + // Trim "docker://" + var hadDockerPrefix = false; + if (!String.IsNullOrEmpty(result.Image) && result.Image.StartsWith(WorkflowTemplateConstants.DockerUriPrefix, StringComparison.Ordinal)) + { + hadDockerPrefix = true; + result.Image = result.Image.Substring(WorkflowTemplateConstants.DockerUriPrefix.Length); + } + if (String.IsNullOrEmpty(result.Image)) { - // Only error during early validation (parse time) - // At runtime (expression evaluation), empty image = no container - if (isEarlyValidation) + // Error at parse-time for: + // 1. container: {image: 'docker://'} + // 2. services.foo: {image: ''} + // 3. services.foo: {image: 'docker://'} + // + // Do not error for: + // 1. container: {image: ''} + if (isEarlyValidation && (hadDockerPrefix || isServiceContainer)) { context.Error(value, "Container image cannot be empty"); } return null; } - if (result.Image.StartsWith(WorkflowTemplateConstants.DockerUriPrefix, StringComparison.Ordinal)) - { - result.Image = result.Image.Substring(WorkflowTemplateConstants.DockerUriPrefix.Length); - } - return result; } @@ -1188,7 +1220,7 @@ namespace GitHub.Actions.WorkflowParser.Conversion foreach (var servicePair in servicesMapping) { var networkAlias = servicePair.Key.AssertString("services key").Value; - var container = ConvertToJobContainer(context, servicePair.Value); + var container = ConvertToJobContainer(context, servicePair.Value, isEarlyValidation, isServiceContainer: true); result.Add(new KeyValuePair(networkAlias, container)); } diff --git a/src/Sdk/WorkflowParser/workflow-v1.0.json b/src/Sdk/WorkflowParser/workflow-v1.0.json index 01601dcb5..0f4c91130 100644 --- a/src/Sdk/WorkflowParser/workflow-v1.0.json +++ b/src/Sdk/WorkflowParser/workflow-v1.0.json @@ -2589,7 +2589,7 @@ "mapping": { "properties": { "image": { - "type": "non-empty-string", + "type": "string", "description": "Use `jobs..container.image` to define the Docker image to use as the container to run the action. The value can be the Docker Hub image or a registry name." }, "options": { @@ -2634,7 +2634,7 @@ "matrix" ], "one-of": [ - "non-empty-string", + "string", "container-mapping" ] }, diff --git a/src/Test/L0/Worker/PipelineTemplateEvaluatorWrapperL0.cs b/src/Test/L0/Worker/PipelineTemplateEvaluatorWrapperL0.cs index 2caf5dba0..e6fae1fa5 100644 --- a/src/Test/L0/Worker/PipelineTemplateEvaluatorWrapperL0.cs +++ b/src/Test/L0/Worker/PipelineTemplateEvaluatorWrapperL0.cs @@ -281,6 +281,140 @@ namespace GitHub.Runner.Common.Tests.Worker } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobContainer_DockerPrefixOnly_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var token = new StringToken(null, null, null, "docker://"); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobContainer(token, contextData, functions); + + Assert.Null(result); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobContainer_DockerPrefixOnlyMapping_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var token = new MappingToken(null, null, null); + token.Add(new StringToken(null, null, null, "image"), new StringToken(null, null, null, "docker://")); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobContainer(token, contextData, functions); + + Assert.Null(result); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobContainer_EmptyImageMapping_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var token = new MappingToken(null, null, null); + token.Add(new StringToken(null, null, null, "image"), new StringToken(null, null, null, "")); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobContainer(token, contextData, functions); + + Assert.Null(result); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobContainer_ValidImage_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var token = new StringToken(null, null, null, "ubuntu:latest"); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobContainer(token, contextData, functions); + + Assert.NotNull(result); + Assert.Equal("ubuntu:latest", result.Image); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobContainer_DockerPrefixWithImage_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var token = new StringToken(null, null, null, "docker://ubuntu:latest"); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobContainer(token, contextData, functions); + + Assert.NotNull(result); + Assert.Equal("ubuntu:latest", result.Image); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] @@ -391,6 +525,144 @@ namespace GitHub.Runner.Common.Tests.Worker } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobServiceContainers_EmptyImage_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + // Build a services mapping token with one service whose image is empty string + // Similar to: services: { db: { image: '' } } + var servicesMapping = new MappingToken(null, null, null); + var serviceMapping = new MappingToken(null, null, null); + serviceMapping.Add(new StringToken(null, null, null, "image"), new StringToken(null, null, null, "")); + servicesMapping.Add(new StringToken(null, null, null, "db"), serviceMapping); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobServiceContainers(servicesMapping, contextData, functions); + + // Should get a list with one entry where the container is null (empty image = no container) + Assert.NotNull(result); + Assert.Single(result); + Assert.Equal("db", result[0].Key); + Assert.Null(result[0].Value); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobServiceContainers_DockerPrefixOnlyImage_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var servicesMapping = new MappingToken(null, null, null); + var serviceMapping = new MappingToken(null, null, null); + serviceMapping.Add(new StringToken(null, null, null, "image"), new StringToken(null, null, null, "docker://")); + servicesMapping.Add(new StringToken(null, null, null, "db"), serviceMapping); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobServiceContainers(servicesMapping, contextData, functions); + + Assert.NotNull(result); + Assert.Single(result); + Assert.Equal("db", result[0].Key); + Assert.Null(result[0].Value); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobServiceContainers_ExpressionEvalsToEmpty_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + // Simulates: services: { db: { image: ${{ condition && 'img' || '' }} } } + // where the expression evaluates to '' at runtime + var servicesMapping = new MappingToken(null, null, null); + var serviceMapping = new MappingToken(null, null, null); + serviceMapping.Add(new StringToken(null, null, null, "image"), new BasicExpressionToken(null, null, null, "''")); + servicesMapping.Add(new StringToken(null, null, null, "db"), serviceMapping); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobServiceContainers(servicesMapping, contextData, functions); + + Assert.NotNull(result); + Assert.Single(result); + Assert.Equal("db", result[0].Key); + Assert.Null(result[0].Value); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EvaluateJobServiceContainers_ValidImage_BothParsersAgree() + { + try + { + Setup(); + _ec.Object.Global.Variables.Set(Constants.Runner.Features.CompareWorkflowParser, "true"); + + var servicesMapping = new MappingToken(null, null, null); + var serviceMapping = new MappingToken(null, null, null); + serviceMapping.Add(new StringToken(null, null, null, "image"), new StringToken(null, null, null, "postgres:latest")); + servicesMapping.Add(new StringToken(null, null, null, "db"), serviceMapping); + + var wrapper = new PipelineTemplateEvaluatorWrapper(_hc, _ec.Object); + var contextData = new DictionaryContextData(); + var functions = new List(); + + var result = wrapper.EvaluateJobServiceContainers(servicesMapping, contextData, functions); + + Assert.NotNull(result); + Assert.Single(result); + Assert.Equal("db", result[0].Key); + Assert.NotNull(result[0].Value); + Assert.Equal("postgres:latest", result[0].Value.Image); + Assert.False(_ec.Object.Global.HasTemplateEvaluatorMismatch); + } + finally + { + Teardown(); + } + } + [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")]