feat(workflows): make shell step timeout configurable (#3327) (#3328)

* feat(workflows): make shell step timeout configurable (#3327)

The `shell` step hardcoded a 300s subprocess timeout, so any command
that legitimately runs longer than five minutes (a full build, a linter
aggregator, an integration-test target) was killed with TimeoutExpired
and failed the whole run, with no YAML knob to raise the limit.

Add an optional `timeout` field (seconds) that defaults to 300 for
backward compatibility and is threaded through to `subprocess.run`. The
timeout failure message now reports the configured value instead of a
hardcoded 300. `validate` rejects a `timeout` that is not a positive
number (bool is rejected explicitly, since it is an int subclass but a
config error rather than a duration).

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

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* test(workflows): cover non-finite timeout rejection in shell step

The isfinite guard added in 955d46a rejects YAML .inf/.nan timeouts, but no test asserted it. inf and nan are floats that pass a plain > 0 check (nan <= 0 is False), so without an explicit case a regression could silently reaccept them and crash subprocess.run(timeout=...) at runtime. Addresses the remaining Copilot review comment on PR #3328.

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

* docs(workflows): document configurable shell step timeout

Address Copilot review feedback on #3328: the per-step `timeout`
option was not reflected in the public workflow docs. The Shell Steps
section only showed `run:`, so readers couldn't discover `timeout:`,
its unit (seconds), or its default (300).

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

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* refactor(workflows): consolidate shell-step timeout validation into one path

Address Copilot review feedback on #3328:

- Remove the dead "fall back to default" timeout block in execute(): it
  re-read `timeout` from config immediately after, so the fallback was
  discarded and its comment contradicted the new fail-on-invalid behavior.
- Extract a single `_timeout_error()` helper shared by execute() and
  validate() so both reject the same values with the same message, instead
  of two drifting copies of the check.
- Hoist the duplicated inline `import math` to module scope.
- Add test_execute_fails_cleanly_on_invalid_timeout: asserts execute()
  fails the step (rather than raising) on an unvalidated string/bool/inf/0
  timeout, covering the engine-skips-validate path Copilot flagged.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: 3 people

src/specify_cli/workflows/steps/shell/__init__.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import json
+import math
 import subprocess
 from typing import Any
 
@@ -25,15 +26,20 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
         run_cmd = str(run_cmd)
 
         cwd = context.project_root or "."
-        # Defensive: the engine does not auto-validate step config, so an
-        # invalid ``timeout`` (string, None, ...) would otherwise raise a
-        # TypeError from subprocess.run() and crash the whole run.  Mirror
-        # the engine's handling of unvalidated ``continue_on_error`` by
-        # only honoring well-formed values and falling back to the default.
+        # Per-step execution timeout in seconds; defaults to 300 for backward
+        # compatibility. The engine does not auto-validate step config, so
+        # validate here as well — a caller that skips WorkflowEngine.validate()
+        # must fail the step cleanly rather than crash subprocess.run() with a
+        # TypeError (or silently coerce ``timeout: true`` to a 1s duration,
+        # since bool is an int subclass).
         timeout = config.get("timeout", 300)
-        if isinstance(timeout, bool) or not isinstance(timeout, int) or timeout <= 0:
-            timeout = 300
-
+        timeout_error = self._timeout_error(config)
+        if timeout_error is not None:
+            return StepResult(
+                status=StepStatus.FAILED,
+                error=timeout_error,
+                output={"exit_code": -1, "stdout": "", "stderr": "invalid timeout"},
+            )
         # NOTE: shell=True is required to support pipes, redirects, and
         # multi-command expressions in workflow YAML.  Workflow authors
         # control commands; catalog-installed workflows should be reviewed
@@ -92,6 +98,32 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
                 output={"exit_code": -1, "stdout": "", "stderr": str(exc)},
             )
 
+    @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. 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(), so they are rejected too.
+        """
+        if "timeout" not in config:
+            return None
+        timeout = config["timeout"]
+        if (
+            isinstance(timeout, bool)
+            or not isinstance(timeout, (int, float))
+            or not math.isfinite(timeout)
+            or timeout <= 0
+        ):
+            return (
+                f"Shell step {config.get('id', '?')!r}: 'timeout' must be a "
+                f"positive number of seconds, got {timeout!r}."
+            )
+        return None
+
     def validate(self, config: dict[str, Any]) -> list[str]:
         errors = super().validate(config)
         if "run" not in config:
@@ -114,16 +146,7 @@ def validate(self, config: dict[str, Any]) -> list[str]:
                 f"Shell step {config.get('id', '?')!r}: 'output_format' must "
                 f"be 'json' when present, got {output_format!r}."
             )
-        if "timeout" in config:
-            timeout = config["timeout"]
-            # bool is an int subclass, so reject it explicitly.
-            if (
-                isinstance(timeout, bool)
-                or not isinstance(timeout, int)
-                or timeout <= 0
-            ):
-                errors.append(
-                    f"Shell step {config.get('id', '?')!r}: 'timeout' must be a "
-                    f"positive integer (seconds) when present, got {timeout!r}."
-                )
+        timeout_error = self._timeout_error(config)
+        if timeout_error is not None:
+            errors.append(timeout_error)
         return errors

tests/test_workflows.py

@@ -1381,107 +1381,6 @@ def test_validate_accepts_string_and_expression_run(self):
         assert step.validate({"id": "s", "run": "echo hi"}) == []
         assert step.validate({"id": "s", "run": "{{ steps.x.output }}"}) == []
 
-    def test_timeout_is_configurable(self, monkeypatch):
-        """A 'timeout' field overrides the 300s default (#3327)."""
-        import subprocess as sp
-
-        from specify_cli.workflows.steps.shell import ShellStep
-        from specify_cli.workflows.base import StepContext, StepStatus
-
-        seen = {}
-        real_run = sp.run
-
-        def spy_run(*args, **kwargs):
-            seen["timeout"] = kwargs.get("timeout")
-            return real_run(*args, **kwargs)
-
-        monkeypatch.setattr(
-            "specify_cli.workflows.steps.shell.subprocess.run", spy_run
-        )
-        step = ShellStep()
-        result = step.execute(
-            {"id": "t", "run": "echo hi", "timeout": 1800}, StepContext()
-        )
-        assert result.status == StepStatus.COMPLETED
-        assert seen["timeout"] == 1800
-
-    def test_timeout_defaults_to_300(self, monkeypatch):
-        import subprocess as sp
-
-        from specify_cli.workflows.steps.shell import ShellStep
-        from specify_cli.workflows.base import StepContext, StepStatus
-
-        seen = {}
-        real_run = sp.run
-
-        def spy_run(*args, **kwargs):
-            seen["timeout"] = kwargs.get("timeout")
-            return real_run(*args, **kwargs)
-
-        monkeypatch.setattr(
-            "specify_cli.workflows.steps.shell.subprocess.run", spy_run
-        )
-        result = ShellStep().execute({"id": "t", "run": "echo hi"}, StepContext())
-        assert result.status == StepStatus.COMPLETED
-        assert seen["timeout"] == 300
-
-    def test_timeout_error_reports_configured_value(self, monkeypatch):
-        import subprocess as sp
-
-        from specify_cli.workflows.steps.shell import ShellStep
-        from specify_cli.workflows.base import StepContext, StepStatus
-
-        def raise_timeout(*args, **kwargs):
-            raise sp.TimeoutExpired(cmd="x", timeout=kwargs.get("timeout"))
-
-        monkeypatch.setattr(
-            "specify_cli.workflows.steps.shell.subprocess.run", raise_timeout
-        )
-        result = ShellStep().execute(
-            {"id": "t", "run": "sleep 999", "timeout": 7}, StepContext()
-        )
-        assert result.status == StepStatus.FAILED
-        assert "7 seconds" in result.error
-
-    @pytest.mark.parametrize("bad", [0, -5, "600", 1.5, None, True])
-    def test_execute_ignores_unvalidated_bad_timeout(self, bad, monkeypatch):
-        """execute() falls back to 300 when config skipped validation (#3327)."""
-        import subprocess as sp
-
-        from specify_cli.workflows.steps.shell import ShellStep
-        from specify_cli.workflows.base import StepContext, StepStatus
-
-        seen = {}
-        real_run = sp.run
-
-        def spy_run(*args, **kwargs):
-            seen["timeout"] = kwargs.get("timeout")
-            return real_run(*args, **kwargs)
-
-        monkeypatch.setattr(
-            "specify_cli.workflows.steps.shell.subprocess.run", spy_run
-        )
-        result = ShellStep().execute(
-            {"id": "t", "run": "echo hi", "timeout": bad}, StepContext()
-        )
-        assert result.status == StepStatus.COMPLETED
-        assert seen["timeout"] == 300
-
-    @pytest.mark.parametrize("bad", [0, -5, "600", 1.5, None, True])
-    def test_validate_rejects_bad_timeout(self, bad):
-        from specify_cli.workflows.steps.shell import ShellStep
-
-        errors = ShellStep().validate({"id": "s", "run": "echo hi", "timeout": bad})
-        assert any("'timeout'" in e for e in errors)
-
-    def test_validate_accepts_positive_int_timeout(self):
-        from specify_cli.workflows.steps.shell import ShellStep
-
-        assert (
-            ShellStep().validate({"id": "s", "run": "echo hi", "timeout": 1800}) == []
-        )
-
-
     def test_output_format_json_exposes_data(self, tmp_path):
         from specify_cli.workflows.steps.shell import ShellStep
         from specify_cli.workflows.base import StepContext, StepStatus
@@ -1538,6 +1437,130 @@ def test_validate_rejects_unknown_output_format(self):
         errors = step.validate({"id": "emit", "run": "exit 0", "output_format": "yaml"})
         assert any("'output_format' must be 'json'" in e for e in errors)
 
+    def test_configured_timeout_is_passed_to_subprocess(self, monkeypatch):
+        """A ``timeout:`` value on the step overrides the 300s default and is
+        threaded through to ``subprocess.run`` (issue #3327)."""
+        import subprocess
+
+        from specify_cli.workflows.steps.shell import ShellStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        captured: dict[str, object] = {}
+
+        def fake_run(*args, **kwargs):
+            captured["timeout"] = kwargs.get("timeout")
+            return subprocess.CompletedProcess(
+                args=args[0] if args else "", returncode=0, stdout="", stderr=""
+            )
+
+        monkeypatch.setattr(subprocess, "run", fake_run)
+        step = ShellStep()
+        result = step.execute(
+            {"id": "qa", "run": "echo hi", "timeout": 1800}, StepContext()
+        )
+        assert result.status == StepStatus.COMPLETED
+        assert captured["timeout"] == 1800
+
+    def test_default_timeout_preserved_when_omitted(self, monkeypatch):
+        """Omitting ``timeout:`` preserves the historical 300s default."""
+        import subprocess
+
+        from specify_cli.workflows.steps.shell import ShellStep
+        from specify_cli.workflows.base import StepContext
+
+        captured: dict[str, object] = {}
+
+        def fake_run(*args, **kwargs):
+            captured["timeout"] = kwargs.get("timeout")
+            return subprocess.CompletedProcess(
+                args=args[0] if args else "", returncode=0, stdout="", stderr=""
+            )
+
+        monkeypatch.setattr(subprocess, "run", fake_run)
+        step = ShellStep()
+        step.execute({"id": "qa", "run": "echo hi"}, StepContext())
+        assert captured["timeout"] == 300
+
+    def test_timeout_error_reports_configured_value(self, monkeypatch):
+        """The timeout failure message reflects the configured duration, not a
+        hardcoded 300."""
+        import subprocess
+
+        from specify_cli.workflows.steps.shell import ShellStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        def fake_run(*args, **kwargs):
+            raise subprocess.TimeoutExpired(cmd="echo hi", timeout=5)
+
+        monkeypatch.setattr(subprocess, "run", fake_run)
+        step = ShellStep()
+        result = step.execute(
+            {"id": "qa", "run": "echo hi", "timeout": 5}, StepContext()
+        )
+        assert result.status == StepStatus.FAILED
+        assert "5 seconds" in (result.error or "")
+
+    def test_execute_fails_cleanly_on_invalid_timeout(self, monkeypatch):
+        """execute() must fail the step (not raise) on an invalid timeout even
+        when validate() was skipped — the engine does not auto-validate step
+        config, so an unvalidated string/bool/non-finite timeout would
+        otherwise crash subprocess.run() and take down the whole run."""
+        import subprocess
+
+        from specify_cli.workflows.steps.shell import ShellStep
+        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 = ShellStep()
+        # A string would raise TypeError; ``True`` would silently become a 1s
+        # timeout (bool is an int subclass); ``.inf`` would raise at runtime.
+        for bad in ("30", True, float("inf"), 0):
+            result = step.execute(
+                {"id": "qa", "run": "echo hi", "timeout": bad}, StepContext()
+            )
+            assert result.status == StepStatus.FAILED
+            assert "'timeout' must be a positive number" in (result.error or "")
+
+    def test_validate_rejects_non_positive_timeout(self):
+        from specify_cli.workflows.steps.shell import ShellStep
+
+        step = ShellStep()
+        for bad in (0, -30):
+            errors = step.validate({"id": "qa", "run": "echo hi", "timeout": bad})
+            assert any("'timeout' must be a positive number" in e for e in errors)
+
+    def test_validate_rejects_non_numeric_timeout(self):
+        from specify_cli.workflows.steps.shell import ShellStep
+
+        step = ShellStep()
+        # A string and a bool are both invalid (bool is an int subclass but a
+        # config error, not a duration).
+        for bad in ("30", True):
+            errors = step.validate({"id": "qa", "run": "echo hi", "timeout": bad})
+            assert any("'timeout' must be a positive number" in e for e in errors)
+
+    def test_validate_rejects_non_finite_timeout(self):
+        from specify_cli.workflows.steps.shell import ShellStep
+
+        step = ShellStep()
+        # inf/nan are floats and slip past a plain ``> 0`` check (``nan <= 0``
+        # is False), but ``subprocess.run(timeout=...)`` would then fail at
+        # runtime. YAML ``.inf``/``.nan`` scalars parse to these via safe_load.
+        for bad in (float("inf"), float("-inf"), float("nan")):
+            errors = step.validate({"id": "qa", "run": "echo hi", "timeout": bad})
+            assert any("'timeout' must be a positive number" in e for e in errors)
+
+    def test_validate_accepts_positive_numeric_timeout(self):
+        from specify_cli.workflows.steps.shell import ShellStep
+
+        step = ShellStep()
+        for good in (1, 300, 1800, 12.5):
+            errors = step.validate({"id": "qa", "run": "echo hi", "timeout": good})
+            assert not any("'timeout'" in e for e in errors)
+
 class _StubStdin:
     """Stdin stub exposing only a fixed ``isatty`` result.
 

workflows/README.md

@@ -112,8 +112,14 @@ Run a shell command and capture output:
 - id: run-tests
   type: shell
   run: "cd {{ inputs.project_dir }} && npm test"
+  timeout: 1800   # Optional: max seconds before the command is killed (default 300)
 ```
 
+`timeout` is the maximum time in seconds the command may run before it is
+killed and the step fails; it must be a positive number and defaults to
+`300` (five minutes) when omitted. Raise it for long-running gates such as
+full builds, linter aggregators, or integration-test targets.
+
 ### Init Steps
 
 Bootstrap a project the same way `specify init` does — scaffolding