fix(workflows): reject non-string/non-boolean 'condition' in if/while/do-while steps

[jawwad-ali]

Problem

IfThenStep, WhileStep, and DoWhileStep validate() confirm that condition is present but never check its type.

At run time execute() feeds condition to evaluate_condition(), which first delegates to evaluate_expression() — that returns a non-string unchanged — and then coerces the result with bool():

def evaluate_expression(template, context):
    if not isinstance(template, str):
        return template   # non-string returned as-is
    ...

def evaluate_condition(condition, context):
    result = evaluate_expression(condition, context)
    ...
    return bool(result)

So a list/dict/number condition silently resolves to its truthiness instead of erroring on the authoring mistake:

• condition: [1, 2] → bool([1, 2]) → always True → the if-branch is always taken / the while-loop spins to max_iterations
• condition: {} → always False, etc.

No error is reported, so the mistake is invisible.

Fix

Reject a present-but-non-str/non-bool condition in each of the three steps' validate(), mirroring the existing prompt/shell/command type checks.

Why booleans are accepted (contract note)

An earlier revision of this PR rejected every non-string, including bool. That was wrong — it would have been a breaking change, so the validator now accepts (str, bool):

• An unquoted condition: false is idiomatic YAML, and it already resolves correctly today: evaluate_expression passes the bool through and evaluate_condition's bool() is a no-op. Verified on main: evaluate_condition(False) → False, evaluate_condition(True) → True, and end-to-end condition: false takes the else-branch while condition: true dispatches then.
• Two of the three steps default condition to the bool False themselves (config.get("condition", False)), so bool is the field's own natural type — not an authoring mistake.

A bool is therefore not part of the silent-coercion bug this PR fixes; only list/dict/int/float are. "true"/"false" and expressions like "{{ inputs.flag }}" are strings and stay valid, as before.

Verification

• New parametrized tests ([list, dict, int, float]) across all three step classes: fail before the guard, pass after.
• test_validate_accepts_string_or_bool_condition covers "true"/"false"/"{{ ... }}" and literal True/False in each step — the regression guard for the contract above.
• Full TestIfThenStep/TestWhileStep/TestDoWhileStep green; ruff@0.15.0 clean.

---

AI-assisted: authored with Claude Code. I traced the runtime path (evaluate_expression → evaluate_condition → bool()), confirmed the silent coercion on main, and confirmed that literal booleans already behave correctly there — which is why they are accepted rather than rejected.

[Copilot]

Pull request overview

Adds validation to prevent non-string workflow conditions from being silently truthiness-coerced.

Changes:

• Validates conditions for if, while, and do-while steps.
• Adds parameterized regression tests for invalid condition types.

Show a summary per file

File	Description
if_then/__init__.py	Rejects non-string if conditions.
while_loop/__init__.py	Rejects non-string while conditions.
do_while/__init__.py	Rejects non-string do-while conditions.
tests/test_workflows.py	Covers invalid and valid condition values.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

[mnriem]

Please address Copilot feedback

[jawwad-ali]

Good catch — fixed in 6a78c02. You're right that evaluate_condition() always returns a bool; the pass-through happens one level down in evaluate_expression(), which evaluate_condition() calls before applying bool(). I reworded all four sites (the if/while/do-while guards and the mirror test comment) to name the two stages explicitly:

execute() feeds 'condition' to evaluate_condition(), which first delegates to evaluate_expression() -- that returns a non-string unchanged -- and then coerces the result with bool().

Comments only; no behaviour change. ruff@0.15.0 clean and the condition tests still pass (28).

[Copilot]

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[jawwad-ali]

Thanks — I took the second option you offered ("update the PR/API contract if boolean literals are intentionally supported"), because rejecting bool would be a breaking change rather than a fix.

Accepting booleans is deliberate. Verified against upstream/main (pre-PR):

condition=False -> completed | condition_result: False | next: []
condition=True  -> completed | condition_result: True  | next: ['i']

So condition: false already resolves correctly today: evaluate_expression passes the bool through and evaluate_condition's bool() is a no-op. It is not part of the silent-coercion bug — unlike [1, 2], which becomes True and always takes the branch. On top of that, two of the three steps default condition to the bool False themselves (config.get("condition", False)), so bool is the field's own natural type. An unquoted condition: false is also idiomatic YAML, so rejecting it would fail workflows that work on main today.

I've updated the title and description to state the contract accurately ("non-string/non-boolean") and documented why booleans are accepted, and the bad-value matrix is now [list, dict, int, float] with True/False moved to the positive test in all three step classes. Happy to flip to string-only if you'd prefer that contract — but it would need a deprecation note, since it drops support for input that currently works.

[Copilot]

Review details

Comments suppressed due to low confidence (3)

tests/test_workflows.py:2494

• This name says every non-string condition is rejected, but the adjacent contract explicitly accepts non-string booleans. Rename it to describe unsupported condition types so test reports do not contradict the behavior under test.

    def test_validate_rejects_non_string_condition(self, bad):

tests/test_workflows.py:3043

• This name says every non-string condition is rejected, but the adjacent contract explicitly accepts non-string booleans. Rename it to describe unsupported condition types so test reports do not contradict the behavior under test.

    def test_validate_rejects_non_string_condition(self, bad):

tests/test_workflows.py:2911

• This name says every non-string condition is rejected, but the adjacent contract explicitly accepts non-string booleans. Rename it to describe unsupported condition types so test reports do not contradict the behavior under test.

This issue also appears on line 3043 of the same file.

    def test_validate_rejects_non_string_condition(self, bad):

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Comments suppressed due to low confidence (3)

tests/test_workflows.py:2494

• This name says every non-string value is rejected, but the adjacent contract test verifies that booleans are accepted. Rename it to describe the actual non-string/non-boolean boundary so test reports do not contradict the behavior.

    def test_validate_rejects_non_string_condition(self, bad):

tests/test_workflows.py:3043

• This name says every non-string value is rejected, but the adjacent contract test verifies that booleans are accepted. Rename it to describe the actual non-string/non-boolean boundary so test reports do not contradict the behavior.

    def test_validate_rejects_non_string_condition(self, bad):

tests/test_workflows.py:2911

• This name says every non-string value is rejected, but the adjacent contract test verifies that booleans are accepted. Rename it to describe the actual non-string/non-boolean boundary so test reports do not contradict the behavior.

This issue also appears on line 3043 of the same file.

    def test_validate_rejects_non_string_condition(self, bad):

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Medium

[jawwad-ali]

Addressed the latest Copilot review (the three suppressed low-confidence notes on tests/test_workflows.py) in e3a42d1.

It was right, and it was my leftover. When I updated the contract to accept booleans I renamed the acceptance test to test_validate_accepts_string_or_bool_condition but left its sibling called test_validate_rejects_non_string_condition — and a bool is a non-string, so the two names contradicted each other in the test report.

Renamed in all three step classes (if/while/do-while):

test_validate_rejects_non_string_condition
  -> test_validate_rejects_non_string_non_bool_condition

That now matches the validator's own message, 'condition' must be a string or boolean, got <type>.

Test names only — no behaviour change, and the parametrized values are untouched ([["a","b"], {"k":"v"}, 5, 1.5] rejected; ["true", "false", "{{ inputs.flag }}", True, False] accepted).

Verification: 38 condition tests pass · uvx ruff@0.15.0 check src tests clean · full tests/test_workflows.py failure set is byte-identical before and after the rename (20 pre-existing Windows symlink-privilege failures, 743 passed).

@mnriem this should clear the outstanding Copilot feedback on this PR. To restate the one earlier point I didn't action, so it's not lost: Copilot asked for bool to be rejected outright, and I took the alternative its own comment offered ("update the PR/API contract if boolean literals are intentionally supported") — because condition: false already resolves correctly today and two of the three steps default condition to the bool False themselves, so rejecting it would be a breaking change rather than a fix. Happy to flip it behind a deprecation if you'd rather have the string-only contract.

[jawwad-ali]

Correction to my comment above: I posted it without realising this PR had already been merged (a9c9905, ~3 minutes earlier), so the commit I referenced never became part of it. Thanks for the merge, @mnriem.

The Copilot note it addressed is still valid against main — test_validate_rejects_non_string_condition sits next to test_validate_accepts_string_or_bool_condition, and a bool is a non-string, so the two names disagree. I've opened it as a separate follow-up instead: #3808 (3 insertions / 3 deletions, test names only, no behaviour change).

Everything else in my comment above still stands — in particular the reason booleans stay valid rather than being rejected.