Fix parser comparison mismatches (#4273)

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)
+                // Error at parse-time for:
-                // At runtime (expression evaluation), empty image = no container
+                //   1. container: {image: 'docker://'}
-                if (isEarlyValidation)
+                //   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<String, JobContainer>(networkAlias, container));
             }
 

src/Sdk/WorkflowParser/workflow-v1.0.json

@@ -2589,7 +2589,7 @@
       "mapping": {
         "properties": {
           "image": {
-            "type": "non-empty-string",
+            "type": "string",
             "description": "Use `jobs.<job_id>.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"
       ]
     },

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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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<LegacyExpressions.IFunctionInfo>();
+
+                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")]