From 063c29ec1b97757d460d0a56c1bcb33e2dee6db4 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sat, 4 Jul 2026 10:29:48 +0500 Subject: [PATCH] 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) --- .../workflows/steps/shell/__init__.py | 22 ++++- tests/test_workflows.py | 89 +++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) 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.