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

thejesh23 · 2026-07-13T17:34:40Z

What

CommandStep.execute() merges step-level options into the workflow-level defaults via dict.update(), but CommandStep.validate() did not check the type of options, and the merge itself was gated only on truthiness (if step_options:). Any truthy non-mapping value therefore passed validation and crashed at run time:

options: [1, 2]   -> TypeError: cannot convert dictionary update sequence element #0 to a sequence
options: "foo"    -> ValueError: dictionary update sequence element #0 has length 1; 2 is required

Sibling of #3492 (same file, same class of type-validation gap in the command step).

Fix

Same shape as the fix already applied in the shell / while-loop / do-while validators and in #3492:

validate() rejects a non-mapping options with a message that names the field.
execute() guards the .update() with isinstance(step_options, dict) so a caller that bypasses WorkflowEngine.validate() (e.g. ad-hoc step.execute(...)) still fails cleanly and the workflow-level defaults survive.

Tests

Two new regression tests in TestCommandStep:

test_validate_rejects_non_mapping_options — a list and a string both report a clear 'options' must be a mapping error.
test_execute_non_mapping_options_does_not_crash — the validate-bypass path returns StepStatus.FAILED and the workflow-level defaults are preserved unchanged (a malformed step-level override is discarded, not partially applied).

Targeted run:

$ .venv/bin/python -m pytest tests/test_workflows.py::TestCommandStep -q
============================== 12 passed in 0.35s ==============================

Manual test results

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

Command tested	Notes
`/speckit.plan`	Not affected — CLI dispatch surface unchanged. `TestCommandStep::test_options_merge` still passes (well-formed step options are still merged over defaults).

Only touches src/specify_cli/workflows/steps/command/__init__.py; no templates/commands/*, no scripts/bash|powershell/*, no src/specify_cli/* CLI entry point. Per CONTRIBUTING.md's mapping rules, no slash command's behavior changes.

Relation to #3495

#3495 (input validation, closes #3492) is an independent sibling fix in the same file. Both share the pattern the recent fix(workflows) commits have been applying elsewhere. They land cleanly on top of each other; happy to rebase either way.

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. Reproducer executed against a fresh clone of main; the fix follows the existing type-validation pattern in sibling step validators; the workflow test suite was run green before submission. The change is minimal — one isinstance guard in execute(), one check in validate(), two focused tests.

🤖 Generated with Claude Code

Copilot

Pull request overview

Adds type validation for command-step options to prevent runtime dict.update() crashes.

Changes:

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

Reviewed changes

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

File	Description
`src/specify_cli/workflows/steps/command/__init__.py`	Validates and guards option merging.
`tests/test_workflows.py`	Tests malformed option validation and execution.

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

Copilot

Review details

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

Address Copilot review on github#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 github#3493.

Copilot

Pull request overview

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

mnriem

Please resolve conflicts

Address Copilot review on github#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 github#3493.

Copilot

Pull request overview

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

Copilot

Review details

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

+        # 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.


thejesh23 · 2026-07-14T12:48:48Z

Superseded by #3262, which landed the same validate()/execute() guards for non-mapping input/options on main (commit 52c1acf) — including the identical error message and the "FAILED before dispatch" behavior in execute(). The remaining delta on this branch is a minor output-payload shape change on the FAILED StepResult, which doesn't warrant a standalone PR. Closing as superseded; #3493 is fixed.

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

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

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

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/command/__init__.py Outdated

thejesh23 mentioned this pull request Jul 13, 2026

fix(workflows): command step crashes with TypeError on non-mapping options #3493

Open

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

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

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/steps/command/__init__.py Outdated

Comment thread tests/test_workflows.py Outdated

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

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

Copilot AI reviewed Jul 13, 2026

View reviewed changes

mnriem requested changes Jul 13, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 14, 2026 01:48

thejesh23 force-pushed the fix/3493-command-options-validation branch from 3b444b2 to ed12de0 Compare July 14, 2026 01:48

Copilot started reviewing on behalf of thejesh23 July 14, 2026 01:49 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

thejesh23 requested a review from mnriem July 14, 2026 02:06

mnriem requested a review from Copilot July 14, 2026 12:40

Copilot started reviewing on behalf of mnriem July 14, 2026 12:40 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

thejesh23 closed this Jul 14, 2026

thejesh23 deleted the fix/3493-command-options-validation branch July 14, 2026 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(workflows): validate non-mapping `options` in command step#3496
thejesh23 wants to merge 1 commit into
github:mainfrom
thejesh23:fix/3493-command-options-validation

thejesh23 commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

mnriem left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

thejesh23 commented Jul 14, 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

Relation to #3495

AI disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mnriem left a comment

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

thejesh23 commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants