mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): reject a non-string 'command' in command-step (#3596)
`CommandStep.validate` only checked that a `command` field is *present*,
never its type. On an unvalidated run (the engine does not auto-validate
before `execute`) a non-string `command` — null, a list, an int — was
passed straight through `_try_dispatch` to the integration's
`build_command_invocation`, which does `command_name.startswith("speckit.")`
and crashes the whole workflow with a raw `AttributeError` once a
resolvable integration with an installed CLI is found.
Guard both paths, mirroring the sibling steps:
- `validate()` rejects a non-string `command` (like prompt-step `prompt`
#3582 and shell-step `run`).
- `execute()` fails the step cleanly with the same contract error before
dispatch (like the existing `input`/`options` guards in this file), so
an unvalidated run FAILs the step instead of crashing the run.
An expression like `{{ inputs.cmd }}` is still a string, so it stays valid.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,21 @@ class CommandStep(StepBase):
|
||||
|
||||
def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
|
||||
command = config.get("command", "")
|
||||
# validate() rejects a non-string 'command', but the engine does not
|
||||
# auto-validate before execute(); an unvalidated run would pass the value
|
||||
# to build_command_invocation() (via _try_dispatch) and crash there with a
|
||||
# raw AttributeError (command_name.startswith(...) on a list/int/None).
|
||||
# Fail the step with the same contract error instead, mirroring the
|
||||
# 'input'/'options' guards below.
|
||||
if not isinstance(command, str):
|
||||
return StepResult(
|
||||
status=StepStatus.FAILED,
|
||||
error=(
|
||||
f"Command step {config.get('id', '?')!r}: 'command' must be a "
|
||||
f"string, got {type(command).__name__}."
|
||||
),
|
||||
)
|
||||
|
||||
input_data = config.get("input", {})
|
||||
# validate() rejects a non-mapping input, but the engine does not
|
||||
# auto-validate before execute(); a workflow that skipped validation can
|
||||
@@ -179,6 +194,17 @@ class CommandStep(StepBase):
|
||||
errors.append(
|
||||
f"Command step {config.get('id', '?')!r} is missing 'command' field."
|
||||
)
|
||||
elif not isinstance(config["command"], str):
|
||||
# execute() passes 'command' straight to the integration's
|
||||
# build_command_invocation(), which does command_name.startswith(...);
|
||||
# a non-string (null, list, int) crashes there with a raw
|
||||
# AttributeError once dispatch is attempted. Reject it at validation,
|
||||
# mirroring the prompt-step 'prompt' and shell-step 'run' type checks.
|
||||
# An expression like "{{ ... }}" is still a str, so it stays valid.
|
||||
errors.append(
|
||||
f"Command step {config.get('id', '?')!r}: 'command' must be a "
|
||||
f"string, got {type(config['command']).__name__}."
|
||||
)
|
||||
# execute() iterates input.items() and options.update(step_options); a
|
||||
# non-mapping here would raise at run time. Validate the shape like the
|
||||
# sibling steps (switch 'cases', fan-out 'step') so it is reported, not
|
||||
|
||||
@@ -1026,6 +1026,41 @@ class TestCommandStep:
|
||||
assert res_opt.status is StepStatus.FAILED
|
||||
assert "'options' must be a mapping" in (res_opt.error or "")
|
||||
|
||||
def test_validate_rejects_non_string_command(self):
|
||||
from specify_cli.workflows.steps.command import CommandStep
|
||||
|
||||
step = CommandStep()
|
||||
# execute() passes 'command' to build_command_invocation(), which does
|
||||
# command_name.startswith(...); a non-string crashes there with a raw
|
||||
# AttributeError. validate() must report it, like prompt-step 'prompt'.
|
||||
for bad in (None, ["a", "b"], 5, {"x": 1}):
|
||||
errs = step.validate({"id": "c", "command": bad})
|
||||
assert any("'command' must be a string" in e for e in errs), bad
|
||||
# a string command (incl. an expression) is still accepted
|
||||
assert step.validate({"id": "c", "command": "/x"}) == []
|
||||
assert step.validate({"id": "c", "command": "{{ inputs.cmd }}"}) == []
|
||||
|
||||
def test_execute_non_string_command_fails_cleanly(self):
|
||||
from unittest.mock import patch
|
||||
from specify_cli.workflows.steps.command import CommandStep
|
||||
from specify_cli.workflows.base import StepContext, StepStatus
|
||||
|
||||
step = CommandStep()
|
||||
# The engine may skip validate(); a non-string 'command' must FAIL the
|
||||
# step with the contract error rather than reaching _try_dispatch and
|
||||
# crashing build_command_invocation with a raw AttributeError. Force a
|
||||
# resolvable integration + installed CLI so, absent the guard, dispatch
|
||||
# would actually be attempted and the crash would fire.
|
||||
ctx = StepContext(default_integration="claude")
|
||||
with patch("specify_cli.workflows.steps.command.shutil.which",
|
||||
return_value="/usr/bin/claude"):
|
||||
for bad in (None, ["a", "b"], 5, {"x": 1}):
|
||||
result = step.execute(
|
||||
{"id": "c", "command": bad, "input": {}}, ctx
|
||||
)
|
||||
assert result.status is StepStatus.FAILED, bad
|
||||
assert "'command' must be a string" in (result.error or ""), bad
|
||||
|
||||
def test_step_override_integration(self):
|
||||
from unittest.mock import patch
|
||||
from specify_cli.workflows.steps.command import CommandStep
|
||||
|
||||
Reference in New Issue
Block a user