fix(workflows): validate non-mapping options in command step#3496
Open
thejesh23 wants to merge 1 commit into
Open
fix(workflows): validate non-mapping options in command step#3496thejesh23 wants to merge 1 commit into
options in command step#3496thejesh23 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds type validation for command-step options to prevent runtime dict.update() crashes.
Changes:
- Rejects non-mapping
optionsduring 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.
Comment on lines
57
to
58
| if isinstance(step_options, dict) and step_options: | ||
| options.update(step_options) |
thejesh23
added a commit
to thejesh23/spec-kit
that referenced
this pull request
Jul 13, 2026
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.
mnriem
requested changes
Jul 13, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
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.
thejesh23
force-pushed
the
fix/3493-command-options-validation
branch
from
3b444b2 to
ed12de0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3493.
What
CommandStep.execute()merges step-leveloptionsinto the workflow-level defaults viadict.update(), butCommandStep.validate()did not check the type ofoptions, 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: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-mappingoptionswith a message that names the field.execute()guards the.update()withisinstance(step_options, dict)so a caller that bypassesWorkflowEngine.validate()(e.g. ad-hocstep.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 mappingerror.test_execute_non_mapping_options_does_not_crash— the validate-bypass path returnsStepStatus.FAILEDand the workflow-level defaults are preserved unchanged (a malformed step-level override is discarded, not partially applied).Targeted run:
Manual test results
Agent: N/A — workflow-engine-only fix, no slash-command surface changed. | OS/Shell: macOS 15 / zsh
/speckit.planTestCommandStep::test_options_mergestill passes (well-formed step options are still merged over defaults).Only touches
src/specify_cli/workflows/steps/command/__init__.py; notemplates/commands/*, noscripts/bash|powershell/*, nosrc/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 ofmain; 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 — oneisinstanceguard inexecute(), one check invalidate(), two focused tests.🤖 Generated with Claude Code