Merge pull request #158 from buildkite-plugins/SUP-4417-better-step-validation

Adds steps validation to prevent failed pipeline uploads

Author: mcncl

README.md

@@ -72,7 +72,48 @@ This is a sub-section that provides configuration for running commands or trigge
 - [Group](https://buildkite.com/docs/pipelines/configure/step-types/group-step)
 - [Conditionals](https://buildkite.com/docs/pipelines/conditionals)
 
-:warning: This plugin may accept configurations that are not valid pipeline steps, this is a known issue to keep its code simple and flexible.
+#### Step Validation
+
+The plugin validates all step configurations before uploading the pipeline. Invalid steps are automatically skipped with a warning logged to the build output.
+
+**A valid step must have:**
+- A `command` or `commands` field (for command steps), OR
+- A `trigger` field (for trigger steps), OR
+- A `group` field with either:
+  - An action (`command`, `commands`, or `trigger`) directly on the group, OR
+  - Valid nested `steps`
+
+**Invalid configurations that will be skipped:**
+
+```yaml
+# ❌ Empty step - no action defined
+- path: "app/"
+  config:
+    label: "Deploy app"  # Only has a label, no command/trigger
+
+# ❌ Empty group - no action and no nested steps
+- path: "services/"
+  config:
+    group: "Deploy"
+    # Missing: steps array or action
+```
+
+**Valid configurations:**
+
+```yaml
+# ✅ Valid - has command
+- path: "app/"
+  config:
+    label: "Deploy app"
+    command: "echo deploying"
+
+# ✅ Valid - group with nested steps
+- path: "services/"
+  config:
+    group: "Deploy"
+    steps:
+      - command: "deploy.sh"
+```
 
 ```yaml
 steps:
@@ -89,7 +130,7 @@ steps:
             - path: docker/
               config:
                 group: docker/**
-                steps:
+                steps:  # Required: groups must have either 'steps' or an action
                   - plugins:
                       - docker#latest:
                           build: service
@@ -489,6 +530,41 @@ steps:
                 command: "echo deploy-bar"
 ```
 
+## Troubleshooting
+
+### "Skipping invalid step" warnings
+
+If you see warnings like `Skipping invalid step: empty step configuration`, check that your step configuration includes:
+
+1. For command steps: `command` or `commands` field
+2. For trigger steps: `trigger` field
+3. For group steps: `group` field with either `steps` array or an action
+
+**Common issues:**
+
+- Forgetting to add `command:` or `trigger:` inside the `config` block
+- Creating empty groups without nested steps
+- Using only metadata fields like `label`, `key`, or `env` without an action
+
+**Example of fixing an invalid configuration:**
+
+```yaml
+# ❌ Invalid - missing action
+- path: "app/"
+  config:
+    label: "Deploy app"
+    env:
+      - ENV=production
+
+# ✅ Fixed - added command
+- path: "app/"
+  config:
+    label: "Deploy app"
+    command: "deploy.sh"
+    env:
+      - ENV=production
+```
+
 ## Compatibility
 
 | Elastic Stack | Agent Stack K8s | Hosted (Mac) | Hosted (Linux) | Notes |

pipeline.go

@@ -126,6 +126,38 @@ func diff(command string) ([]string, error) {
 	return paths, nil
 }
 
+// filterValidSteps splits steps into valid and invalid
+func filterValidSteps(steps []Step) (valid []Step, invalid []Step) {
+	valid = []Step{}
+	invalid = []Step{}
+
+	for _, step := range steps {
+		if step.isValid() {
+			valid = append(valid, step)
+		} else {
+			invalid = append(invalid, step)
+		}
+	}
+	return valid, invalid
+}
+
+// logInvalidStep logs why a step is invalid
+func logInvalidStep(step Step) {
+	context := "empty step configuration"
+
+	if step.Group != "" {
+		if len(step.Steps) == 0 {
+			context = fmt.Sprintf("group '%s' has no valid nested steps", step.Group)
+		} else {
+			context = fmt.Sprintf("group '%s' has invalid nested steps", step.Group)
+		}
+	} else if step.Label != "" {
+		context = fmt.Sprintf("step with label '%s' has no command, trigger, or group", step.Label)
+	}
+
+	log.Warnf("Skipping invalid step: %s. Steps must have at least one of: command, commands, trigger, or group with nested steps.", context)
+}
+
 func stepsToTrigger(files []string, watch []WatchConfig) ([]Step, error) {
 	steps := []Step{}
 	var defaultStep *Step
@@ -192,7 +224,15 @@ func stepsToTrigger(files []string, watch []WatchConfig) ([]Step, error) {
 		steps = append(steps, *defaultStep)
 	}
 
-	return dedupSteps(steps), nil
+	deduped := dedupSteps(steps)
+	valid, invalid := filterValidSteps(deduped)
+
+	// Log all invalid steps with helpful context
+	for _, step := range invalid {
+		logInvalidStep(step)
+	}
+
+	return valid, nil
 }
 
 // matchPath checks if the file f matches the path p.

pipeline_test.go

@@ -825,3 +825,204 @@ func TestGeneratePipelineWithSecretsInGroup(t *testing.T) {
 	assert.Contains(t, string(got), "- PROD_DB_HOST")
 	assert.Contains(t, string(got), "- PROD_DB_PASS")
 }
+
+func TestFilterValidSteps_AllValid(t *testing.T) {
+	steps := []Step{
+		{Command: "echo valid 1"},
+		{Trigger: "valid-trigger"},
+		{Commands: []string{"echo valid 2"}},
+	}
+
+	valid, invalid := filterValidSteps(steps)
+
+	assert.Len(t, valid, 3)
+	assert.Len(t, invalid, 0)
+}
+
+func TestFilterValidSteps_AllInvalid(t *testing.T) {
+	steps := []Step{
+		{},
+		{Label: "no command"},
+		{Env: map[string]string{"KEY": "value"}},
+	}
+
+	valid, invalid := filterValidSteps(steps)
+
+	assert.Len(t, valid, 0)
+	assert.Len(t, invalid, 3)
+}
+
+func TestFilterValidSteps_MixedValidInvalid(t *testing.T) {
+	steps := []Step{
+		{Command: "echo valid"},
+		{},
+		{Trigger: "valid-trigger"},
+		{Label: "invalid - no command/trigger"},
+		{Commands: []string{"echo also valid"}},
+	}
+
+	valid, invalid := filterValidSteps(steps)
+
+	assert.Len(t, valid, 3)
+	assert.Len(t, invalid, 2)
+	assert.Equal(t, "echo valid", valid[0].Command)
+	assert.Equal(t, "valid-trigger", valid[1].Trigger)
+	assert.NotNil(t, valid[2].Commands)
+}
+
+func TestFilterValidSteps_GroupSteps(t *testing.T) {
+	steps := []Step{
+		{
+			Group: "valid-group",
+			Steps: []Step{
+				{Command: "echo test"},
+			},
+		},
+		{
+			Group: "empty-group",
+			Steps: []Step{},
+		},
+		{
+			Group: "invalid-nested-group",
+			Steps: []Step{
+				{Label: "no command"},
+			},
+		},
+	}
+
+	valid, invalid := filterValidSteps(steps)
+
+	assert.Len(t, valid, 1)
+	assert.Len(t, invalid, 2)
+	assert.Equal(t, "valid-group", valid[0].Group)
+}
+
+func TestStepsToTrigger_Issue83(t *testing.T) {
+	// Integration test for issue #83 - empty step configuration
+	watch := []WatchConfig{
+		{
+			Paths: []string{"some-path/**"},
+			Step:  Step{}, // Empty step - should be filtered
+		},
+		{
+			Paths: []string{"other-path/**"},
+			Step:  Step{Command: "echo valid"},
+		},
+	}
+
+	changedFiles := []string{
+		"some-path/file.txt",
+		"other-path/file.txt",
+	}
+
+	steps, err := stepsToTrigger(changedFiles, watch)
+
+	assert.NoError(t, err)
+	assert.Len(t, steps, 1)
+	assert.Equal(t, "echo valid", steps[0].Command)
+}
+
+func TestStepsToTrigger_DefaultWithEmptyStep(t *testing.T) {
+	// Test that empty default step is filtered
+	watch := []WatchConfig{
+		{
+			Paths: []string{"app/"},
+			Step:  Step{Command: "echo app"},
+		},
+		{
+			Default: struct{}{},
+			Step:    Step{}, // Empty default - should be filtered
+		},
+	}
+
+	changedFiles := []string{"unmatched/file.txt"}
+
+	steps, err := stepsToTrigger(changedFiles, watch)
+
+	assert.NoError(t, err)
+	assert.Len(t, steps, 0)
+}
+
+func TestStepsToTrigger_AllStepsInvalid(t *testing.T) {
+	// Test that when all matched steps are invalid, empty array is returned
+	watch := []WatchConfig{
+		{
+			Paths: []string{"path1/"},
+			Step:  Step{},
+		},
+		{
+			Paths: []string{"path2/"},
+			Step:  Step{Label: "no command"},
+		},
+	}
+
+	changedFiles := []string{"path1/file.txt", "path2/file.txt"}
+
+	steps, err := stepsToTrigger(changedFiles, watch)
+
+	assert.NoError(t, err)
+	assert.Len(t, steps, 0)
+}
+
+func TestStepsToTrigger_EmptyGroupInConfig(t *testing.T) {
+	// Test that empty group steps are filtered
+	watch := []WatchConfig{
+		{
+			Paths: []string{"services/"},
+			Step: Step{
+				Group: "deploy",
+				Steps: []Step{}, // Empty group
+			},
+		},
+	}
+
+	changedFiles := []string{"services/main.go"}
+
+	steps, err := stepsToTrigger(changedFiles, watch)
+
+	assert.NoError(t, err)
+	assert.Len(t, steps, 0)
+}
+
+func TestStepsToTrigger_ValidAndInvalidStepsMixed(t *testing.T) {
+	// Test that valid steps are kept while invalid are filtered
+	watch := []WatchConfig{
+		{
+			Paths: []string{"app/"},
+			Step:  Step{Command: "echo valid app"},
+		},
+		{
+			Paths: []string{"tests/"},
+			Step:  Step{}, // Invalid
+		},
+		{
+			Paths: []string{"deploy/"},
+			Step:  Step{Trigger: "deploy-pipeline"},
+		},
+	}
+
+	changedFiles := []string{
+		"app/main.go",
+		"tests/test.go",
+		"deploy/script.sh",
+	}
+
+	steps, err := stepsToTrigger(changedFiles, watch)
+
+	assert.NoError(t, err)
+	assert.Len(t, steps, 2)
+
+	// Verify we got the valid steps
+	hasAppStep := false
+	hasDeployStep := false
+	for _, step := range steps {
+		if step.Command == "echo valid app" {
+			hasAppStep = true
+		}
+		if step.Trigger == "deploy-pipeline" {
+			hasDeployStep = true
+		}
+	}
+	assert.True(t, hasAppStep)
+	assert.True(t, hasDeployStep)
+}

plugin.go

@@ -99,6 +99,37 @@ type Step struct {
 	Steps         []Step                   `yaml:"steps,omitempty"`
 }
 
+// isValid checks if a step has required fields (command, trigger, or group with steps)
+func (s Step) isValid() bool {
+	if s.Group != "" {
+		return s.hasValidNesting()
+	}
+	return s.hasAction()
+}
+
+// hasAction checks if a step has a command or trigger
+func (s Step) hasAction() bool {
+	return s.Command != nil || s.Commands != nil || s.Trigger != ""
+}
+
+// hasValidNesting validates group step nesting
+func (s Step) hasValidNesting() bool {
+	if s.hasAction() {
+		return true
+	}
+
+	if len(s.Steps) > 0 {
+		for _, nestedStep := range s.Steps {
+			if !nestedStep.isValid() {
+				return false
+			}
+		}
+		return true
+	}
+
+	return false
+}
+
 // UnmarshalJSON handles both "artifacts" and "artifact_paths" field names for backward compatibility
 // Both fields are supported by the Buildkite API; "artifact_paths" is preferred per documentation
 func (step *Step) UnmarshalJSON(data []byte) error {