From ad689998b80021a82f40232ddac1706dc0a58759 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 14:33:08 +0000 Subject: [PATCH 1/2] Initial plan From 34da5f0ea0de532930e88f1470af20bc17e82fb8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 14:58:26 +0000 Subject: [PATCH 2/2] Warn when GitHub App safe-outputs can self-cancel comment-triggered workflows via shared concurrency Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ff1e0bdb-71d5-461f-99d7-c0f08aa19e41 --- pkg/workflow/compiler.go | 4 + .../safe_outputs_app_concurrency_warning.go | 116 ++++++++++ ...fe_outputs_app_concurrency_warning_test.go | 206 ++++++++++++++++++ 3 files changed, 326 insertions(+) create mode 100644 pkg/workflow/safe_outputs_app_concurrency_warning.go create mode 100644 pkg/workflow/safe_outputs_app_concurrency_warning_test.go diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 6bcaec47fe..ec63a1783e 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -165,6 +165,10 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath log.Printf("Validating push-to-pull-request-branch configuration") c.validatePushToPullRequestBranchWarnings(workflowData.SafeOutputs, workflowData.CheckoutConfigs) + // Emit warning for safe-outputs github-app + comment trigger + cancel-in-progress: true + log.Printf("Checking for safe-outputs github-app self-cancellation risk") + c.validateSafeOutputsAppConcurrencyWarning(workflowData) + // Validate network allowed domains configuration log.Printf("Validating network allowed domains") if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { diff --git a/pkg/workflow/safe_outputs_app_concurrency_warning.go b/pkg/workflow/safe_outputs_app_concurrency_warning.go new file mode 100644 index 0000000000..b9b235cd19 --- /dev/null +++ b/pkg/workflow/safe_outputs_app_concurrency_warning.go @@ -0,0 +1,116 @@ +package workflow + +import ( + "fmt" + "os" + "strings" + + "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/logger" +) + +var safeOutputsAppConcurrencyLog = logger.New("workflow:safe_outputs_app_concurrency") + +// hasCancelInProgress reports whether the given WorkflowData has workflow-level +// cancel-in-progress: true set. It checks two sources: +// +// 1. The structured RawFrontmatter map (most reliable; works for user-defined concurrency). +// 2. The rendered Concurrency YAML string (covers compiler-generated concurrency e.g. for PR +// workflows where cancel-in-progress is emitted by GenerateConcurrencyConfig). +func hasCancelInProgress(workflowData *WorkflowData) bool { + // Check 1: structured frontmatter + if raw := workflowData.RawFrontmatter; raw != nil { + if concurrencyRaw, ok := raw["concurrency"]; ok { + if concurrencyMap, ok := concurrencyRaw.(map[string]any); ok { + if cancelRaw, ok := concurrencyMap["cancel-in-progress"]; ok { + // The YAML library parses `true` as bool; accept the boolean form + if cancelBool, ok := cancelRaw.(bool); ok && cancelBool { + return true + } + // Defensive: also accept the string "true" + if cancelStr, ok := cancelRaw.(string); ok && cancelStr == "true" { + return true + } + } + } + } + } + + // Check 2: rendered concurrency YAML string (e.g. compiler-generated) + return strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") +} + +// validateSafeOutputsAppConcurrencyWarning warns when a workflow combines three +// conditions that can cause the original run to self-cancel: +// +// 1. workflow-level cancel-in-progress: true +// 2. comment-based triggers (issue_comment or slash_command) +// 3. safe-outputs.github-app is configured +// +// When all three are present, the GitHub App-authored safe-output comment can +// trigger a passive issue_comment run of the same workflow. Because the new run +// shares the same workflow-level concurrency group (keyed by issue number), the +// cancel-in-progress policy cancels the original run before it finishes. +// +// Note: safe-outputs.concurrency-group does NOT protect against this pattern. +// It only sets concurrency on the safe_outputs job itself, not on the triggered +// passive workflow run that enters the workflow-level concurrency group. +func (c *Compiler) validateSafeOutputsAppConcurrencyWarning(workflowData *WorkflowData) { + // Check 1: safe-outputs.github-app is configured (directly or via top-level fallback). + // The fallback is already applied before compilation so SafeOutputs.GitHubApp is set. + if workflowData.SafeOutputs == nil || workflowData.SafeOutputs.GitHubApp == nil { + safeOutputsAppConcurrencyLog.Print("No safe-outputs github-app configured, skipping warning") + return + } + + // Check 2: workflow has comment-based triggers (issue_comment or slash_command). + // slash_command expands to issue_comment + workflow_dispatch at compile time, + // so both cases mean the workflow can be triggered by comments. + on := workflowData.On + hasCommentTrigger := isIssueWorkflow(on) || isSlashCommandWorkflow(on) + if !hasCommentTrigger { + safeOutputsAppConcurrencyLog.Print("No comment-based triggers found, skipping warning") + return + } + + // Check 3: workflow-level concurrency has cancel-in-progress: true. + if !hasCancelInProgress(workflowData) { + safeOutputsAppConcurrencyLog.Print("cancel-in-progress: true not found in concurrency config, skipping warning") + return + } + + safeOutputsAppConcurrencyLog.Print("Dangerous combination detected: emitting self-cancel warning") + + msg := strings.Join([]string{ + "safe-outputs.github-app combined with comment triggers and cancel-in-progress: true can cause self-cancellation.", + "", + "When safe-outputs posts a comment using the GitHub App token, that comment triggers a passive", + "issue_comment run of the same workflow. If the passive run resolves to the same workflow-level", + "concurrency group as the original run, cancel-in-progress: true will cancel the original run", + "before it finishes — even before the passive run reaches pre_activation.", + "", + "Note: safe-outputs.concurrency-group does NOT protect against this. It only controls concurrency", + "on the safe_outputs job itself, not on the passive workflow run that enters the workflow-level", + "concurrency group.", + "", + "To prevent this, give passive comment-triggered runs a unique workflow-level concurrency key.", + "For example, check github.event.comment.body before resolving to a shared per-issue key:", + "", + " concurrency:", + " group: >-", + " gh-aw-${{ github.workflow }}-${{", + " github.event_name == 'issue_comment' &&", + " !startsWith(github.event.comment.body, '/your-command') &&", + " format('passive-comment-{0}', github.run_id) ||", + " github.event.issue.number ||", + " github.event.inputs.issue_number ||", + " github.run_id", + " }}", + " cancel-in-progress: true", + "", + "See: https://github.github.com/gh-aw/reference/concurrency/", + }, "\n") + + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) + c.IncrementWarningCount() +} diff --git a/pkg/workflow/safe_outputs_app_concurrency_warning_test.go b/pkg/workflow/safe_outputs_app_concurrency_warning_test.go new file mode 100644 index 0000000000..30c3ff7688 --- /dev/null +++ b/pkg/workflow/safe_outputs_app_concurrency_warning_test.go @@ -0,0 +1,206 @@ +//go:build !integration + +package workflow + +import ( + "bytes" + "io" + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSafeOutputsAppConcurrencyWarning tests that a warning is emitted when +// safe-outputs.github-app, comment-based triggers, and cancel-in-progress: true +// are combined, and that no warning is emitted when any condition is missing. +func TestSafeOutputsAppConcurrencyWarning(t *testing.T) { + tests := []struct { + name string + content string + expectWarning bool + }{ + { + name: "all three conditions: github-app + issue_comment + cancel-in-progress emits warning", + content: `--- +on: + issue_comment: + types: [created] + workflow_dispatch: +engine: copilot +concurrency: + group: gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.run_id }} + cancel-in-progress: true +safe-outputs: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + add-comment: +--- + +Post a comment. +`, + expectWarning: true, + }, + { + name: "slash_command trigger + github-app + cancel-in-progress emits warning", + content: `--- +on: + slash_command: + name: test + events: [issue_comment] +engine: copilot +concurrency: + group: >- + gh-aw-${{ github.workflow }}-${{ + github.event.issue.number || + github.event.inputs.issue_number || + github.run_id + }} + cancel-in-progress: true +safe-outputs: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + add-comment: +--- + +Post a comment to the triggering issue. +`, + expectWarning: true, + }, + { + name: "github-app + issue_comment but cancel-in-progress: false does not emit warning", + content: `--- +on: + issue_comment: + types: [created] + workflow_dispatch: +engine: copilot +concurrency: + group: gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.run_id }} + cancel-in-progress: false +safe-outputs: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + add-comment: +--- + +Post a comment. +`, + expectWarning: false, + }, + { + name: "github-app + cancel-in-progress but no comment trigger does not emit warning", + content: `--- +on: + workflow_dispatch: +engine: copilot +concurrency: + group: gh-aw-${{ github.workflow }}-${{ github.event.inputs.issue_number || github.run_id }} + cancel-in-progress: true +safe-outputs: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + add-comment: +--- + +Post a comment. +`, + expectWarning: false, + }, + { + name: "issue_comment + cancel-in-progress but no github-app does not emit warning", + content: `--- +on: + issue_comment: + types: [created] + workflow_dispatch: +engine: copilot +concurrency: + group: gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.run_id }} + cancel-in-progress: true +safe-outputs: + add-comment: +--- + +Post a comment. +`, + expectWarning: false, + }, + { + name: "no safe-outputs at all does not emit warning", + content: `--- +on: + issue_comment: + types: [created] + workflow_dispatch: +engine: copilot +concurrency: + group: gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.run_id }} + cancel-in-progress: true +--- + +Read comments. +`, + expectWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "safe-outputs-app-concurrency-warning-test") + + testFile := filepath.Join(tmpDir, "test-workflow.md") + err := os.WriteFile(testFile, []byte(tt.content), 0600) + require.NoError(t, err, "Should write test file") + + // Capture stderr to check for warnings + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + compiler := NewCompiler() + compiler.SetStrictMode(false) + compileErr := compiler.CompileWorkflow(testFile) + + // Restore stderr + w.Close() + os.Stderr = oldStderr + var buf bytes.Buffer + io.Copy(&buf, r) + stderrOutput := buf.String() + + require.NoError(t, compileErr, "Compilation should succeed") + + expectedPhrases := []string{ + "safe-outputs.github-app combined with comment triggers and cancel-in-progress: true", + "self-cancellation", + "passive", + "safe-outputs.concurrency-group does NOT protect against this", + } + + if tt.expectWarning { + for _, phrase := range expectedPhrases { + assert.Contains(t, stderrOutput, phrase, + "Expected warning to contain %q, got stderr:\n%s", phrase, stderrOutput) + } + assert.Contains(t, stderrOutput, "⚠", + "Expected warning indicator '⚠' in stderr output, got:\n%s", stderrOutput) + assert.Positive(t, compiler.GetWarningCount(), + "Expected warning count > 0") + } else { + // None of the self-cancel warning phrases should appear + for _, phrase := range expectedPhrases { + assert.NotContains(t, stderrOutput, phrase, + "Did not expect warning containing %q, but got stderr:\n%s", phrase, stderrOutput) + } + } + }) + } +}