Skip to content

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

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/shell-step-validate-run-type
Open

fix(workflows): shell step validate() rejects non-string run#3348
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/shell-step-validate-run-type

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

ShellStep.validate() only checks that run is present:

if "run" not in config:
    errors.append("...missing 'run' field.")

So run: (null) or a GitHub-Actions-style list validates clean. execute() then does:

run_cmd = config.get("run", "")
...
run_cmd = str(run_cmd)   # -> 'None'  or  "['echo', 'hi']"

and invokes that under shell=True — literally running the Python repr as a command. Reproduced on main @ bba473c: validate({'id':'s','run':None}) and validate({'id':'s','run':['echo','hi']}) both return [], and execute then runs 'None' / ['echo'... (shell error "'None' is not recognized").

Fix

Add a type check after the presence check, mirroring the command-step (#3262) and gate-options validation patterns. An expression like {{ steps.x.output }} is still a str, so expression configs stay valid.

Testing

  • test_validate_rejects_non_string_run parametrized over None, a list, and an int → all now report 'run' must be a string. Fails before (validate returns [] — verified by source-stash), passes after.
  • test_validate_accepts_string_and_expression_run: plain and {{ ... }} strings return [].
  • Full TestShellStep: 11 passed. uvx ruff check clean.

Note: open PR #3328 also edits this validate() (adds timeout validation) and the TestShellStep region — I placed the new checks/tests in distinct spots to minimize any textual conflict; the two changes are independent.

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Fable 5) under my direction. AI flagged the validate-vs-execute type gap; I reproduced the shell-injection of the repr, verified fail-before/pass-after, and reviewed the diff.

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 (github#3262) and gate options
validation. Expression strings ('{{ ... }}') are strings, so they stay
valid.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jawwad-ali jawwad-ali requested a review from mnriem as a code owner July 5, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant