Skip to content

fix: propagate yaml.Unmarshal errors in workflow_builder.go (5 silent sites) #41577

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 4 commits into from
Jun 26, 2026
Merged

fix: propagate yaml.Unmarshal errors in workflow_builder.go (5 silent sites) #41577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0960590
Initial plan
Copilot Jun 26, 2026
f089e7c
fix: propagate yaml.Unmarshal errors in workflow_builder.go (5 silent…
Copilot Jun 26, 2026
d52c536
fix: address review feedback - propagate imported pre/post step error…
Copilot Jun 26, 2026
bda3bb6
fix: propagate SliceToSteps errors at all 7 step-merge sites to preve…
Copilot Jun 26, 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
77 changes: 69 additions & 8 deletions pkg/workflow/compiler_orchestrator_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1807,9 +1807,9 @@ func TestMergeJobsFromYAMLImports_PreservesJobOrder(t *testing.T) {
assert.Contains(t, result, "job-d")
}

// TestProcessAndMergeSteps_InvalidYAML tests handling of invalid YAML in imported steps
func TestProcessAndMergeSteps_InvalidYAML(t *testing.T) {
tmpDir := testutil.TempDir(t, "invalid-steps-yaml")
// TestProcessAndMergeSteps_InvalidYAML_MergedSteps tests that malformed MergedSteps YAML returns an error
func TestProcessAndMergeSteps_InvalidYAML_MergedSteps(t *testing.T) {
tmpDir := testutil.TempDir(t, "invalid-merged-steps-yaml")
compiler := NewCompiler()
actionCache := NewActionCache(tmpDir)
actionResolver := NewActionResolver(actionCache)
Expand All @@ -1829,12 +1829,73 @@ func TestProcessAndMergeSteps_InvalidYAML(t *testing.T) {
MergedSteps: "invalid: [yaml",
}

// Should handle gracefully without panicking
require.NoError(t, compiler.processAndMergeSteps(frontmatter, workflowData, importsResult))
// Malformed MergedSteps YAML must propagate as an error
err := compiler.processAndMergeSteps(frontmatter, workflowData, importsResult)
require.Error(t, err, "malformed MergedSteps YAML should return an error")
assert.Contains(t, err.Error(), "failed to parse imported steps")

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Four of the five changed error paths have no test coverage, leaving the fix fragile.

💡 Paths that need tests

This PR fixes 5 unmarshal sites, but only MergedSteps gets a test. The other four changed return paths have no assertions:

Error path Location
workflowData.CustomSteps invalid YAML workflow_builder.go line 438–439
mainPreStepsYAML invalid YAML processAndMergePreSteps line 515–516
mainPreAgentStepsYAML invalid YAML processAndMergePreAgentSteps line 578–579
mainPostStepsYAML invalid YAML processAndMergePostSteps line 642–643

Any of these could silently revert to err == nil in a refactor and nothing would catch it. Add a test per error site following the same require.Error + assert.Contains pattern used here.

}

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Only the MergedSteps path is tested; the CustomSteps error path (also in processAndMergeSteps) has no test.

💡 Suggested additional test

Add a companion test alongside this one:

// TestProcessAndMergeSteps_InvalidYAML_CustomSteps tests malformed CustomSteps YAML returns an error
func TestProcessAndMergeSteps_InvalidYAML_CustomSteps(t *testing.T) {
	tmpDir := testutil.TempDir(t, "invalid-custom-steps-yaml")
	compiler := NewCompiler()
	actionCache := NewActionCache(tmpDir)
	actionResolver := NewActionResolver(actionCache)
	workflowData := &WorkflowData{
		ActionCache:    actionCache,
		ActionResolver: actionResolver,
		CustomSteps:    "steps: [invalid: yaml",
	}
	frontmatter := map[string]any{}
	importsResult := &parser.ImportsResult{}

	err := compiler.processAndMergeSteps(frontmatter, workflowData, importsResult)
	require.Error(t, err)
	assert.Contains(t, err.Error(), "failed to parse steps")
}

A regression on the CustomSteps path would otherwise go undetected.


// Should still have main steps
assert.NotEmpty(t, workflowData.CustomSteps)
assert.Contains(t, workflowData.CustomSteps, "Main")
// TestProcessAndMergePreSteps_InvalidYAML tests that malformed imported pre-steps YAML returns an error
func TestProcessAndMergePreSteps_InvalidYAML(t *testing.T) {
tmpDir := testutil.TempDir(t, "invalid-pre-steps-yaml")
compiler := NewCompiler()
actionCache := NewActionCache(tmpDir)
actionResolver := NewActionResolver(actionCache)
workflowData := &WorkflowData{
ActionCache: actionCache,
ActionResolver: actionResolver,
}

frontmatter := map[string]any{}
importsResult := &parser.ImportsResult{
MergedPreSteps: "invalid: [yaml",
}

err := compiler.processAndMergePreSteps(frontmatter, workflowData, importsResult)
require.Error(t, err, "malformed imported pre-steps YAML should return an error")
assert.Contains(t, err.Error(), "failed to parse imported pre-steps")
}

// TestProcessAndMergePreAgentSteps_InvalidYAML tests that malformed imported pre-agent-steps YAML returns an error
func TestProcessAndMergePreAgentSteps_InvalidYAML(t *testing.T) {
tmpDir := testutil.TempDir(t, "invalid-pre-agent-steps-yaml")
compiler := NewCompiler()
actionCache := NewActionCache(tmpDir)
actionResolver := NewActionResolver(actionCache)
workflowData := &WorkflowData{
ActionCache: actionCache,
ActionResolver: actionResolver,
}

frontmatter := map[string]any{}
importsResult := &parser.ImportsResult{
MergedPreAgentSteps: "invalid: [yaml",
}

err := compiler.processAndMergePreAgentSteps(frontmatter, workflowData, importsResult)
require.Error(t, err, "malformed imported pre-agent-steps YAML should return an error")
assert.Contains(t, err.Error(), "failed to parse imported pre-agent-steps")
}

// TestProcessAndMergePostSteps_InvalidYAML tests that malformed imported post-steps YAML returns an error
func TestProcessAndMergePostSteps_InvalidYAML(t *testing.T) {
tmpDir := testutil.TempDir(t, "invalid-post-steps-yaml")
compiler := NewCompiler()
actionCache := NewActionCache(tmpDir)
actionResolver := NewActionResolver(actionCache)
workflowData := &WorkflowData{
ActionCache: actionCache,
ActionResolver: actionResolver,
}

frontmatter := map[string]any{}
importsResult := &parser.ImportsResult{
MergedPostSteps: "invalid: [yaml",
}

err := compiler.processAndMergePostSteps(frontmatter, workflowData, importsResult)
require.Error(t, err, "malformed imported post-steps YAML should return an error")
assert.Contains(t, err.Error(), "failed to parse imported post-steps")
}

// TestProcessAndMergeServices_EmptyImportedServices tests handling of empty imported services
Expand Down
212 changes: 103 additions & 109 deletions pkg/workflow/workflow_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,45 +413,45 @@ func (c *Compiler) processAndMergeSteps(frontmatter map[string]any, workflowData
// Parse other imported steps if present (these go after copilot-setup but before main steps)
var otherImportedSteps []any
if importsResult.MergedSteps != "" {
if err := yaml.Unmarshal([]byte(importsResult.MergedSteps), &otherImportedSteps); err == nil {
// Convert to typed steps for action pinning
typedOtherSteps, err := SliceToSteps(otherImportedSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert other imported steps to typed steps: %v", err)
} else {
// Apply action pinning to other imported steps
typedOtherSteps, err = applyActionPinsToTypedSteps(typedOtherSteps, workflowData)
if err != nil {
return fmt.Errorf("imported steps: %w", err)
}
// Convert back to []any for YAML marshaling
otherImportedSteps = StepsToSlice(typedOtherSteps)
}
if err := yaml.Unmarshal([]byte(importsResult.MergedSteps), &otherImportedSteps); err != nil {
return fmt.Errorf("failed to parse imported steps: %w", err)
}
// Convert to typed steps for action pinning
typedOtherSteps, err := SliceToSteps(otherImportedSteps)
if err != nil {
return fmt.Errorf("failed to convert imported steps: %w", err)
}
// Apply action pinning to other imported steps
typedOtherSteps, err = applyActionPinsToTypedSteps(typedOtherSteps, workflowData)
if err != nil {
return fmt.Errorf("imported steps: %w", err)
}
// Convert back to []any for YAML marshaling
otherImportedSteps = StepsToSlice(typedOtherSteps)
}

// If there are main workflow steps, parse them
var mainSteps []any
if workflowData.CustomSteps != "" {
var mainStepsWrapper map[string]any
if err := yaml.Unmarshal([]byte(workflowData.CustomSteps), &mainStepsWrapper); err == nil {
if mainStepsVal, hasSteps := mainStepsWrapper["steps"]; hasSteps {
if steps, ok := mainStepsVal.([]any); ok {
mainSteps = steps
// Convert to typed steps for action pinning
typedMainSteps, err := SliceToSteps(mainSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert main steps to typed steps: %v", err)
} else {
// Apply action pinning to main steps
typedMainSteps, err = applyActionPinsToTypedSteps(typedMainSteps, workflowData)
if err != nil {
return fmt.Errorf("steps: %w", err)
}
// Convert back to []any for YAML marshaling
mainSteps = StepsToSlice(typedMainSteps)
}
if err := yaml.Unmarshal([]byte(workflowData.CustomSteps), &mainStepsWrapper); err != nil {
return fmt.Errorf("failed to parse custom steps: %w", err)
}
if mainStepsVal, hasSteps := mainStepsWrapper["steps"]; hasSteps {
if steps, ok := mainStepsVal.([]any); ok {
mainSteps = steps
// Convert to typed steps for action pinning
typedMainSteps, err := SliceToSteps(mainSteps)
if err != nil {
return fmt.Errorf("failed to convert main steps: %w", err)
}
// Apply action pinning to main steps
typedMainSteps, err = applyActionPinsToTypedSteps(typedMainSteps, workflowData)
if err != nil {
return fmt.Errorf("steps: %w", err)
}
// Convert back to []any for YAML marshaling
mainSteps = StepsToSlice(typedMainSteps)
}
}
}
Expand Down Expand Up @@ -491,40 +491,38 @@ func (c *Compiler) processAndMergePreSteps(frontmatter map[string]any, workflowD
var importedPreSteps []any
if importsResult.MergedPreSteps != "" {
if err := yaml.Unmarshal([]byte(importsResult.MergedPreSteps), &importedPreSteps); err != nil {
workflowBuilderLog.Printf("Failed to unmarshal imported pre-steps: %v", err)
} else {
typedImported, err := SliceToSteps(importedPreSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert imported pre-steps to typed steps: %v", err)
} else {
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported pre-steps: %w", err)
}
importedPreSteps = StepsToSlice(typedImported)
}
return fmt.Errorf("failed to parse imported pre-steps: %w", err)
}
typedImported, err := SliceToSteps(importedPreSteps)
if err != nil {
return fmt.Errorf("failed to convert imported pre-steps: %w", err)
}
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported pre-steps: %w", err)
}
importedPreSteps = StepsToSlice(typedImported)
}

// Parse main workflow pre-steps if present
var mainPreSteps []any
if mainPreStepsYAML != "" {
var mainWrapper map[string]any
if err := yaml.Unmarshal([]byte(mainPreStepsYAML), &mainWrapper); err == nil {
if mainVal, ok := mainWrapper["pre-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPreSteps = steps
typedMain, err := SliceToSteps(mainPreSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert main pre-steps to typed steps: %v", err)
} else {
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("pre-steps: %w", err)
}
mainPreSteps = StepsToSlice(typedMain)
}
if err := yaml.Unmarshal([]byte(mainPreStepsYAML), &mainWrapper); err != nil {

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] processAndMergePreSteps (and the analogous processAndMergePreAgentSteps / processAndMergePostSteps) now propagate YAML parse errors, but none of these three new error paths have tests.

💡 What to add

For each of the three functions, a minimal test like this is sufficient:

func TestProcessAndMergePreSteps_InvalidYAML(t *testing.T) {
	compiler := NewCompiler()
	// ... minimal workflowData setup ...
	// supply malformed YAML via the field that feeds mainPreStepsYAML
	err := compiler.processAndMergePreSteps(frontmatter, workflowData, importsResult)
	require.Error(t, err)
	assert.Contains(t, err.Error(), "failed to parse pre-steps")
}

Three quick tests (pre-steps, pre-agent-steps, post-steps) complete the coverage started by TestProcessAndMergeSteps_InvalidYAML_MergedSteps.

return fmt.Errorf("failed to parse pre-steps: %w", err)

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function now has two conflicting error strategies in the same pipeline: mainPreStepsYAML parse failure returns an error (this line), but MergedPreSteps parse failure (lines 495–496) is silently logged and swallowed.

💡 Why this matters

Both importedPreSteps and mainPreSteps flow into the same allPreSteps merge at line 537. Their YAML is structurally equivalent — yet a malformed main pre-steps block now aborts compilation with a clear error, while a malformed imported pre-steps block silently produces empty importedPreSteps and returns nil.

An operator whose imported workflow has malformed pre-steps YAML will see steps silently vanish with no diagnostic. The fix should be symmetric: either propagate both, or have a documented reason why one is tolerated and the other is fatal.

The same asymmetry exists in processAndMergePreAgentSteps (line 579 vs lines 559–560) and processAndMergePostSteps (line 643 vs lines 622–623).

}
if mainVal, ok := mainWrapper["pre-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPreSteps = steps
typedMain, err := SliceToSteps(mainPreSteps)
if err != nil {
return fmt.Errorf("failed to convert pre-steps: %w", err)
}
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("pre-steps: %w", err)
}
mainPreSteps = StepsToSlice(typedMain)
}
}
}
Expand Down Expand Up @@ -554,39 +552,37 @@ func (c *Compiler) processAndMergePreAgentSteps(frontmatter map[string]any, work
var importedPreAgentSteps []any
if importsResult.MergedPreAgentSteps != "" {
if err := yaml.Unmarshal([]byte(importsResult.MergedPreAgentSteps), &importedPreAgentSteps); err != nil {
workflowBuilderLog.Printf("Failed to unmarshal imported pre-agent-steps: %v", err)
} else {
typedImported, err := SliceToSteps(importedPreAgentSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert imported pre-agent-steps to typed steps: %v", err)
} else {
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported pre-agent-steps: %w", err)
}
importedPreAgentSteps = StepsToSlice(typedImported)
}
return fmt.Errorf("failed to parse imported pre-agent-steps: %w", err)
}
typedImported, err := SliceToSteps(importedPreAgentSteps)
if err != nil {
return fmt.Errorf("failed to convert imported pre-agent-steps: %w", err)
}
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported pre-agent-steps: %w", err)
}
importedPreAgentSteps = StepsToSlice(typedImported)
}

var mainPreAgentSteps []any
if mainPreAgentStepsYAML != "" {
var mainWrapper map[string]any
if err := yaml.Unmarshal([]byte(mainPreAgentStepsYAML), &mainWrapper); err == nil {
if mainVal, ok := mainWrapper["pre-agent-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPreAgentSteps = steps
typedMain, err := SliceToSteps(mainPreAgentSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert main pre-agent-steps to typed steps: %v", err)
} else {
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("pre-agent-steps: %w", err)
}
mainPreAgentSteps = StepsToSlice(typedMain)
}
if err := yaml.Unmarshal([]byte(mainPreAgentStepsYAML), &mainWrapper); err != nil {
return fmt.Errorf("failed to parse pre-agent-steps: %w", err)
}
if mainVal, ok := mainWrapper["pre-agent-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPreAgentSteps = steps
typedMain, err := SliceToSteps(mainPreAgentSteps)
if err != nil {
return fmt.Errorf("failed to convert pre-agent-steps: %w", err)
}
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("pre-agent-steps: %w", err)
}
mainPreAgentSteps = StepsToSlice(typedMain)
}
}
}
Expand Down Expand Up @@ -616,40 +612,38 @@ func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflow
var importedPostSteps []any
if importsResult.MergedPostSteps != "" {
if err := yaml.Unmarshal([]byte(importsResult.MergedPostSteps), &importedPostSteps); err != nil {
workflowBuilderLog.Printf("Failed to unmarshal imported post-steps: %v", err)
} else {
typedImported, err := SliceToSteps(importedPostSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert imported post-steps to typed steps: %v", err)
} else {
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported post-steps: %w", err)
}
importedPostSteps = StepsToSlice(typedImported)
}
return fmt.Errorf("failed to parse imported post-steps: %w", err)
}
typedImported, err := SliceToSteps(importedPostSteps)
if err != nil {
return fmt.Errorf("failed to convert imported post-steps: %w", err)
}
typedImported, err = applyActionPinsToTypedSteps(typedImported, workflowData)
if err != nil {
return fmt.Errorf("imported post-steps: %w", err)
}
importedPostSteps = StepsToSlice(typedImported)
}

// Parse main workflow post-steps if present
var mainPostSteps []any
if mainPostStepsYAML != "" {
var mainWrapper map[string]any
if err := yaml.Unmarshal([]byte(mainPostStepsYAML), &mainWrapper); err == nil {
if mainVal, ok := mainWrapper["post-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPostSteps = steps
typedMain, err := SliceToSteps(mainPostSteps)
if err != nil {
workflowBuilderLog.Printf("Failed to convert main post-steps to typed steps: %v", err)
} else {
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("post-steps: %w", err)
}
mainPostSteps = StepsToSlice(typedMain)
}
if err := yaml.Unmarshal([]byte(mainPostStepsYAML), &mainWrapper); err != nil {
return fmt.Errorf("failed to parse post-steps: %w", err)
}
if mainVal, ok := mainWrapper["post-steps"]; ok {
if steps, ok := mainVal.([]any); ok {
mainPostSteps = steps
typedMain, err := SliceToSteps(mainPostSteps)
if err != nil {
return fmt.Errorf("failed to convert post-steps: %w", err)
}
typedMain, err = applyActionPinsToTypedSteps(typedMain, workflowData)
if err != nil {
return fmt.Errorf("post-steps: %w", err)
}
mainPostSteps = StepsToSlice(typedMain)
}
}
}
Expand Down
Loading