fix(workflows): validate prompt step 'timeout' like the shell step - #3847
Merged
mnriem merged 3 commits intoJul 29, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Validates prompt-step timeouts before subprocess execution, aligning behavior with shell steps.
Changes:
- Adds timeout validation to
PromptStep.validate()andexecute(). - Adds tests for valid and invalid timeout values.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/steps/prompt/__init__.py |
Validates prompt timeout values. |
tests/test_workflows.py |
Tests timeout validation and execution failure behavior. |
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: 1
- Review effort level: Medium
mnriem
requested changes
Jul 29, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Collaborator
|
Please fix test & lint errors |
PR github#3768 added a `timeout` to the prompt step and passed it straight into `subprocess.run(timeout=...)`. Neither `validate()` nor `execute()` checks it, so a bad value from a user-authored `workflow.yml` escapes as a raw exception: steps: - id: first type: shell run: echo side-effect - id: ask type: prompt prompt: do it timeout: abc $ specify workflow run wf.yml > [first] shell ... Workflow failed: unsupported operand type(s) for +: 'float' and 'str' The engine re-raises anything a step throws, so this takes down the whole run — after `first` has already run its side effect — with a message that names neither the step nor the field. `timeout: .nan` raises `ValueError: cannot convert float NaN to integer` the same way, and a non-positive `timeout` (`0`, `-5`) makes `subprocess.run` report an immediate TimeoutExpired for a command that never got the time to run. `timeout: true` silently becomes a 1-second limit, since bool is an int subclass. The sibling shell step already rejects exactly these values via a `_timeout_error()` helper shared by `execute()` and `validate()`, so the same workflow failed validation cleanly as a shell step and crashed as a prompt one. Mirrored that helper onto PromptStep: `validate()` reports the contract error, and `execute()` re-checks it so an unvalidated run fails just that step instead of aborting. Now: Workflow validation failed: - Prompt step 'ask': 'timeout' must be a positive number of seconds, got 'abc'. caught before the first step runs. Positive int/float timeouts and an absent `timeout` are unaffected. Regression tests in `TestPromptStep` mirror the shell step's: validate rejects "30"/True/inf/nan/0/-5/list/None, validate accepts 300/5/0.5 and an absent field, and execute fails cleanly with `subprocess.run` patched to assert it is never reached. With the source fix reverted, all 9 rejection tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The autofix commit wrapped the prompt step's `_timeout_error()` check in `try/except OverflowError` but added no test, so nothing pins the behaviour it introduced. `math.isfinite(10**400)` raises `OverflowError: int too large to convert to float` — the value is an `int`, is `> 0`, and is not a `bool`, so it clears every other clause of the guard and reaches `isfinite()`. Without the `except`, validating ```yaml - id: ask type: prompt prompt: do it timeout: 1000...0 # 400 digits ``` raises that `OverflowError` out of `validate()`/`execute()` — exactly the uncaught-crash failure mode this guard was added to prevent. The same value raises `OverflowError` from `subprocess.run(timeout=...)`. Add `10**400` to both parametrized rejection lists (`validate()` and the `execute()` fails-cleanly loop). Test-the-test: reverting the `try/except` fails both new cases with `OverflowError` and leaves the rest passing. Assisted-by: Claude Opus 5 (model: claude-opus-5, autonomous)
Noor-ul-ain001
force-pushed
the
fix/prompt-step-timeout-validation
branch
from
July 29, 2026 15:38
b14b4f3 to
bbf3534
Compare
mnriem
self-requested a review
July 29, 2026 15:50
mnriem
approved these changes
Jul 29, 2026
mnriem
pushed a commit
that referenced
this pull request
Jul 29, 2026
…Error (#3865) PR #3847 hardened the prompt step's `timeout` guard against a huge-int value, but its twin in the shell step — the step the prompt one was mirrored from — still has the hole. `math.isfinite(10**400)` raises `OverflowError: int too large to convert to float`. A 400-digit YAML scalar is an `int` and is not a `bool`, so it clears every clause before `isfinite()` and raises there, escaping `_timeout_error()` as exactly the uncaught crash that helper exists to prevent: steps: - id: qa type: shell run: echo hi timeout: 1000...0 # 400 digits $ specify workflow run wf.yml Traceback (most recent call last): ... File "src/specify_cli/workflows/engine.py", line 361, in _validate_steps step_errors = step_impl.validate(step_config) File "src/specify_cli/workflows/steps/shell/__init__.py", line 127 or not math.isfinite(timeout) OverflowError: int too large to convert to float `workflow_run` calls `engine.validate()` before executing any step, so the OverflowError propagates out of `validate_workflow` and kills the command with a bare traceback that names neither the step nor the field, instead of the "Workflow validation failed" report. `execute()` shares the same helper, so an unvalidated run raises there too — and the engine re-raises anything a step throws, aborting the whole workflow after earlier steps have already run their side effects. The value is genuinely invalid rather than merely unrepresentable in the check: `subprocess.run(timeout=10**400)` raises the same OverflowError. Unlike the prompt step, the shell step checks `isfinite()` *before* `timeout <= 0`, so a negative huge int (`-(10**400)`) crashes as well rather than being caught by the sign check. Wrapped the condition in `try/except OverflowError` and treated the value as invalid, mirroring the prompt step's guard so both steps reject the same values with the same message. Now: Workflow validation failed: - Shell step 'qa': 'timeout' must be a positive number of seconds, got 1000...0. Valid int/float timeouts, non-finite floats, bools, strings and non-positive values are unaffected — the existing clauses are unchanged. Regression tests in `TestShellStep`: `validate()` rejects both signs of the huge int, `validate_workflow()` reports it end to end (pinning the path the CLI actually takes, not just the helper), and `execute()` fails only that step with `subprocess.run` patched to assert it is never reached. Test-the-test: reverting the source change fails all three with `OverflowError` and leaves the rest of `TestShellStep` passing. Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
#3768 added a
timeoutto the prompt step and passed it straight intosubprocess.run(timeout=...). Neithervalidate()norexecute()checks it, so a bad value from a user-authoredworkflow.ymlescapes as a raw exception.The engine re-raises anything a step throws, so this takes down the whole run — after
firsthas already run its side effect — with a message naming neither the step nor the field.timeout:abc,[30]TypeError: unsupported operand type(s) for +: 'float' and 'str'— run aborts.nanValueError: cannot convert float NaN to integer— run aborts0,-5TimeoutExpiredfor a command that never got the time to runtrueThe sibling shell step already rejects exactly these values via a
_timeout_error()helper shared byexecute()andvalidate()— so the same workflow fails validation cleanly as a shell step and crashes as a prompt one.Fix
Mirror that helper onto
PromptStep:validate()reports the contract error, andexecute()re-checks it so an unvalidated run (the engine does not auto-validate step config) fails just that step instead of aborting.Caught before the first step runs. Positive int/float timeouts and an absent
timeoutare unaffected.Tests
12 tests in
TestPromptStep, mirroring the shell step's:test_validate_rejects_invalid_timeout—"30",True,inf,nan,0,-5,["30"],Nonetest_validate_accepts_valid_timeout—300,5,0.5, and an absent fieldtest_execute_fails_cleanly_on_invalid_timeout— withsubprocess.runpatched to assert it is never reachedTest-the-test: with the source fix reverted, all 9 rejection tests fail; the 3 accepts-valid tests correctly still pass.
pytest tests/test_workflows.py→ 725 passed, 17 failed, 67 errors, identical to the pre-existing baseline on this Windows box (symlink tests requiring elevation +pytest-of-Etmpdir permission errors). None are in the touched paths.🤖 Generated with Claude Code