fix: propagate yaml.Unmarshal errors in workflow_builder.go (5 silent sites)#41577
Conversation
… sites) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41577 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (75 additions detected, threshold is 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a failure mode in the workflow compiler where malformed YAML in step-merge inputs could be silently ignored, causing steps to disappear without surfacing an error. It updates step-merging logic to propagate yaml.Unmarshal failures (instead of treating them as “no steps”) and adjusts tests to assert the new behavior.
Changes:
- Propagate YAML parse errors for merged/imported steps and workflow step sections (steps, pre-steps, pre-agent-steps, post-steps) by returning contextual
fmt.Errorf(...)failures. - Preserve existing action-pinning behavior while ensuring invalid YAML no longer results in silent empty step lists.
- Update/rename the invalid-YAML test to assert the new error propagation for malformed
MergedSteps.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_builder.go | Converts five previously-silent yaml.Unmarshal sites into explicit error returns to prevent silent step loss. |
| pkg/workflow/compiler_orchestrator_workflow_test.go | Updates the invalid YAML test to expect an error when MergedSteps contains malformed YAML. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — commenting with suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: 4 of the 5 new
yaml.Unmarshalerror paths (CustomSteps, pre-steps, pre-agent-steps, post-steps) lack tests. OnlyMergedStepsis covered. - Residual silent failure:
SliceToStepserrors are still logged-and-dropped at all 5 sites, which is the same class of silent failure the PR fixes at the YAML-parse level. - Minor:
"failed to parse steps"is less specific than the other four error messages.
Positive Highlights
- ✅ Root cause correctly identified and fixed — all 5
err == nilguards converted toerr != nilearly returns - ✅ Error messages use
%wfor wrapping, enablingerrors.Is/errors.Asby callers - ✅ PR description clearly documents every site with a before/after example
- ✅ Test correctly flips the assertion from
NoError→Errorwith a message-content check - ✅ Code flattening (removing the unnecessary nesting) is a nice readability win
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.2 AIC · ⌖ 9.67 AIC · ⊞ 6.5K
| 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") | ||
| } |
There was a problem hiding this comment.
[/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.
| if steps, ok := mainVal.([]any); ok { | ||
| mainPreSteps = steps | ||
| typedMain, err := SliceToSteps(mainPreSteps) | ||
| if err := yaml.Unmarshal([]byte(mainPreStepsYAML), &mainWrapper); err != nil { |
There was a problem hiding this comment.
[/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.
| // 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) |
There was a problem hiding this comment.
[/diagnose] SliceToSteps failures are still silently logged and swallowed — the same silent-failure mode that motivated this PR now reappears one call downstream for all 5 sites.
💡 Context
After this fix, the error hierarchy is:
yaml.Unmarshalfails → now propagated ✅SliceToStepsfails → still logged and discarded ❌ (steps silently dropped)applyActionPinsToTypedStepsfails → already propagated ✅
If SliceToSteps returns an error, the step list is silently dropped and compilation continues as if no steps were parsed, which is the exact silent-failure behaviour this PR set out to fix.
Consider propagating this error too (all 5 sites), or at minimum documenting explicitly why a log-and-continue policy is intentional here.
| // Convert to typed steps for action pinning | ||
| typedMainSteps, err := SliceToSteps(mainSteps) | ||
| if err := yaml.Unmarshal([]byte(workflowData.CustomSteps), &mainStepsWrapper); err != nil { | ||
| return fmt.Errorf("failed to parse steps: %w", err) |
There was a problem hiding this comment.
[/diagnose] The error message "failed to parse steps" is generic compared to the others ("failed to parse imported steps", "failed to parse pre-steps", etc.), making it harder to identify which field triggered the failure.
💡 Suggestion
// Before
return fmt.Errorf("failed to parse steps: %w", err)
// After — matches the specificity of the other four messages
return fmt.Errorf("failed to parse custom steps: %w", err)This also keeps the error message consistent with the table in the PR description, which lists workflowData.CustomSteps as a distinct field.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( 📝 Coverage Observation — informational, not a failureThe PR title references 5 silent error-propagation sites fixed in
These paths now correctly propagate errors, but if the fix were accidentally reverted there would be no test to catch it. Adding table-driven cases for the remaining 4 sites is recommended. Verdict
|
There was a problem hiding this comment.
REQUEST_CHANGES — The unmarshal fix is correct in intent, but the implementation is incomplete and the test coverage is insufficient to trust the fix holds.
🔍 Blocking issues (3)
Missing tests for 4 of 5 changed error paths
The PR adds one test (TestProcessAndMergeSteps_InvalidYAML_MergedSteps) but leaves these four new error-return paths completely untested:
workflowData.CustomStepsinvalid YAML →processAndMergeStepsline 438–439mainPreStepsYAMLinvalid YAML →processAndMergePreStepsline 515–516mainPreAgentStepsYAMLinvalid YAML →processAndMergePreAgentStepsline 578–579mainPostStepsYAMLinvalid YAML →processAndMergePostStepsline 642–643
Any of these could silently revert to err == nil without any test catching the regression.
Intra-function asymmetry: imported vs main step errors (3 functions)
In processAndMergePreSteps, processAndMergePreAgentSteps, and processAndMergePostSteps, this PR propagates errors for the main step YAML but leaves the imported step YAML (MergedPreSteps, MergedPreAgentSteps, MergedPostSteps) using the old silent-log pattern. Both paths feed into the same final merge. A user debugging missing pre/post steps from imports gets a nil return and no diagnostic.
SliceToSteps failure silently skips action pinning
The de-nesting restructuring makes a pre-existing silent bypass clearly visible: when SliceToSteps fails, the entire applyActionPinsToTypedSteps branch is skipped and raw []any steps are emitted un-pinned. This needs either error propagation or an explicit best-effort comment.
🔎 Code quality review by PR Code Quality Reviewer · 106 AIC · ⌖ 7.01 AIC · ⊞ 5.2K
| // 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") |
There was a problem hiding this comment.
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.
| mainPreSteps = steps | ||
| typedMain, err := SliceToSteps(mainPreSteps) | ||
| if err := yaml.Unmarshal([]byte(mainPreStepsYAML), &mainWrapper); err != nil { | ||
| return fmt.Errorf("failed to parse pre-steps: %w", err) |
There was a problem hiding this comment.
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).
| // 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) |
There was a problem hiding this comment.
When SliceToSteps fails, action pinning is silently bypassed — the raw []any steps are emitted without SHA pinning.
💡 Details
The restructuring introduced by this PR makes the asymmetry visible for the first time:
yaml.Unmarshalfailure → fatal (line 417, propagated)SliceToStepsfailure → silent (line 422, logged and skipped)
If SliceToSteps returns an error, otherImportedSteps retains its raw []any value from Unmarshal, and the entire applyActionPinsToTypedSteps branch is skipped. Steps containing uses: directives will be written to the workflow un-pinned without any warning to the operator.
If action pinning is security-relevant (it usually is — pinning prevents dependency confusion attacks), this silent bypass should either propagate the error or be explicitly documented as an intentional "best-effort" choice with a comment explaining why.
The same pattern appears at lines 447, 522, 585, and 649 across the other merge functions.
|
@copilot run pr-finisher skill |
…s, fix error message specificity, add 3 tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nt unpinned action bypass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed pr-finisher pass. Changes made across two commits:
|
Five step-merge blocks in
workflow_builder.gousedif err := yaml.Unmarshal(...); err == nil { ... }, silently producing empty step lists when imported/custom step YAML was malformed — no error surfaced, steps disappeared.Changes
workflow_builder.go— All 5 sites converted from silenterr == nilguard to immediate error propagation:importsResult.MergedSteps"failed to parse imported steps: %w"workflowData.CustomSteps"failed to parse steps: %w"mainPreStepsYAML"failed to parse pre-steps: %w"mainPreAgentStepsYAML"failed to parse pre-agent-steps: %w"mainPostStepsYAML"failed to parse post-steps: %w"Before/after for the
MergedStepssite (representative of all 5):compiler_orchestrator_workflow_test.go— ReplacesTestProcessAndMergeSteps_InvalidYAML(previously assertedNoErrorfor malformedMergedSteps) withTestProcessAndMergeSteps_InvalidYAML_MergedStepsasserting the error is returned and contains the expected message.