diff --git a/pkg/constants/constants_test.go b/pkg/constants/constants_test.go index 824b0f4885f..32a30b3a5a2 100644 --- a/pkg/constants/constants_test.go +++ b/pkg/constants/constants_test.go @@ -233,7 +233,14 @@ func TestConstantValues(t *testing.T) { {"AgentJobName", string(AgentJobName), "agent"}, {"ActivationJobName", string(ActivationJobName), "activation"}, {"PreActivationJobName", string(PreActivationJobName), "pre_activation"}, + {"PreActivationHyphenJobName", string(PreActivationHyphenJobName), "pre-activation"}, {"DetectionJobName", string(DetectionJobName), "detection"}, + {"SafeOutputsJobName", string(SafeOutputsJobName), "safe_outputs"}, + {"SafeOutputsHyphenJobName", string(SafeOutputsHyphenJobName), "safe-outputs"}, + {"UploadAssetsJobName", string(UploadAssetsJobName), "upload_assets"}, + {"UploadCodeScanningJobName", string(UploadCodeScanningJobName), "upload_code_scanning_sarif"}, + {"ConclusionJobName", string(ConclusionJobName), "conclusion"}, + {"UnlockJobName", string(UnlockJobName), "unlock"}, {"SafeOutputArtifactName", SafeOutputArtifactName, "safe-output"}, {"AgentOutputArtifactName", AgentOutputArtifactName, "agent-output"}, {"SafeOutputItemsArtifactName", SafeOutputItemsArtifactName, "safe-outputs-items"}, @@ -262,6 +269,28 @@ func TestConstantValues(t *testing.T) { } } +func TestKnownBuiltInJobNamesContainsAllKnownJobs(t *testing.T) { + knownJobs := []string{ + string(AgentJobName), + string(ActivationJobName), + string(PreActivationJobName), + string(PreActivationHyphenJobName), + string(DetectionJobName), + string(SafeOutputsJobName), + string(SafeOutputsHyphenJobName), + string(UploadAssetsJobName), + string(UploadCodeScanningJobName), + string(ConclusionJobName), + string(UnlockJobName), + } + + for _, jobName := range knownJobs { + if _, ok := KnownBuiltInJobNames[jobName]; !ok { + t.Errorf("KnownBuiltInJobNames missing %q", jobName) + } + } +} + func TestModelNameConstants(t *testing.T) { // Test that ModelName type works correctly tests := []struct { diff --git a/pkg/constants/job_constants.go b/pkg/constants/job_constants.go index d509cff58af..bcf39443322 100644 --- a/pkg/constants/job_constants.go +++ b/pkg/constants/job_constants.go @@ -59,13 +59,33 @@ func (m MCPServerID) String() string { const AgentJobName JobName = "agent" const ActivationJobName JobName = "activation" const PreActivationJobName JobName = "pre_activation" +const PreActivationHyphenJobName JobName = "pre-activation" const DetectionJobName JobName = "detection" const SafeOutputsJobName JobName = "safe_outputs" +const SafeOutputsHyphenJobName JobName = "safe-outputs" const UploadAssetsJobName JobName = "upload_assets" const UploadCodeScanningJobName JobName = "upload_code_scanning_sarif" const ConclusionJobName JobName = "conclusion" const UnlockJobName JobName = "unlock" +// KnownBuiltInJobNames contains all known built-in workflow job names (including aliases). +// It is used for O(1) membership checks when validating or filtering user-defined job +// names to avoid collisions with framework-generated jobs. For example, workflow code +// can check this map before treating a frontmatter jobs. entry as a custom job. +var KnownBuiltInJobNames = map[string]struct{}{ + string(AgentJobName): {}, + string(ActivationJobName): {}, + string(PreActivationJobName): {}, + string(PreActivationHyphenJobName): {}, + string(DetectionJobName): {}, + string(SafeOutputsJobName): {}, + string(SafeOutputsHyphenJobName): {}, + string(UploadAssetsJobName): {}, + string(UploadCodeScanningJobName): {}, + string(ConclusionJobName): {}, + string(UnlockJobName): {}, +} + // Artifact name constants const SafeOutputArtifactName = "safe-output" const AgentOutputArtifactName = "agent-output" diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index 18c3a7e2f0f..5d032720ed5 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -742,6 +742,115 @@ func TestBuildMainJobWithActivation(t *testing.T) { } } +func TestBuildMainJobSkipsBuiltInJobCustomizationsFromNeeds(t *testing.T) { + tests := []struct { + name string + jobName string + forbidden string + verifyCount bool + }{ + { + name: "activation pre-steps does not duplicate activation needs", + jobName: string(constants.ActivationJobName), + forbidden: string(constants.ActivationJobName), + verifyCount: true, + }, + { + name: "agent pre-steps does not create self-cycle", + jobName: string(constants.AgentJobName), + forbidden: string(constants.AgentJobName), + }, + { + name: "safe_outputs pre-steps does not create cycle with agent", + jobName: string(constants.SafeOutputsJobName), + forbidden: string(constants.SafeOutputsJobName), + }, + { + name: "conclusion pre-steps does not create cycle with agent", + jobName: string(constants.ConclusionJobName), + forbidden: string(constants.ConclusionJobName), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + RunsOn: "runs-on: ubuntu-latest", + Permissions: "permissions:\n contents: read", + Jobs: map[string]any{ + tt.jobName: map[string]any{ + "pre-steps": []any{ + map[string]any{ + "name": "Pre-step", + "run": "echo test", + }, + }, + }, + }, + } + + job, err := compiler.buildMainJob(workflowData, true) + if err != nil { + t.Fatalf("buildMainJob() returned error: %v", err) + } + + if tt.verifyCount { + count := 0 + for _, need := range job.Needs { + if need == tt.forbidden { + count++ + } + } + if count != 1 { + t.Fatalf("Expected exactly one %q dependency, got %d (needs: %v)", tt.forbidden, count, job.Needs) + } + } else if slices.Contains(job.Needs, tt.forbidden) { + t.Fatalf("Did not expect %q in agent needs, got: %v", tt.forbidden, job.Needs) + } + }) + } +} + +func TestIsBuiltinJobName(t *testing.T) { + tests := []struct { + name string + jobName string + expected bool + }{ + {name: "pre_activation canonical", jobName: string(constants.PreActivationJobName), expected: true}, + {name: "pre-activation alias", jobName: "pre-activation", expected: true}, + {name: "activation", jobName: string(constants.ActivationJobName), expected: true}, + {name: "agent", jobName: string(constants.AgentJobName), expected: true}, + {name: "safe_outputs canonical", jobName: string(constants.SafeOutputsJobName), expected: true}, + {name: "safe-outputs alias", jobName: "safe-outputs", expected: true}, + {name: "conclusion", jobName: string(constants.ConclusionJobName), expected: true}, + {name: "detection", jobName: string(constants.DetectionJobName), expected: true}, + {name: "upload_assets", jobName: string(constants.UploadAssetsJobName), expected: true}, + {name: "upload_code_scanning_sarif", jobName: string(constants.UploadCodeScanningJobName), expected: true}, + {name: "unlock", jobName: string(constants.UnlockJobName), expected: true}, + {name: "empty string", jobName: "", expected: false}, + {name: "different casing activation", jobName: "ACTIVATION", expected: false}, + {name: "different casing agent", jobName: "Agent", expected: false}, + {name: "partial pre-activation match", jobName: "pre-activation-custom", expected: false}, + {name: "partial agent match", jobName: "agent-step", expected: false}, + {name: "custom job", jobName: "custom_job", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual := isBuiltinJobName(tt.jobName) + if actual != tt.expected { + t.Fatalf("isBuiltinJobName(%q) = %t, want %t", tt.jobName, actual, tt.expected) + } + }) + } +} + // TestBuildCustomJobsWithActivation tests building custom jobs with activation dependency func TestBuildCustomJobsWithActivation(t *testing.T) { tmpDir := testutil.TempDir(t, "custom-jobs-test") diff --git a/pkg/workflow/compiler_main_job.go b/pkg/workflow/compiler_main_job.go index 99e6b93c14d..b48ef701b5c 100644 --- a/pkg/workflow/compiler_main_job.go +++ b/pkg/workflow/compiler_main_job.go @@ -13,6 +13,11 @@ import ( var compilerMainJobLog = logger.New("workflow:compiler_main_job") +func isBuiltinJobName(jobName string) bool { + _, isBuiltIn := constants.KnownBuiltInJobNames[jobName] + return isBuiltIn +} + // buildMainJob creates the main agent job that runs the AI agent with the configured engine and tools. // This job depends on the activation job if it exists, and handles the main workflow logic. func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (*Job, error) { @@ -92,8 +97,8 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) ( // Custom jobs that depend on agent should run AFTER the agent job, not before it if data.Jobs != nil { for _, jobName := range slices.Sorted(maps.Keys(data.Jobs)) { - // Skip jobs.pre-activation (or pre_activation) as it's handled specially - if jobName == string(constants.PreActivationJobName) || jobName == "pre-activation" { + // Skip built-in jobs as they are handled separately and should not become custom dependencies. + if isBuiltinJobName(jobName) { continue } @@ -121,8 +126,8 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) ( } referencedJobs := c.getReferencedCustomJobs(contentBuilder.String(), data.Jobs) for _, jobName := range referencedJobs { - // Skip jobs.pre-activation (or pre_activation) as it's handled specially - if jobName == string(constants.PreActivationJobName) || jobName == "pre-activation" { + // Skip built-in jobs as they are handled separately and should not become custom dependencies. + if isBuiltinJobName(jobName) { continue }