fix(workflows): fail fan-out loudly on a truthy non-mapping step template#3537
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
fix(workflows): fail fan-out loudly on a truthy non-mapping step template#3537Noor-ul-ain001 wants to merge 1 commit into
Noor-ul-ain001 wants to merge 1 commit into
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
| # unvalidated run instead, mirroring the ``items`` guard below. An empty | ||
| # or absent ``step`` defaults to ``{}`` (falsy) and the engine's | ||
| # ``if template and items`` skips fan-out, so it stays valid here. | ||
| if not isinstance(step_template, dict): |
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
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
@