mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): validate prompt step 'timeout' like the shell step (#3847)
* fix(workflows): validate prompt step 'timeout' like the shell step PR #3768 added a `timeout` to the prompt step and passed it straight into `subprocess.run(timeout=...)`. Neither `validate()` nor `execute()` checks it, so a bad value from a user-authored `workflow.yml` escapes as a raw exception: steps: - id: first type: shell run: echo side-effect - id: ask type: prompt prompt: do it timeout: abc $ specify workflow run wf.yml > [first] shell ... Workflow failed: unsupported operand type(s) for +: 'float' and 'str' The engine re-raises anything a step throws, so this takes down the whole run — after `first` has already run its side effect — with a message that names neither the step nor the field. `timeout: .nan` raises `ValueError: cannot convert float NaN to integer` the same way, and a non-positive `timeout` (`0`, `-5`) makes `subprocess.run` report an immediate TimeoutExpired for a command that never got the time to run. `timeout: true` silently becomes a 1-second limit, since bool is an int subclass. The sibling shell step already rejects exactly these values via a `_timeout_error()` helper shared by `execute()` and `validate()`, so the same workflow failed validation cleanly as a shell step and crashed as a prompt one. Mirrored that helper onto PromptStep: `validate()` reports the contract error, and `execute()` re-checks it so an unvalidated run fails just that step instead of aborting. Now: Workflow validation failed: - Prompt step 'ask': 'timeout' must be a positive number of seconds, got 'abc'. caught before the first step runs. Positive int/float timeouts and an absent `timeout` are unaffected. Regression tests in `TestPromptStep` mirror the shell step's: validate rejects "30"/True/inf/nan/0/-5/list/None, validate accepts 300/5/0.5 and an absent field, and execute fails cleanly with `subprocess.run` patched to assert it is never reached. With the source fix reverted, all 9 rejection tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(workflows): cover the huge-int timeout OverflowError guard The autofix commit wrapped the prompt step's `_timeout_error()` check in `try/except OverflowError` but added no test, so nothing pins the behaviour it introduced. `math.isfinite(10**400)` raises `OverflowError: int too large to convert to float` — the value is an `int`, is `> 0`, and is not a `bool`, so it clears every other clause of the guard and reaches `isfinite()`. Without the `except`, validating ```yaml - id: ask type: prompt prompt: do it timeout: 1000...0 # 400 digits ``` raises that `OverflowError` out of `validate()`/`execute()` — exactly the uncaught-crash failure mode this guard was added to prevent. The same value raises `OverflowError` from `subprocess.run(timeout=...)`. Add `10**400` to both parametrized rejection lists (`validate()` and the `execute()` fails-cleanly loop). Test-the-test: reverting the `try/except` fails both new cases with `OverflowError` and leaves the rest passing. Assisted-by: Claude Opus 5 (model: claude-opus-5, autonomous) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import math
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
@@ -88,6 +89,15 @@ class PromptStep(StepBase):
|
||||
),
|
||||
)
|
||||
|
||||
# An invalid timeout reaches subprocess.run() and raises a raw
|
||||
# TypeError ("unsupported operand type(s) for +: 'float' and 'str'")
|
||||
# or ValueError, which the engine re-raises — taking down the whole
|
||||
# run with a message that names neither the step nor 'timeout'. Fail
|
||||
# this step cleanly instead, mirroring the shell step.
|
||||
timeout_error = self._timeout_error(config)
|
||||
if timeout_error is not None:
|
||||
return StepResult(status=StepStatus.FAILED, error=timeout_error)
|
||||
|
||||
# Attempt CLI dispatch
|
||||
timeout = config.get("timeout", 300)
|
||||
dispatch_result = self._try_dispatch(
|
||||
@@ -131,6 +141,41 @@ class PromptStep(StepBase):
|
||||
),
|
||||
)
|
||||
|
||||
@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, mirroring the shell step. 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(), and a
|
||||
non-positive timeout makes subprocess.run() report an immediate
|
||||
TimeoutExpired, so both are rejected too.
|
||||
"""
|
||||
if "timeout" not in config:
|
||||
return None
|
||||
timeout = config["timeout"]
|
||||
try:
|
||||
valid_timeout = (
|
||||
not isinstance(timeout, bool)
|
||||
and isinstance(timeout, (int, float))
|
||||
and timeout > 0
|
||||
and math.isfinite(timeout)
|
||||
)
|
||||
except OverflowError:
|
||||
# An int too large to convert to float (e.g. a 400-digit YAML
|
||||
# scalar) clears every clause above and raises here — and would
|
||||
# raise the same from subprocess.run(timeout=...).
|
||||
valid_timeout = False
|
||||
if not valid_timeout:
|
||||
return (
|
||||
f"Prompt step {config.get('id', '?')!r}: 'timeout' must be a "
|
||||
f"positive number of seconds, got {timeout!r}."
|
||||
)
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _try_dispatch(
|
||||
prompt: str,
|
||||
@@ -250,4 +295,7 @@ class PromptStep(StepBase):
|
||||
f"Prompt step {config.get('id', '?')!r}: 'model' must be a "
|
||||
f"string, got {type(model).__name__}."
|
||||
)
|
||||
timeout_error = self._timeout_error(config)
|
||||
if timeout_error is not None:
|
||||
errors.append(timeout_error)
|
||||
return errors
|
||||
|
||||
@@ -1705,6 +1705,90 @@ class TestPromptStep:
|
||||
assert res.status is StepStatus.FAILED, falsey
|
||||
assert "'model' must be a string" in (res.error or ""), falsey
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad", ["30", True, float("inf"), float("nan"), 0, -5, ["30"], None, 10**400]
|
||||
)
|
||||
def test_validate_rejects_invalid_timeout(self, bad):
|
||||
"""'timeout' reaches subprocess.run(), so validate() must reject junk.
|
||||
|
||||
The sibling shell step already rejects exactly these values; the
|
||||
prompt step gained a ``timeout`` without the matching guard, so a
|
||||
workflow that fails validation as a shell step passed as a prompt one.
|
||||
|
||||
``10**400`` is an int too large to convert to float: it passes
|
||||
``isinstance``/``> 0`` but makes ``math.isfinite()`` — and later
|
||||
``subprocess.run()`` — raise ``OverflowError``, so the guard has to
|
||||
catch that rather than let it escape as the crash it exists to stop.
|
||||
"""
|
||||
from specify_cli.workflows.steps.prompt import PromptStep
|
||||
|
||||
step = PromptStep()
|
||||
errors = step.validate(
|
||||
{"id": "p", "type": "prompt", "prompt": "hi", "timeout": bad}
|
||||
)
|
||||
assert any("'timeout' must be a positive number" in e for e in errors), (
|
||||
bad,
|
||||
errors,
|
||||
)
|
||||
|
||||
@pytest.mark.parametrize("good", [300, 5, 0.5])
|
||||
def test_validate_accepts_valid_timeout(self, good):
|
||||
"""A positive int/float timeout — and an absent one — stay valid."""
|
||||
from specify_cli.workflows.steps.prompt import PromptStep
|
||||
|
||||
step = PromptStep()
|
||||
for config in (
|
||||
{"id": "p", "type": "prompt", "prompt": "hi", "timeout": good},
|
||||
{"id": "p", "type": "prompt", "prompt": "hi"},
|
||||
):
|
||||
errors = step.validate(config)
|
||||
assert not any("'timeout'" in e for e in errors), (config, errors)
|
||||
|
||||
def test_execute_fails_cleanly_on_invalid_timeout(self, monkeypatch):
|
||||
"""execute() must fail the step, not raise, on an invalid timeout.
|
||||
|
||||
The engine does not auto-validate step config and re-raises anything a
|
||||
step throws, so an unvalidated ``timeout`` reaching subprocess.run()
|
||||
raised a raw ``TypeError: unsupported operand type(s) for +: 'float'
|
||||
and 'str'`` (or ``ValueError`` for NaN) that aborted the entire run —
|
||||
naming neither the step nor the field — after earlier steps had
|
||||
already run their side effects.
|
||||
"""
|
||||
import subprocess
|
||||
from unittest.mock import patch
|
||||
|
||||
from specify_cli.workflows.steps.prompt import PromptStep
|
||||
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 = PromptStep()
|
||||
ctx = StepContext(inputs={}, default_integration="claude")
|
||||
# A string/list raises TypeError and NaN raises ValueError inside
|
||||
# subprocess.run(); ``True`` would silently become a 1s timeout (bool
|
||||
# is an int subclass); a non-positive value reports an immediate
|
||||
# TimeoutExpired for a command that never got the time to run; an int
|
||||
# too large to convert to float raises OverflowError.
|
||||
for bad in ("30", True, float("nan"), 0, -5, ["30"], 10**400):
|
||||
with patch(
|
||||
"specify_cli.workflows.steps.prompt.shutil.which",
|
||||
return_value="/opt/claude",
|
||||
):
|
||||
result = step.execute(
|
||||
{
|
||||
"id": "p",
|
||||
"type": "prompt",
|
||||
"prompt": "hi",
|
||||
"integration": "claude",
|
||||
"timeout": bad,
|
||||
},
|
||||
ctx,
|
||||
)
|
||||
assert result.status is StepStatus.FAILED, bad
|
||||
assert "'timeout' must be a positive number" in (result.error or ""), bad
|
||||
|
||||
|
||||
class TestShellStep:
|
||||
"""Test the shell step type."""
|
||||
|
||||
Reference in New Issue
Block a user