fix(workflows): validate command step input/options are mappings by jawwad-ali · Pull Request #3262 · github/spec-kit

jawwad-ali · 2026-06-30T16:15:43Z

Description

CommandStep.validate() only checks that command is present. But execute() then does:

for key, value in input_data.items(): ...        # input_data = config.get("input", {})
options.update(step_options)                       # step_options = config.get("options", {})

So a non-mapping input: or options: (e.g. a YAML list or scalar) raises AttributeError at run time, bypassing the per-step FAILED/continue_on_error contract. Every sibling step validates the shape of its structured fields (switch 'cases', fan-out 'step') — CommandStep was the lone outlier.

Fix

validate(): report 'input' must be a mapping / 'options' must be a mapping (mirroring the sibling steps).
execute(): defense-in-depth — coerce a non-mapping input/skip non-mapping options rather than crash, since the engine does not auto-validate before running a step.

Testing

uvx ruff check clean; TestCommandStep passes (11 tests).
New test_validate_rejects_non_mapping_input_and_options: input ∈ {None, str, list, int} and options: 42 now return the mapping error (fail-before/pass-after); a valid mapping config still returns []; execute() with input: None returns a result instead of raising.

AI Disclosure

I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Opus 4.8) under my direction. AI identified the validate()-contract gap versus the sibling steps; I reproduced the run-time AttributeError, confirmed the validate + execute-guard fix, and reviewed the diff before submitting.

Copilot

Pull request overview

This PR closes a validation gap in the workflow command step so malformed input: / options: values (non-mappings) are caught during validation (and handled defensively at runtime), avoiding AttributeError crashes that bypass the step failure/continue_on_error contract.

Changes:

Extend CommandStep.validate() to reject non-mapping input and options with clear validation errors.
Harden CommandStep.execute() to coerce/ignore non-mapping input/options defensively since the engine may execute unvalidated workflows.
Add regression tests covering invalid shapes and the execute-time defense-in-depth behavior.

Show a summary per file

File	Description
tests/test_workflows.py	Adds a regression test ensuring non-mapping `input`/`options` are rejected by `validate()` and don’t crash `execute()`.
src/specify_cli/workflows/steps/command/init.py	Adds mapping-shape validation and defensive handling in `execute()` for `input`/`options`.

Review details

Tip

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

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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

