From 16c1614dd8ba62755e8eedf41a35a0bde9767890 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Jun 2026 06:44:09 +0000 Subject: [PATCH 1/3] Initial plan From 3366c8bae1c54daf1a353ffec58af029b21621b8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Jun 2026 06:50:57 +0000 Subject: [PATCH 2/3] Refactor duplicate glob list validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_filters_validation.go | 34 +++++++------------ pkg/workflow/glob_validation.go | 22 ++++++++++++ pkg/workflow/safe_outputs_validation.go | 13 ++----- ...puts_validation_merge_pull_request_test.go | 12 +++++++ 4 files changed, 50 insertions(+), 31 deletions(-) diff --git a/pkg/workflow/compiler_filters_validation.go b/pkg/workflow/compiler_filters_validation.go index 067d13b67de..ba785f11089 100644 --- a/pkg/workflow/compiler_filters_validation.go +++ b/pkg/workflow/compiler_filters_validation.go @@ -197,26 +197,18 @@ func validateGlobList(eventMap map[string]any, eventName, filterKey string, isPa return nil } - for _, pat := range patterns { - var errs []invalidGlobPattern - if isPath { - errs = validatePathGlob(pat) - } else { - errs = validateRefGlob(pat) - } - if len(errs) > 0 { - msgs := make([]string, 0, len(errs)) - for _, e := range errs { - msgs = append(msgs, e.Message) - } - filterValidationLog.Printf("ERROR: invalid glob pattern %q in %s.%s: %s", pat, eventName, filterKey, strings.Join(msgs, "; ")) - return NewValidationError( - fmt.Sprintf("on.%s.%s", eventName, filterKey), - pat, - "expected a valid GitHub Actions glob pattern: "+strings.Join(msgs, "; "), - fmt.Sprintf("Use valid glob syntax for on.%s.%s:\n\non:\n %s:\n %s:\n - src/**", eventName, filterKey, eventName, filterKey), - ) - } + validate := validateRefGlob + if isPath { + validate = validatePathGlob } - return nil + + return validateGlobPatternList(patterns, validate, func(_ int, pat string, msgs []string) error { + filterValidationLog.Printf("ERROR: invalid glob pattern %q in %s.%s: %s", pat, eventName, filterKey, strings.Join(msgs, "; ")) + return NewValidationError( + fmt.Sprintf("on.%s.%s", eventName, filterKey), + pat, + "expected a valid GitHub Actions glob pattern: "+strings.Join(msgs, "; "), + fmt.Sprintf("Use valid glob syntax for on.%s.%s:\n\non:\n %s:\n %s:\n - src/**", eventName, filterKey, eventName, filterKey), + ) + }) } diff --git a/pkg/workflow/glob_validation.go b/pkg/workflow/glob_validation.go index da2d682db01..755d84f69d6 100644 --- a/pkg/workflow/glob_validation.go +++ b/pkg/workflow/glob_validation.go @@ -217,6 +217,28 @@ func runGlobValidation(pat string, isRef bool) []invalidGlobPattern { return v.errs } +func validateGlobPatternList( + patterns []string, + validate func(string) []invalidGlobPattern, + onInvalid func(index int, pattern string, messages []string) error, +) error { + for i, pat := range patterns { + errs := validate(pat) + if len(errs) == 0 { + continue + } + + msgs := make([]string, 0, len(errs)) + for _, err := range errs { + msgs = append(msgs, err.Message) + } + + return onInvalid(i, pat, msgs) + } + + return nil +} + // validateRefGlob validates a GitHub Actions ref filter glob (branch or tag pattern). // It returns a non-empty slice of invalidGlobPattern when the pattern is invalid. func validateRefGlob(pat string) []invalidGlobPattern { diff --git a/pkg/workflow/safe_outputs_validation.go b/pkg/workflow/safe_outputs_validation.go index aac2865c889..dbf02a50b72 100644 --- a/pkg/workflow/safe_outputs_validation.go +++ b/pkg/workflow/safe_outputs_validation.go @@ -232,16 +232,9 @@ func validateSafeOutputsMergePullRequest(config *SafeOutputsConfig) error { } validateRefGlobList := func(field string, patterns []string) error { - for i, pat := range patterns { - if errs := validateRefGlob(pat); len(errs) > 0 { - msgs := make([]string, 0, len(errs)) - for _, e := range errs { - msgs = append(msgs, e.Message) - } - return fmt.Errorf("invalid glob pattern %q in safe-outputs.merge-pull-request.%s[%d]: %s", pat, field, i, strings.Join(msgs, "; ")) - } - } - return nil + return validateGlobPatternList(patterns, validateRefGlob, func(i int, pat string, msgs []string) error { + return fmt.Errorf("invalid glob pattern %q in safe-outputs.merge-pull-request.%s[%d]: %s", pat, field, i, strings.Join(msgs, "; ")) + }) } if err := validateNonEmptyStringList("required-labels", c.RequiredLabels); err != nil { diff --git a/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go b/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go index 230860b1ec0..7357770d204 100644 --- a/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go +++ b/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go @@ -47,3 +47,15 @@ func TestValidateSafeOutputsMergePullRequestLabelValidation(t *testing.T) { }) } } + +func TestValidateSafeOutputsMergePullRequestAllowedBranchesValidation(t *testing.T) { + config := &SafeOutputsConfig{ + MergePullRequest: &MergePullRequestConfig{ + AllowedBranches: []string{"feature branch"}, + }, + } + + err := validateSafeOutputsMergePullRequest(config) + require.Error(t, err, "expected merge-pull-request allowed-branches validation to fail") + assert.Contains(t, err.Error(), `invalid glob pattern "feature branch" in safe-outputs.merge-pull-request.allowed-branches[0]`, "expected field-specific allowed-branches error") +} From e20a4447f4f75c1f3df6a6941a41d403c9a34c59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Jun 2026 14:47:25 +0000 Subject: [PATCH 3/3] Address review feedback on glob helper docs and tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/glob_validation.go | 11 +++++ ...puts_validation_merge_pull_request_test.go | 49 ++++++++++++++++--- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/pkg/workflow/glob_validation.go b/pkg/workflow/glob_validation.go index 755d84f69d6..98521af83f2 100644 --- a/pkg/workflow/glob_validation.go +++ b/pkg/workflow/glob_validation.go @@ -11,6 +11,7 @@ // // - validateRefGlob() - Validates a glob pattern for Git ref names (branches/tags) // - validatePathGlob() - Validates a glob pattern for file paths +// - validateGlobPatternList() - Iterates and validates a list of glob patterns // // Both return a slice of invalidGlobPattern describing any violations found. @@ -217,6 +218,16 @@ func runGlobValidation(pat string, isRef bool) []invalidGlobPattern { return v.errs } +// validateGlobPatternList validates patterns in order and stops at the first +// invalid entry. +// +// For that first invalid pattern, onInvalid receives the list index, the pattern +// value, and the collected validation messages. The return value from onInvalid +// is returned directly to the caller, and onInvalid may include side effects +// such as logging. +// +// Contract: onInvalid must return a non-nil error when called. Returning nil is +// treated as success. func validateGlobPatternList( patterns []string, validate func(string) []invalidGlobPattern, diff --git a/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go b/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go index 7357770d204..17112d00052 100644 --- a/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go +++ b/pkg/workflow/safe_outputs_validation_merge_pull_request_test.go @@ -49,13 +49,50 @@ func TestValidateSafeOutputsMergePullRequestLabelValidation(t *testing.T) { } func TestValidateSafeOutputsMergePullRequestAllowedBranchesValidation(t *testing.T) { - config := &SafeOutputsConfig{ - MergePullRequest: &MergePullRequestConfig{ - AllowedBranches: []string{"feature branch"}, + tests := []struct { + name string + config *SafeOutputsConfig + wantErr string + }{ + { + name: "invalid pattern at index 0 fails", + config: &SafeOutputsConfig{ + MergePullRequest: &MergePullRequestConfig{ + AllowedBranches: []string{"feature branch"}, + }, + }, + wantErr: `invalid glob pattern "feature branch" in safe-outputs.merge-pull-request.allowed-branches[0]`, + }, + { + name: "invalid pattern at index 1 reports index 1", + config: &SafeOutputsConfig{ + MergePullRequest: &MergePullRequestConfig{ + AllowedBranches: []string{"main", "feature branch"}, + }, + }, + wantErr: `invalid glob pattern "feature branch" in safe-outputs.merge-pull-request.allowed-branches[1]`, + }, + { + name: "valid patterns pass", + config: &SafeOutputsConfig{ + MergePullRequest: &MergePullRequestConfig{ + AllowedBranches: []string{"main", "feature/*"}, + }, + }, + wantErr: "", }, } - err := validateSafeOutputsMergePullRequest(config) - require.Error(t, err, "expected merge-pull-request allowed-branches validation to fail") - assert.Contains(t, err.Error(), `invalid glob pattern "feature branch" in safe-outputs.merge-pull-request.allowed-branches[0]`, "expected field-specific allowed-branches error") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateSafeOutputsMergePullRequest(tt.config) + if tt.wantErr == "" { + assert.NoError(t, err, "expected merge-pull-request allowed-branches validation to pass") + return + } + + require.Error(t, err, "expected merge-pull-request allowed-branches validation to fail") + assert.Contains(t, err.Error(), tt.wantErr, "expected field-specific allowed-branches error") + }) + } }