From ed12de098f04ceef7b0c8a0ec566bb789afa2ef1 Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 13:49:21 -0700 Subject: [PATCH] fix(workflows): fail command-step before dispatch on non-mapping options Address Copilot review on #3496. The prior guard silently skipped the merge for a non-mapping ``options`` and still called ``_try_dispatch``, so with an installed CLI a malformed step could return ``COMPLETED`` instead of the clean field-specific failure the PR claimed. The regression test also masked this by forcing ``shutil.which`` to ``None``, which made the observed ``FAILED`` unrelated to ``options``. Changes: - ``execute()`` returns a field-named ``FAILED`` result *before* dispatch when ``options`` is not a mapping. Workflow-level defaults are preserved in ``output.options`` and ``dispatched`` is ``False``. - ``test_execute_non_mapping_options_fails_before_dispatch`` (renamed) now mocks ``_try_dispatch`` to a *success* result so an accidental dispatch would surface as ``COMPLETED``, and asserts the dispatch mock is never called and the error string names ``'options'``. - ``test_validate_rejects_non_mapping_options`` covers ``None`` (a bare YAML ``options:``), matching the documented behavior. Refs #3493. --- .../workflows/steps/command/__init__.py | 18 ++++++-- tests/test_workflows.py | 41 ++++++++++++++++++- 2 files changed, 53 insertions(+), 6 deletions(-) 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