Skip to content

fix(workflows): reject bool max_iterations in while/do-while validation#3237

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/while-max-iterations-bool
Open

fix(workflows): reject bool max_iterations in while/do-while validation#3237
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/while-max-iterations-bool

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

WhileStep.validate() and DoWhileStep.validate() check:

if not isinstance(max_iter, int) or max_iter < 1:

Since bool is a subclass of int, isinstance(True, int) is True and True < 1 is False — so max_iterations: true passes validation. It is then used as a loop bound, and range(True) == range(1), so the loop silently runs one iteration instead of the author's intent being flagged as a type error.

This is the same bool-is-int trap already guarded against for number inputs (_coerce_input, #3199) and gate options.

Fix

Reject bools explicitly in both while and do-while validators.

Testing

  • uvx ruff check clean; TestWhileStep + TestDoWhileStep pass (10 tests).
  • Extended test_validate_invalid_max_iterations (while) and added test_validate_rejects_bool_max_iterations (do-while): max_iterations: true now returns the "must be an integer >= 1" error; a real positive integer is still accepted. Fails before, passes after.
  • Verified against both real step validators on current main.

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Opus 4.8) under my direction. AI identified the bool-is-int trap shared by both loop validators (consistent with the existing number/gate guards); I confirmed the silent single-iteration behavior and the fix on both real validators, and reviewed the diff before submitting.

while/do-while validate() checked 'not isinstance(max_iter, int) or max_iter < 1'. Since bool is a subclass of int, isinstance(True, int) is True and True < 1 is False, so 'max_iterations: true' passed validation and then ran as a single iteration (range(True) == range(1)) instead of being reported as a type error. Reject bools explicitly, matching the fail-fast-on-bool handling already used for number inputs and gate options.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a validation hole in workflow loop steps where max_iterations: true could pass type checks (because bool is an int subclass in Python) and be silently treated as a single iteration, instead of being rejected as an invalid configuration.

Changes:

  • Update WhileStep.validate() and DoWhileStep.validate() to explicitly reject boolean values for max_iterations.
  • Extend workflow tests to cover max_iterations=True rejection for both while and do-while steps.
Show a summary per file
File Description
src/specify_cli/workflows/steps/while_loop/__init__.py Explicitly rejects boolean max_iterations during while-step validation to avoid silent single-iteration behavior.
src/specify_cli/workflows/steps/do_while/__init__.py Mirrors the while-step fix for do-while validation by explicitly rejecting boolean max_iterations.
tests/test_workflows.py Adds/extends regression tests ensuring max_iterations=True is rejected and valid integers are still accepted.

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: 1
  • Review effort level: Low

Comment thread tests/test_workflows.py
Comment on lines +1842 to +1845
ok = step.validate(
{"id": "test", "condition": "{{ true }}", "max_iterations": 3, "steps": []}
)
assert not any("max_iterations" in e for e in ok)
@mnriem

mnriem commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants