From 063c29ec1b97757d460d0a56c1bcb33e2dee6db4 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sat, 4 Jul 2026 10:29:48 +0500 Subject: [PATCH 1/6] 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. From 955d46afc68e33295e4dba9ea9481e807634d611 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Mon, 6 Jul 2026 19:58:24 +0500 Subject: [PATCH 2/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/steps/shell/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py index 9da36174a0..68aa5fd239 100644 --- a/src/specify_cli/workflows/steps/shell/__init__.py +++ b/src/specify_cli/workflows/steps/shell/__init__.py @@ -103,11 +103,14 @@ def validate(self, config: dict[str, Any]) -> list[str]: ) if "timeout" in config: timeout = config["timeout"] + import math + # 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 not math.isfinite(timeout) or timeout <= 0 ): errors.append( From 41cb5f194fe5af55f5c59f4d97b6986501beb174 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 7 Jul 2026 10:47:11 +0500 Subject: [PATCH 3/6] 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) --- tests/test_workflows.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index f4462548e5..aa56ece6be 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1321,6 +1321,17 @@ def test_validate_rejects_non_numeric_timeout(self): 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 From 053603a0cfdefb99082448a6c688724c46b60ef1 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 8 Jul 2026 20:15:29 +0500 Subject: [PATCH 4/6] 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) --- workflows/README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/workflows/README.md b/workflows/README.md index 966e2d390f..93b006ec5d 100644 --- a/workflows/README.md +++ b/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 From 17674fb54b8b1add6dad618a984b6957c075f2b3 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Thu, 9 Jul 2026 23:11:09 +0500 Subject: [PATCH 5/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../workflows/steps/shell/__init__.py | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py index 68aa5fd239..e0825ea4ef 100644 --- a/src/specify_cli/workflows/steps/shell/__init__.py +++ b/src/specify_cli/workflows/steps/shell/__init__.py @@ -27,10 +27,28 @@ 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. + # compatibility. Validate here as well so callers that skip + # WorkflowEngine.validate() don't crash in subprocess.run(). timeout = config.get("timeout", 300) + if "timeout" in config: + import math + # 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 not math.isfinite(timeout) + or timeout <= 0 + ): + return StepResult( + status=StepStatus.FAILED, + error=( + f"Shell step {config.get('id', '?')!r}: 'timeout' must be a " + f"positive number of seconds, got {timeout!r}." + ), + 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 From 03266516ab7c981e206f781aa26ac57bc8e72310 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sat, 11 Jul 2026 10:08:08 +0500 Subject: [PATCH 6/6] 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) --- .../workflows/steps/shell/__init__.py | 88 +++++++++---------- tests/test_workflows.py | 24 +++++ 2 files changed, 66 insertions(+), 46 deletions(-) diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py index 32d6af4018..9faac62e4d 100644 --- a/src/specify_cli/workflows/steps/shell/__init__.py +++ b/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,38 +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. - timeout = config.get("timeout", 300) - if isinstance(timeout, bool) or not isinstance(timeout, int) or timeout <= 0: - timeout = 300 - # Per-step execution timeout in seconds; defaults to 300 for backward - # compatibility. Validate here as well so callers that skip - # WorkflowEngine.validate() don't crash in subprocess.run(). + # 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 "timeout" in config: - import math - - # 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 not math.isfinite(timeout) - or timeout <= 0 - ): - return StepResult( - status=StepStatus.FAILED, - error=( - f"Shell step {config.get('id', '?')!r}: 'timeout' must be a " - f"positive number of seconds, got {timeout!r}." - ), - output={"exit_code": -1, "stdout": "", "stderr": "invalid timeout"}, - ) + 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 @@ -115,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: @@ -137,20 +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"] - import math - - # 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 not math.isfinite(timeout) - or timeout <= 0 - ): - errors.append( - f"Shell step {config.get('id', '?')!r}: 'timeout' must be a " - f"positive number of seconds, got {timeout!r}." - ) + 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 c4ab12f9d4..0244f83e1c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1472,6 +1472,30 @@ def fake_run(*args, **kwargs): 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