fix(workflows): if step execute() fails cleanly on a non-list branch by jawwad-ali · Pull Request #3528 · github/spec-kit

jawwad-ali · 2026-07-14T18:10:04Z

What

IfThenStep.execute returns config['then']/['else'] directly as StepResult.next_steps with no check that it's a list:

branch = config.get("then", [])
return StepResult(status=COMPLETED, next_steps=branch)   # branch could be "oops" / {...} / 42

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 rather than take down the run.

Fix

Add an isinstance(..., list) guard returning a FAILED StepResult. This completes the same guard #3519 added to the while/do-while steps — whose comment already references the if step as a peer — and mirrors the switch step's cases guard.

Note: this PR originally also touched while/do-while, but #3519 landed that guard for those two steps in the meantime. Rebased down to the still-missing if case only, to avoid duplicating merged work.

Test

Parametrized test_execute_rejects_non_list_branch feeds a str/dict/int branch and asserts FAILED — fails before (the non-iterable next_steps crashed execution), passes after.

🤖 Written with the assistance of Claude Code (AI). Bug self-found; fix/tests verified locally (fail-before / pass-after), ruff clean.

jawwad-ali · 2026-07-15T05:28:55Z

Heads-up: since opening this, #3519 landed the same non-list guard for the while/do-while steps. I've rebased this PR down to the one case #3519 didn't cover — the if step's execute() — so it now complements #3519 rather than duplicating it (and the earlier merge conflict is resolved).

Copilot

Pull request overview

Adds runtime validation so malformed if branches fail cleanly instead of crashing workflow execution.

Changes:

Rejects selected non-list then/else branches.
Adds parameterized tests for strings, mappings, and integers.

Show a summary per file

File	Description
`src/specify_cli/workflows/steps/if_then/__init__.py`	Adds branch type validation.
`tests/test_workflows.py`	Tests malformed branch 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

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Medium

mnriem

Please address Copilot feedback. If not applicable, please explain why

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>

The non-list guard turned a valid 'else: null' into a FAILED result on a false condition — but validate() deliberately accepts None as 'no else branch' (test_validate_accepts_valid_else). Normalize a selected None branch to [] before the guard so validation and execution stay consistent; the guard still rejects genuine non-list values (str/dict/int). Test: false condition + else: null now COMPLETES with no next steps (fails before: FAILED 'else must be a list ... NoneType'). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

A rebase left two copies of the non-list branch guard in IfThenStep.execute: an earlier block that normalized ANY selected None branch to [] (so a true condition with then: null silently completed instead of failing — validate() rejects a null then) plus a redundant non-list return, followed by the correct branch-specific block. Remove the duplicate earlier block and keep the single guard that normalizes None only for the else branch and rejects every other non-list value. Tests: else: null still COMPLETES (no next steps); then: null now FAILS (not silently normalized). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Review details

Comments suppressed due to low confidence (2)

src/specify_cli/workflows/steps/if_then/init.py:29

The behavioral guard described by the PR is not part of the current diff: the guard at lines 31–50 is unchanged, while this hunk only swaps two independent assignments and has no runtime effect. The current base also already covers non-list branches at tests/test_workflows.py:2196–2239, so this PR no longer delivers the stated fix; please rebase and drop the now-redundant changes (or close the PR if the fix landed elsewhere).

            branch = config.get("then", [])
            branch_name = "then"
        else:
            branch = config.get("else", [])
            branch_name = "else"

tests/test_workflows.py:2160

This repeats the existing test_execute_none_else_stays_empty coverage at lines 2241–2257. Remove the duplicate and keep the established test.

    def test_execute_allows_null_else_branch(self):
        """`else: null` is the supported 'no else branch' form (validate accepts
        it); a false condition must run cleanly (COMPLETED, no next steps), not
        be turned into FAILED by the non-list guard."""

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Medium

+    @pytest.mark.parametrize("bad_branch", ["oops", {"a": 1}, 42])
+    def test_execute_rejects_non_list_branch(self, bad_branch):
+        """execute() fails cleanly on a non-list then/else branch. The engine
+        doesn't auto-validate step config, so a non-iterable next_steps would
+        otherwise crash the run — completing the while/do-while guard (#3519) for
+        the if step (mirrors the switch step's 'cases' guard)."""
+        from specify_cli.workflows.steps.if_then import IfThenStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        step = IfThenStep()
+        cond = "{{ inputs.scope == 'full' }}"
+        res_then = step.execute(
+            {"id": "i", "condition": cond, "then": bad_branch},
+            StepContext(inputs={"scope": "full"}),
+        )
+        assert res_then.status is StepStatus.FAILED
+        assert "'then' must be a list" in (res_then.error or "")
+        res_else = step.execute(
+            {"id": "i", "condition": cond, "then": [], "else": bad_branch},
+            StepContext(inputs={"scope": "backend"}),
+        )
+        assert res_else.status is StepStatus.FAILED
+        assert "'else' must be a list" in (res_else.error or "")


mnriem · 2026-07-21T14:39:58Z

Please address Copilot feedback

jawwad-ali · 2026-07-21T16:01:24Z

Closing this as superseded. Since I opened it, the if step's execute() non-list guard — with the exact same else-only None normalization and equivalent test_execute_non_list_then/else_fails_loudly / test_execute_none_else_stays_empty tests — landed on main independently. Rebased onto current main, this PR reduces to a cosmetic line reorder plus tests that duplicate the ones already there (which is exactly what the review flagged). No point maintaining the duplication, so closing. Thanks!

jawwad-ali requested a review from mnriem as a code owner July 14, 2026 18:10

jawwad-ali force-pushed the fix/control-flow-nonlist-branch-guard branch from 9ef71fa to c89db54 Compare July 15, 2026 05:28

jawwad-ali changed the title ~~fix(workflows): if/while/do-while execute() fail cleanly on a non-list branch/body~~ fix(workflows): if step execute() fails cleanly on a non-list branch Jul 15, 2026

mnriem requested a review from Copilot July 16, 2026 16:40

Copilot started reviewing on behalf of mnriem July 16, 2026 16:41 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/if_then/__init__.py Outdated

jawwad-ali force-pushed the fix/control-flow-nonlist-branch-guard branch from c89db54 to 5f90f89 Compare July 17, 2026 10:29

mnriem requested a review from Copilot July 17, 2026 12:34

Copilot started reviewing on behalf of mnriem July 17, 2026 12:35 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/if_then/__init__.py Outdated

Comment thread src/specify_cli/workflows/steps/if_then/__init__.py Outdated

mnriem requested changes Jul 17, 2026

View reviewed changes

jawwad-ali and others added 3 commits July 19, 2026 01:37

jawwad-ali force-pushed the fix/control-flow-nonlist-branch-guard branch from 5f90f89 to e847931 Compare July 18, 2026 20:53

mnriem requested a review from Copilot July 21, 2026 12:40

Copilot started reviewing on behalf of mnriem July 21, 2026 12:41 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

jawwad-ali closed this Jul 21, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(workflows): if step execute() fails cleanly on a non-list branch#3528

fix(workflows): if step execute() fails cleanly on a non-list branch#3528
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/control-flow-nonlist-branch-guard

jawwad-ali commented Jul 14, 2026 •

edited

Loading

Uh oh!

jawwad-ali commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

mnriem commented Jul 21, 2026

Uh oh!

jawwad-ali commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jawwad-ali commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Fix

Test

Uh oh!

jawwad-ali commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

mnriem commented Jul 21, 2026

Uh oh!

jawwad-ali commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jawwad-ali commented Jul 14, 2026 •

edited

Loading