diff --git a/.github/workflows/test-safe-output-missing-tool.lock.yml b/.github/workflows/test-safe-output-missing-tool.lock.yml index d008cd60f40..ac15cb6f619 100644 --- a/.github/workflows/test-safe-output-missing-tool.lock.yml +++ b/.github/workflows/test-safe-output-missing-tool.lock.yml @@ -19,14 +19,7 @@ concurrency: run-name: "Test Safe Output - Missing Tool" jobs: - task: - runs-on: ubuntu-latest - steps: - - name: Task job condition barrier - run: echo "Task job executed - conditions satisfied" - test-safe-output-missing-tool: - needs: task runs-on: ubuntu-latest permissions: read-all outputs: diff --git a/pkg/cli/workflows/test-playwright-accessibility-contrast.lock.yml b/pkg/cli/workflows/test-playwright-accessibility-contrast.lock.yml index 99e73d3cfe2..6d3554c0d61 100644 --- a/pkg/cli/workflows/test-playwright-accessibility-contrast.lock.yml +++ b/pkg/cli/workflows/test-playwright-accessibility-contrast.lock.yml @@ -1589,6 +1589,20 @@ jobs: GITHUB_AW_REQUIRED_ROLES: admin,maintainer with: script: | + async function setCancelled(message) { + try { + await github.rest.actions.cancelWorkflowRun({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.runId, + }); + core.info(`Cancellation requested for this workflow run: ${message}`); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + core.warning(`Failed to cancel workflow run: ${errorMessage}`); + core.setFailed(message); // Fallback if API call fails + } + } async function main() { const { eventName } = context; // skip check for safe events @@ -1607,7 +1621,9 @@ jobs: core.error( "❌ Configuration error: Required permissions not specified. Contact repository administrator." ); - core.setFailed("Configuration error: Required permissions not specified"); + await setCancelled( + "Configuration error: Required permissions not specified" + ); return; } // Check if the actor has the required repository permissions @@ -1641,14 +1657,14 @@ jobs: const errorMessage = repoError instanceof Error ? repoError.message : String(repoError); core.error(`Repository permission check failed: ${errorMessage}`); - core.setFailed(`Repository permission check failed: ${errorMessage}`); + await setCancelled(`Repository permission check failed: ${errorMessage}`); return; } - // Cancel the job when permission check fails + // Cancel the workflow when permission check fails core.warning( `❌ Access denied: Only authorized users can trigger this workflow. User '${actor}' is not authorized. Required permissions: ${requiredPermissions.join(", ")}` ); - core.setFailed( + await setCancelled( `Access denied: User '${actor}' is not authorized. Required permissions: ${requiredPermissions.join(", ")}` ); } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 55340d05a98..9e399ee5ca0 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -1779,13 +1779,13 @@ func (c *Compiler) needsPermissionChecksWithFrontmatter(data *WorkflowData, fron } // isTaskJobNeeded determines if the task job is required -func (c *Compiler) isTaskJobNeeded(data *WorkflowData) bool { +func (c *Compiler) isTaskJobNeeded(data *WorkflowData, needsPermissionCheck bool) bool { // Task job is needed if: // 1. Command is configured (for team member checking) // 2. Text output is needed (for compute-text action) // 3. If condition is specified (to handle runtime conditions) // 4. Permission checks are needed (consolidated team member validation) - return data.Command != "" || data.NeedsTextOutput || data.If != "" || c.needsPermissionChecks(data) + return data.Command != "" || data.NeedsTextOutput || data.If != "" || needsPermissionCheck } // buildJobs creates all jobs for the workflow and adds them to the job manager @@ -1812,7 +1812,7 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { needsPermissionCheck = c.needsPermissionChecks(data) } - if c.isTaskJobNeeded(data) || needsPermissionCheck { + if c.isTaskJobNeeded(data, needsPermissionCheck) { taskJob, err := c.buildTaskJob(data, frontmatter) if err != nil { return fmt.Errorf("failed to build task job: %w", err) diff --git a/pkg/workflow/is_task_job_needed_test.go b/pkg/workflow/is_task_job_needed_test.go index 7311dba40a5..d607b74066b 100644 --- a/pkg/workflow/is_task_job_needed_test.go +++ b/pkg/workflow/is_task_job_needed_test.go @@ -9,22 +9,33 @@ func TestIsTaskJobNeeded(t *testing.T) { data := &WorkflowData{ Roles: []string{"all"}, // Explicitly disable permission checks } - if compiler.isTaskJobNeeded(data) { + // Pass false for needsPermissionCheck since roles: all is specified + if compiler.isTaskJobNeeded(data, false) { t.Errorf("Expected isTaskJobNeeded to be false when no alias, no needsTextOutput, no If condition, and roles: all") } }) t.Run("if_condition_present", func(t *testing.T) { data := &WorkflowData{If: "if: github.ref == 'refs/heads/main'"} - if !compiler.isTaskJobNeeded(data) { + // Pass false for needsPermissionCheck, but should still return true due to If condition + if !compiler.isTaskJobNeeded(data, false) { t.Errorf("Expected isTaskJobNeeded to be true when If condition is specified") } }) t.Run("default_permission_check", func(t *testing.T) { data := &WorkflowData{} // No explicit Roles field, permission checks are consolidated in task job - if !compiler.isTaskJobNeeded(data) { + // Pass true for needsPermissionCheck to simulate permission checks being needed + if !compiler.isTaskJobNeeded(data, true) { t.Errorf("Expected isTaskJobNeeded to be true - permission checks are now consolidated in task job") } }) + + t.Run("permission_check_not_needed", func(t *testing.T) { + data := &WorkflowData{} // No other conditions that would require task job + // Pass false for needsPermissionCheck - task job should not be needed + if compiler.isTaskJobNeeded(data, false) { + t.Errorf("Expected isTaskJobNeeded to be false when no conditions require task job") + } + }) } diff --git a/pkg/workflow/task_job_generation_fix_test.go b/pkg/workflow/task_job_generation_fix_test.go new file mode 100644 index 00000000000..f10f4e225fa --- /dev/null +++ b/pkg/workflow/task_job_generation_fix_test.go @@ -0,0 +1,172 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestTaskJobGenerationFix tests that task job is only generated when required +func TestTaskJobGenerationFix(t *testing.T) { + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "task-job-generation-test*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + t.Run("no_task_job_for_safe_events_and_roles_all", func(t *testing.T) { + // This workflow should NOT generate a task job because: + // 1. Uses only safe events (workflow_dispatch) + // 2. Has roles: all (no permission checks needed) + // 3. No command, no if condition, no text output needed + workflowContent := `--- +on: + workflow_dispatch: +roles: all +--- + +# Simple Workflow + +This is a simple workflow that should not need a task job. +Do some simple work.` + + workflowFile := filepath.Join(tmpDir, "safe-workflow.md") + err = os.WriteFile(workflowFile, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Verify that NO task job is generated + if strings.Contains(lockContentStr, "task:") { + t.Error("Expected NO task job for safe events with roles: all") + } + + // Verify that the main job exists (should be named after the workflow) + if !strings.Contains(lockContentStr, "jobs:") { + t.Error("Expected jobs section to be present") + } + + // Verify main job doesn't have "needs: task" + if strings.Contains(lockContentStr, "needs: task") { + t.Error("Main job should not depend on task job when task job is not generated") + } + }) + + t.Run("task_job_for_unsafe_events", func(t *testing.T) { + // This workflow SHOULD generate a task job because: + // 1. Uses unsafe events (push) which require permission checks + workflowContent := `--- +on: + push: + branches: [main] +--- + +# Unsafe Event Workflow + +This workflow uses push events and should generate a task job for permission checks. +Do some work.` + + workflowFile := filepath.Join(tmpDir, "unsafe-workflow.md") + err = os.WriteFile(workflowFile, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Verify that task job IS generated + if !strings.Contains(lockContentStr, "task:") { + t.Error("Expected task job for unsafe events (push)") + } + + // Verify main job depends on task + if !strings.Contains(lockContentStr, "needs: task") { + t.Error("Main job should depend on task job when task job is generated") + } + }) + + t.Run("task_job_for_if_condition", func(t *testing.T) { + // This workflow SHOULD generate a task job because: + // 1. Has an if condition (even though events are safe and roles: all) + workflowContent := `--- +on: + workflow_dispatch: +roles: all +if: ${{ github.ref == 'refs/heads/main' }} +--- + +# Conditional Workflow + +This workflow has an if condition and should generate a task job. +Do conditional work.` + + workflowFile := filepath.Join(tmpDir, "conditional-workflow.md") + err = os.WriteFile(workflowFile, []byte(workflowContent), 0644) + if err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile the workflow + compiler := NewCompiler(false, "", "test") + err = compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read the generated lock file + lockFile := strings.TrimSuffix(workflowFile, ".md") + ".lock.yml" + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + + // Verify that task job IS generated due to if condition + if !strings.Contains(lockContentStr, "task:") { + t.Error("Expected task job for workflow with if condition") + } + + // Verify task job has the if condition + if !strings.Contains(lockContentStr, "if: ${{ github.ref == 'refs/heads/main' }}") { + t.Error("Expected task job to have the if condition") + } + + // Verify main job depends on task + if !strings.Contains(lockContentStr, "needs: task") { + t.Error("Main job should depend on task job when task job is generated") + } + }) +}