Skip to content

fix: label trigger shorthand missing label filter condition in compiled workflow #19824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Mar 6, 2026
Merged

fix: label trigger shorthand missing label filter condition in compiled workflow #19824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af9a3a1
Initial plan
Copilot Mar 6, 2026
0a82b55
fix: convert []string to []any for label names in expandLabelTriggerS…
Copilot Mar 6, 2026
6bdb7ac
fix: convert []string to []any for label names to fix label filter co…
Copilot Mar 6, 2026
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
5 changes: 4 additions & 1 deletion .github/workflows/go-pattern-detector.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 24 additions & 5 deletions .github/workflows/scout.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion .github/workflows/smoke-claude.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion pkg/workflow/label_trigger_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ func expandLabelTriggerShorthand(entityType string, labelNames []string) map[str
// All event types use `names` field for job condition filtering
// The `names` field is an internal representation for job condition generation
// and won't be rendered in the final GitHub Actions YAML for these event types
triggerConfig["names"] = labelNames
// Convert []string to []any so applyLabelFilter can type-assert correctly
namesAny := make([]any, len(labelNames))
for i, name := range labelNames {
namesAny[i] = name
}
triggerConfig["names"] = namesAny
Comment on lines +113 to +118
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

This conversion to []any works around applyLabelFilter only handling string and []any. However, it also makes the internal representation inconsistent with other Go code that might naturally use []string (and the applyLabelFilter comment in filters.go still claims it supports names: []string). Consider instead (or additionally) updating applyLabelFilter to accept []string too and/or update the documentation/comments to reflect the actual supported types, so future producers don’t have to remember to convert.

Copilot uses AI. Check for mistakes.

// Create workflow_dispatch with item_number input
workflowDispatchConfig := map[string]any{
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/label_trigger_parser_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ func FuzzExpandLabelTriggerShorthand(f *testing.F) {
// Check for names field (all event types use names for job condition filtering)
if names, hasNames := triggerMap["names"]; !hasNames {
t.Errorf("trigger missing names field for entityType=%q", entityType)
} else if namesArray, ok := names.([]string); !ok {
t.Errorf("names is not a string array for entityType=%q", entityType)
} else if namesArray, ok := names.([]any); !ok {
t.Errorf("names is not a []any for entityType=%q", entityType)
} else if len(namesArray) != len(labelNames) {
t.Errorf("names array length mismatch: got %d, want %d for entityType=%q", len(namesArray), len(labelNames), entityType)
}
Expand Down
11 changes: 9 additions & 2 deletions pkg/workflow/label_trigger_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,9 +379,16 @@ func TestExpandLabelTriggerShorthand(t *testing.T) {
// Check names field:
// All entity types use 'names' field for job condition filtering
// (GitHub Actions doesn't support native label filtering)
names, ok := triggerConfig["names"].([]string)
// names is stored as []any so applyLabelFilter can type-assert correctly
namesRaw, ok := triggerConfig["names"].([]any)
if !ok {
t.Fatalf("expandLabelTriggerShorthand() names is not a string array for %s", tt.entityType)
t.Fatalf("expandLabelTriggerShorthand() names is not a []any for %s", tt.entityType)
}
var names []string
for _, n := range namesRaw {
if s, isStr := n.(string); isStr {
names = append(names, s)
}
Comment on lines +387 to +391
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The loop building names silently drops non-string entries from namesRaw. Since names is expected to be a string list, this can mask incorrect types (the test could still pass if enough valid strings remain). Consider failing the test if any element is not a string and also assert the final length matches tt.labelNames to make the check strict.

Suggested change
var names []string
for _, n := range namesRaw {
if s, isStr := n.(string); isStr {
names = append(names, s)
}
if len(namesRaw) != len(tt.labelNames) {
t.Fatalf("expandLabelTriggerShorthand() names length = %d, want %d for %s", len(namesRaw), len(tt.labelNames), tt.entityType)
}
names := make([]string, len(namesRaw))
for i, n := range namesRaw {
s, isStr := n.(string)
if !isStr {
t.Fatalf("expandLabelTriggerShorthand() names[%d] is %T, want string for %s", i, n, tt.entityType)
}
names[i] = s

Copilot uses AI. Check for mistakes.
}
if !slicesEqual(names, tt.labelNames) {
t.Errorf("expandLabelTriggerShorthand() names = %v, want %v", names, tt.labelNames)
Expand Down
8 changes: 7 additions & 1 deletion pkg/workflow/schedule_preprocessing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,7 +1646,13 @@ func TestLabelTriggerShorthandPreprocessing(t *testing.T) {
t.Errorf("expected types=[labeled], got %v", types)
}
// names must match the expected labels
names, _ := triggerMap["names"].([]string)
namesRaw, _ := triggerMap["names"].([]any)
var names []string
for _, n := range namesRaw {
if s, isStr := n.(string); isStr {
names = append(names, s)
}
}
Comment on lines +1649 to +1655
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

In this test, the type assertion result for triggerMap["names"] is ignored (namesRaw, _ := ...). If the value is missing or not a []any, namesRaw will be nil and the test may produce a misleading failure (or even pass when wantLabelNames is empty). Please assert the cast succeeded and fail fast when it doesn’t, and also validate that every element in namesRaw is a string (don’t silently skip non-strings).

See below for a potential fix:

					typesRaw, ok := triggerMap["types"]
					if !ok {
						t.Fatalf("expected trigger %q to have 'types' key", tt.wantTriggerKey)
					}
					types, ok := typesRaw.([]any)
					if !ok {
						t.Fatalf("expected trigger %q 'types' to be []any, got %T", tt.wantTriggerKey, typesRaw)
					}
					if len(types) != 1 || types[0] != "labeled" {
						t.Errorf("expected types=[labeled], got %v", types)
					}
					// names must match the expected labels
					namesVal, ok := triggerMap["names"]
					if !ok {
						t.Fatalf("expected trigger %q to have 'names' key", tt.wantTriggerKey)
					}
					namesRaw, ok := namesVal.([]any)
					if !ok {
						t.Fatalf("expected trigger %q 'names' to be []any, got %T", tt.wantTriggerKey, namesVal)
					}
					var names []string
					for _, n := range namesRaw {
						s, isStr := n.(string)
						if !isStr {
							t.Fatalf("expected all trigger %q 'names' elements to be strings, got element of type %T", tt.wantTriggerKey, n)
						}
						names = append(names, s)

Copilot uses AI. Check for mistakes.
if !slicesEqual(names, tt.wantLabelNames) {
t.Errorf("expected names %v, got %v", tt.wantLabelNames, names)
}
Expand Down
Loading