Fix safe-output jobs running when workflow is cancelled

Issue: Safe-output jobs (create_issue, add_comment, etc.) were running even
when the workflow was cancelled, causing failures due to missing artifacts.

Root Cause: The condition 'if: (!cancelled())' evaluates to true when
dependency jobs are skipped (which happens during workflow cancellation).
GitHub Actions treats 'skipped' differently from 'cancelled'.

Solution: Add check for 'needs.agent.result != "skipped"' to prevent
safe-output jobs from running when the agent job was skipped due to
workflow cancellation.

This ensures:
- ✅ Runs when agent succeeds
- ✅ Runs when agent fails (for error reporting)
- ❌ Does NOT run when agent is skipped (workflow cancelled)

Related: #2890

Author: github-actions[bot]

pkg/workflow/expressions.go

@@ -336,26 +336,39 @@ func BuildNotFromFork() *ComparisonNode {
 }
 
 func BuildSafeOutputType(outputType string, min int) ConditionNode {
-	// Use !cancelled() instead of always() to respect workflow cancellation
+	// Use !cancelled() && needs.agent.result != 'skipped' to properly handle workflow cancellation
 	// !cancelled() allows jobs to run when dependencies fail (for error reporting)
-	// but skips them when the workflow is cancelled (desired behavior)
+	// needs.agent.result != 'skipped' prevents running when workflow is cancelled (dependencies get skipped)
 	notCancelledFunc := &NotNode{
 		Child: BuildFunctionCall("cancelled"),
 	}
 
-	// If min > 0, only return !cancelled() without the contains check
+	// Check that agent job was not skipped (happens when workflow is cancelled)
+	agentNotSkipped := &ComparisonNode{
+		Left:     BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.AgentJobName)),
+		Operator: "!=",
+		Right:    BuildStringLiteral("skipped"),
+	}
+
+	// Combine !cancelled() with agent not skipped check
+	baseCondition := &AndNode{
+		Left:  notCancelledFunc,
+		Right: agentNotSkipped,
+	}
+
+	// If min > 0, only return base condition without the contains check
 	// This is needed to ensure the job runs even with 0 outputs to enforce the minimum constraint
 	// Wrap in parentheses to ensure proper YAML interpretation
 	if min > 0 {
-		return &ParenthesesNode{Child: notCancelledFunc}
+		return &ParenthesesNode{Child: baseCondition}
 	}
 
 	containsFunc := BuildFunctionCall("contains",
 		BuildPropertyAccess(fmt.Sprintf("needs.%s.outputs.output_types", constants.AgentJobName)),
 		BuildStringLiteral(outputType),
 	)
 	return &AndNode{
-		Left:  notCancelledFunc,
+		Left:  baseCondition,
 		Right: containsFunc,
 	}
 }

pkg/workflow/expressions_cancelled_test.go

@@ -5,17 +5,17 @@ import (
 	"testing"
 )
 
-// TestBuildSafeOutputTypeWithCancelled verifies that BuildSafeOutputType uses !cancelled()
-// instead of always() to properly respect workflow cancellation.
+// TestBuildSafeOutputTypeWithCancelled verifies that BuildSafeOutputType properly handles workflow cancellation.
 //
 // Background:
 // - always() runs even when the workflow is cancelled (incorrect behavior)
-// - !cancelled() runs unless the workflow is cancelled (correct behavior)
+// - !cancelled() alone is insufficient (returns true when dependencies are skipped during cancellation)
+// - !cancelled() && needs.agent.result != 'skipped' is correct (prevents running when workflow is cancelled)
 //
 // This test ensures safe-output jobs:
 // 1. Run when dependencies succeed
 // 2. Run when dependencies fail (for error reporting)
