diff --git a/src/specify_cli/workflows/steps/prompt/__init__.py b/src/specify_cli/workflows/steps/prompt/__init__.py index 5bf10fbffc..3bb9a2708c 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 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ), ) + # 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 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ), ) + @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 @@ def validate(self, config: dict[str, Any]) -> list[str]: 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 6e191fe799..bc6893e9d9 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1705,6 +1705,90 @@ def test_execute_falsey_non_string_integration_fails_loudly(self, falsey): 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."""