fix(workflows): fail a gate whose on_reject is not abort/skip/retry instead of silently completing a rejection - #3888
Merged
Conversation
execute() reads `on_reject = config.get("on_reject", "abort")` and, in the
reject branch, handles only "abort" and "retry" before falling through to
its `# on_reject == "skip"` case. So any other value makes a REJECTED gate
report COMPLETED and the run walks straight past the review the gate
exists to enforce:
on_reject='abort' -> failed "Gate rejected by user at step 'g'"
on_reject='retry' -> paused
on_reject='skip' -> completed (by design)
on_reject='Abort' -> completed <-- rejection silently discarded
on_reject='fail' -> completed <-- same
on_reject='stop' -> completed <-- same
on_reject=None -> completed <-- same
on_reject=5 -> completed <-- same
Reachable by a capitalisation slip, a guessed verb, a non-string, or a
bare `on_reject:` — note `config.get(k, default)` does NOT substitute the
default for an explicit YAML null.
`validate` already rejects anything outside abort/skip/retry, but the
engine does not auto-validate before execute(). Fail loudly instead,
mirroring the `options` and `verdict_input` guards in the same method, and
placed before the non-TTY short-circuit so it surfaces in CI too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The guard matches existing validation rules and regression tests cover representative invalid inputs.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds execute-time validation so invalid on_reject values fail closed instead of bypassing rejected gates.
Changes:
- Validates
on_rejectbefore gate execution. - Adds parametrized regression coverage for invalid values.
File summaries
| File | Description |
|---|---|
src/specify_cli/workflows/steps/gate/__init__.py |
Returns a failed result for invalid rejection policies. |
tests/test_workflows.py |
Tests invalid on_reject values during execution. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GateStep.executereadson_reject = config.get("on_reject", "abort"). In the reject branch it handles only two literals before falling through:So any other value lands in the
"skip"case. A gate that was rejected reports COMPLETED, and the run continues past the review the gate exists to enforce.Reproduction on current
main(81bf741)Same config, REJECT verdict, varying only
on_reject:This is a fail-open failure mode on a safety control, and every trigger is an ordinary authoring mistake:
Abort,SKIPfail,stopon_reject:in YAML, which parses toNone. Noteconfig.get(key, default)does not substitute the default for an explicit null.Fix
validatealready rejects anything outside the three literals:…but the engine does not auto-validate before
execute(seeWorkflowEngine.load_workflow), so this PR adds the matching execute-side guard. It mirrors theoptionsandverdict_inputguards already in this method, and is placed before the non-TTY short-circuit so the error surfaces in CI rather than pausing and failing later on interactive resume.No breaking change. All three documented values behave exactly as before — the existing
test_reject_verdict_input_preserves_reject_behaviorparametrizes over them and still passes untouched. The only inputs whose behaviour changes are ones that previously discarded a rejection.Verification
srcand pass with the fix.tests/test_workflows.py: 27 failed → 20 failed, 837 → 844 passed.mainbaseline I captured on81bf741(all Windows symlink-privilege).uvx ruff@0.15.0 check src tests→ cleanTest goes into the existing
TestGateStep, besidetest_validate_invalid_on_reject— the validate-side half of the same contract.Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current
main.