From 7204c40bdd65d7b68a9eaba5633210e62a1276cd Mon Sep 17 00:00:00 2001
From: marcelsafin <179933638+marcelsafin@users.noreply.github.com>
Date: Wed, 8 Jul 2026 10:38:35 +0200
Subject: [PATCH 1/3] feat(workflows): make shell step timeout configurable

The shell step hardcoded a 300s subprocess timeout, killing any
legitimate long-running QA command. Read an optional timeout field
(seconds, positive integer, default 300) and validate it.

Fixes #3327

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 .../workflows/steps/shell/__init__.py         | 13 +++-
 tests/test_workflows.py                       | 75 +++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py
index 3ef94892ef..7a577732b7 100644
--- a/src/specify_cli/workflows/steps/shell/__init__.py
+++ b/src/specify_cli/workflows/steps/shell/__init__.py
@@ -25,6 +25,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
         run_cmd = str(run_cmd)
 
         cwd = context.project_root or "."
+        timeout = config.get("timeout", 300)
 
         # NOTE: shell=True is required to support pipes, redirects, and
         # multi-command expressions in workflow YAML.  Workflow authors
@@ -37,7 +38,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
                 capture_output=True,
                 text=True,
                 cwd=cwd,
-                timeout=300,
+                timeout=timeout,
             )
             output = {
                 "exit_code": proc.returncode,
@@ -74,7 +75,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
         except subprocess.TimeoutExpired:
             return StepResult(
                 status=StepStatus.FAILED,
-                error="Shell command timed out after 300 seconds.",
+                error=f"Shell command timed out after {timeout} seconds.",
                 output={"exit_code": -1, "stdout": "", "stderr": "timeout"},
             )
         except OSError as exc:
@@ -106,4 +107,12 @@ 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}."
+                )
         return errors
diff --git a/tests/test_workflows.py b/tests/test_workflows.py
index 41522d3e81..afad2ab021 100644
--- a/tests/test_workflows.py
+++ b/tests/test_workflows.py
@@ -1286,6 +1286,81 @@ 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
+
+        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
+        )
+        ShellStep().execute({"id": "t", "run": "echo hi"}, StepContext())
+        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_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 bf549d642e61cee5699245bccba47a1e58fba607 Mon Sep 17 00:00:00 2001
From: marcelsafin <179933638+marcelsafin@users.noreply.github.com>
Date: Wed, 8 Jul 2026 10:49:36 +0200
Subject: [PATCH 2/3] style: multi-line timeout validation, assert status in
 default test

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 src/specify_cli/workflows/steps/shell/__init__.py | 6 +++++-
 tests/test_workflows.py                           | 5 +++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py
index 7a577732b7..89c35af1ce 100644
--- a/src/specify_cli/workflows/steps/shell/__init__.py
+++ b/src/specify_cli/workflows/steps/shell/__init__.py
@@ -110,7 +110,11 @@ def validate(self, config: dict[str, Any]) -> list[str]:
         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:
+            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}."
diff --git a/tests/test_workflows.py b/tests/test_workflows.py
index afad2ab021..ab07398586 100644
--- a/tests/test_workflows.py
+++ b/tests/test_workflows.py
@@ -1314,7 +1314,7 @@ 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
+        from specify_cli.workflows.base import StepContext, StepStatus
 
         seen = {}
         real_run = sp.run
@@ -1326,7 +1326,8 @@ def spy_run(*args, **kwargs):
         monkeypatch.setattr(
             "specify_cli.workflows.steps.shell.subprocess.run", spy_run
         )
-        ShellStep().execute({"id": "t", "run": "echo hi"}, StepContext())
+        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):

From 4b6e1fc3d3a875f5ce0cf8635cd806e8190050ad Mon Sep 17 00:00:00 2001
From: marcelsafin <179933638+marcelsafin@users.noreply.github.com>
Date: Wed, 8 Jul 2026 10:58:40 +0200
Subject: [PATCH 3/3] Guard against unvalidated timeout in ShellStep.execute()

The engine does not auto-validate step config, so a string or null
timeout would reach subprocess.run() and crash the run with a
TypeError. Fall back to the 300s default for malformed values,
mirroring how the engine treats unvalidated continue_on_error.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 .../workflows/steps/shell/__init__.py         |  7 ++++++
 tests/test_workflows.py                       | 24 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py
index 89c35af1ce..ed5df8fc36 100644
--- a/src/specify_cli/workflows/steps/shell/__init__.py
+++ b/src/specify_cli/workflows/steps/shell/__init__.py
@@ -25,7 +25,14 @@ 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.
         timeout = config.get("timeout", 300)
+        if isinstance(timeout, bool) or not isinstance(timeout, int) or timeout <= 0:
+            timeout = 300
 
         # NOTE: shell=True is required to support pipes, redirects, and
         # multi-command expressions in workflow YAML.  Workflow authors
diff --git a/tests/test_workflows.py b/tests/test_workflows.py
index ab07398586..18907d7d2a 100644
--- a/tests/test_workflows.py
+++ b/tests/test_workflows.py
@@ -1348,6 +1348,30 @@ def raise_timeout(*args, **kwargs):
         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