feat(workflows): make shell step timeout configurable (#3327) by Noor-ul-ain001 · Pull Request #3328 · github/spec-kit

Noor-ul-ain001 · 2026-07-04T05:30:19Z

Summary

Closes #3327. The workflow shell step hardcoded a 300-second subprocess timeout with no way to override it, so any command that legitimately runs longer than five minutes — a full build, a MegaLinter/golangci-lint aggregator, an integration-test target — was killed with TimeoutExpired and failed the entire run. There was no YAML knob to raise the limit, which made shell steps unusable as a probe/gate over real project QA commands.

Changes

Add an optional timeout field (seconds) to the shell step, read via config.get("timeout", 300) and threaded into subprocess.run. Defaults to 300 so existing workflows are unaffected.
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 — it is an int subclass, but timeout: true is a config error, not a duration.

- id: qa
  type: shell
  timeout: 1800        # seconds; optional, default 300
  run: make code-qa

Acceptance criteria (from the issue)

A timeout: value overrides the 300s default and is honored by subprocess.run.
Omitting timeout: preserves current behavior (300s).
validate rejects a non-positive or non-numeric timeout.
A test asserts the configured value is threaded through.

Testing

Seven new tests under TestShellStep:

configured timeout is passed to subprocess.run
default 300 preserved when omitted
timeout error message reflects the configured value
validate rejects non-positive (0, -30) and non-numeric ("30", True) values
validate accepts positive int/float

pytest tests/test_workflows.py::TestShellStep → 13 passed. Confirmed the behavior-changing tests fail without the source change (test-the-test via git stash).

🤖 Generated with Claude Code

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) <noreply@anthropic.com>

Copilot

Pull request overview

This PR makes the workflow shell step’s subprocess timeout configurable per-step (defaulting to the historical 300s), so longer-running QA/build commands can run without being killed by a hardcoded limit.

Changes:

Add optional timeout (seconds) to shell step execution, defaulting to 300.
Update the timeout failure message to report the configured timeout.
Add/extend ShellStep validation and unit tests to cover timeout behavior and invalid values.

Show a summary per file

File	Description
src/specify_cli/workflows/steps/shell/init.py	Threads `timeout` from config into `subprocess.run`, updates timeout error message, and validates `timeout` input.
tests/test_workflows.py	Adds `TestShellStep` coverage for default/configured timeout wiring, error messaging, and validation cases.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Low

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Low

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 github#3328. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Low

mnriem

Please address Copilot feedback. Also please do not resolve conversations

Address Copilot review feedback on github#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) <noreply@anthropic.com>

Copilot

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Low

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…eout # Conflicts: # src/specify_cli/workflows/steps/shell/__init__.py

Copilot

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Low

mnriem · 2026-07-10T19:32:45Z

Please address Copilot feedback

…ne path Address Copilot review feedback on github#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) <noreply@anthropic.com>

Copilot

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Low

mnriem · 2026-07-13T15:33:04Z

Thank you!

Noor-ul-ain001 requested a review from mnriem as a code owner July 4, 2026 05:30

jawwad-ali mentioned this pull request Jul 5, 2026

fix(workflows): shell step validate() rejects non-string run #3348

Merged

1 task

mnriem requested a review from Copilot July 6, 2026 14:33

Copilot started reviewing on behalf of mnriem July 6, 2026 14:34 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/shell/__init__.py Outdated

Comment thread tests/test_workflows.py

Potential fix for pull request finding

955d46a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

mnriem requested a review from Copilot July 6, 2026 21:47

Copilot started reviewing on behalf of mnriem July 6, 2026 21:48 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

mdnyeemakhtar approved these changes Jul 7, 2026

View reviewed changes

mnriem requested a review from Copilot July 7, 2026 20:08

Copilot started reviewing on behalf of mnriem July 7, 2026 20:19 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/shell/__init__.py Outdated

Noor-ul-ain001 requested a review from mdnyeemakhtar July 8, 2026 11:53

mnriem requested changes Jul 8, 2026

View reviewed changes

mnriem requested a review from Copilot July 9, 2026 17:51

Copilot started reviewing on behalf of mnriem July 9, 2026 17:52 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/shell/__init__.py Outdated

Noor-ul-ain001 and others added 2 commits July 9, 2026 23:11

Potential fix for pull request finding

17674fb

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into feat/3327-shell-tim…

bba45fb

…eout # Conflicts: # src/specify_cli/workflows/steps/shell/__init__.py

mnriem requested a review from Copilot July 10, 2026 13:45

Copilot started reviewing on behalf of mnriem July 10, 2026 13:45 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/shell/__init__.py Outdated

mnriem requested a review from Copilot July 13, 2026 13:10

Copilot started reviewing on behalf of mnriem July 13, 2026 13:11 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

mnriem self-requested a review July 13, 2026 15:31

mnriem approved these changes Jul 13, 2026

View reviewed changes

mnriem merged commit 32952c9 into github:main Jul 13, 2026
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(workflows): make shell step timeout configurable (#3327)#3328

feat(workflows): make shell step timeout configurable (#3327)#3328
mnriem merged 7 commits into
github:mainfrom
Noor-ul-ain001:feat/3327-shell-timeout

Noor-ul-ain001 commented Jul 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

mnriem left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

mnriem commented Jul 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

mnriem commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Noor-ul-ain001 commented Jul 4, 2026

Summary

Changes

Acceptance criteria (from the issue)

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem commented Jul 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants