diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py index 2a65fca444..9da36174a0 100644 --- a/src/specify_cli/workflows/steps/shell/__init__.py +++ b/src/specify_cli/workflows/steps/shell/__init__.py @@ -26,6 +26,11 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: cwd = context.project_root or "." + # Per-step execution timeout in seconds; defaults to 300 for backward + # compatibility. ``validate`` guarantees a positive number when the + # field is present, so ``execute`` can pass it straight through. + timeout = config.get("timeout", 300) + # 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 @@ -37,7 +42,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 +79,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: @@ -96,4 +101,17 @@ 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 a subclass of int, but ``timeout: true`` is a config + # error rather than a duration — reject it explicitly. + if ( + isinstance(timeout, bool) + or not isinstance(timeout, (int, float)) + or timeout <= 0 + ): + errors.append( + f"Shell step {config.get('id', '?')!r}: 'timeout' must be a " + f"positive number of seconds, got {timeout!r}." + ) return errors diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2fdbf887b3..f4462548e5 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1240,6 +1240,95 @@ 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_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_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.