fix(workflows): validate non-mapping `input` in command step by thejesh23 · Pull Request #3495 · github/spec-kit

thejesh23 · 2026-07-13T17:33:19Z

What

CommandStep.execute() iterates input.items() to resolve template expressions, but CommandStep.validate() did not check the type. A workflow with input: null (which is what YAML parses input: to when no value follows), input: [1, 2], or input: "foo" passed validation and then crashed at run time with a bare:

AttributeError: 'NoneType' object has no attribute 'items'

Fix

Mirror the pattern already applied in the shell / while-loop / do-while validators:

validate() rejects a non-mapping input with a message that names the field, so the failure surfaces at load time.
execute() defensively coerces a non-mapping input to {} so a caller that bypasses WorkflowEngine.validate() (e.g. ad-hoc step.execute(...)) still fails cleanly rather than raising an internal AttributeError.

Tests

Two new regression tests in TestCommandStep:

test_validate_rejects_non_mapping_input — None, list, string all report a clear 'input' must be a mapping error.
test_execute_non_mapping_input_does_not_crash — the validate-bypass path returns StepStatus.FAILED with output["input"] == {} instead of raising.

Full targeted run:

$ .venv/bin/python -m pytest tests/test_workflows.py -q --tb=no
======================= 414 passed, 5 warnings in 2.11s ========================

Manual test results

Agent: N/A — this is a workflow-engine-only fix; no slash-command surface changed. | OS/Shell: macOS 15 / zsh

Command tested	Notes
`/speckit.specify`	Not affected — CLI dispatch surface unchanged. Verified `TestCommandStep::test_execute_basic` still passes.

Per CONTRIBUTING.md's mapping rules, this change only touches src/specify_cli/workflows/steps/command/__init__.py (a step-type module inside the engine, not a slash-command template). No templates/commands/* file, no scripts/bash|powershell/* file, and no src/specify_cli/* CLI entry point is modified, so no slash command's behavior changes.

AI disclosure

Per CONTRIBUTING.md#ai-contributions-in-spec-kit: this PR (bug identification, patch, and tests) was prepared with Claude (Anthropic) assistance in an autonomous agent session. The reproducer was executed against a fresh clone of main, the fix was written to match the existing type-validation pattern in sibling step validators, and the full workflow test suite (tests/test_workflows.py, 414 tests) was run green before submission. The change is intentionally minimal — one guard in execute(), one check in validate(), two focused tests.

🤖 Generated with Claude Code

…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 github#3492

Copilot

Pull request overview

Adds type validation for command-step input mappings and regression coverage.

Changes:

