Skip to content

Commit d6b1868

Browse files
fix(workflows): reject a non-string 'command' in command-step
`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>
1 parent 57cc518 commit d6b1868

2 files changed

Lines changed: 61 additions & 0 deletions

File tree

src/specify_cli/workflows/steps/command/__init__.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,21 @@ class CommandStep(StepBase):
3030

3131
def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
3232
command = config.get("command", "")
33+
# validate() rejects a non-string 'command', but the engine does not
34+
# auto-validate before execute(); an unvalidated run would pass the value
35+
# to build_command_invocation() (via _try_dispatch) and crash there with a
36+
# raw AttributeError (command_name.startswith(...) on a list/int/None).
37+
# Fail the step with the same contract error instead, mirroring the
38+
# 'input'/'options' guards below.
39+
if not isinstance(command, str):
40+
return StepResult(
41+
status=StepStatus.FAILED,
42+
error=(
43+
f"Command step {config.get('id', '?')!r}: 'command' must be a "
44+
f"string, got {type(command).__name__}."
45+
),
46+
)
47+
3348
input_data = config.get("input", {})
3449
# validate() rejects a non-mapping input, but the engine does not
3550
# auto-validate before execute(); a workflow that skipped validation can
@@ -179,6 +194,17 @@ def validate(self, config: dict[str, Any]) -> list[str]:
179194
errors.append(
180195
f"Command step {config.get('id', '?')!r} is missing 'command' field."
181196
)
197+
elif not isinstance(config["command"], str):
198+
# execute() passes 'command' straight to the integration's
199+
# build_command_invocation(), which does command_name.startswith(...);
200+
# a non-string (null, list, int) crashes there with a raw
201+
# AttributeError once dispatch is attempted. Reject it at validation,
202+
# mirroring the prompt-step 'prompt' and shell-step 'run' type checks.
203+
# An expression like "{{ ... }}" is still a str, so it stays valid.
204+
errors.append(
205+
f"Command step {config.get('id', '?')!r}: 'command' must be a "
206+
f"string, got {type(config['command']).__name__}."
207+
)
182208
# execute() iterates input.items() and options.update(step_options); a
183209
# non-mapping here would raise at run time. Validate the shape like the
184210
# sibling steps (switch 'cases', fan-out 'step') so it is reported, not

tests/test_workflows.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,6 +1026,41 @@ def test_validate_rejects_non_mapping_input_and_options(self):
10261026
assert res_opt.status is StepStatus.FAILED
10271027
assert "'options' must be a mapping" in (res_opt.error or "")
10281028

1029+
def test_validate_rejects_non_string_command(self):
1030+
from specify_cli.workflows.steps.command import CommandStep
1031+
1032+
step = CommandStep()
1033+
# execute() passes 'command' to build_command_invocation(), which does
1034+
# command_name.startswith(...); a non-string crashes there with a raw
1035+
# AttributeError. validate() must report it, like prompt-step 'prompt'.
1036+
for bad in (None, ["a", "b"], 5, {"x": 1}):
1037+
errs = step.validate({"id": "c", "command": bad})
1038+
assert any("'command' must be a string" in e for e in errs), bad
1039+
# a string command (incl. an expression) is still accepted
1040+
assert step.validate({"id": "c", "command": "/x"}) == []
1041+
assert step.validate({"id": "c", "command": "{{ inputs.cmd }}"}) == []
1042+
1043+
def test_execute_non_string_command_fails_cleanly(self):
1044+
from unittest.mock import patch
1045+
from specify_cli.workflows.steps.command import CommandStep
1046+
from specify_cli.workflows.base import StepContext, StepStatus
1047+
1048+
step = CommandStep()
1049+
# The engine may skip validate(); a non-string 'command' must FAIL the
1050+
# step with the contract error rather than reaching _try_dispatch and
1051+
# crashing build_command_invocation with a raw AttributeError. Force a
1052+
# resolvable integration + installed CLI so, absent the guard, dispatch
1053+
# would actually be attempted and the crash would fire.
1054+
ctx = StepContext(default_integration="claude")
1055+
with patch("specify_cli.workflows.steps.command.shutil.which",
1056+
return_value="/usr/bin/claude"):
1057+
for bad in (None, ["a", "b"], 5, {"x": 1}):
1058+
result = step.execute(
1059+
{"id": "c", "command": bad, "input": {}}, ctx
1060+
)
1061+
assert result.status is StepStatus.FAILED, bad
1062+
assert "'command' must be a string" in (result.error or ""), bad
1063+
10291064
def test_step_override_integration(self):
10301065
from unittest.mock import patch
10311066
from specify_cli.workflows.steps.command import CommandStep

0 commit comments

Comments
 (0)