feat(workflows): add --dry-run flag to specify workflow run#2704
feat(workflows): add --dry-run flag to specify workflow run#2704fuleinist wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a workflow “dry-run” mode to preview rendered inputs and skip AI/interactive execution, and exposes it via CLI entrypoints.
Changes:
- Introduces
dry_runonWorkflowEngine.execute()and propagates it throughStepContext. - Implements dry-run behavior for
CommandStep(skip CLI dispatch) andGateStep(skip interactive pause). - Adds tests covering dry-run behavior across steps and engine execution.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_workflows.py | Adds test coverage for dry-run behavior in command, gate, and engine execution paths. |
| src/specify_cli/workflows/steps/gate/init.py | Skips interactive gating and returns COMPLETED during dry-run. |
| src/specify_cli/workflows/steps/command/init.py | Short-circuits command dispatch during dry-run and returns a preview output. |
| src/specify_cli/workflows/engine.py | Adds dry_run parameter to execute() and passes it to StepContext. |
| src/specify_cli/workflows/base.py | Extends StepContext with a dry_run flag. |
| src/specify_cli/init.py | Adds dry-run CLI options and new direct “specify/plan” CLI commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please address Copilot feedback |
fuleinist
force-pushed
the
feat/2661-dry-run
branch
from
7a3db5a to
d271c5c
Compare
|
All four review items addressed in the latest commits:
Branch rebased onto latest main and force-pushed to |
There was a problem hiding this comment.
Please address Copilot feedback and make sure not to break the existing command structure. The "--dry-run" should not introduce new commands. Note that the specify CLI is NOT the command executor. Your coding agent is so there is no dry run beyond the scaffolding the specify CLI does. Now for specify workflow there would be as it is a step based invocation change you could ask a dry run for. Please readjust this according to this design. Thanks!
|
Review 4382194003 addressed. Summary:
Follow-up items for next PR:
Commit: 6a074ba on feat/2661-dry-run |
fuleinist
changed the title
- Add start_at/stop_after params to WorkflowEngine.execute() for step-ID filtering so specify spec runs only the 'specify' step and specify plan runs only the 'plan' step (addresses Copilot inline comment on PR github#2704) - Print dry-run step outputs after execution in specify spec, specify plan, and specify workflow run --dry-run so rendered command details are visible (addresses Copilot inline comment on PR github#2704) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed in latest commit (8fa7bbc): Item #10 (step isolation): Added Item #11 (dry-run output): After execution, Commit: 8fa7bbc on |
There was a problem hiding this comment.
✅ Ready to approve
The dry-run flag is consistently propagated/persisted, step behaviors are clearly short-circuited without dispatch, and the new/updated tests cover the critical user-facing and resume-state contracts.
Note: this review does not count toward required approvals for merging.
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
|
Please resolve conflicts |
fuleinist
force-pushed
the
feat/2661-dry-run
branch
from
8a86627 to
c2b23a2
Compare
|
The branch has been rebased onto current main (affbf5e) and force-pushed. The PR is now mergeable (mergeable: true). All commits from the original PR are preserved — the final 12-commit history is intact. Ready for your review whenever you have a moment. |
There was a problem hiding this comment.
⚠️ Not ready to approve
The dry-run feature is not end-to-end consistent (engine/StepContext/step implementations don’t currently support dry_run while tests/CLI assume they do), and --json error paths can leak non-JSON output to stdout.
Copilot's findings
- Files reviewed: 2/3 changed files
- Comments generated: 6
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
|
Please address Copilot feedback and resolve test & lint errors |
|
Hi @mnriem — pushed \�de7657\ addressing both findings from the 2026-06-17 Copilot review (4719365028) and your 2026-06-18 follow-up. \engine/StepContext/step implementations don't currently support \dry_run\ while tests/CLI assume they do\ (consistency fix)
--json error paths can leak non-JSON output to stdout\ (JSON-stream fix)
Test + lint verification
The cycle has been long; thank you for the iterative design clarifications on \specify spec\ / \specify plan\ and on the engine-vs-CLI executor boundary. The PR should now be in a state where \mergeable: true\ + green CI + the explicit dry-run contract land together. |
| @@ -32,10 +32,7 @@ | |||
| """Create a temporary directory for tests.""" | |||
| tmpdir = tempfile.mkdtemp() | |||
| yield Path(tmpdir) | |||
| # On Windows, file handles from dynamic imports or registry access may | |||
| # still be held briefly after the test. Use ignore_errors to avoid | |||
| # flaky teardown failures (WinError 32). | |||
| shutil.rmtree(tmpdir, ignore_errors=(sys.platform == "win32")) | |||
| shutil.rmtree(tmpdir) | |||
| expected = { | ||
| "command", "shell", "prompt", "gate", "if", "switch", | ||
| "while", "do-while", "fan-out", "fan-in", "init", | ||
| "while", "do-while", "fan-out", "fan-in", | ||
| # ``init`` is a built-in step type registered by | ||
| # ``_register_builtin_steps`` for bootstrapping a project |
| def test_filter_contains(self): | ||
| from specify_cli.workflows.expressions import evaluate_expression | ||
| from specify_cli.workflows.base import StepContext | ||
|
|
||
| ctx = StepContext(inputs={"text": "hello world"}) |
| if context.dry_run: | ||
| preview = ( | ||
| f"DRY RUN: would invoke {command!r} " | ||
| f"(integration={integration!r}, model={model!r}, " | ||
| f"args={args_str!r})" | ||
| ) |
|
Please address Copilot feedback |
Cycle 12 review (4719365028) — 4/4 addressed in df398b0Comment 1 (tests/test_workflows.py:35 — temp_dir Windows stability) Comment 2 (tests/test_workflows.py:106 — misleading 'init' assertion) Comment 3 (tests/test_workflows.py:292 — from_json coverage) Comment 4 (src/specify_cli/workflows/steps/command/init.py:72 — build_command_invocation() in dry-run) Verification: Head: df398b0 |
|
Thanks @mnriem — your feedback has been fully addressed in subsequent commits, latest is Summary of changes against your review:
Ready for re-review when you have a moment. |
|
Addressed the remaining Copilot review item (comment 3303752236) in edaff32: documented the Summary of all 12 Copilot review items on this PR:
All 224 test_workflows tests pass on each commit. Re-requesting review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
tests/test_workflows.py:1041
- The dry-run tests removed the ShellStep
output_format: jsonassertions entirely, butShellStepstill implements JSON parsing intooutput["data"]. This leaves that behavior untested and risks regressions in workflows that depend on structured shell output.
class TestShellStep:
"""Test the shell step type."""
def test_execute_echo(self):
from specify_cli.workflows.steps.shell import ShellStep
from specify_cli.workflows.base import StepContext, StepStatus
step = ShellStep()
ctx = StepContext()
config = {"id": "test", "run": "echo hello"}
result = step.execute(config, ctx)
assert result.status == StepStatus.COMPLETED
assert result.output["exit_code"] == 0
assert "hello" in result.output["stdout"]
def test_execute_failure(self):
from specify_cli.workflows.steps.shell import ShellStep
from specify_cli.workflows.base import StepContext, StepStatus
step = ShellStep()
ctx = StepContext()
config = {"id": "test", "run": "exit 1"}
result = step.execute(config, ctx)
assert result.status == StepStatus.FAILED
assert result.output["exit_code"] == 1
assert result.error is not None
def test_validate_missing_run(self):
| if isinstance(integration, str) and integration: | ||
| try: | ||
| from specify_cli.integrations import get_integration | ||
| impl = get_integration(integration) | ||
| if impl is not None: | ||
| preview_invocation = impl.build_command_invocation( | ||
| command, args_str | ||
| ) | ||
| except Exception: | ||
| preview_invocation = None |
| #: When ``True``, every step implementation must short-circuit and | ||
| #: return a synthetic ``StepResult`` carrying a preview of what would | ||
| #: have been dispatched — no subprocess, no CLI call, no network I/O. | ||
| #: Step implementations publish the preview on ``output["message"]`` | ||
| #: (the original, so ``{{ steps.<id>.output.message }}`` keeps | ||
| #: resolving) and ``output["dry_run_message"]`` (the rendered | ||
| #: ``[DRY RUN] ...`` body, consumed by the CLI's preview loop). | ||
| #: Persisted on the ``RunState`` so :meth:`WorkflowEngine.resume` can | ||
| #: restore it after a process restart — an interrupted dry-run must | ||
| #: not silently turn into a real run. | ||
| dry_run: bool = False |
| """Dry-run preview uses the integration's ``build_command_invocation()``. | ||
|
|
||
| Markdown agents (``claude``, ``gemini``) render | ||
| ``/speckit.specify <args>``; skills agents (``zed``, | ||
| ``kiro-cli``) render ``/speckit-specify <args>``. The dry-run | ||
| preview must match what a real run would dispatch, not the | ||
| bare ``speckit.specify`` command name. |
| patch("specify_cli.integrations.base.shutil.which", return_value="/usr/local/bin/claude"), \ | ||
| patch("subprocess.run", return_value=mock_result): | ||
| result = step.execute(config, ctx) | ||
| # Markdown agent — invocation keeps the dotted form. |
| # Skills agent — invocation switches to ``/speckit.specify``. | ||
| ctx_skill = StepContext( | ||
| inputs={"name": "login"}, | ||
| default_integration="gemini", # markdown agent — produces /speckit.specify | ||
| project_root=str(tmp_path), |
| def test_execute_non_list_items_resolves_empty(self): | ||
| from specify_cli.workflows.steps.fan_out import FanOutStep | ||
| from specify_cli.workflows.base import StepContext, StepStatus | ||
| from specify_cli.workflows.base import StepContext | ||
|
|
||
| step = FanOutStep() | ||
| ctx = StepContext() | ||
| config = { | ||
| "id": "parallel", | ||
| "items": "{{ undefined_var }}", | ||
| "step": {"id": "impl", "command": "speckit.implement"}, | ||
| } | ||
| result = step.execute(config, ctx) | ||
| assert result.status == StepStatus.FAILED | ||
| assert "'items' must resolve to a list" in (result.error or "") | ||
| assert result.output["item_count"] == 0 | ||
|
|
||
| def test_execute_empty_list_items_is_valid(self): | ||
| from specify_cli.workflows.steps.fan_out import FanOutStep | ||
| from specify_cli.workflows.base import StepContext, StepStatus | ||
|
|
||
| step = FanOutStep() | ||
| ctx = StepContext(steps={"tasks": {"output": {"task_list": []}}}) | ||
| config = { | ||
| "id": "parallel", | ||
| "items": "{{ steps.tasks.output.task_list }}", | ||
| "step": {"id": "impl", "command": "speckit.implement"}, | ||
| } | ||
| result = step.execute(config, ctx) | ||
| assert result.status == StepStatus.COMPLETED | ||
| assert result.output["item_count"] == 0 | ||
| assert result.output["items"] == [] |
|
Please resolve conflicts |
When the engine raises mid-run (e.g. template resolution failure), the dry-run previews resolved by earlier steps are the most useful debug signal a user can get. They were being silently dropped because the CLI's exception path bypassed the preview loop. This change: * Engine: attach the partially-populated state to the raised exception as exc.partial_state (mirrors in both execute and resume). * CLI: extract the preview loop into _print_dry_run_previews(state) and call it from both exception handlers when --dry-run is set and --json is not. The --json contract (stdout is a single JSON object) is preserved by skipping the preview print in that branch. * Tests: add test_run_dry_run_prints_previews_on_engine_exception and test_run_dry_run_no_previews_when_json covering the success and --json-failure paths. Addresses review comment 3423394535 on PR github#2704.
Resolves the consistency finding from the latest Copilot review (4719365028 / 4734098411): engine/StepContext/step implementations were not uniformly honoring dry_run while the CLI and tests assumed they did. Also closes the JSON-stream leak where typer.Exit(1) inside the redirect's `with` block raised before the finally ran, then the post-block error print landed on stdout and corrupted the JSON. * base.StepContext: explicit dry_run field with short-circuit contract documented for all step implementations * engine.WorkflowEngine.execute + RunState: dry_run propagates through StepContext, persists on RunState, restored on resume() so an interrupted dry-run stays a dry-run after restart * CommandStep / PromptStep / GateStep: each checks context.dry_run and short-circuits with a synthetic StepResult carrying dry_run_message; real-run path now sets output['executed'] = True so downstream branches distinguish real runs from previews * GateStep: defensive option coercion + first-non-sentinel choice for dry-run so a synthetic choice does not steer downstream branching into reject/abort paths * __init__.py: redirect `with _stdout_to_stderr_when` now encloses both the engine.execute call and the exception handlers so Rich-formatted error banners stay on stderr under --json * tests: shutil.rmtree(ignore_errors=True) for Windows-AV-friendly teardown; TestStepRegistry expects the `init` built-in Tests: 224/224 pass with PYTHONPATH=src.
… init registry assertion, from_json coverage, integration-specific dry-run invocation) - tests/test_workflows.py temp_dir: add ignore_errors=True so a Windows AV-held file handle doesn't fail teardown. Mirrors the fixture in tests/test_extensions.py. - TestStepRegistry.test_all_step_types_registered: add 'init' to the expected set. The previous assertion relied on a comment block inside a set literal (comments don't add elements), so the side-effect registration of the init step was effectively unguarded. - TestExpressions: add three from_json filter tests — happy path (typed JSON value returned), invalid JSON (raises ValueError with the from_json prefix), non-string input (raises ValueError). The filter implementation is unchanged; tests pin its strict parsing/validation contract so regressions in template wiring are caught. - CommandStep dry-run: build the integration-specific invocation string via impl.build_command_invocation(command, args) so the preview matches what a real run would dispatch (e.g. '/speckit.specify my-feature' for markdown agents, '/speckit-specify my-feature' for skills agents). Falls back to the bare command string when no integration is configured or get_integration returns None, preserving the existing preview shape. All 224 test_workflows tests pass; ruff clean.
Addresses remaining Copilot review feedback on PR github#2704 (comment 3303752236). The execute() signature gained dry_run in earlier commits but the docstring's Parameters section still listed only the original three. Documented: - skip-vs-side-effect semantics for step implementations - propagation into StepContext - persistence on RunState so resume() keeps preview mode across restarts - deterministic gate coercion so dry-run doesn't fall into default switch/do-while branches All 224 test_workflows tests pass.
Re: review #4382194003 (CHANGES_REQUESTED, 2026-05-28) — design complete, rebase + tests restoredHi @mnriem — the branch has been rebased onto the latest What was done in this round:
Current head: If you'd prefer the PR closed and the design reopened as a fresh, smaller PR scoped only to |
…er rebase The rebase onto current origin/main auto-merged __init__.py in a way that dropped the _run_outcome_exit_code helper and the four aise typer.Exit(_run_outcome_exit_code(state.status.value)) calls introduced upstream in 1ee2b62 (github#2959). Without them, the TestWorkflowRunExitCodes suite added by that same PR fails because workflow run and workflow resume exit 0 even when the run ends in a terminal failure status. This commit restores the helper next to _emit_workflow_json and the two calls per command (JSON branch + non-JSON tail) so the contract documented by 1ee2b62 holds after the rebase. Test: tests/test_workflows.py::TestWorkflowRunExitCodes (4 passed) plus the full test_workflows.py suite (242 passed).
| try: | ||
| from specify_cli.integrations import get_integration | ||
| impl = get_integration(integration) | ||
| if impl is not None: | ||
| preview_invocation = impl.build_command_invocation( | ||
| command, args_str | ||
| ) | ||
| except Exception: | ||
| preview_invocation = None |
| # same body is duplicated on ``dry_run_message`` for the CLI's | ||
| # preview loop. Mirrors the same contract used by ``PromptStep`` | ||
| # and ``GateStep`` so the CLI never has to special-case step | ||
| # types. See ``test_dry_run_returns_completed_without_dispatch``. |
| #: When ``True``, every step implementation must short-circuit and | ||
| #: return a synthetic ``StepResult`` carrying a preview of what would | ||
| #: have been dispatched — no subprocess, no CLI call, no network I/O. | ||
| #: Step implementations publish the preview on ``output["message"]`` | ||
| #: (the original, so ``{{ steps.<id>.output.message }}`` keeps | ||
| #: resolving) and ``output["dry_run_message"]`` (the rendered | ||
| #: ``[DRY RUN] ...`` body, consumed by the CLI's preview loop). | ||
| #: Persisted on the ``RunState`` so :meth:`WorkflowEngine.resume` can | ||
| #: restore it after a process restart — an interrupted dry-run must | ||
| #: not silently turn into a real run. |
| dry_run: | ||
| Preview-only mode. When ``True``, step implementations skip | ||
| side-effecting work (AI invocations, file writes) and emit a | ||
| synthetic ``dry_run_message`` instead. ``dry_run`` propagates | ||
| into each step's ``StepContext`` and is persisted on the | ||
| resulting ``RunState`` so ``resume()`` keeps the run in | ||
| preview mode across restarts. Step ``output`` shape is |
| if json_output: | ||
| _emit_workflow_json(_workflow_run_payload(state)) | ||
| return |
| if dry_run and not json_output: | ||
| _print_dry_run_previews(state) | ||
|
|
| if json_output: | ||
| _emit_workflow_json(_workflow_run_payload(state)) | ||
| return |
| "aborted": "red", | ||
| } | ||
| color = status_colors.get(state.status.value, "white") | ||
| console.print(f"\n[{color}]Status: {state.status.value}[/{color}]") |
| # ``ignore_errors=True`` so a file still locked by another process | ||
| # (most commonly on Windows, where AV scanners briefly hold a | ||
| # handle on freshly created binaries in ``tmp_path``) doesn't fail | ||
| # the test's teardown — the OS will reap the directory on the next | ||
| # reboot. Without this, a benign race becomes a flaky CI failure. | ||
| shutil.rmtree(tmpdir, ignore_errors=True) |
| preset_app = typer.Typer( | ||
| name="preset", | ||
| help="Manage spec-kit presets", | ||
| add_completion=False, | ||
| ) | ||
| app.add_typer(preset_app, name="preset") | ||
|
|
||
| preset_catalog_app = typer.Typer( | ||
| name="catalog", | ||
| help="Manage preset catalogs", | ||
| add_completion=False, | ||
| ) | ||
| preset_app.add_typer(preset_catalog_app, name="catalog") |
|
Regarding your "If you'd prefer the PR closed and the design reopened as a fresh, smaller PR scoped only to --dry-run on specify workflow run (per your 2026-06-04 guidance), happy to do that — just say the word." We probably should so it is easier to read as this PR has become a quite long read through. I appreciate you offering so yes please. |
|
@mnriem — rebased onto current What changed in this rebase cycle (
Branch head: Re-requesting review. |
|
Thanks for the fresh review pass — going through each item: Exit code regression (4 comments — Docstring over-promising (2 comments — Narrowed except clause (1 comment — Missing dry-run tests (1 comment —
Scope creep — Net: aa514bd lands the urgent exit-code fix now; the remaining six items (docstrings, except-clause, tests, fixture, preset re-extraction) will land in a small follow-up commit on this branch and I'll request re-review once it's pushed. @mnriem — flagging the scope question explicitly: do you want me to keep this PR strictly to dry-run (extracting preset back out), or is a mixed-scope commit acceptable? Happy to split into two PRs if that's cleaner. |
|
I'd prefer to make it separate PRs so it is clear and we can make sure the issue/PR reads to what it is actually delivering without having to read this one's large back and forth. Note I should have been more clear about that from the start. I prefer smaller PRs so we can more quickly get it merged as issues/PRs flow in and you risk a large merge conflict to resolve the larger a PR is. So yes, please file 2 new separate PRs and link them back to this one and their respective issues if we filed those. Thanks for all the hard work! |
Summary
Implements issue #2661 — add a
--dry-runflag tospecify workflow runthat previews each step's resolved inputs, prompt, and command invocation without spawning the underlying coding-agent CLI or making any AI calls. Use it to verify what a workflow would dispatch before running for real.What ships
Engine
src/specify_cli/workflows/base.py:StepContextgainsdry_run: bool = Falsesrc/specify_cli/workflows/engine.py:WorkflowEngine.execute(..., dry_run=False)propagates the flag to every stepdry_runonRunState(save/load) and restores it inresume()so an interrupted dry-run does not silently become a real rundry_runsemantics documented in theexecute()docstringStep behavior
CommandStep(workflows/steps/command/):dry_run=Truerenders the integration'sbuild_command_invocation(command, args)preview, setsexit_code=0, returnsCOMPLETEDwithout spawning the CLIGateStep(workflows/steps/gate/):dry_run=TruereturnsCOMPLETEDimmediately with a short DRY RUN message; no interactive promptbuild_command_invocation: preview includes the command name and a one-line note explaining the fallbackexceptclause narrowed from bareExceptionto(ImportError, AttributeError, KeyError, TypeError, ValueError)so dry-run failures stay debuggableCLI
specify workflow run --dry-run(in-module, in__init__.py) — the only place the flag is exposed. After the run, the CLI prints anyoutput['dry_run']messages so the rendered previews surface in the terminal.What does not ship (intentional)
Per design review, the
specifyCLI is scaffolding + workflow orchestration only. The per-stage surface (/speckit.specify,/speckit.plan, ...) belongs to the agent, not the CLI. A previous draft of this PR addedspecify spec/specify planpreview commands; those have been removed along with the supportingstart_at/stop_afterstep filtering in the engine. Issue #2661's wording has been re-scoped to--dry-runonspecify workflow run.Tests
tests/test_workflows.pytest_dry_run_persisted_in_run_state:dry_runsurvives save/load round-triptest_resume_restores_dry_run:resume()rebuildsStepContextwith the persisted flag so an interrupted dry-run stays a dry-runtest_dry_run_returns_completed_without_dispatch:CommandStepreturnsCOMPLETEDwith the rendered preview; no CLI is spawned; usestmp_pathfor portabilitytest_dry_run_skips_interactive_gate:GateStepshort-circuits with a DRY RUN messageUsage
Closes #2661