From d9bfae9e32adc7fa67c23252ba049c3d60be3ce0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 04:19:11 +0000 Subject: [PATCH 1/5] Initial plan From 3d111092f3274840a3fd91238fa62312328a72bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 04:24:46 +0000 Subject: [PATCH 2/5] Initial investigation: understanding safe-jobs support Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/aw/actions-lock.json | 4 ++-- .github/workflows/release.lock.yml | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/aw/actions-lock.json b/.github/aw/actions-lock.json index 11344e2bc6..fe8d5f183c 100644 --- a/.github/aw/actions-lock.json +++ b/.github/aw/actions-lock.json @@ -65,7 +65,7 @@ "version": "v8.0.0", "sha": "ed597411d8f924073f98dfc5c65a23a2325f34cd" }, - "actions/setup-dotnet@v4": { + "actions/setup-dotnet@v4.3.1": { "repo": "actions/setup-dotnet", "version": "v4.3.1", "sha": "67a3573c9a986a3f9c594539f4ab511d57bb3ce9" @@ -80,7 +80,7 @@ "version": "v6.1.0", "sha": "4dc6199c7b1a012772edbd06daecab0f50c9053c" }, - "actions/setup-java@v4": { + "actions/setup-java@v4.8.0": { "repo": "actions/setup-java", "version": "v4.8.0", "sha": "c1e323688fd81a25caa38c78aa6df2d33d3e20d9" diff --git a/.github/workflows/release.lock.yml b/.github/workflows/release.lock.yml index cb56201092..923c9030bd 100644 --- a/.github/workflows/release.lock.yml +++ b/.github/workflows/release.lock.yml @@ -1359,13 +1359,13 @@ jobs: - name: Download Go modules run: go mod download - name: Generate SBOM (SPDX format) - uses: anchore/sbom-action@62ad5284b8ced813296287a0b63906cb364b73ee # v0 + uses: anchore/sbom-action@deef08a0db64bfad603422135db61477b16cef56 # v0 with: artifact-name: sbom.spdx.json format: spdx-json output-file: sbom.spdx.json - name: Generate SBOM (CycloneDX format) - uses: anchore/sbom-action@62ad5284b8ced813296287a0b63906cb364b73ee # v0 + uses: anchore/sbom-action@deef08a0db64bfad603422135db61477b16cef56 # v0 with: artifact-name: sbom.cdx.json format: cyclonedx-json @@ -1399,7 +1399,7 @@ jobs: - name: Setup Docker Buildx uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 - name: Log in to GitHub Container Registry - uses: docker/login-action@5e57cd118135c172c3672efd75eb46360885c0ef # v3 + uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 with: password: ${{ secrets.GITHUB_TOKEN }} registry: ghcr.io From b34a978b386c4115cdddd66f35abdbb519ac92d2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 04:29:16 +0000 Subject: [PATCH 3/5] Refactor safe-jobs parsing and add validation to reject top-level safe-jobs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../compiler_orchestrator_frontmatter.go | 30 ++++ pkg/workflow/data/action_pins.json | 4 +- pkg/workflow/safe_jobs.go | 25 ++- pkg/workflow/safe_jobs_test.go | 142 +++++++++--------- pkg/workflow/safe_outputs_config.go | 5 +- .../top_level_safe_jobs_validation_test.go | 102 +++++++++++++ 6 files changed, 214 insertions(+), 94 deletions(-) create mode 100644 pkg/workflow/top_level_safe_jobs_validation_test.go diff --git a/pkg/workflow/compiler_orchestrator_frontmatter.go b/pkg/workflow/compiler_orchestrator_frontmatter.go index f0fb593b18..2fe281614c 100644 --- a/pkg/workflow/compiler_orchestrator_frontmatter.go +++ b/pkg/workflow/compiler_orchestrator_frontmatter.go @@ -74,6 +74,12 @@ func (c *Compiler) parseFrontmatterSection(markdownPath string) (*frontmatterPar if !hasOnField { detectionLog.Printf("No 'on' field detected - treating as shared agentic workflow") + // Reject top-level "safe-jobs" field (only safe-outputs.jobs is supported) + if err := c.validateNoTopLevelSafeJobs(frontmatterForValidation, cleanPath); err != nil { + orchestratorFrontmatterLog.Printf("Top-level safe-jobs validation failed: %v", err) + return nil, err + } + // Validate as an included/shared workflow (uses main_workflow_schema with forbidden field checks) if err := parser.ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatterForValidation, cleanPath); err != nil { orchestratorFrontmatterLog.Printf("Shared workflow validation failed: %v", err) @@ -96,6 +102,12 @@ func (c *Compiler) parseFrontmatterSection(markdownPath string) (*frontmatterPar return nil, fmt.Errorf("no markdown content found") } + // Reject top-level "safe-jobs" field before schema validation (provides better error message) + if err := c.validateNoTopLevelSafeJobs(frontmatterForValidation, cleanPath); err != nil { + orchestratorFrontmatterLog.Printf("Top-level safe-jobs validation failed: %v", err) + return nil, err + } + // Validate main workflow frontmatter contains only expected entries orchestratorFrontmatterLog.Printf("Validating main workflow frontmatter schema") if err := parser.ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatterForValidation, cleanPath); err != nil { @@ -165,3 +177,21 @@ func (c *Compiler) copyFrontmatterWithoutInternalMarkers(frontmatter map[string] } return copy } + +// validateNoTopLevelSafeJobs validates that the frontmatter does not contain a top-level "safe-jobs" field. +// Only safe-outputs.jobs is supported. +func (c *Compiler) validateNoTopLevelSafeJobs(frontmatter map[string]any, filePath string) error { + if _, hasSafeJobs := frontmatter["safe-jobs"]; hasSafeJobs { + return fmt.Errorf("%s: top-level 'safe-jobs' field is not supported. Use 'safe-outputs.jobs' instead.\n"+ + "Change from:\n"+ + " safe-jobs:\n"+ + " my-job: ...\n"+ + "To:\n"+ + " safe-outputs:\n"+ + " jobs:\n"+ + " my-job: ...", + filePath, + ) + } + return nil +} diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json index 11344e2bc6..fe8d5f183c 100644 --- a/pkg/workflow/data/action_pins.json +++ b/pkg/workflow/data/action_pins.json @@ -65,7 +65,7 @@ "version": "v8.0.0", "sha": "ed597411d8f924073f98dfc5c65a23a2325f34cd" }, - "actions/setup-dotnet@v4": { + "actions/setup-dotnet@v4.3.1": { "repo": "actions/setup-dotnet", "version": "v4.3.1", "sha": "67a3573c9a986a3f9c594539f4ab511d57bb3ce9" @@ -80,7 +80,7 @@ "version": "v6.1.0", "sha": "4dc6199c7b1a012772edbd06daecab0f50c9053c" }, - "actions/setup-java@v4": { + "actions/setup-java@v4.8.0": { "repo": "actions/setup-java", "version": "v4.8.0", "sha": "c1e323688fd81a25caa38c78aa6df2d33d3e20d9" diff --git a/pkg/workflow/safe_jobs.go b/pkg/workflow/safe_jobs.go index 47857d6e03..97cb0fedfc 100644 --- a/pkg/workflow/safe_jobs.go +++ b/pkg/workflow/safe_jobs.go @@ -34,24 +34,18 @@ func HasSafeJobsEnabled(safeJobs map[string]*SafeJobConfig) bool { return len(safeJobs) > 0 } -// parseSafeJobsConfig parses safe-jobs configuration from a frontmatter map. -// This is an internal helper function that expects a map with a "safe-jobs" key. -// User workflows should use "safe-outputs.jobs" syntax; the top-level "safe-jobs" key is NOT supported. -func (c *Compiler) parseSafeJobsConfig(frontmatter map[string]any) map[string]*SafeJobConfig { - safeJobsSection, exists := frontmatter["safe-jobs"] - if !exists { +// parseSafeJobsConfig parses safe-jobs configuration from a jobs map. +// This function expects a map of job configurations directly (from safe-outputs.jobs). +// The top-level "safe-jobs" key is NOT supported - only "safe-outputs.jobs" is valid. +func (c *Compiler) parseSafeJobsConfig(jobsMap map[string]any) map[string]*SafeJobConfig { + if jobsMap == nil { return nil } - safeJobsMap, ok := safeJobsSection.(map[string]any) - if !ok { - return nil - } - - safeJobsLog.Printf("Parsing %d safe-jobs from frontmatter", len(safeJobsMap)) + safeJobsLog.Printf("Parsing %d safe-jobs from jobs map", len(jobsMap)) result := make(map[string]*SafeJobConfig) - for jobName, jobValue := range safeJobsMap { + for jobName, jobValue := range jobsMap { jobConfig, ok := jobValue.(map[string]any) if !ok { continue @@ -300,7 +294,7 @@ func (c *Compiler) buildSafeJobs(data *WorkflowData, threatDetectionEnabled bool } // extractSafeJobsFromFrontmatter extracts safe-jobs configuration from frontmatter. -// Only checks the safe-outputs.jobs location. The old top-level "safe-jobs" syntax is NOT supported. +// Only checks the safe-outputs.jobs location. The top-level "safe-jobs" syntax is NOT supported. func extractSafeJobsFromFrontmatter(frontmatter map[string]any) map[string]*SafeJobConfig { // Check location: safe-outputs.jobs if safeOutputs, exists := frontmatter["safe-outputs"]; exists { @@ -308,8 +302,7 @@ func extractSafeJobsFromFrontmatter(frontmatter map[string]any) map[string]*Safe if jobs, exists := safeOutputsMap["jobs"]; exists { if jobsMap, ok := jobs.(map[string]any); ok { c := &Compiler{} // Create a temporary compiler instance for parsing - frontmatterCopy := map[string]any{"safe-jobs": jobsMap} - return c.parseSafeJobsConfig(frontmatterCopy) + return c.parseSafeJobsConfig(jobsMap) } } } diff --git a/pkg/workflow/safe_jobs_test.go b/pkg/workflow/safe_jobs_test.go index 86ddda69fc..8a9793dd54 100644 --- a/pkg/workflow/safe_jobs_test.go +++ b/pkg/workflow/safe_jobs_test.go @@ -10,48 +10,46 @@ import ( func TestParseSafeJobsConfig(t *testing.T) { c := NewCompiler() - // Test parseSafeJobsConfig internal function which expects a "safe-jobs" key. + // Test parseSafeJobsConfig internal function which now expects a jobs map directly. // Note: User workflows should use "safe-outputs.jobs" syntax; this test validates // the internal parsing logic used by extractSafeJobsFromFrontmatter and safe_outputs.go. - frontmatter := map[string]any{ - "safe-jobs": map[string]any{ - "deploy": map[string]any{ - "runs-on": "ubuntu-latest", - "if": "github.event.issue.number", - "needs": []any{"task"}, - "env": map[string]any{ - "DEPLOY_ENV": "production", - }, - "permissions": map[string]any{ - "contents": "write", - "issues": "read", + jobsMap := map[string]any{ + "deploy": map[string]any{ + "runs-on": "ubuntu-latest", + "if": "github.event.issue.number", + "needs": []any{"task"}, + "env": map[string]any{ + "DEPLOY_ENV": "production", + }, + "permissions": map[string]any{ + "contents": "write", + "issues": "read", + }, + "github-token": "${{ secrets.CUSTOM_TOKEN }}", + "inputs": map[string]any{ + "environment": map[string]any{ + "description": "Target deployment environment", + "required": true, + "type": "choice", + "options": []any{"staging", "production"}, }, - "github-token": "${{ secrets.CUSTOM_TOKEN }}", - "inputs": map[string]any{ - "environment": map[string]any{ - "description": "Target deployment environment", - "required": true, - "type": "choice", - "options": []any{"staging", "production"}, - }, - "force": map[string]any{ - "description": "Force deployment even if tests fail", - "required": false, - "type": "boolean", - "default": "false", - }, + "force": map[string]any{ + "description": "Force deployment even if tests fail", + "required": false, + "type": "boolean", + "default": "false", }, - "steps": []any{ - map[string]any{ - "name": "Deploy application", - "run": "echo 'Deploying to ${{ inputs.environment }}'", - }, + }, + "steps": []any{ + map[string]any{ + "name": "Deploy application", + "run": "echo 'Deploying to ${{ inputs.environment }}'", }, }, }, } - result := c.parseSafeJobsConfig(frontmatter) + result := c.parseSafeJobsConfig(jobsMap) if result == nil { t.Fatal("Expected safe-jobs config to be parsed, got nil") @@ -680,52 +678,50 @@ func TestMergeSafeJobsFromIncludedConfigs(t *testing.T) { func TestSafeJobsInputTypes(t *testing.T) { c := NewCompiler() - frontmatter := map[string]any{ - "safe-jobs": map[string]any{ - "test-job": map[string]any{ - "runs-on": "ubuntu-latest", - "inputs": map[string]any{ - "message": map[string]any{ - "description": "String input", - "type": "string", - "default": "Hello World", - "required": true, - }, - "debug": map[string]any{ - "description": "Boolean input", - "type": "boolean", - "default": false, - "required": false, - }, - "count": map[string]any{ - "description": "Number input", - "type": "number", - "default": 100, - "required": true, - }, - "environment": map[string]any{ - "description": "Choice input", - "type": "choice", - "default": "staging", - "options": []any{"dev", "staging", "prod"}, - }, - "deploy_env": map[string]any{ - "description": "Environment input", - "type": "environment", - "required": false, - }, + jobsMap := map[string]any{ + "test-job": map[string]any{ + "runs-on": "ubuntu-latest", + "inputs": map[string]any{ + "message": map[string]any{ + "description": "String input", + "type": "string", + "default": "Hello World", + "required": true, }, - "steps": []any{ - map[string]any{ - "name": "Test step", - "run": "echo 'Testing inputs'", - }, + "debug": map[string]any{ + "description": "Boolean input", + "type": "boolean", + "default": false, + "required": false, + }, + "count": map[string]any{ + "description": "Number input", + "type": "number", + "default": 100, + "required": true, + }, + "environment": map[string]any{ + "description": "Choice input", + "type": "choice", + "default": "staging", + "options": []any{"dev", "staging", "prod"}, + }, + "deploy_env": map[string]any{ + "description": "Environment input", + "type": "environment", + "required": false, + }, + }, + "steps": []any{ + map[string]any{ + "name": "Test step", + "run": "echo 'Testing inputs'", }, }, }, } - result := c.parseSafeJobsConfig(frontmatter) + result := c.parseSafeJobsConfig(jobsMap) if result == nil { t.Fatal("Expected safe-jobs config to be parsed, got nil") diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index 9ed36dc033..e33300690e 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -360,12 +360,11 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut config.Mentions = parseMentionsConfig(mentions) } - // Handle jobs (safe-jobs moved under safe-outputs) + // Handle jobs (safe-jobs must be under safe-outputs) if jobs, exists := outputMap["jobs"]; exists { if jobsMap, ok := jobs.(map[string]any); ok { c := &Compiler{} // Create a temporary compiler instance for parsing - jobsFrontmatter := map[string]any{"safe-jobs": jobsMap} - config.Jobs = c.parseSafeJobsConfig(jobsFrontmatter) + config.Jobs = c.parseSafeJobsConfig(jobsMap) } } diff --git a/pkg/workflow/top_level_safe_jobs_validation_test.go b/pkg/workflow/top_level_safe_jobs_validation_test.go new file mode 100644 index 0000000000..f40d2fe1ce --- /dev/null +++ b/pkg/workflow/top_level_safe_jobs_validation_test.go @@ -0,0 +1,102 @@ +//go:build !integration + +package workflow + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestTopLevelSafeJobsRejected verifies that top-level "safe-jobs" field is rejected +func TestTopLevelSafeJobsRejected(t *testing.T) { + c := NewCompiler() + + // Create a test workflow markdown with top-level safe-jobs + markdown := `--- +on: + workflow_dispatch: +engine: copilot +safe-jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo "test" +--- +Test workflow with top-level safe-jobs (should be rejected) +` + + // Write to a temporary file + tmpFile := "/tmp/test-top-level-safe-jobs-validation.md" + err := os.WriteFile(tmpFile, []byte(markdown), 0644) + assert.NoError(t, err, "Should write test file") + defer os.Remove(tmpFile) + + // Try to compile - should fail with clear error message + _, err = c.ParseWorkflowFile(tmpFile) + assert.Error(t, err, "Should reject top-level safe-jobs") + assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") + assert.Contains(t, err.Error(), "safe-outputs.jobs", "Error should suggest using safe-outputs.jobs") +} + +// TestSafeOutputsJobsAccepted verifies that safe-outputs.jobs is accepted +func TestSafeOutputsJobsAccepted(t *testing.T) { + c := NewCompiler() + + // Create a test workflow markdown with safe-outputs.jobs + markdown := `--- +on: + workflow_dispatch: +engine: copilot +safe-outputs: + jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo "test" +--- +Test workflow with safe-outputs.jobs (should be accepted) +` + + // Write to a temporary file + tmpFile := "/tmp/test-safe-outputs-jobs-validation.md" + err := os.WriteFile(tmpFile, []byte(markdown), 0644) + assert.NoError(t, err, "Should write test file") + defer os.Remove(tmpFile) + + // Try to compile - should succeed (or fail for other reasons, but not safe-jobs) + _, err = c.ParseWorkflowFile(tmpFile) + // If there's an error, it shouldn't be about safe-jobs + if err != nil { + assert.NotContains(t, err.Error(), "safe-jobs", "Should not mention safe-jobs if using safe-outputs.jobs") + } +} + +// TestSharedWorkflowSafeJobsRejected verifies that shared workflows also reject top-level safe-jobs +func TestSharedWorkflowSafeJobsRejected(t *testing.T) { + c := NewCompiler() + + // Create a test shared workflow markdown with top-level safe-jobs (no 'on' field) + markdown := `--- +engine: copilot +safe-jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo "test" +--- +Shared workflow with top-level safe-jobs (should be rejected) +` + + // Write to a temporary file + tmpFile := "/tmp/test-shared-safe-jobs-validation.md" + err := os.WriteFile(tmpFile, []byte(markdown), 0644) + assert.NoError(t, err, "Should write test file") + defer os.Remove(tmpFile) + + // Try to compile - should fail with clear error message + _, err = c.ParseWorkflowFile(tmpFile) + assert.Error(t, err, "Should reject top-level safe-jobs in shared workflows") + assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") +} From 985ff29a1fcf3e6ed9fdf479c21005513b057992 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 04:33:49 +0000 Subject: [PATCH 4/5] Fix linting issues in validation code and tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_frontmatter.go | 13 +++---------- pkg/workflow/top_level_safe_jobs_validation_test.go | 11 ++++++----- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/pkg/workflow/compiler_orchestrator_frontmatter.go b/pkg/workflow/compiler_orchestrator_frontmatter.go index 2fe281614c..136b1b92b0 100644 --- a/pkg/workflow/compiler_orchestrator_frontmatter.go +++ b/pkg/workflow/compiler_orchestrator_frontmatter.go @@ -182,16 +182,9 @@ func (c *Compiler) copyFrontmatterWithoutInternalMarkers(frontmatter map[string] // Only safe-outputs.jobs is supported. func (c *Compiler) validateNoTopLevelSafeJobs(frontmatter map[string]any, filePath string) error { if _, hasSafeJobs := frontmatter["safe-jobs"]; hasSafeJobs { - return fmt.Errorf("%s: top-level 'safe-jobs' field is not supported. Use 'safe-outputs.jobs' instead.\n"+ - "Change from:\n"+ - " safe-jobs:\n"+ - " my-job: ...\n"+ - "To:\n"+ - " safe-outputs:\n"+ - " jobs:\n"+ - " my-job: ...", - filePath, - ) + msg := fmt.Sprintf("%s: top-level 'safe-jobs' field is not supported; use 'safe-outputs.jobs' instead", filePath) + suggestion := "\nChange from:\n safe-jobs:\n my-job: ...\nTo:\n safe-outputs:\n jobs:\n my-job: ..." + return fmt.Errorf("%s%s", msg, suggestion) } return nil } diff --git a/pkg/workflow/top_level_safe_jobs_validation_test.go b/pkg/workflow/top_level_safe_jobs_validation_test.go index f40d2fe1ce..b0f2a6d903 100644 --- a/pkg/workflow/top_level_safe_jobs_validation_test.go +++ b/pkg/workflow/top_level_safe_jobs_validation_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestTopLevelSafeJobsRejected verifies that top-level "safe-jobs" field is rejected @@ -30,12 +31,12 @@ Test workflow with top-level safe-jobs (should be rejected) // Write to a temporary file tmpFile := "/tmp/test-top-level-safe-jobs-validation.md" err := os.WriteFile(tmpFile, []byte(markdown), 0644) - assert.NoError(t, err, "Should write test file") + require.NoError(t, err, "Should write test file") defer os.Remove(tmpFile) // Try to compile - should fail with clear error message _, err = c.ParseWorkflowFile(tmpFile) - assert.Error(t, err, "Should reject top-level safe-jobs") + require.Error(t, err, "Should reject top-level safe-jobs") assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") assert.Contains(t, err.Error(), "safe-outputs.jobs", "Error should suggest using safe-outputs.jobs") } @@ -62,7 +63,7 @@ Test workflow with safe-outputs.jobs (should be accepted) // Write to a temporary file tmpFile := "/tmp/test-safe-outputs-jobs-validation.md" err := os.WriteFile(tmpFile, []byte(markdown), 0644) - assert.NoError(t, err, "Should write test file") + require.NoError(t, err, "Should write test file") defer os.Remove(tmpFile) // Try to compile - should succeed (or fail for other reasons, but not safe-jobs) @@ -92,11 +93,11 @@ Shared workflow with top-level safe-jobs (should be rejected) // Write to a temporary file tmpFile := "/tmp/test-shared-safe-jobs-validation.md" err := os.WriteFile(tmpFile, []byte(markdown), 0644) - assert.NoError(t, err, "Should write test file") + require.NoError(t, err, "Should write test file") defer os.Remove(tmpFile) // Try to compile - should fail with clear error message _, err = c.ParseWorkflowFile(tmpFile) - assert.Error(t, err, "Should reject top-level safe-jobs in shared workflows") + require.Error(t, err, "Should reject top-level safe-jobs in shared workflows") assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") } From feacfe285e0e0cc949cfb5b7fc7c51d7162208f4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 04:58:34 +0000 Subject: [PATCH 5/5] Remove redundant safe-jobs validation - schema validation already catches it Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../compiler_orchestrator_frontmatter.go | 23 ---- .../top_level_safe_jobs_validation_test.go | 103 ------------------ 2 files changed, 126 deletions(-) delete mode 100644 pkg/workflow/top_level_safe_jobs_validation_test.go diff --git a/pkg/workflow/compiler_orchestrator_frontmatter.go b/pkg/workflow/compiler_orchestrator_frontmatter.go index 136b1b92b0..f0fb593b18 100644 --- a/pkg/workflow/compiler_orchestrator_frontmatter.go +++ b/pkg/workflow/compiler_orchestrator_frontmatter.go @@ -74,12 +74,6 @@ func (c *Compiler) parseFrontmatterSection(markdownPath string) (*frontmatterPar if !hasOnField { detectionLog.Printf("No 'on' field detected - treating as shared agentic workflow") - // Reject top-level "safe-jobs" field (only safe-outputs.jobs is supported) - if err := c.validateNoTopLevelSafeJobs(frontmatterForValidation, cleanPath); err != nil { - orchestratorFrontmatterLog.Printf("Top-level safe-jobs validation failed: %v", err) - return nil, err - } - // Validate as an included/shared workflow (uses main_workflow_schema with forbidden field checks) if err := parser.ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatterForValidation, cleanPath); err != nil { orchestratorFrontmatterLog.Printf("Shared workflow validation failed: %v", err) @@ -102,12 +96,6 @@ func (c *Compiler) parseFrontmatterSection(markdownPath string) (*frontmatterPar return nil, fmt.Errorf("no markdown content found") } - // Reject top-level "safe-jobs" field before schema validation (provides better error message) - if err := c.validateNoTopLevelSafeJobs(frontmatterForValidation, cleanPath); err != nil { - orchestratorFrontmatterLog.Printf("Top-level safe-jobs validation failed: %v", err) - return nil, err - } - // Validate main workflow frontmatter contains only expected entries orchestratorFrontmatterLog.Printf("Validating main workflow frontmatter schema") if err := parser.ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatterForValidation, cleanPath); err != nil { @@ -177,14 +165,3 @@ func (c *Compiler) copyFrontmatterWithoutInternalMarkers(frontmatter map[string] } return copy } - -// validateNoTopLevelSafeJobs validates that the frontmatter does not contain a top-level "safe-jobs" field. -// Only safe-outputs.jobs is supported. -func (c *Compiler) validateNoTopLevelSafeJobs(frontmatter map[string]any, filePath string) error { - if _, hasSafeJobs := frontmatter["safe-jobs"]; hasSafeJobs { - msg := fmt.Sprintf("%s: top-level 'safe-jobs' field is not supported; use 'safe-outputs.jobs' instead", filePath) - suggestion := "\nChange from:\n safe-jobs:\n my-job: ...\nTo:\n safe-outputs:\n jobs:\n my-job: ..." - return fmt.Errorf("%s%s", msg, suggestion) - } - return nil -} diff --git a/pkg/workflow/top_level_safe_jobs_validation_test.go b/pkg/workflow/top_level_safe_jobs_validation_test.go deleted file mode 100644 index b0f2a6d903..0000000000 --- a/pkg/workflow/top_level_safe_jobs_validation_test.go +++ /dev/null @@ -1,103 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "os" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestTopLevelSafeJobsRejected verifies that top-level "safe-jobs" field is rejected -func TestTopLevelSafeJobsRejected(t *testing.T) { - c := NewCompiler() - - // Create a test workflow markdown with top-level safe-jobs - markdown := `--- -on: - workflow_dispatch: -engine: copilot -safe-jobs: - test: - runs-on: ubuntu-latest - steps: - - run: echo "test" ---- -Test workflow with top-level safe-jobs (should be rejected) -` - - // Write to a temporary file - tmpFile := "/tmp/test-top-level-safe-jobs-validation.md" - err := os.WriteFile(tmpFile, []byte(markdown), 0644) - require.NoError(t, err, "Should write test file") - defer os.Remove(tmpFile) - - // Try to compile - should fail with clear error message - _, err = c.ParseWorkflowFile(tmpFile) - require.Error(t, err, "Should reject top-level safe-jobs") - assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") - assert.Contains(t, err.Error(), "safe-outputs.jobs", "Error should suggest using safe-outputs.jobs") -} - -// TestSafeOutputsJobsAccepted verifies that safe-outputs.jobs is accepted -func TestSafeOutputsJobsAccepted(t *testing.T) { - c := NewCompiler() - - // Create a test workflow markdown with safe-outputs.jobs - markdown := `--- -on: - workflow_dispatch: -engine: copilot -safe-outputs: - jobs: - test: - runs-on: ubuntu-latest - steps: - - run: echo "test" ---- -Test workflow with safe-outputs.jobs (should be accepted) -` - - // Write to a temporary file - tmpFile := "/tmp/test-safe-outputs-jobs-validation.md" - err := os.WriteFile(tmpFile, []byte(markdown), 0644) - require.NoError(t, err, "Should write test file") - defer os.Remove(tmpFile) - - // Try to compile - should succeed (or fail for other reasons, but not safe-jobs) - _, err = c.ParseWorkflowFile(tmpFile) - // If there's an error, it shouldn't be about safe-jobs - if err != nil { - assert.NotContains(t, err.Error(), "safe-jobs", "Should not mention safe-jobs if using safe-outputs.jobs") - } -} - -// TestSharedWorkflowSafeJobsRejected verifies that shared workflows also reject top-level safe-jobs -func TestSharedWorkflowSafeJobsRejected(t *testing.T) { - c := NewCompiler() - - // Create a test shared workflow markdown with top-level safe-jobs (no 'on' field) - markdown := `--- -engine: copilot -safe-jobs: - test: - runs-on: ubuntu-latest - steps: - - run: echo "test" ---- -Shared workflow with top-level safe-jobs (should be rejected) -` - - // Write to a temporary file - tmpFile := "/tmp/test-shared-safe-jobs-validation.md" - err := os.WriteFile(tmpFile, []byte(markdown), 0644) - require.NoError(t, err, "Should write test file") - defer os.Remove(tmpFile) - - // Try to compile - should fail with clear error message - _, err = c.ParseWorkflowFile(tmpFile) - require.Error(t, err, "Should reject top-level safe-jobs in shared workflows") - assert.Contains(t, err.Error(), "top-level 'safe-jobs' field is not supported", "Error should mention top-level safe-jobs not supported") -}