Rejects non-mapping input during validation.
Adds defensive execution handling and tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/specify_cli/workflows/steps/command/__init__.py`	Validates and guards command input.
`tests/test_workflows.py`	Adds non-mapping input regression tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thejesh23 · 2026-07-13T20:47:41Z

+        if not isinstance(input_data, dict):
+            input_data = {}


Addressed in df551e1.

execute() now returns a FAILED StepResult (with dispatched: false, exit_code: 1, and a field-specific 'input' must be a mapping' error) before _try_dispatch is ever called. The silent-coerce-and-continue path is gone.

The regression test was also rewritten to patch _try_dispatch itself (via patch.object(CommandStep, "_try_dispatch")) instead of shutil.which. It asserts mock_dispatch.assert_not_called() and that the surfaced error contains 'input' must be a mapping, so it now specifically verifies the guard rather than passing on the incidental "CLI not found" branch.

Verified the test catches regressions: temporarily reintroducing the silent-coerce version made the new test fail with Expected '_try_dispatch' to not have been called. Called 1 times.

Full suite: 414 passed.

thejesh23 · 2026-07-13T20:47:43Z

+        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"] == {}


Addressed in df551e1.

execute() now returns a FAILED StepResult (with dispatched: false, exit_code: 1, and a field-specific 'input' must be a mapping' error) before _try_dispatch is ever called. The silent-coerce-and-continue path is gone.

The regression test was also rewritten to patch _try_dispatch itself (via patch.object(CommandStep, "_try_dispatch")) instead of shutil.which. It asserts mock_dispatch.assert_not_called() and that the surfaced error contains 'input' must be a mapping, so it now specifically verifies the guard rather than passing on the incidental "CLI not found" branch.

Verified the test catches regressions: temporarily reintroducing the silent-coerce version made the new test fail with Expected '_try_dispatch' to not have been called. Called 1 times.

Full suite: 414 passed.

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Medium

thejesh23 · 2026-07-13T20:47:44Z

+        if not isinstance(input_data, dict):
+            input_data = {}


Addressed in df551e1.

execute() now returns a FAILED StepResult (with dispatched: false, exit_code: 1, and a field-specific 'input' must be a mapping' error) before _try_dispatch is ever called. The silent-coerce-and-continue path is gone.

The regression test was also rewritten to patch _try_dispatch itself (via patch.object(CommandStep, "_try_dispatch")) instead of shutil.which. It asserts mock_dispatch.assert_not_called() and that the surfaced error contains 'input' must be a mapping, so it now specifically verifies the guard rather than passing on the incidental "CLI not found" branch.

Verified the test catches regressions: temporarily reintroducing the silent-coerce version made the new test fail with Expected '_try_dispatch' to not have been called. Called 1 times.

Full suite: 414 passed.

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) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

mnriem · 2026-07-13T21:45:23Z

@copilot resolve the merge conflicts in this pull request

…t-validation # Conflicts: # src/specify_cli/workflows/steps/command/__init__.py # tests/test_workflows.py

Copilot

Copilot wasn't able to review any files in this pull request.

thejesh23 · 2026-07-13T22:25:25Z

Closing as superseded by #3262, which merged into main and added a strict superset of this PR's change: the same validate() + execute() guards for non-mapping input, plus equivalent treatment for options, plus a combined regression test covering both fields via validate() and execute().

After merging origin/main into this branch and resolving the two conflicts (in src/specify_cli/workflows/steps/command/__init__.py and tests/test_workflows.py) by taking main's version, git diff origin/main on the merge commit is empty — there is no remaining delta to land. tests/test_workflows.py runs green (418 passed).

Thanks to @jawwad-ali for the more comprehensive fix in #3262.

thejesh23 requested a review from mnriem as a code owner July 13, 2026 17:33

Copilot AI review requested due to automatic review settings July 13, 2026 17:33

Copilot started reviewing on behalf of thejesh23 July 13, 2026 17:33 View session

thejesh23 mentioned this pull request Jul 13, 2026

fix(workflows): validate non-mapping options in command step #3496

Open

Copilot AI reviewed Jul 13, 2026

View reviewed changes

thejesh23 mentioned this pull request Jul 13, 2026

fix(workflows): command step crashes with AttributeError on input: null #3492

Open

mnriem requested a review from Copilot July 13, 2026 20:02

Copilot started reviewing on behalf of mnriem July 13, 2026 20:03 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 13, 2026 20:47

Copilot started reviewing on behalf of thejesh23 July 13, 2026 20:47 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/3492-command-inpu…

b81596a

…t-validation # Conflicts: # src/specify_cli/workflows/steps/command/__init__.py # tests/test_workflows.py

Copilot AI review requested due to automatic review settings July 13, 2026 22:24

Copilot AI reviewed Jul 13, 2026

View reviewed changes

thejesh23 closed this Jul 13, 2026

thejesh23 deleted the fix/3492-command-input-validation branch July 13, 2026 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(workflows): validate non-mapping `input` in command step#3495

fix(workflows): validate non-mapping `input` in command step#3495
thejesh23 wants to merge 3 commits into
github:mainfrom
thejesh23:fix/3492-command-input-validation

thejesh23 commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

thejesh23 Jul 13, 2026

Uh oh!

thejesh23 Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

thejesh23 Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mnriem commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

thejesh23 commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

thejesh23 commented Jul 13, 2026

What

Fix

Tests

Manual test results

AI disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

thejesh23 Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

thejesh23 Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

thejesh23 Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mnriem commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

thejesh23 commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants