fix(workflows): fail while/do-while steps on non-list steps instead of crashing#3519
Merged
Merged
Conversation
…f crashing `WhileStep.validate()` and `DoWhileStep.validate()` already reject a non-list `steps` body, but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run the body is returned as `next_steps`, and the engine feeds it straight into `_execute_steps`, which iterates it as step mappings. A non-list `steps` — a single mapping or scalar authoring mistake — was iterated element-wise (a dict yields its string keys, a str its characters) and raised `AttributeError` on `.get()`, taking down the whole run; the engine invokes `step_impl.execute()` with no surrounding try/except. Guard both `execute` paths to return a FAILED StepResult naming the type error instead, mirroring the if/switch non-list-branch and fan-out non-list `items` handling. The do-while body always dispatches on the first call, so its guard is unconditional; the while body only dispatches when the condition is truthy, so its guard fires only then — a false condition leaves a non-list `steps` benign and the step completes, unchanged. The condition/expression is still evaluated first, so its result is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds runtime guards preventing malformed loop bodies from crashing workflow execution.
Changes:
- Fail
whileanddo-whilesteps when dispatched bodies are not lists. - Preserve false-condition behavior for
while. - Add parametrized regression tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/steps/while_loop/__init__.py |
Validates dispatched loop bodies at runtime. |
src/specify_cli/workflows/steps/do_while/__init__.py |
Rejects malformed mandatory loop bodies. |
tests/test_workflows.py |
Covers dict, string, and integer bodies. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
Collaborator
|
Thank you! |
jawwad-ali
added a commit
to jawwad-ali/spec-kit
that referenced
this pull request
Jul 15, 2026
IfThenStep.execute returns config['then']/['else'] directly as StepResult.next_steps with no check that it is a list. The engine does not auto-validate step config before execute(), so an unvalidated run with a non-list branch (a scalar or mapping authoring mistake) passes a non-iterable as next_steps and crashes the whole run. validate() catches this, but execute() should fail the step loudly instead. Add an isinstance(list) guard returning a FAILED StepResult, completing the same guard github#3519 added to the while/do-while steps (and that its comment already references for the 'if' step) and mirroring the switch step's 'cases' guard. Parametrized test feeds a str/dict/int branch and asserts FAILED (fails before: the non-iterable next_steps crashed execution). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kanfil
added a commit
to tikalk/agentic-sdlc-spec-kit
that referenced
this pull request
Jul 15, 2026
Upstream merge — 2 commits (post-0.12.15): - While/do-while non-list steps guard (github#3519): returns FAILED instead of crashing on non-list 'steps' on unvalidated run, mirrors if/switch/fan-out - PyPI install docs (github#3425/github#3516): adopted docs additions, kept fork README install section (fork installs from tikalk repo, not PyPI) 1 conflict resolved: - README.md: kept fork tikalk install instructions, dropped upstream PyPI block 613 tests pass. Smoke test confirms 0.12.15+adlc2. Assisted-by: opencode (model: glm-5.2, autonomous)
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
WhileStep.validate()andDoWhileStep.validate()already reject a non-liststepsbody, but the engine'sexecute()path does not auto-validate — seeWorkflowEngine.load_workflow, whose docstring notes the definition is "not yet validated". On an unvalidated run the body is returned asnext_steps, and the engine feeds it straight into_execute_steps, which iterates it as step mappings. A non-liststeps(a single mapping or scalar authoring mistake) was iterated element-wise — a dict yields its string keys, a str its characters — and raisedAttributeErroron.get(), taking down the whole run; the engine invokesstep_impl.execute()with no surrounding try/except.This guards both
executepaths to return a FAILEDStepResultnaming the type error instead, mirroring the if/switch non-list-branch guards and the fan-out non-listitemshandling.Changes
while— guard fires only when the condition is truthy (the only case where the body is dispatched). A false condition leaves a non-liststepsbenign and the step completes, unchanged.do-while— body always dispatches on the first call, so the guard is unconditional.Tests
Added parametrized tests (dict / str / int) for:
while— non-liststepsfails loudly when condition is true; stays benign/COMPLETED when condition is false.do-while— non-liststepsfails loudly (always dispatched).All while/do-while step tests pass locally.
Follows the same pattern as the merged switch (non-mapping cases), fan-in (non-list
wait_for), and if/switch non-list-branch crash guards.