From afbb2c7b6520281036b58a5c8b031e2cf4398c78 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Wed, 29 Jul 2026 20:50:45 +0500 Subject: [PATCH] fix(workflows): validate prompt step 'timeout' like the shell step (#3847) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflows): validate prompt step 'timeout' like the shell step PR #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) Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(workflows): cover the huge-int timeout OverflowError guard 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) --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../workflows/steps/prompt/__init__.py | 48 +++++++++++ tests/test_workflows.py | 84 +++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/src/specify_cli/workflows/steps/prompt/__init__.py b/src/specify_cli/workflows/steps/prompt/__init__.py index 5bf10fbff..3bb9a2708 100644 --- a/src/specify_cli/workflows/steps/prompt/__init__.py +++ b/src/specify_cli/workflows/steps/prompt/__init__.py @@ -2,6 +2,7 @@ from __future__ import annotations +import math import shutil from pathlib import Path from typing import Any @@ -88,6 +89,15 @@ class PromptStep(StepBase): ), ) + # An invalid timeout reaches subprocess.run() and raises a raw + # TypeError ("unsupported operand type(s) for +: 'float' and 'str'") + # or ValueError, which the engine re-raises — taking down the whole + # run with a message that names neither the step nor 'timeout'. Fail + # this step cleanly instead, mirroring the shell step. + timeout_error = self._timeout_error(config) + if timeout_error is not None: + return StepResult(status=StepStatus.FAILED, error=timeout_error) + # Attempt CLI dispatch timeout = config.get("timeout", 300) dispatch_result = self._try_dispatch( @@ -131,6 +141,41 @@ class PromptStep(StepBase): ), ) + @staticmethod + def _timeout_error(config: dict[str, Any]) -> str | None: + """Return an error message if ``config['timeout']`` is invalid, else None. + + Shared by execute() and validate() so both paths reject the same + values with the same message, mirroring the shell step. An absent + ``timeout`` is valid (the default is used). bool is a subclass of int, + but ``timeout: true`` is a config error rather than a duration, so it + is rejected explicitly. Non-finite floats (YAML ``.inf``/``.nan``) pass + a plain ``> 0`` check but would raise in subprocess.run(), and a + non-positive timeout makes subprocess.run() report an immediate + TimeoutExpired, so both are rejected too. + """ + if "timeout" not in config: + return None + timeout = config["timeout"] + try: + valid_timeout = ( + not isinstance(timeout, bool) + and isinstance(timeout, (int, float)) + and timeout > 0 + and math.isfinite(timeout) + ) + except OverflowError: + # An int too large to convert to float (e.g. a 400-digit YAML + # scalar) clears every clause above and raises here — and would + # raise the same from subprocess.run(timeout=...). + valid_timeout = False + if not valid_timeout: + return ( + f"Prompt step {config.get('id', '?')!r}: 'timeout' must be a " + f"positive number of seconds, got {timeout!r}." + ) + return None + @staticmethod def _try_dispatch( prompt: str, @@ -250,4 +295,7 @@ class PromptStep(StepBase): f"Prompt step {config.get('id', '?')!r}: 'model' must be a " f"string, got {type(model).__name__}." ) + timeout_error = self._timeout_error(config) + if timeout_error is not None: + errors.append(timeout_error) return errors diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 653ad6d3f..0cdd22ce6 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1705,6 +1705,90 @@ class TestPromptStep: assert res.status is StepStatus.FAILED, falsey assert "'model' must be a string" in (res.error or ""), falsey + @pytest.mark.parametrize( + "bad", ["30", True, float("inf"), float("nan"), 0, -5, ["30"], None, 10**400] + ) + def test_validate_rejects_invalid_timeout(self, bad): + """'timeout' reaches subprocess.run(), so validate() must reject junk. + + The sibling shell step already rejects exactly these values; the + prompt step gained a ``timeout`` without the matching guard, so a + workflow that fails validation as a shell step passed as a prompt one. + + ``10**400`` is an int too large to convert to float: it passes + ``isinstance``/``> 0`` but makes ``math.isfinite()`` — and later + ``subprocess.run()`` — raise ``OverflowError``, so the guard has to + catch that rather than let it escape as the crash it exists to stop. + """ + from specify_cli.workflows.steps.prompt import PromptStep + + step = PromptStep() + errors = step.validate( + {"id": "p", "type": "prompt", "prompt": "hi", "timeout": bad} + ) + assert any("'timeout' must be a positive number" in e for e in errors), ( + bad, + errors, + ) + + @pytest.mark.parametrize("good", [300, 5, 0.5]) + def test_validate_accepts_valid_timeout(self, good): + """A positive int/float timeout — and an absent one — stay valid.""" + from specify_cli.workflows.steps.prompt import PromptStep + + step = PromptStep() + for config in ( + {"id": "p", "type": "prompt", "prompt": "hi", "timeout": good}, + {"id": "p", "type": "prompt", "prompt": "hi"}, + ): + errors = step.validate(config) + assert not any("'timeout'" in e for e in errors), (config, errors) + + def test_execute_fails_cleanly_on_invalid_timeout(self, monkeypatch): + """execute() must fail the step, not raise, on an invalid timeout. + + The engine does not auto-validate step config and re-raises anything a + step throws, so an unvalidated ``timeout`` reaching subprocess.run() + raised a raw ``TypeError: unsupported operand type(s) for +: 'float' + and 'str'`` (or ``ValueError`` for NaN) that aborted the entire run — + naming neither the step nor the field — after earlier steps had + already run their side effects. + """ + import subprocess + from unittest.mock import patch + + from specify_cli.workflows.steps.prompt import PromptStep + from specify_cli.workflows.base import StepContext, StepStatus + + def fail_if_called(*args, **kwargs): + raise AssertionError("subprocess.run should not run on invalid timeout") + + monkeypatch.setattr(subprocess, "run", fail_if_called) + step = PromptStep() + ctx = StepContext(inputs={}, default_integration="claude") + # A string/list raises TypeError and NaN raises ValueError inside + # subprocess.run(); ``True`` would silently become a 1s timeout (bool + # is an int subclass); a non-positive value reports an immediate + # TimeoutExpired for a command that never got the time to run; an int + # too large to convert to float raises OverflowError. + for bad in ("30", True, float("nan"), 0, -5, ["30"], 10**400): + with patch( + "specify_cli.workflows.steps.prompt.shutil.which", + return_value="/opt/claude", + ): + result = step.execute( + { + "id": "p", + "type": "prompt", + "prompt": "hi", + "integration": "claude", + "timeout": bad, + }, + ctx, + ) + assert result.status is StepStatus.FAILED, bad + assert "'timeout' must be a positive number" in (result.error or ""), bad + class TestShellStep: """Test the shell step type."""