ShellStep.validate() only checked that 'run' was present, so run: (null) or a GitHub-Actions-style list validated clean; execute() then str()-coerces the value and invokes it under shell=True, literally running 'None' or "['echo', 'hi']" as a command. Add a type check after the presence check, mirroring the command-step (#3262) and gate options validation. Expression strings ('{{ ... }}') are strings, so they stay valid. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

FanInStep.validate() only checked wait_for, so a non-mapping 'output' (a list or scalar) validated clean; execute() then silently coerces it to {}, so the author's declared aggregation keys vanish with COMPLETED status and no diagnostic. Reject a non-mapping output at validation, mirroring the command-step (#3262) non-mapping fix. execute()'s defensive coercion is left in place for unvalidated callers. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

CommandStep.validate() only checked for 'command'; execute() then does input.items() and options.update(step_options). A non-mapping input:/options: (e.g. a YAML list or scalar) raised AttributeError at run time, bypassing the per-step FAILED/continue-on-error contract -- unlike the sibling steps (switch 'cases', fan-out 'step') which type-check their config fields in validate(). Add the same checks, plus a defense-in-depth coercion in execute() since the engine does not auto-validate before running a step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The explanatory comment said options.update(options) but execute() does options.update(step_options). Comment-only change; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Review details

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

mnriem

Please address Copilot feedback

… of coercing execute() previously coerced a non-mapping 'input' to {} and silently ignored a non-mapping 'options', then dispatched the command anyway. For a workflow that skipped validation (the engine does not auto-validate before execute()), that let an explicitly malformed step run with empty args and report COMPLETED — masking the config error and defeating the per-step FAILED / continue_on_error semantics this change is meant to provide. Both now return a FAILED StepResult with the same contract error validate() reports (never crashing on .items()/.update()). Valid mapping configs are unaffected. Strengthened the execute() test to assert FAILED + the exact 'must be a mapping' error for input and options (fails before: the result carried the downstream dispatch error, not the shape error). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Review details

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

…hub#3262) * fix(workflows): validate command step input/options are mappings CommandStep.validate() only checked for 'command'; execute() then does input.items() and options.update(step_options). A non-mapping input:/options: (e.g. a YAML list or scalar) raised AttributeError at run time, bypassing the per-step FAILED/continue-on-error contract -- unlike the sibling steps (switch 'cases', fan-out 'step') which type-check their config fields in validate(). Add the same checks, plus a defense-in-depth coercion in execute() since the engine does not auto-validate before running a step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: fix code-comment typo in CommandStep.validate The explanatory comment said options.update(options) but execute() does options.update(step_options). Comment-only change; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflows): command step FAILS on malformed input/options instead of coercing execute() previously coerced a non-mapping 'input' to {} and silently ignored a non-mapping 'options', then dispatched the command anyway. For a workflow that skipped validation (the engine does not auto-validate before execute()), that let an explicitly malformed step run with empty args and report COMPLETED — masking the config error and defeating the per-step FAILED / continue_on_error semantics this change is meant to provide. Both now return a FAILED StepResult with the same contract error validate() reports (never crashing on .items()/.update()). Valid mapping configs are unaffected. Strengthened the execute() test to assert FAILED + the exact 'must be a mapping' error for input and options (fails before: the result carried the downstream dispatch error, not the shape error). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

jawwad-ali requested a review from mnriem as a code owner June 30, 2026 16:15

mnriem requested a review from Copilot June 30, 2026 16:21

Copilot started reviewing on behalf of mnriem June 30, 2026 16:22 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

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

mnriem requested a review from Copilot June 30, 2026 21:14

Copilot started reviewing on behalf of mnriem June 30, 2026 21:15 View session

Copilot AI reviewed Jun 30, 2026

This was referenced Jul 5, 2026

fix(workflows): shell step validate() rejects non-string run #3348

Merged

fix(workflows): fan-in validate() rejects non-mapping output #3349

Merged

jawwad-ali and others added 2 commits July 10, 2026 21:44

docs: fix code-comment typo in CommandStep.validate

160598b

The explanatory comment said options.update(options) but execute() does options.update(step_options). Comment-only change; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

jawwad-ali force-pushed the fix/command-step-validate-non-mapping branch from 77598a5 to 160598b Compare July 10, 2026 17:24

mnriem requested a review from Copilot July 13, 2026 12:39

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

Copilot AI reviewed Jul 13, 2026

View reviewed changes

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

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

mnriem requested changes Jul 13, 2026

View reviewed changes

mnriem requested a review from Copilot July 13, 2026 19:58

Copilot started reviewing on behalf of mnriem July 13, 2026 19:59 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

mnriem self-requested a review July 13, 2026 20:54

mnriem approved these changes Jul 13, 2026

View reviewed changes

mnriem merged commit 52c1acf into github:main Jul 13, 2026
12 checks passed

thejesh23 mentioned this pull request Jul 13, 2026

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

Closed

Copilot AI mentioned this pull request Jul 14, 2026

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

Closed

kanfil mentioned this pull request Jul 14, 2026

chore: merge upstream 0.12.14, bump to 0.12.14+adlc1 tikalk/agentic-sdlc-spec-kit#102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(workflows): validate command step input/options are mappings#3262

fix(workflows): validate command step input/options are mappings#3262
mnriem merged 3 commits into
github:mainfrom
jawwad-ali:fix/command-step-validate-non-mapping

jawwad-ali commented Jun 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jawwad-ali commented Jun 30, 2026

Description

Fix

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

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!

Uh oh!

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

Review details

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants