From 8466f303a18ac3079e77180149782ceefbc6a239 Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 10:32:52 -0700 Subject: [PATCH 1/2] fix(workflows): validate non-mapping 'input' in command step (#3492) The `command` step's `execute()` iterates `input.items()` to resolve template expressions, but `validate()` did not check the type. A workflow with `input: null` (or `input: [1, 2]`, `input: "foo"`) passed validation and then crashed at runtime with a bare `AttributeError: 'NoneType' object has no attribute 'items'`. Follow the pattern already applied by the shell / while-loop / do-while validators: reject the non-mapping value in `validate()` with a message that names the field, and defensively coerce it to `{}` in `execute()` so a caller that skips `WorkflowEngine.validate()` fails cleanly rather than raising an internal `AttributeError`. Fixes #3492 --- .../workflows/steps/command/__init__.py | 15 ++++++++ tests/test_workflows.py | 38 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 891b9da4e7..92337ef9b8 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -31,6 +31,12 @@ class CommandStep(StepBase): def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: command = config.get("command", "") input_data = config.get("input", {}) + # A non-mapping ``input`` (``null``, list, string, …) is a config error. + # ``validate()`` catches this at load time; guard here so a caller that + # bypasses ``WorkflowEngine.validate()`` still fails cleanly rather than + # raising ``AttributeError`` from ``input_data.items()``. + if not isinstance(input_data, dict): + input_data = {} # Resolve expressions in input resolved_input: dict[str, Any] = {} @@ -155,4 +161,13 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"Command step {config.get('id', '?')!r} is missing 'command' field." ) + if "input" in config and not isinstance(config["input"], dict): + # ``execute`` iterates ``input.items()`` to resolve template expressions; + # a non-mapping ``input`` (``null``, list, string) would crash there + # with a bare ``AttributeError``. Reject at validation so the failure + # surfaces at load time with a message that points at the field. + errors.append( + f"Command step {config.get('id', '?')!r}: 'input' must be a " + f"mapping, got {type(config['input']).__name__}." + ) return errors diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 11ff138074..7d05e98884 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -953,6 +953,44 @@ def test_validate_missing_command(self): errors = step.validate({"id": "test"}) assert any("missing 'command'" in e for e in errors) + def test_validate_rejects_non_mapping_input(self): + """`input: null` / list / string must fail validation, not crash at run. + + Before the fix, execute() iterated ``input.items()`` and raised + ``AttributeError`` for a non-mapping value, bypassing validate(). + """ + from specify_cli.workflows.steps.command import CommandStep + + step = CommandStep() + for bad in (None, [1, 2], "foo"): + errors = step.validate({"id": "t", "command": "/x", "input": bad}) + assert any("'input' must be a mapping" in e for e in errors), ( + f"expected mapping error for input={bad!r}, got {errors!r}" + ) + + def test_execute_non_mapping_input_does_not_crash(self): + """execute() must fail cleanly when validate() is bypassed. + + Some callers (e.g. ad-hoc step execution) skip + ``WorkflowEngine.validate()``. In that path, a non-mapping ``input`` + used to raise ``AttributeError: 'NoneType' object has no attribute + 'items'`` from execute(). Now it is treated as an empty mapping and + the step reports the usual dispatch failure. + """ + from unittest.mock import patch + from specify_cli.workflows.steps.command import CommandStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = CommandStep() + ctx = StepContext(default_integration="claude", project_root="/tmp") + with patch("specify_cli.workflows.steps.command.shutil.which", return_value=None): + result = step.execute( + {"id": "t", "command": "/speckit.specify", "input": None}, + ctx, + ) + assert result.status == StepStatus.FAILED + assert result.output["input"] == {} + def test_step_override_integration(self): from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep From df551e1ca7f68d465772cff4744fc81aa8215084 Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 13:47:12 -0700 Subject: [PATCH 2/2] fix(workflows): fail command step before dispatch on non-mapping input Copilot flagged that the previous defensive coercion (`if not isinstance( input_data, dict): input_data = {}`) silently continued into `_try_dispatch`. When the integration CLI is installed, that path could run the command with empty arguments and return `COMPLETED`, hiding the misconfiguration and possibly executing an unintended external command. Return a `FAILED` `StepResult` with a field-specific error before ever calling `_try_dispatch`. Also rewrite the regression test to patch `_try_dispatch` itself (not `shutil.which`) so it verifies dispatch is never attempted and that the surfaced error is the mapping-check error, rather than passing on the incidental "CLI not found" branch. Verified: dropping the guard now causes the new test to fail with "Expected '_try_dispatch' to not have been called. Called 1 times." Full targeted suite: `.venv/bin/python -m pytest tests/test_workflows.py` 414 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../workflows/steps/command/__init__.py | 20 +++++++++++++++++-- tests/test_workflows.py | 15 ++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 92337ef9b8..33a9522d15 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -34,9 +34,25 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # A non-mapping ``input`` (``null``, list, string, …) is a config error. # ``validate()`` catches this at load time; guard here so a caller that # bypasses ``WorkflowEngine.validate()`` still fails cleanly rather than - # raising ``AttributeError`` from ``input_data.items()``. + # raising ``AttributeError`` from ``input_data.items()``. Silently + # coercing to ``{}`` and continuing would let ``_try_dispatch`` run the + # command with empty arguments when the CLI is installed, masking the + # misconfiguration and possibly executing an unintended external + # command; fail before dispatch instead. if not isinstance(input_data, dict): - input_data = {} + return StepResult( + status=StepStatus.FAILED, + output={ + "command": command, + "input": {}, + "dispatched": False, + "exit_code": 1, + }, + error=( + f"Command step {config.get('id', '?')!r}: 'input' must be a " + f"mapping, got {type(input_data).__name__}." + ), + ) # Resolve expressions in input resolved_input: dict[str, Any] = {} diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 7d05e98884..2736c7cbb4 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -969,13 +969,15 @@ def test_validate_rejects_non_mapping_input(self): ) def test_execute_non_mapping_input_does_not_crash(self): - """execute() must fail cleanly when validate() is bypassed. + """execute() must fail cleanly *before* dispatch when validate() is bypassed. Some callers (e.g. ad-hoc step execution) skip ``WorkflowEngine.validate()``. In that path, a non-mapping ``input`` used to raise ``AttributeError: 'NoneType' object has no attribute - 'items'`` from execute(). Now it is treated as an empty mapping and - the step reports the usual dispatch failure. + 'items'`` from execute(). It must now fail with a field-specific + validation error *before* ``_try_dispatch`` runs — silent coercion to + ``{}`` would let the integration CLI (if installed) execute the command + with empty arguments and return ``COMPLETED``, masking the config error. """ from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep @@ -983,12 +985,17 @@ def test_execute_non_mapping_input_does_not_crash(self): step = CommandStep() ctx = StepContext(default_integration="claude", project_root="/tmp") - with patch("specify_cli.workflows.steps.command.shutil.which", return_value=None): + # Patch ``_try_dispatch`` (not ``shutil.which``) so the test would also + # catch a regression where the guard is removed and dispatch runs. + with patch.object(CommandStep, "_try_dispatch") as mock_dispatch: result = step.execute( {"id": "t", "command": "/speckit.specify", "input": None}, ctx, ) + mock_dispatch.assert_not_called() assert result.status == StepStatus.FAILED + assert "'input' must be a mapping" in (result.error or "") + assert result.output["dispatched"] is False assert result.output["input"] == {} def test_step_override_integration(self):