Skip to content

fix(workflows): gate validate() must not crash on non-string options#3233

Merged
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/gate-validate-non-string-options
Jun 29, 2026
Merged

fix(workflows): gate validate() must not crash on non-string options#3233
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/gate-validate-non-string-options

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

GateStep.validate() reports non-string options as an error, but then — when on_reject is abort/retry (abort is the default) — still runs the reject-choice check:

if on_reject in ("abort", "retry") and isinstance(options, list):
    if not any(o.lower() in reject_choices for o in options):

For a non-string option (e.g. options: [123]), o.lower() raises AttributeError: 'int' object has no attribute 'lower', which escapes validate() and breaks validate_workflow's documented contract — "return a list of error messages", never raise. A single malformed gate option crashes validation of the whole workflow instead of being reported.

Fix

Run the option-text check only when every option is a string (the non-string case is already reported by the earlier check). One guard clause.

Testing

  • uvx ruff check clean; TestGateStep passes (14 tests).
  • New test_validate_non_string_options_does_not_raise: options: [123] (default on_reject) and options: [True], on_reject: retry now return the "must be strings" error instead of raising. Fails before (AttributeError), passes after.
  • Verified against the real GateStep on current main; valid string options still produce the correct reject-choice error.

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Opus 4.8) under my direction. AI traced the uncaught AttributeError from o.lower() through validate_workflow's never-raise contract; I reproduced the crash against the real GateStep, confirmed string options still validate correctly, and reviewed the diff before submitting.

GateStep.validate() reports non-string options as an error, but then -- when on_reject is 'abort'/'retry' -- still runs the reject-choice check 'any(o.lower() in ... for o in options)'. For a non-string option (e.g. options: [123]) o.lower() raised AttributeError, which escaped validate() and broke validate_workflow's documented 'return a list of errors, never raise' contract. Guard the check so it only runs when every option is a string (the non-string case is already reported above).

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 an uncaught AttributeError in GateStep.validate() when options contains non-string values and on_reject is abort/retry, ensuring workflow validation continues to follow the “return a list of error messages” contract instead of raising.

Changes:

  • Added a guard so the reject-choice text check only runs when all options entries are strings.
  • Added a regression test covering non-string options for default on_reject and explicit on_reject: retry.
Show a summary per file
File Description
src/specify_cli/workflows/steps/gate/__init__.py Prevents validate() from calling .lower() on non-string options by gating the reject-choice check.
tests/test_workflows.py Adds a regression test to ensure non-string options are reported as validation errors rather than raising.

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

@mnriem mnriem merged commit 876dca8 into github:main Jun 29, 2026
12 checks passed
@mnriem

mnriem commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

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