diff --git a/.github/workflows/test-claude.lock.yml b/.github/workflows/test-claude.lock.yml index 389d408bbe4..22763c0943c 100644 --- a/.github/workflows/test-claude.lock.yml +++ b/.github/workflows/test-claude.lock.yml @@ -4,6 +4,9 @@ name: "Test Claude" on: + pull_request: + branches: + - "*claude*" push: branches: - "*claude*" @@ -12,7 +15,8 @@ on: permissions: {} concurrency: - group: "gh-aw-${{ github.workflow }}" + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" + cancel-in-progress: true run-name: "Test Claude" diff --git a/.github/workflows/test-claude.md b/.github/workflows/test-claude.md index 959c1901799..f506cae909a 100644 --- a/.github/workflows/test-claude.md +++ b/.github/workflows/test-claude.md @@ -3,6 +3,9 @@ on: push: branches: - "*claude*" + pull_request: + branches: + - "*claude*" workflow_dispatch: engine: id: claude diff --git a/docs/frontmatter.md b/docs/frontmatter.md index d66744fca27..3d1e6f2ebbd 100644 --- a/docs/frontmatter.md +++ b/docs/frontmatter.md @@ -300,7 +300,52 @@ concurrency: cancel-in-progress: true ``` -Defaults to single instance per workflow. +#### Enhanced Concurrency Policies + +GitHub Agentic Workflows automatically generates enhanced concurrency policies based on workflow trigger types to provide better isolation and resource management. Different workflow types receive different concurrency groups and cancellation behavior: + +| Trigger Type | Concurrency Group | Cancellation | Description | +|--------------|-------------------|--------------|-------------| +| `issues` | `gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}` | ❌ | Issue workflows include issue number for isolation | +| `pull_request` | `gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number \|\| github.ref }}` | ✅ | PR workflows include PR number with cancellation | +| `discussion` | `gh-aw-${{ github.workflow }}-${{ github.event.discussion.number }}` | ❌ | Discussion workflows include discussion number | +| Mixed issue/PR | `gh-aw-${{ github.workflow }}-${{ github.event.issue.number \|\| github.event.pull_request.number }}` | ✅ | Mixed workflows handle both contexts with cancellation | +| Alias workflows | `gh-aw-${{ github.workflow }}-${{ github.event.issue.number \|\| github.event.pull_request.number }}` | ❌ | Alias workflows handle both contexts without cancellation | +| Other triggers | `gh-aw-${{ github.workflow }}` | ❌ | Default behavior for schedule, push, etc. | + +**Benefits:** +- **Better Isolation**: Workflows operating on different issues/PRs can run concurrently +- **Conflict Prevention**: No interference between unrelated workflow executions +- **Resource Management**: Pull request workflows can cancel previous runs when updated +- **Predictable Behavior**: Consistent concurrency rules based on trigger type + +**Examples:** + +```yaml +# Issue workflow - no cancellation, isolated by issue number +on: + issues: + types: [opened, edited] +# Generates: group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}" + +# PR workflow - with cancellation, isolated by PR number +on: + pull_request: + types: [opened, synchronize] +# Generates: group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" +# cancel-in-progress: true + +# Mixed workflow - handles both issues and PRs with cancellation +on: + issues: + types: [opened, edited] + pull_request: + types: [opened, synchronize] +# Generates: group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}" +# cancel-in-progress: true +``` + +If you need custom concurrency behavior, you can override the automatic generation by specifying your own `concurrency` section in the frontmatter. ### Environment Variables diff --git a/pkg/workflow/concurrency.go b/pkg/workflow/concurrency.go index 0809f124501..d6a4f705f6c 100644 --- a/pkg/workflow/concurrency.go +++ b/pkg/workflow/concurrency.go @@ -1,6 +1,7 @@ package workflow import ( + "fmt" "strings" ) @@ -12,25 +13,73 @@ func GenerateConcurrencyConfig(workflowData *WorkflowData, isAliasTrigger bool) return workflowData.Concurrency } - // Generate concurrency configuration based on workflow type - // Note: Check alias trigger first since alias workflows also contain pull_request events - if isAliasTrigger { - // For alias workflows: use issue/PR number for concurrency but do NOT enable cancellation - return `concurrency: - group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}"` - } else if isPullRequestWorkflow(workflowData.On) { - // For PR workflows: include ref and enable cancellation - return `concurrency: - group: "gh-aw-${{ github.workflow }}-${{ github.ref }}" - cancel-in-progress: true` - } else { - // For other workflows: use static concurrency without cancellation - return `concurrency: - group: "gh-aw-${{ github.workflow }}"` + // Build concurrency group keys + keys := buildConcurrencyGroupKeys(workflowData, isAliasTrigger) + groupValue := strings.Join(keys, "-") + + // Build the concurrency configuration + concurrencyConfig := fmt.Sprintf("concurrency:\n group: \"%s\"", groupValue) + + // Add cancel-in-progress if appropriate + if shouldEnableCancelInProgress(workflowData, isAliasTrigger) { + concurrencyConfig += "\n cancel-in-progress: true" } + + return concurrencyConfig } // isPullRequestWorkflow checks if a workflow's "on" section contains pull_request triggers func isPullRequestWorkflow(on string) bool { return strings.Contains(on, "pull_request") } + +// isIssueWorkflow checks if a workflow's "on" section contains issue-related triggers +func isIssueWorkflow(on string) bool { + return strings.Contains(on, "issues") || strings.Contains(on, "issue_comment") +} + +// isDiscussionWorkflow checks if a workflow's "on" section contains discussion-related triggers +func isDiscussionWorkflow(on string) bool { + return strings.Contains(on, "discussion") +} + +// buildConcurrencyGroupKeys builds an array of keys for the concurrency group +func buildConcurrencyGroupKeys(workflowData *WorkflowData, isAliasTrigger bool) []string { + keys := []string{"gh-aw", "${{ github.workflow }}"} + + if isAliasTrigger { + // For alias workflows: use issue/PR number + keys = append(keys, "${{ github.event.issue.number || github.event.pull_request.number }}") + } else if isPullRequestWorkflow(workflowData.On) && isIssueWorkflow(workflowData.On) { + // Mixed workflows with both issue and PR triggers: use issue/PR number + keys = append(keys, "${{ github.event.issue.number || github.event.pull_request.number }}") + } else if isPullRequestWorkflow(workflowData.On) && isDiscussionWorkflow(workflowData.On) { + // Mixed workflows with PR and discussion triggers: use PR/discussion number + keys = append(keys, "${{ github.event.pull_request.number || github.event.discussion.number }}") + } else if isIssueWorkflow(workflowData.On) && isDiscussionWorkflow(workflowData.On) { + // Mixed workflows with issue and discussion triggers: use issue/discussion number + keys = append(keys, "${{ github.event.issue.number || github.event.discussion.number }}") + } else if isPullRequestWorkflow(workflowData.On) { + // Pure PR workflows: use PR number if available, otherwise fall back to ref for compatibility + keys = append(keys, "${{ github.event.pull_request.number || github.ref }}") + } else if isIssueWorkflow(workflowData.On) { + // Issue workflows: use issue number + keys = append(keys, "${{ github.event.issue.number }}") + } else if isDiscussionWorkflow(workflowData.On) { + // Discussion workflows: use discussion number + keys = append(keys, "${{ github.event.discussion.number }}") + } + + return keys +} + +// shouldEnableCancelInProgress determines if cancel-in-progress should be enabled +func shouldEnableCancelInProgress(workflowData *WorkflowData, isAliasTrigger bool) bool { + // Never enable cancellation for alias workflows + if isAliasTrigger { + return false + } + + // Enable cancellation for pull request workflows (including mixed workflows) + return isPullRequestWorkflow(workflowData.On) +} diff --git a/pkg/workflow/concurrency_test.go b/pkg/workflow/concurrency_test.go index 833e7956a82..251e833e3b1 100644 --- a/pkg/workflow/concurrency_test.go +++ b/pkg/workflow/concurrency_test.go @@ -31,15 +31,16 @@ func TestConcurrencyRules(t *testing.T) { on: pull_request: types: [opened, edited] +tools: github: allowed: [list_issues] ---`, filename: "pr-workflow.md", expectedConcurrency: `concurrency: - group: "gh-aw-${{ github.workflow }}-${{ github.ref }}" + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" cancel-in-progress: true`, shouldHaveCancel: true, - description: "PR workflows should use dynamic concurrency with ref and cancellation", + description: "PR workflows should use dynamic concurrency with PR number and cancellation", }, { name: "alias workflow should have dynamic concurrency without cancel", @@ -89,6 +90,22 @@ tools: shouldHaveCancel: false, description: "Push workflows should use static concurrency without cancellation", }, + { + name: "issue workflow should have dynamic concurrency with issue number", + frontmatter: `--- +on: + issues: + types: [opened, edited] +tools: + github: + allowed: [list_issues] +---`, + filename: "issue-workflow.md", + expectedConcurrency: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}"`, + shouldHaveCancel: false, + description: "Issue workflows should use dynamic concurrency with issue number but no cancellation", + }, } for _, tt := range tests { @@ -129,18 +146,23 @@ This is a test workflow for concurrency behavior. t.Errorf("Did not expect cancel-in-progress: true for %s workflow, but found in: %s", tt.name, workflowData.Concurrency) } - // For PR workflows, check for ref inclusion; for alias workflows, check for issue/PR numbers + // For PR workflows, check for PR number inclusion; for alias workflows, check for issue/PR numbers; for issue workflows, check for issue number isPRWorkflow := strings.Contains(tt.name, "PR workflow") isAliasWorkflow := strings.Contains(tt.name, "alias workflow") + isIssueWorkflow := strings.Contains(tt.name, "issue workflow") if isPRWorkflow { - if !strings.Contains(workflowData.Concurrency, "github.ref") { - t.Errorf("Expected concurrency to include github.ref for %s workflow, got: %s", tt.name, workflowData.Concurrency) + if !strings.Contains(workflowData.Concurrency, "github.event.pull_request.number") { + t.Errorf("Expected concurrency to include github.event.pull_request.number for %s workflow, got: %s", tt.name, workflowData.Concurrency) } } else if isAliasWorkflow { if !strings.Contains(workflowData.Concurrency, "github.event.issue.number || github.event.pull_request.number") { t.Errorf("Expected concurrency to include issue/PR numbers for %s workflow, got: %s", tt.name, workflowData.Concurrency) } + } else if isIssueWorkflow { + if !strings.Contains(workflowData.Concurrency, "github.event.issue.number") { + t.Errorf("Expected concurrency to include github.event.issue.number for %s workflow, got: %s", tt.name, workflowData.Concurrency) + } } else { if strings.Contains(workflowData.Concurrency, "github.ref") { t.Errorf("Did not expect concurrency to include github.ref for %s workflow, got: %s", tt.name, workflowData.Concurrency) @@ -159,7 +181,7 @@ func TestGenerateConcurrencyConfig(t *testing.T) { description string }{ { - name: "PR workflow should have dynamic concurrency with cancel", + name: "PR workflow should have dynamic concurrency with cancel and PR number", workflowData: &WorkflowData{ On: `on: pull_request: @@ -168,9 +190,9 @@ func TestGenerateConcurrencyConfig(t *testing.T) { }, isAliasTrigger: false, expected: `concurrency: - group: "gh-aw-${{ github.workflow }}-${{ github.ref }}" + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" cancel-in-progress: true`, - description: "PR workflows should use dynamic concurrency with ref and cancellation", + description: "PR workflows should use PR number or ref with cancellation", }, { name: "Alias workflow should have dynamic concurrency without cancel", @@ -198,6 +220,76 @@ func TestGenerateConcurrencyConfig(t *testing.T) { group: "gh-aw-${{ github.workflow }}"`, description: "Regular workflows should use static concurrency without cancellation", }, + { + name: "Issue workflow should have dynamic concurrency with issue number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited]`, + Concurrency: "", // Empty, should be generated + }, + isAliasTrigger: false, + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}"`, + description: "Issue workflows should use issue number without cancellation", + }, + { + name: "Issue comment workflow should have dynamic concurrency with issue number", + workflowData: &WorkflowData{ + On: `on: + issue_comment: + types: [created, edited]`, + Concurrency: "", // Empty, should be generated + }, + isAliasTrigger: false, + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}"`, + description: "Issue comment workflows should use issue number without cancellation", + }, + { + name: "Mixed issue and PR workflow should have dynamic concurrency with issue/PR number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited] + pull_request: + types: [opened, synchronize]`, + Concurrency: "", // Empty, should be generated + }, + isAliasTrigger: false, + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}" + cancel-in-progress: true`, + description: "Mixed workflows should use issue/PR number with cancellation enabled", + }, + { + name: "Discussion workflow should have dynamic concurrency with discussion number", + workflowData: &WorkflowData{ + On: `on: + discussion: + types: [created, edited]`, + Concurrency: "", // Empty, should be generated + }, + isAliasTrigger: false, + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.discussion.number }}"`, + description: "Discussion workflows should use discussion number without cancellation", + }, + { + name: "Mixed issue and discussion workflow should have dynamic concurrency with issue/discussion number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited] + discussion: + types: [created, edited]`, + Concurrency: "", // Empty, should be generated + }, + isAliasTrigger: false, + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.discussion.number }}"`, + description: "Mixed issue and discussion workflows should use issue/discussion number without cancellation", + }, { name: "Existing concurrency should not be overridden", workflowData: &WorkflowData{ @@ -279,3 +371,301 @@ func TestIsPullRequestWorkflow(t *testing.T) { }) } } + +func TestIsIssueWorkflow(t *testing.T) { + tests := []struct { + name string + on string + expected bool + }{ + { + name: "Issues workflow should be identified", + on: `on: + issues: + types: [opened, edited]`, + expected: true, + }, + { + name: "Issue comment workflow should be identified", + on: `on: + issue_comment: + types: [created]`, + expected: true, + }, + { + name: "Pull request workflow should not be identified as issue workflow", + on: `on: + pull_request: + types: [opened, synchronize]`, + expected: false, + }, + { + name: "Schedule workflow should not be identified as issue workflow", + on: `on: + schedule: + - cron: "0 9 * * 1"`, + expected: false, + }, + { + name: "Mixed workflow with issues should be identified", + on: `on: + issues: + types: [opened, edited] + push: + branches: [main]`, + expected: true, + }, + { + name: "Mixed workflow with issue_comment should be identified", + on: `on: + issue_comment: + types: [created] + schedule: + - cron: "0 9 * * 1"`, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isIssueWorkflow(tt.on) + if result != tt.expected { + t.Errorf("isIssueWorkflow() for %s = %v, expected %v", tt.name, result, tt.expected) + } + }) + } +} + +func TestIsDiscussionWorkflow(t *testing.T) { + tests := []struct { + name string + on string + expected bool + }{ + { + name: "Discussion workflow should be identified", + on: `on: + discussion: + types: [created, edited]`, + expected: true, + }, + { + name: "Discussion comment workflow should be identified", + on: `on: + discussion_comment: + types: [created]`, + expected: true, + }, + { + name: "Issues workflow should not be identified as discussion workflow", + on: `on: + issues: + types: [opened, edited]`, + expected: false, + }, + { + name: "Mixed workflow with discussion should be identified", + on: `on: + discussion: + types: [created, edited] + push: + branches: [main]`, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isDiscussionWorkflow(tt.on) + if result != tt.expected { + t.Errorf("isDiscussionWorkflow() for %s = %v, expected %v", tt.name, result, tt.expected) + } + }) + } +} + +func TestBuildConcurrencyGroupKeys(t *testing.T) { + tests := []struct { + name string + workflowData *WorkflowData + isAliasTrigger bool + expected []string + description string + }{ + { + name: "Alias workflow should include issue/PR number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited]`, + }, + isAliasTrigger: true, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.issue.number || github.event.pull_request.number }}"}, + description: "Alias workflows should use issue/PR number", + }, + { + name: "Pure PR workflow should include PR number", + workflowData: &WorkflowData{ + On: `on: + pull_request: + types: [opened, synchronize]`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.pull_request.number || github.ref }}"}, + description: "Pure PR workflows should use PR number", + }, + { + name: "Pure issue workflow should include issue number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited]`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.issue.number }}"}, + description: "Pure issue workflows should use issue number", + }, + { + name: "Mixed issue and PR workflow should include issue/PR number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited] + pull_request: + types: [opened, synchronize]`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.issue.number || github.event.pull_request.number }}"}, + description: "Mixed workflows should use issue/PR number", + }, + { + name: "Pure discussion workflow should include discussion number", + workflowData: &WorkflowData{ + On: `on: + discussion: + types: [created, edited]`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.discussion.number }}"}, + description: "Pure discussion workflows should use discussion number", + }, + { + name: "Mixed issue and discussion workflow should include issue/discussion number", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited] + discussion: + types: [created, edited]`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}", "${{ github.event.issue.number || github.event.discussion.number }}"}, + description: "Mixed issue and discussion workflows should use issue/discussion number", + }, + { + name: "Other workflow should not include additional keys", + workflowData: &WorkflowData{ + On: `on: + schedule: + - cron: "0 9 * * 1"`, + }, + isAliasTrigger: false, + expected: []string{"gh-aw", "${{ github.workflow }}"}, + description: "Other workflows should use just workflow name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := buildConcurrencyGroupKeys(tt.workflowData, tt.isAliasTrigger) + + if len(result) != len(tt.expected) { + t.Errorf("buildConcurrencyGroupKeys() for %s returned %d keys, expected %d", tt.description, len(result), len(tt.expected)) + return + } + + for i, key := range result { + if key != tt.expected[i] { + t.Errorf("buildConcurrencyGroupKeys() for %s key[%d] = %s, expected %s", tt.description, i, key, tt.expected[i]) + } + } + }) + } +} + +func TestShouldEnableCancelInProgress(t *testing.T) { + tests := []struct { + name string + workflowData *WorkflowData + isAliasTrigger bool + expected bool + description string + }{ + { + name: "Alias workflow should not enable cancellation", + workflowData: &WorkflowData{ + On: `on: + pull_request: + types: [opened, synchronize]`, + }, + isAliasTrigger: true, + expected: false, + description: "Alias workflows should never enable cancellation", + }, + { + name: "PR workflow should enable cancellation", + workflowData: &WorkflowData{ + On: `on: + pull_request: + types: [opened, synchronize]`, + }, + isAliasTrigger: false, + expected: true, + description: "PR workflows should enable cancellation", + }, + { + name: "Issue workflow should not enable cancellation", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited]`, + }, + isAliasTrigger: false, + expected: false, + description: "Issue workflows should not enable cancellation", + }, + { + name: "Mixed issue and PR workflow should enable cancellation", + workflowData: &WorkflowData{ + On: `on: + issues: + types: [opened, edited] + pull_request: + types: [opened, synchronize]`, + }, + isAliasTrigger: false, + expected: true, + description: "Mixed workflows with PR should enable cancellation", + }, + { + name: "Other workflow should not enable cancellation", + workflowData: &WorkflowData{ + On: `on: + schedule: + - cron: "0 9 * * 1"`, + }, + isAliasTrigger: false, + expected: false, + description: "Other workflows should not enable cancellation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := shouldEnableCancelInProgress(tt.workflowData, tt.isAliasTrigger) + if result != tt.expected { + t.Errorf("shouldEnableCancelInProgress() for %s = %v, expected %v", tt.description, result, tt.expected) + } + }) + } +}