From 04ee71a5d13aed17d5938f44600d225996d995de Mon Sep 17 00:00:00 2001 From: Salman Muin Kayser Chishti Date: Fri, 10 Apr 2026 14:04:28 +0000 Subject: [PATCH] fix: remove stale flag gate, validate repo format, add edge case tests - Remove AddCheckRunIdToJobContext flag gate from job context hydration. The flag is permanently force-true server-side (acquirejobhandler.go:1658). The server controls what to send via its own feature flags. - Add owner/repo format validation to DeriveWorkflowRefComponents() - Add tests for PR merge refs, tag refs, invalid repo format, and .github repo names - Update check_run_id flag-disabled test to reflect always-copy behavior --- src/Runner.Worker/ExecutionContext.cs | 19 ++++------ src/Runner.Worker/JobContext.cs | 14 ++++++- src/Test/L0/Worker/ExecutionContextL0.cs | 48 +++++++++--------------- src/Test/L0/Worker/JobContextL0.cs | 34 +++++++++++++++++ 4 files changed, 72 insertions(+), 43 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 28c27ddc0..6d95fd81c 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -892,21 +892,18 @@ namespace GitHub.Runner.Worker Trace.Info("Initializing Job context"); var jobContext = new JobContext(); - if (Global.Variables.GetBoolean(Constants.Runner.Features.AddCheckRunIdToJobContext) ?? false) + ExpressionValues.TryGetValue("job", out var jobDictionary); + if (jobDictionary != null) { - ExpressionValues.TryGetValue("job", out var jobDictionary); - if (jobDictionary != null) + foreach (var pair in jobDictionary.AssertDictionary("job")) { - foreach (var pair in jobDictionary.AssertDictionary("job")) - { - jobContext[pair.Key] = pair.Value; - } + jobContext[pair.Key] = pair.Value; } - - // Derive workflow_repository and workflow_file_path from workflow_ref - // if the server sent workflow_ref but not the decomposed fields - jobContext.DeriveWorkflowRefComponents(); } + + // Derive workflow_repository and workflow_file_path from workflow_ref + // if the server sent workflow_ref but not the decomposed fields + jobContext.DeriveWorkflowRefComponents(); ExpressionValues["job"] = jobContext; Trace.Info("Initialize GitHub context"); diff --git a/src/Runner.Worker/JobContext.cs b/src/Runner.Worker/JobContext.cs index beff55d77..e5b9797c1 100644 --- a/src/Runner.Worker/JobContext.cs +++ b/src/Runner.Worker/JobContext.cs @@ -172,14 +172,24 @@ namespace GitHub.Runner.Worker return; } + var repo = pathPart.Substring(0, markerIndex); + var filePath = pathPart.Substring(markerIndex + 1); // skip leading '/' + + // Validate repo is owner/repo format (must have at least one slash with non-empty segments) + var slashIndex = repo.IndexOf('/'); + if (slashIndex <= 0 || slashIndex >= repo.Length - 1) + { + return; + } + if (WorkflowRepository == null || WorkflowRepository == "") { - WorkflowRepository = pathPart.Substring(0, markerIndex); + WorkflowRepository = repo; } if (WorkflowFilePath == null || WorkflowFilePath == "") { - WorkflowFilePath = pathPart.Substring(markerIndex + 1); // skip leading '/' + WorkflowFilePath = filePath; } } } diff --git a/src/Test/L0/Worker/ExecutionContextL0.cs b/src/Test/L0/Worker/ExecutionContextL0.cs index e72e8f9d3..629150f6a 100644 --- a/src/Test/L0/Worker/ExecutionContextL0.cs +++ b/src/Test/L0/Worker/ExecutionContextL0.cs @@ -1203,19 +1203,19 @@ namespace GitHub.Runner.Common.Tests.Worker } } - // TODO: this test can be deleted when `AddCheckRunIdToJobContext` is fully rolled out + // AddCheckRunIdToJobContext is now permanently enabled server-side (hardcoded to "true" + // in acquirejobhandler.go). The runner always copies ContextData["job"] entries, so the + // flag-disabled test is no longer applicable. Replaced with a test that verifies + // check_run_id is always hydrated regardless of the flag value. [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void InitializeJob_HydratesJobContextWithCheckRunId_FeatureFlagDisabled() + public void InitializeJob_HydratesJobContextWithCheckRunId_AlwaysCopied() { using (TestHostContext hc = CreateTestContext()) { - // Arrange: Create a job request message and make sure the feature flag is disabled - var variables = new Dictionary() - { - [Constants.Runner.Features.AddCheckRunIdToJobContext] = new VariableValue("false"), - }; + // Arrange: No feature flag set at all + var variables = new Dictionary(); var jobRequest = new Pipelines.AgentJobRequestMessage(new TaskOrchestrationPlanReference(), new TimelineReference(), Guid.NewGuid(), "some job name", "some job name", null, null, null, variables, new List(), new Pipelines.JobResources(), new Pipelines.ContextData.DictionaryContextData(), new Pipelines.WorkspaceOptions(), new List(), null, null, null, null, null); var pagingLogger = new Moq.Mock(); var jobServerQueue = new Moq.Mock(); @@ -1233,9 +1233,9 @@ namespace GitHub.Runner.Common.Tests.Worker // Act ec.InitializeJob(jobRequest, CancellationToken.None); - // Assert + // Assert: check_run_id is always copied regardless of flag Assert.NotNull(ec.JobContext); - Assert.Null(ec.JobContext.CheckRunId); // with the feature flag disabled we should not have added a CheckRunId to the JobContext + Assert.Equal(123456, ec.JobContext.CheckRunId); } } @@ -1246,11 +1246,8 @@ namespace GitHub.Runner.Common.Tests.Worker { using (TestHostContext hc = CreateTestContext()) { - // Arrange: Create a job request message with the feature flag enabled - var variables = new Dictionary() - { - [Constants.Runner.Features.AddCheckRunIdToJobContext] = new VariableValue("true"), - }; + // Arrange + var variables = new Dictionary(); var jobRequest = new Pipelines.AgentJobRequestMessage(new TaskOrchestrationPlanReference(), new TimelineReference(), Guid.NewGuid(), "some job name", "some job name", null, null, null, variables, new List(), new Pipelines.JobResources(), new Pipelines.ContextData.DictionaryContextData(), new Pipelines.WorkspaceOptions(), new List(), null, null, null, null, null); var pagingLogger = new Moq.Mock(); var jobServerQueue = new Moq.Mock(); @@ -1283,15 +1280,12 @@ namespace GitHub.Runner.Common.Tests.Worker [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] - public void InitializeJob_WorkflowIdentityNotSet_WhenFeatureFlagDisabled() + public void InitializeJob_WorkflowIdentityNotSet_WhenServerSendsNoData() { using (TestHostContext hc = CreateTestContext()) { - // Arrange: Create a job request message with the feature flag disabled - var variables = new Dictionary() - { - [Constants.Runner.Features.AddCheckRunIdToJobContext] = new VariableValue("false"), - }; + // Arrange: Server sends no workflow identity in job context + var variables = new Dictionary(); var jobRequest = new Pipelines.AgentJobRequestMessage(new TaskOrchestrationPlanReference(), new TimelineReference(), Guid.NewGuid(), "some job name", "some job name", null, null, null, variables, new List(), new Pipelines.JobResources(), new Pipelines.ContextData.DictionaryContextData(), new Pipelines.WorkspaceOptions(), new List(), null, null, null, null, null); var pagingLogger = new Moq.Mock(); var jobServerQueue = new Moq.Mock(); @@ -1300,17 +1294,14 @@ namespace GitHub.Runner.Common.Tests.Worker var ec = new Runner.Worker.ExecutionContext(); ec.Initialize(hc); - // Arrange: Add workflow identity to the job context - var jobContext = new Pipelines.ContextData.DictionaryContextData(); - jobContext["workflow_ref"] = new StringContextData("my-org/my-repo/.github/workflows/reusable.yml@refs/heads/main"); - jobContext["workflow_sha"] = new StringContextData("abc123def456"); - jobRequest.ContextData["job"] = jobContext; + // Arrange: empty job context + jobRequest.ContextData["job"] = new Pipelines.ContextData.DictionaryContextData(); jobRequest.ContextData["github"] = new Pipelines.ContextData.DictionaryContextData(); // Act ec.InitializeJob(jobRequest, CancellationToken.None); - // Assert: properties should not be populated when flag is off + // Assert: no workflow identity Assert.NotNull(ec.JobContext); Assert.Null(ec.JobContext.WorkflowRef); Assert.Null(ec.JobContext.WorkflowSha); @@ -1327,10 +1318,7 @@ namespace GitHub.Runner.Common.Tests.Worker using (TestHostContext hc = CreateTestContext()) { // Arrange: Server sends all 4 fields explicitly - var variables = new Dictionary() - { - [Constants.Runner.Features.AddCheckRunIdToJobContext] = new VariableValue("true"), - }; + var variables = new Dictionary(); var jobRequest = new Pipelines.AgentJobRequestMessage(new TaskOrchestrationPlanReference(), new TimelineReference(), Guid.NewGuid(), "some job name", "some job name", null, null, null, variables, new List(), new Pipelines.JobResources(), new Pipelines.ContextData.DictionaryContextData(), new Pipelines.WorkspaceOptions(), new List(), null, null, null, null, null); var pagingLogger = new Moq.Mock(); var jobServerQueue = new Moq.Mock(); diff --git a/src/Test/L0/Worker/JobContextL0.cs b/src/Test/L0/Worker/JobContextL0.cs index f1b1be6ec..9c0427029 100644 --- a/src/Test/L0/Worker/JobContextL0.cs +++ b/src/Test/L0/Worker/JobContextL0.cs @@ -219,5 +219,39 @@ namespace GitHub.Runner.Common.Tests.Worker Assert.Equal("my-org/my-repo", ctx.WorkflowRepository); Assert.Equal(".github/workflows/deploy.yml", ctx.WorkflowFilePath); } + + [Fact] + public void DeriveWorkflowRefComponents_HandlesPRMergeRef() + { + var ctx = new JobContext(); + ctx.WorkflowRef = "my-org/my-repo/.github/workflows/ci.yml@refs/pull/42/merge"; + ctx.DeriveWorkflowRefComponents(); + + Assert.Equal("my-org/my-repo", ctx.WorkflowRepository); + Assert.Equal(".github/workflows/ci.yml", ctx.WorkflowFilePath); + } + + [Fact] + public void DeriveWorkflowRefComponents_HandlesTagRef() + { + var ctx = new JobContext(); + ctx.WorkflowRef = "my-org/my-repo/.github/workflows/release.yml@refs/tags/v1.0.0"; + ctx.DeriveWorkflowRefComponents(); + + Assert.Equal("my-org/my-repo", ctx.WorkflowRepository); + Assert.Equal(".github/workflows/release.yml", ctx.WorkflowFilePath); + } + + [Fact] + public void DeriveWorkflowRefComponents_RejectsInvalidRepoFormat() + { + // No owner/repo slash — should no-op + var ctx = new JobContext(); + ctx.WorkflowRef = "noslash.github/workflows/ci.yml@refs/heads/main"; + ctx.DeriveWorkflowRefComponents(); + + Assert.Null(ctx.WorkflowRepository); + Assert.Null(ctx.WorkflowFilePath); + } } }