diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 7a6d893ed0..b639f3b4a8 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -61,15 +61,25 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if model and isinstance(model, str) and "{{" in model: model = evaluate_expression(model, context) - # Merge options (workflow defaults ← step overrides) + # Merge options (workflow defaults ← step overrides). Same rationale as + # 'input': a malformed options fails the step *before dispatch* rather + # than being silently ignored — a silent skip would let an installed + # CLI report ``COMPLETED`` for a malformed step, and ``dict.update`` + # would still crash on a list. options = dict(context.default_options) step_options = config.get("options", {}) - # Same rationale as 'input': a malformed options fails the step rather - # than being silently ignored (which would let an invalid step run and - # apparently complete). if not isinstance(step_options, dict): return StepResult( status=StepStatus.FAILED, + output={ + "command": command, + "integration": integration, + "model": model, + "options": options, + "input": resolved_input, + "dispatched": False, + "exit_code": 1, + }, error=( f"Command step {config.get('id', '?')!r}: 'options' must be a " f"mapping, got {type(step_options).__name__}." diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 81aec96d3e..525b854225 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -960,11 +960,13 @@ def test_validate_rejects_non_mapping_input_and_options(self): step = CommandStep() # execute() does input.items() / options.update(); a non-mapping must be # reported by validate(), not crash at run time (like switch 'cases'). + # ``None`` is included because a bare YAML ``options:`` parses as ``None``. for bad in (None, "args", ["a", "b"], 5): errs = step.validate({"id": "c", "command": "/x", "input": bad}) assert any("'input' must be a mapping" in e for e in errs), bad - errs = step.validate({"id": "c", "command": "/x", "options": 42}) - assert any("'options' must be a mapping" in e for e in errs) + for bad in (None, "foo", [1, 2], 42): + errs = step.validate({"id": "c", "command": "/x", "options": bad}) + assert any("'options' must be a mapping" in e for e in errs), bad # a valid mapping config is still accepted assert step.validate({"id": "c", "command": "/x", "input": {"args": "y"}, "options": {"k": 1}}) == [] # execute() has no auto-validation guarantee (the engine may skip @@ -980,6 +982,41 @@ def test_validate_rejects_non_mapping_input_and_options(self): assert res_opt.status is StepStatus.FAILED assert "'options' must be a mapping" in (res_opt.error or "") + def test_execute_non_mapping_options_fails_before_dispatch(self): + """execute() must fail *before dispatch* when validate() is bypassed. + + Silently discarding a non-mapping ``options`` and continuing would + let an installed CLI return exit code 0 and mark the malformed step + ``COMPLETED``. Force ``_try_dispatch`` to a success result so an + accidental dispatch would surface as ``COMPLETED`` rather than + being masked by an unrelated CLI-missing failure. + """ + from unittest.mock import patch, MagicMock + from specify_cli.workflows.steps.command import CommandStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = CommandStep() + ctx = StepContext( + default_integration="claude", + default_options={"max-tokens": 8000}, + project_root="/tmp", + ) + dispatch_ok = MagicMock( + return_value={"exit_code": 0, "stdout": "", "stderr": ""} + ) + with patch.object(CommandStep, "_try_dispatch", dispatch_ok): + result = step.execute( + {"id": "t", "command": "/speckit.plan", "options": [1, 2]}, + ctx, + ) + assert result.status == StepStatus.FAILED + assert "'options' must be a mapping" in (result.error or "") + # Dispatch must not be attempted for a malformed options value. + assert not dispatch_ok.called + # Workflow defaults preserved; malformed step-level override discarded. + assert result.output["options"] == {"max-tokens": 8000} + assert result.output["dispatched"] is False + def test_step_override_integration(self): from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep