You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Add opt-in strict: true for workflow shell steps. Strict mode rejects workflow expression markers in run during validation and execution, while retaining fixed shell scripts, pipes, and redirects.
Add per-step env bindings that evaluate expressions separately and pass string values through the child process environment rather than inserting them into shell source. Invalid bindings fail before process launch, and SPECKIT_WORKFLOW_DIR remains engine-owned.
Existing raw interpolation remains unchanged when strict is omitted or false. Update the workflow reference, authoring guidance, and step design to explain adoption and remaining risks: strict mode is not a sandbox and does not protect scripts that reinterpret environment data as code.
Testing
Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest
Tested with a sample project (if applicable)
Used the documented worktree-local interpreter instead of the legacy test command in the checklist:
Sample-project CLI execution with strict: true and an expression-bound environment value — completed; shell metacharacters remained literal output.
Regression coverage includes strict-mode rejection before subprocess launch, invalid configuration and resolved values, input/prior-output/fan-out-item bindings, environment isolation, legacy interpolation, fixed shell pipelines, and engine failure/continuation behavior.
AI Disclosure
I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)
AI disclosure: GitHub Copilot, powered by GPT-6.1 Sol (gpt-6.1-sol), authored the implementation, tests, and documentation and executed validation, Git, and PR commands on behalf of @mnriem. Work was performed in an interactive session with autonomous authoring and execution under user-directed scope; reasoning effort was not exposed. The user selected the opt-in design; no human line-by-line review is claimed. A security-review subagent assisted the initial assessment; its model and settings were not reported.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
On POSIX, lone-surrogate shell bindings can pass validation and trigger an uncaught UnicodeEncodeError, aborting workflows even when continue_on_error is enabled.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Negative coverage for None and list run overrides is missing, leaving unverified that validation and execution both reject them before calling subprocess.run.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Add negative tests for non-string strict run values
tests/workflows/test_shell_step.py:70
The strict-mode guard for non-string run values has no negative coverage. Add None and list overrides here to verify validation and execution both reject them before subprocess.run is called. This covers the prevented behavior required by CONTRIBUTING.md:229–235.
Verify that strict shell steps reject null and list-valued run fields during validation and execution without invoking subprocess.run.
Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the strict binding guard as the sole run diagnostic in strict mode and preserve legacy validation behavior. Cover missing, null, and list-valued run fields with exact single-error assertions and pre-launch rejection checks.
Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human review is needed to judge whether the pre-install audit adequately covers environment bindings that can alter shell-step execution, even for strict steps.
No open issues 🎉
Review effort: Max (auto)
This branch has not been deployed
No deployments
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
triage-nice-to-haveVerdict: evidence-backed fix or greenlit feature — land after review
2 participants
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.
Description
Add opt-in
strict: truefor workflow shell steps. Strict mode rejects workflow expression markers inrunduring validation and execution, while retaining fixed shell scripts, pipes, and redirects.Add per-step
envbindings that evaluate expressions separately and pass string values through the child process environment rather than inserting them into shell source. Invalid bindings fail before process launch, andSPECKIT_WORKFLOW_DIRremains engine-owned.Existing raw interpolation remains unchanged when
strictis omitted orfalse. Update the workflow reference, authoring guidance, and step design to explain adoption and remaining risks: strict mode is not a sandbox and does not protect scripts that reinterpret environment data as code.Testing
uv run specify --helpuv sync && uv run pytestUsed the documented worktree-local interpreter instead of the legacy test command in the checklist:
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q -n 4— passed: 13,704 passed, 130 skipped.uv run specify --help— passed.strict: trueand an expression-bound environment value — completed; shell metacharacters remained literal output.Regression coverage includes strict-mode rejection before subprocess launch, invalid configuration and resolved values, input/prior-output/fan-out-item bindings, environment isolation, legacy interpolation, fixed shell pipelines, and engine failure/continuation behavior.
AI Disclosure
AI disclosure: GitHub Copilot, powered by GPT-6.1 Sol (
gpt-6.1-sol), authored the implementation, tests, and documentation and executed validation, Git, and PR commands on behalf of @mnriem. Work was performed in an interactive session with autonomous authoring and execution under user-directed scope; reasoning effort was not exposed. The user selected the opt-in design; no human line-by-line review is claimed. A security-review subagent assisted the initial assessment; its model and settings were not reported.