diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index c89cffc80bb..cb60142d430 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -156,6 +156,10 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath return formatCompilerError(markdownPath, "error", err.Error(), err) } + // Emit warnings for push-to-pull-request-branch misconfiguration + log.Printf("Validating push-to-pull-request-branch configuration") + c.validatePushToPullRequestBranchWarnings(workflowData.SafeOutputs, workflowData.CheckoutConfigs) + // Validate network allowed domains configuration log.Printf("Validating network allowed domains") if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { diff --git a/pkg/workflow/push_to_pull_request_branch_validation.go b/pkg/workflow/push_to_pull_request_branch_validation.go new file mode 100644 index 00000000000..0f7ed7e27d5 --- /dev/null +++ b/pkg/workflow/push_to_pull_request_branch_validation.go @@ -0,0 +1,82 @@ +package workflow + +import ( + "fmt" + "os" + "strings" + + "github.com/github/gh-aw/pkg/console" +) + +var pushToPullRequestBranchValidationLog = newValidationLogger("push_to_pull_request_branch_validation") + +// validatePushToPullRequestBranchWarnings emits warnings for common misconfiguration +// patterns when push-to-pull-request-branch is used with target: "*". +// +// Two warnings are emitted when applicable: +// +// 1. No wildcard fetch in checkout — target: "*" allows pushing to any PR branch, but +// without a wildcard fetch pattern (e.g., fetch: ["*"]) the agent cannot access +// those branches at runtime. +// +// 2. No constraints — target: "*" without title-prefix or labels means the agent may +// push to any PR in the repository with no additional gating. +func (c *Compiler) validatePushToPullRequestBranchWarnings(safeOutputs *SafeOutputsConfig, checkoutConfigs []*CheckoutConfig) { + if safeOutputs == nil || safeOutputs.PushToPullRequestBranch == nil { + return + } + + cfg := safeOutputs.PushToPullRequestBranch + if cfg.Target != "*" { + return + } + + pushToPullRequestBranchValidationLog.Printf("Validating push-to-pull-request-branch with target: \"*\"") + + // Warning 1: no wildcard fetch pattern in any checkout configuration. + if !hasWildcardFetch(checkoutConfigs) { + msg := strings.Join([]string{ + "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout.", + "Your checkout configuration does not include a wildcard fetch pattern (e.g., fetch: [\"*\"]).", + "Without this the agent may fail to access PR branches when pushing.", + "", + "Add a wildcard fetch to your checkout configuration:", + "", + " checkout:", + " fetch: [\"*\"] # fetch all remote branches", + " fetch-depth: 0 # fetch full history", + }, "\n") + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) + c.IncrementWarningCount() + } + + // Warning 2: no constraints restricting which PRs can be targeted. + if cfg.TitlePrefix == "" && len(cfg.Labels) == 0 { + msg := strings.Join([]string{ + "push-to-pull-request-branch: target: \"*\" allows pushing to any PR branch with no additional constraints.", + "Consider adding title-prefix: or labels: to restrict which PRs can receive pushes.", + "", + "Example:", + "", + " push-to-pull-request-branch:", + " target: \"*\"", + " title-prefix: \"[bot] \" # only PRs whose title starts with this prefix", + " labels: [automated] # only PRs that carry all of these labels", + }, "\n") + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) + c.IncrementWarningCount() + } +} + +// hasWildcardFetch reports whether any checkout configuration includes a fetch pattern +// that contains a wildcard ("*"), such as fetch: ["*"] or fetch: ["feature/*"]. +func hasWildcardFetch(checkoutConfigs []*CheckoutConfig) bool { + for _, cfg := range checkoutConfigs { + for _, ref := range cfg.Fetch { + if strings.Contains(ref, "*") { + return true + } + } + } + return false +} diff --git a/pkg/workflow/push_to_pull_request_branch_warning_test.go b/pkg/workflow/push_to_pull_request_branch_warning_test.go new file mode 100644 index 00000000000..9e7db7db2c4 --- /dev/null +++ b/pkg/workflow/push_to_pull_request_branch_warning_test.go @@ -0,0 +1,329 @@ +//go:build !integration + +package workflow + +import ( + "bytes" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/testutil" +) + +// TestPushToPullRequestBranchWildcardFetchWarning tests that a warning is emitted +// when push-to-pull-request-branch has target: "*" but no wildcard fetch in checkout. +func TestPushToPullRequestBranchWildcardFetchWarning(t *testing.T) { + tests := []struct { + name string + content string + expectWarning bool + warningText string + }{ + { + name: "target=* without wildcard fetch emits warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " +--- + +# Test Workflow +`, + expectWarning: true, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + { + name: "target=* with fetch=[*] does not emit warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " +checkout: + fetch: ["*"] + fetch-depth: 0 +--- + +# Test Workflow +`, + expectWarning: false, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + { + name: "target=* with wildcard glob fetch does not emit warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " +checkout: + fetch: ["feature/*"] +--- + +# Test Workflow +`, + expectWarning: false, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + { + name: "target=triggering does not emit warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "triggering" +--- + +# Test Workflow +`, + expectWarning: false, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + { + name: "no push-to-pull-request-branch does not emit warning", + content: `--- +on: push +safe-outputs: + add-comment: + max: 1 +--- + +# Test Workflow +`, + expectWarning: false, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + { + name: "target=* with non-wildcard fetch emits warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " +checkout: + fetch: ["main"] +--- + +# Test Workflow +`, + expectWarning: true, + warningText: "push-to-pull-request-branch: target: \"*\" requires that all PR branches are fetched at checkout", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "push-pr-branch-fetch-warning-*") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + // Capture stderr to check for warnings + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + compiler := NewCompiler() + err := compiler.CompileWorkflow(testFile) + + // Restore stderr + w.Close() + os.Stderr = oldStderr + var buf bytes.Buffer + io.Copy(&buf, r) //nolint:errcheck + stderrOutput := buf.String() + + if err != nil { + t.Fatalf("Expected compilation to succeed but got error: %v", err) + } + + if tt.expectWarning { + if !strings.Contains(stderrOutput, tt.warningText) { + t.Errorf("Expected warning containing %q\ngot stderr:\n%s", tt.warningText, stderrOutput) + } + } else { + if strings.Contains(stderrOutput, tt.warningText) { + t.Errorf("Unexpected warning %q in stderr output:\n%s", tt.warningText, stderrOutput) + } + } + }) + } +} + +// TestPushToPullRequestBranchNoConstraintsWarning tests that a warning is emitted +// when push-to-pull-request-branch has target: "*" without title-prefix or labels. +func TestPushToPullRequestBranchNoConstraintsWarning(t *testing.T) { + tests := []struct { + name string + content string + expectWarning bool + }{ + { + name: "target=* without constraints emits warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" +checkout: + fetch: ["*"] + fetch-depth: 0 +--- + +# Test Workflow +`, + expectWarning: true, + }, + { + name: "target=* with title-prefix does not emit constraint warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " +checkout: + fetch: ["*"] + fetch-depth: 0 +--- + +# Test Workflow +`, + expectWarning: false, + }, + { + name: "target=* with labels does not emit constraint warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + labels: [automated] +checkout: + fetch: ["*"] + fetch-depth: 0 +--- + +# Test Workflow +`, + expectWarning: false, + }, + { + name: "target=* with both title-prefix and labels does not emit constraint warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "*" + title-prefix: "[bot] " + labels: [automated] +checkout: + fetch: ["*"] + fetch-depth: 0 +--- + +# Test Workflow +`, + expectWarning: false, + }, + { + name: "target=triggering without constraints does not emit warning", + content: `--- +on: push +safe-outputs: + push-to-pull-request-branch: + target: "triggering" +--- + +# Test Workflow +`, + expectWarning: false, + }, + } + + const constraintWarningText = "push-to-pull-request-branch: target: \"*\" allows pushing to any PR branch with no additional constraints" + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "push-pr-branch-constraint-warning-*") + testFile := filepath.Join(tmpDir, "test-workflow.md") + if err := os.WriteFile(testFile, []byte(tt.content), 0644); err != nil { + t.Fatal(err) + } + + // Capture stderr to check for warnings + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + compiler := NewCompiler() + err := compiler.CompileWorkflow(testFile) + + // Restore stderr + w.Close() + os.Stderr = oldStderr + var buf bytes.Buffer + io.Copy(&buf, r) //nolint:errcheck + stderrOutput := buf.String() + + if err != nil { + t.Fatalf("Expected compilation to succeed but got error: %v", err) + } + + if tt.expectWarning { + if !strings.Contains(stderrOutput, constraintWarningText) { + t.Errorf("Expected warning containing %q\ngot stderr:\n%s", constraintWarningText, stderrOutput) + } + } else { + if strings.Contains(stderrOutput, constraintWarningText) { + t.Errorf("Unexpected warning %q in stderr output:\n%s", constraintWarningText, stderrOutput) + } + } + }) + } +} + +// TestHasWildcardFetch tests the hasWildcardFetch helper function. +func TestHasWildcardFetch(t *testing.T) { + ref := func(s string) []string { return []string{s} } + + tests := []struct { + name string + configs []*CheckoutConfig + expected bool + }{ + {"nil configs", nil, false}, + {"empty configs", []*CheckoutConfig{}, false}, + {"no fetch", []*CheckoutConfig{{Repository: "owner/repo"}}, false}, + {"exact ref no wildcard", []*CheckoutConfig{{Fetch: ref("main")}}, false}, + {"star wildcard", []*CheckoutConfig{{Fetch: ref("*")}}, true}, + {"glob wildcard", []*CheckoutConfig{{Fetch: ref("feature/*")}}, true}, + {"multiple refs one wildcard", []*CheckoutConfig{{Fetch: []string{"main", "feature/*"}}}, true}, + {"multiple configs one with wildcard", []*CheckoutConfig{ + {Fetch: ref("main")}, + {Fetch: ref("*")}, + }, true}, + {"multiple configs none with wildcard", []*CheckoutConfig{ + {Fetch: ref("main")}, + {Fetch: ref("develop")}, + }, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := hasWildcardFetch(tt.configs) + if got != tt.expected { + t.Errorf("hasWildcardFetch() = %v, want %v", got, tt.expected) + } + }) + } +}