Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/workflow/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
82 changes: 82 additions & 0 deletions pkg/workflow/push_to_pull_request_branch_validation.go
Original file line number Diff line number Diff line change
@@ -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")
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newValidationLogger already appends _validation to the logger name. Passing "push_to_pull_request_branch_validation" here produces a duplicated suffix (e.g., workflow:push_to_pull_request_branch_validation_validation) and diverges from the convention used by other *_validation.go files (e.g., agent_validation.go). Use a domain like "push_to_pull_request_branch" instead so the logger name is consistent.

Suggested change
var pushToPullRequestBranchValidationLog = newValidationLogger("push_to_pull_request_branch_validation")
var pushToPullRequestBranchValidationLog = newValidationLogger("push_to_pull_request_branch")

Copilot uses AI. Check for mistakes.

// 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
}
}
Comment on lines +71 to +79
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasWildcardFetch currently returns true for any pattern containing * (e.g., feature/*). That doesn’t satisfy the warning text (“requires that all PR branches are fetched”) and will suppress the warning even though many PR head branches would still be unavailable. Consider tightening the check to patterns that actually cover arbitrary PR branch names (e.g., "*", and possibly "refs/pulls/open/*" if that’s intended), and keep the warning message aligned with what the check guarantees.

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +79
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasWildcardFetch dereferences each element of checkoutConfigs without a nil check (cfg.Fetch). While ParseCheckoutConfigs returns non-nil entries today, this helper is generic and is unit-tested independently; a nil entry in the slice would panic. Add if cfg == nil { continue } (and optionally trim/normalize refs) to make the helper robust.

Copilot uses AI. Check for mistakes.
}
return false
}
Loading
Loading