Skip to content

fix(workflows): reject a retry gate whose verdict enum forbids the reset value - #3912

Merged
mnriem merged 1 commit into
github:mainfrom
Noor-ul-ain001:fix/gate-verdict-retry-enum
Jul 31, 2026
Merged

fix(workflows): reject a retry gate whose verdict enum forbids the reset value#3912
mnriem merged 1 commit into
github:mainfrom
Noor-ul-ain001:fix/gate-verdict-retry-enum

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Problem

A gate with on_reject: retry consumes a bound reject verdict before pausing by resetting the named input to "" — documented behaviour, so a later resume prompts again. Every resume() re-resolves the persisted inputs through _coerce_input.

Those two rules collide when the bound input declares an enum that does not list "". The reset writes a value the input's own enum forbids, and the run wedges:

inputs:
  spec_verdict:
    type: string
    enum: [approve, reject]
steps:
  - id: review
    type: gate
    options: [approve, reject]
    on_reject: retry
    verdict_input: spec_verdict
$ specify workflow run wf --input spec_verdict=reject
Status: paused
$ specify workflow resume <run_id> --input note=b
Error: Input 'spec_verdict' value '' not in allowed values: ['approve', 'reject'].

The workflow validates clean and the first run looks fine, so the failure only appears at the second resume. It is also unrecoverable in practice: _resolve_inputs re-coerces the whole persisted map, so any resume that supplies an input dies on the stored "". Only a resume with no inputs at all still works — and that is precisely the call that cannot deliver a new verdict, which is the one thing the retry cycle exists to allow.

Fix

Extend the existing verdict_input cross-check (which already confirms the name is declared) to also require that a retry-bound input's enum admits the reset sentinel, and report it with a fix hint. To do that, thread the input definitions through _validate_steps instead of just their names.

Rejected the alternative of popping the key instead of writing "": that lets the input's default flow back in on the next resume, so a gate the user just rejected would silently auto-approve.

Docs: note the enum requirement next to the reset behaviour it follows from.

Tests

4 validation tests for the new guard (rejects a retry gate whose enum omits ""; passes when the enum includes it, when on_reject is abort/skip, and when there is no enum at all), plus a characterization test that drives the engine directly to pin the wedge the guard prevents.

Verified the guard test fails without the engine change. Full tests/test_workflows.py: 870 passed, 20 failed — the same 20 symlink tests fail identically on an unmodified upstream/main checkout (they need symlink privileges this Windows box lacks), so no regressions.

🤖 Generated with Claude Code

…set value

A gate with `on_reject: retry` consumes a bound reject verdict before
pausing by resetting the named input to `""` (documented behaviour, so a
later resume prompts again). Every `resume()` re-resolves the persisted
inputs through `_coerce_input`.

Those two rules collide when the bound input declares an `enum` that does
not list `""`. The reset writes a value the input's own enum forbids, and
the run wedges:

    inputs:
      spec_verdict:
        type: string
        enum: [approve, reject]
    steps:
      - id: review
        type: gate
        options: [approve, reject]
        on_reject: retry
        verdict_input: spec_verdict

    $ specify workflow run wf --input spec_verdict=reject
    Status: paused
    $ specify workflow resume <run_id> --input note=b
    Error: Input 'spec_verdict' value '' not in allowed values:
           ['approve', 'reject'].

The workflow validates clean and the first run looks fine, so the failure
only appears at the second resume. It is also unrecoverable in practice:
`_resolve_inputs` re-coerces the whole persisted map, so *any* resume that
supplies an input dies on the stored `""`. Only a resume with no inputs at
all still works -- and that is precisely the call that cannot deliver a new
verdict, which is the one thing the retry cycle exists to allow.

Extend the existing `verdict_input` cross-check (which already confirms the
name is declared) to also require that a retry-bound input's `enum` admits
the reset sentinel, and report it with a fix hint. To do that, thread the
input *definitions* through `_validate_steps` instead of just their names.

Rejected the alternative of popping the key instead of writing `""`: that
lets the input's `default` flow back in on the next resume, so a gate the
user just rejected would silently auto-approve.

Docs: note the `enum` requirement next to the reset behaviour it follows
from.

Adds 4 validation tests for the new guard plus a characterization test that
drives the engine directly to pin the wedge it prevents.

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

Assisted-by: Claude Opus 5 (1M context)

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

Prevents retry gates from persisting an empty verdict that violates the bound input’s enum.

Changes:

  • Validates retry-gate verdict enums include "".
  • Adds validation and regression tests.
  • Documents the empty-string requirement.
Show a summary per file
File Description
src/specify_cli/workflows/engine.py Adds the enum cross-check.
tests/test_workflows.py Covers valid, invalid, and wedged scenarios.
docs/reference/workflows.md Documents retry reset requirements.

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: Balanced

@mnriem
mnriem merged commit cf71d00 into github:main Jul 31, 2026
14 checks passed
@mnriem

mnriem commented Jul 31, 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