fix(workflows): fail fan-out loudly on a truthy non-mapping step template#3537
Merged
mnriem merged 2 commits intoJul 17, 2026
Merged
Conversation
…late
A fan-out step whose `step:` is a truthy scalar or list (an authoring mistake) passed execute and reached the engine, which calls template.get("id", ...) in _run_fan_out — raising AttributeError and taking down the whole run. validate already rejects a non-mapping step, but the engine does not auto-validate, so an unvalidated run crashed.
Guard execute to FAIL the step (with a clear error and normalized empty output) instead, mirroring the existing non-list items guard and the switch non-dict cases guard. Add the matching test_execute_non_dict_step_fails_loudly covering the execute-path guard (validate was already covered).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents malformed fan-out step templates from crashing workflow runs.
Changes:
- Rejects non-dictionary fan-out templates during execution.
- Adds regression coverage for list and scalar templates.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/steps/fan_out/__init__.py |
Adds the runtime type guard. |
tests/test_workflows.py |
Tests malformed template handling. |
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: 1
- Review effort level: Medium
mnriem
requested changes
Jul 15, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Noor-ul-ain001
added a commit
to Noor-ul-ain001/spec-kit
that referenced
this pull request
Jul 17, 2026
fix(workflows): reject explicit fan-out `step: null` in validate()
The runtime guard in execute() rejects a truthy non-mapping step, but
`config.get("step", {})` only substitutes the `{}` default for an *absent*
key — an explicit `step: null` reaches the guard as None and FAILS the step.
validate() previously exempted None (`step is not None and ...`), so such a
workflow passed validation and then failed during execution.
Align validate() with the runtime guard: a present-but-non-mapping `step`
(including `None`) is an authoring mistake and is now rejected up front.
Extend the validate and execute regression cases to cover None.
Addresses Copilot review feedback on github#3537.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@
Noor-ul-ain001
added a commit
to Noor-ul-ain001/spec-kit
that referenced
this pull request
Jul 17, 2026
fix(workflows): reject explicit fan-out `step: null` in validate()
The runtime guard in execute() rejects a truthy non-mapping step, but
`config.get("step", {})` only substitutes the `{}` default for an *absent*
key — an explicit `step: null` reaches the guard as None and FAILS the step.
validate() previously exempted None (`step is not None and ...`), so such a
workflow passed validation and then failed during execution.
Align validate() with the runtime guard: a present-but-non-mapping `step`
(including `None`) is an authoring mistake and is now rejected up front.
Extend the validate and execute regression cases to cover None.
Addresses Copilot review feedback on github#3537.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@
Noor-ul-ain001
force-pushed
the
fix/fanout-nondict-template-crash
branch
from
July 17, 2026 05:12
af67f3d to
c3fa1bf
Compare
The runtime guard in execute() rejects a truthy non-mapping step, but
`config.get("step", {})` only substitutes the `{}` default for an *absent*
key — an explicit `step: null` reaches the guard as None and FAILS the step.
validate() previously exempted None (`step is not None and ...`), so such a
workflow passed validation and then failed during execution.
Align validate() with the runtime guard: a present-but-non-mapping `step`
(including `None`) is an authoring mistake and is now rejected up front.
Extend the validate and execute regression cases to cover None.
Addresses Copilot review feedback on github#3537.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Noor-ul-ain001
force-pushed
the
fix/fanout-nondict-template-crash
branch
from
July 17, 2026 05:18
c3fa1bf to
c3822b8
Compare
mnriem
self-requested a review
July 17, 2026 14:12
mnriem
approved these changes
Jul 17, 2026
Collaborator
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@
Summary
A fan-out step whose
step:field is a truthy scalar or list (an authoring mistake) passedexecute(), produced aCOMPLETEDresult, and reached the engine — which callstemplate.get("id", ...)in_run_fan_out(engine.py, guarded only byif template and items:). A non-dictstepraisedAttributeErrorthere, with no surrounding try/except, taking down the whole run instead of failing the single step.validate()already rejects a non-mappingstep, but the engine does not auto-validate (seeWorkflowEngine.load_workflow), so an unvalidated run crashed. This is the same bug class as the switch non-dictcasesand fan-in non-listwait_forguards.Changes
fan_out/__init__.py: guardexecute()toFAILthe step with a clear error and normalized empty output whenstepis not a mapping — mirroring the existing non-listitemsguard and the switchcasesguard.tests/test_workflows.py: addtest_execute_non_dict_step_fails_loudlycovering list/scalar/intstepvalues. (validatewas already covered bytest_validate_step_not_mapping; theexecute()-path guard was not.)Testing
All 25 fan-out tests pass locally.
🤖 Generated with Claude Code
@