Skip to content

Commit b68187e

Browse files
committed
fix(workflows): validate non-mapping 'options' in command step (#3493)
The `command` step's `execute()` merges `options` into a dict via `options.update(step_options)`, but `validate()` did not check the type and the merge was gated only on truthiness (`if step_options`). A workflow with `options: [1, 2]` or `options: "foo"` therefore passed validation and crashed at runtime with: TypeError: cannot convert dictionary update sequence element #0 to a sequence Follow the pattern already applied by the shell / while-loop / do-while validators and by the sibling `input`-validation change in #3492: reject the non-mapping value in `validate()` with a message that names the field, and defensively guard the `.update()` in `execute()` so a caller that skips `WorkflowEngine.validate()` fails cleanly rather than raising an internal `dict.update` traceback. Fixes #3493
1 parent 6664cf8 commit b68187e

2 files changed

Lines changed: 59 additions & 2 deletions

File tree

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,14 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
4747
if model and isinstance(model, str) and "{{" in model:
4848
model = evaluate_expression(model, context)
4949

50-
# Merge options (workflow defaults ← step overrides)
50+
# Merge options (workflow defaults ← step overrides). A non-mapping
51+
# ``options`` (list, string, ``null``) is a config error; ``validate()``
52+
# catches this at load time, but guard here so a caller that bypasses
53+
# ``WorkflowEngine.validate()`` still fails cleanly instead of raising
54+
# an internal ``TypeError``/``ValueError`` from ``dict.update``.
5155
options = dict(context.default_options)
5256
step_options = config.get("options", {})
53-
if step_options:
57+
if isinstance(step_options, dict) and step_options:
5458
options.update(step_options)
5559

5660
# Attempt CLI dispatch
@@ -155,4 +159,13 @@ def validate(self, config: dict[str, Any]) -> list[str]:
155159
errors.append(
156160
f"Command step {config.get('id', '?')!r} is missing 'command' field."
157161
)
162+
if "options" in config and not isinstance(config["options"], dict):
163+
# ``execute`` merges ``options`` into a dict via ``dict.update``;
164+
# a non-mapping value (list, string) would raise ``TypeError`` or
165+
# ``ValueError`` there. Reject at validation so the failure surfaces
166+
# at load time with a message that points at the field.
167+
errors.append(
168+
f"Command step {config.get('id', '?')!r}: 'options' must be a "
169+
f"mapping, got {type(config['options']).__name__}."
170+
)
158171
return errors

tests/test_workflows.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -953,6 +953,50 @@ def test_validate_missing_command(self):
953953
errors = step.validate({"id": "test"})
954954
assert any("missing 'command'" in e for e in errors)
955955

956+
def test_validate_rejects_non_mapping_options(self):
957+
"""`options: [...]` / string / null must fail validation, not crash at run.
958+
959+
Before the fix, execute() called ``dict.update(step_options)`` and
960+
raised ``TypeError``/``ValueError`` for a non-mapping value,
961+
bypassing validate().
962+
"""
963+
from specify_cli.workflows.steps.command import CommandStep
964+
965+
step = CommandStep()
966+
for bad in ([1, 2], "foo"):
967+
errors = step.validate({"id": "t", "command": "/x", "options": bad})
968+
assert any("'options' must be a mapping" in e for e in errors), (
969+
f"expected mapping error for options={bad!r}, got {errors!r}"
970+
)
971+
972+
def test_execute_non_mapping_options_does_not_crash(self):
973+
"""execute() must fail cleanly when validate() is bypassed.
974+
975+
Some callers skip ``WorkflowEngine.validate()``. In that path, a
976+
non-mapping ``options`` used to raise ``TypeError: cannot convert
977+
dictionary update sequence element #0 to a sequence`` from
978+
``dict.update``. Now it is skipped and the step reports the usual
979+
dispatch failure.
980+
"""
981+
from unittest.mock import patch
982+
from specify_cli.workflows.steps.command import CommandStep
983+
from specify_cli.workflows.base import StepContext, StepStatus
984+
985+
step = CommandStep()
986+
ctx = StepContext(
987+
default_integration="claude",
988+
default_options={"max-tokens": 8000},
989+
project_root="/tmp",
990+
)
991+
with patch("specify_cli.workflows.steps.command.shutil.which", return_value=None):
992+
result = step.execute(
993+
{"id": "t", "command": "/speckit.plan", "options": [1, 2]},
994+
ctx,
995+
)
996+
assert result.status == StepStatus.FAILED
997+
# Workflow defaults still applied; malformed step-level override discarded.
998+
assert result.output["options"] == {"max-tokens": 8000}
999+
9561000
def test_step_override_integration(self):
9571001
from unittest.mock import patch
9581002
from specify_cli.workflows.steps.command import CommandStep

0 commit comments

Comments
 (0)