feat(workflows): make shell step timeout configurable#3404
Conversation
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 github#3327 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR makes the workflow shell step’s execution timeout configurable via an optional timeout (seconds) field, defaulting to 300 seconds for backward compatibility, and updates validation and error reporting accordingly.
Changes:
- Thread a per-step
timeoutvalue throughShellStep.execute()(default 300s) and report the configured timeout inTimeoutExpirederrors. - Add
ShellStep.validate()checks to reject invalidtimeoutvalues (non-int, bool, <= 0, null). - Add tests covering override/default behavior, timeout error messaging, and validation acceptance/rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/specify_cli/workflows/steps/shell/__init__.py |
Reads timeout from step config, passes it to subprocess.run, updates timeout error text, and validates timeout values. |
tests/test_workflows.py |
Adds TestShellStep coverage for configurable timeouts, defaults, error messaging, and validation rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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( |
There was a problem hiding this comment.
Reformatted to the multi-line conditional style.
| 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 |
There was a problem hiding this comment.
Added the status assert, matching test_timeout_is_configurable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| run_cmd = str(run_cmd) | ||
|
|
||
| cwd = context.project_root or "." | ||
| timeout = config.get("timeout", 300) |
There was a problem hiding this comment.
Good catch. execute() now falls back to the 300s default for malformed timeout values (bool/non-int/non-positive), mirroring the engine's is True handling of unvalidated continue_on_error. Covered by test_execute_ignores_unvalidated_bad_timeout (parametrized over the same bad values as validate). 4b6e1fc
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>
Description
Fixes #3327
The shell step hardcoded
timeout=300in itssubprocess.runcall, so any workflow gate longer than 5 minutes (full builds, linter aggregators, integration tests) died withTimeoutExpiredregardless of whether the command would pass.The step now reads an optional
timeoutfield (seconds), defaulting to 300 for backward compatibility:validaterejects non-integer, boolean, zero, negative, and null values, and the timeout error message reports the configured value instead of a hardcoded 300.Testing
uv run specify --helpuv sync && uv run pytest(3819 passed)Six new tests in
TestShellStep: override reachessubprocess.run, default stays 300, timeout error reports the configured value, validation rejects bad values (includingTrueand YAML-null), and accepts a positive integer.AI Disclosure
Written with GitHub Copilot CLI; I reviewed the diff and test results.