-// 3. Skip when the workflow is cancelled (respecting user intent)
+// 3. Skip when the workflow is cancelled (dependencies get skipped, not cancelled)
 func TestBuildSafeOutputTypeWithCancelled(t *testing.T) {
 	tests := []struct {
 		name               string
@@ -25,35 +25,38 @@ func TestBuildSafeOutputTypeWithCancelled(t *testing.T) {
 		unexpectedContains []string
 	}{
 		{
-			name:       "min=0 should use !cancelled() with contains check",
+			name:       "min=0 should use !cancelled() and agent not skipped with contains check",
 			outputType: "create_issue",
 			min:        0,
 			expectedContains: []string{
 				"!cancelled()",
+				"needs.agent.result != 'skipped'",
 				"contains(needs.agent.outputs.output_types, 'create_issue')",
 			},
 			unexpectedContains: []string{
 				"always()",
 			},
 		},
 		{
-			name:       "min>0 should use (!cancelled()) without contains check",
+			name:       "min>0 should use !cancelled() and agent not skipped without contains check",
 			outputType: "create_issue",
 			min:        1,
 			expectedContains: []string{
-				"(!cancelled())",
+				"!cancelled()",
+				"needs.agent.result != 'skipped'",
 			},
 			unexpectedContains: []string{
 				"always()",
 				"contains(needs.agent.outputs.output_types, 'create_issue')",
 			},
 		},
 		{
-			name:       "push-to-pull-request-branch should use !cancelled()",
+			name:       "push-to-pull-request-branch should use !cancelled() and agent not skipped",
 			outputType: "push_to_pull_request_branch",
 			min:        0,
 			expectedContains: []string{
 				"!cancelled()",
+				"needs.agent.result != 'skipped'",
 				"contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch')",
 			},
 			unexpectedContains: []string{

pkg/workflow/safe_outputs_min_condition_test.go

@@ -38,7 +38,7 @@ func TestSafeOutputConditionWithMin(t *testing.T) {
 					},
 				},
 			},
-			expectedCondition:   "(!cancelled())",
+			expectedCondition:   "needs.agent.result != 'skipped'",
 			unexpectedCondition: "contains(needs.agent.outputs.output_types, 'missing_tool')",
 		},
 		{
@@ -67,7 +67,7 @@ func TestSafeOutputConditionWithMin(t *testing.T) {
 					"missing-tool": false,
 				},
 			},
-			expectedCondition:   "(!cancelled())",
+			expectedCondition:   "needs.agent.result != 'skipped'",
 			unexpectedCondition: "contains(needs.agent.outputs.output_types, 'create_issue')",
 		},
 		{
@@ -81,7 +81,7 @@ func TestSafeOutputConditionWithMin(t *testing.T) {
 					"missing-tool": false,
 				},
 			},
-			expectedCondition:   "(!cancelled())",
+			expectedCondition:   "needs.agent.result != 'skipped'",
 			unexpectedCondition: "contains(needs.agent.outputs.output_types, 'add-comment')",
 		},
 	}
@@ -148,10 +148,10 @@ func TestBuildSafeOutputTypeWithMin(t *testing.T) {
 			unexpectedCondition: "",
 		},
 		{
-			name:                "with min>0 should only have (!cancelled())",
+			name:                "with min>0 should only have (!cancelled()) and agent not skipped",
 			outputType:          "create-issue",
 			min:                 1,
-			expectedCondition:   "(!cancelled())",
+			expectedCondition:   "needs.agent.result != 'skipped'",
 			unexpectedCondition: "contains(needs.agent.outputs.output_types, 'create-issue')",
 		},
 		{
@@ -165,7 +165,7 @@ func TestBuildSafeOutputTypeWithMin(t *testing.T) {
 			name:                "missing-tool with min>0",
 			outputType:          "missing-tool",
 			min:                 2,
-			expectedCondition:   "(!cancelled())",
+			expectedCondition:   "needs.agent.result != 'skipped'",
 			unexpectedCondition: "contains(needs.agent.outputs.output_types, 'missing-tool')",
 		},
 	}
@@ -219,9 +219,9 @@ func TestMinConditionInCompiledWorkflow(t *testing.T) {
 		t.Fatalf("Failed to build job: %v", err)
 	}
 
-	// Verify that the condition only contains (!cancelled()) and not the contains check
-	if !strings.Contains(job.If, "(!cancelled())") {
-		t.Error("Expected condition to contain '(!cancelled())'")
+	// Verify that the condition contains agent not skipped check
+	if !strings.Contains(job.If, "needs.agent.result != 'skipped'") {
+		t.Error("Expected condition to contain 'needs.agent.result != 'skipped''")
 	}
 	if strings.Contains(job.If, "contains(needs.agent.outputs.output_types, 'missing-tool')") {
 		t.Error("Expected condition NOT to contain contains check when min > 0")