Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions pkg/constants/constants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down Expand Up @@ -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 {
Expand Down
20 changes: 20 additions & 0 deletions pkg/constants/job_constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.<name> 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"
Expand Down
109 changes: 109 additions & 0 deletions pkg/workflow/compiler_jobs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
13 changes: 9 additions & 4 deletions pkg/workflow/compiler_main_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +16 to 22
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isBuiltinJobName only includes a subset of built-in job IDs. There are additional built-in jobs defined in pkg/constants/job_constants.go (e.g. detection, upload_assets, upload_code_scanning_sarif, unlock) that can also appear under jobs: as customization-only entries. If one of those appears in data.Jobs, buildMainJob will currently treat it as a custom job and may inject it into agent.needs, which can create self-dependency cycles (e.g. detection depends on agent, so adding agent -> detection creates a cycle) or reference a job that may not exist. Expand isBuiltinJobName (and its tests) to include all built-in job IDs (and any supported dash/underscore aliases), or centralize this logic by reusing an existing reserved-job predicate.

Copilot uses AI. Check for mistakes.
func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (*Job, error) {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down
Loading