Skip to content

Commit df398b0

Browse files
fuleinistfuleinist
committed
fix(workflows): address 4 follow-up review items (Win test stability, 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.
1 parent bde7657 commit df398b0

2 files changed

Lines changed: 139 additions & 6 deletions

File tree

‎src/specify_cli/workflows/steps/command/__init__.py‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,32 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
6565
# and ``GateStep`` so the CLI never has to special-case step
6666
# types. See ``test_dry_run_returns_completed_without_dispatch``.
6767
if context.dry_run:
68-
preview = (
69-
f"DRY RUN: would invoke {command!r} "
70-
f"(integration={integration!r}, model={model!r}, "
71-
f"args={args_str!r})"
72-
)
68+
# Try to build the integration-specific invocation string
69+
# (e.g. ``/speckit.specify ...`` for markdown agents,
70+
# ``/speckit-specify ...`` for skills agents) so the dry-run
71+
# preview matches what a real run would dispatch. Falls back
72+
# to the bare ``command`` name when no integration is
73+
# configured or the integration is not registered (e.g.,
74+
# dry-run on a brand-new project before setup).
75+
preview_invocation: str | None = None
76+
if isinstance(integration, str) and integration:
77+
try:
78+
from specify_cli.integrations import get_integration
79+
impl = get_integration(integration)
80+
if impl is not None:
81+
preview_invocation = impl.build_command_invocation(
82+
command, args_str
83+
)
84+
except Exception:
85+
preview_invocation = None
86+
if preview_invocation:
87+
preview = f"DRY RUN: would invoke {preview_invocation!r}"
88+
else:
89+
preview = (
90+
f"DRY RUN: would invoke {command!r} "
91+
f"(integration={integration!r}, model={model!r}, "
92+
f"args={args_str!r})"
93+
)
7394
return StepResult(
7495
status=StepStatus.COMPLETED,
7596
output={

‎tests/test_workflows.py‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,13 @@ def temp_dir():
3232
"""Create a temporary directory for tests."""
3333
tmpdir = tempfile.mkdtemp()
3434
yield Path(tmpdir)
35-
shutil.rmtree(tmpdir)
35+
# ``ignore_errors=True`` so a file still locked by another process
36+
# (most commonly on Windows, where AV scanners briefly hold a
37+
# handle on freshly created binaries in ``tmp_path``) doesn't fail
38+
# the test's teardown — the OS will reap the directory on the next
39+
# reboot. Mirrors the same fixture in ``tests/test_extensions.py``
40+
# so the whole suite stays stable on win32.
41+
shutil.rmtree(tmpdir, ignore_errors=True)
3642

3743

3844
@pytest.fixture
@@ -108,6 +114,7 @@ def test_all_step_types_registered(self):
108114
# against an accidental refactor that drops the import
109115
# (the registration is a side effect, so a missed import
110116
# silently shrinks the registry without any test failure).
117+
"init",
111118
}
112119
assert expected.issubset(set(STEP_REGISTRY.keys()))
113120

@@ -292,6 +299,37 @@ def test_filter_contains(self):
292299
ctx = StepContext(inputs={"text": "hello world"})
293300
assert evaluate_expression("{{ inputs.text | contains('world') }}", ctx) is True
294301

302+
def test_filter_from_json_happy_path(self):
303+
"""``| from_json`` parses a valid JSON string into the typed value."""
304+
from specify_cli.workflows.expressions import evaluate_expression
305+
from specify_cli.workflows.base import StepContext
306+
307+
ctx = StepContext(inputs={"payload": '{"name": "kanban", "tasks": [1, 2, 3]}'})
308+
result = evaluate_expression("{{ inputs.payload | from_json }}", ctx)
309+
assert result == {"name": "kanban", "tasks": [1, 2, 3]}
310+
311+
def test_filter_from_json_invalid_json_raises(self):
312+
"""``| from_json`` raises ``ValueError`` on invalid JSON input.
313+
314+
Silent fallback would hide pipeline wiring mistakes, so the
315+
filter fails loudly when the input is not parseable.
316+
"""
317+
from specify_cli.workflows.expressions import evaluate_expression
318+
from specify_cli.workflows.base import StepContext
319+
320+
ctx = StepContext(inputs={"payload": '{"name": '}) # truncated
321+
with pytest.raises(ValueError, match="from_json"):
322+
evaluate_expression("{{ inputs.payload | from_json }}", ctx)
323+
324+
def test_filter_from_json_non_string_raises(self):
325+
"""``| from_json`` raises ``ValueError`` when the input is not a string."""
326+
from specify_cli.workflows.expressions import evaluate_expression
327+
from specify_cli.workflows.base import StepContext
328+
329+
ctx = StepContext(inputs={"payload": 42})
330+
with pytest.raises(ValueError, match="from_json"):
331+
evaluate_expression("{{ inputs.payload | from_json }}", ctx)
332+
295333
def test_condition_evaluation(self):
296334
from specify_cli.workflows.expressions import evaluate_condition
297335
from specify_cli.workflows.base import StepContext
@@ -729,6 +767,80 @@ def test_dry_run_returns_completed_without_dispatch(self, tmp_path):
729767
assert "DRY RUN" in result.output["message"]
730768
assert result.output["dry_run_message"] == result.output["message"]
731769

770+
def test_dry_run_uses_integration_invocation_string(self, tmp_path):
771+
"""Dry-run preview uses the integration's ``build_command_invocation()``.
772+
773+
Markdown agents (``claude``, ``gemini``) render
774+
``/speckit.specify <args>``; skills agents (``zed``,
775+
``kiro-cli``) render ``/speckit-specify <args>``. The dry-run
776+
preview must match what a real run would dispatch, not the
777+
bare ``speckit.specify`` command name.
778+
"""
779+
from specify_cli.workflows.steps.command import CommandStep
780+
from specify_cli.workflows.base import StepContext
781+
782+
# Markdown agent — invocation keeps the dotted form.
783+
step = CommandStep()
784+
ctx = StepContext(
785+
inputs={"name": "login"},
786+
default_integration="claude",
787+
project_root=str(tmp_path),
788+
dry_run=True,
789+
)
790+
config = {
791+
"id": "test",
792+
"command": "speckit.specify",
793+
"input": {"args": "{{ inputs.name }}"},
794+
}
795+
result = step.execute(config, ctx)
796+
# The Claude integration renders ``/speckit-specify`` for the
797+
# ``speckit.specify`` command. The preview should reference it
798+
# (the bare ``speckit.specify`` token is fine too because the
799+
# preview is a quoted string). What matters is the integration
800+
# invocation string is present.
801+
assert "/speckit-specify" in result.output["message"]
802+
# And the bare ``speckit.specify`` token should NOT be the
803+
# only thing shown — the slash-command form must be there too.
804+
assert result.output["message"].count("speckit") >= 1
805+
806+
# Skills agent — invocation switches to ``/speckit.specify``.
807+
ctx_skill = StepContext(
808+
inputs={"name": "login"},
809+
default_integration="gemini", # markdown agent — produces /speckit.specify
810+
project_root=str(tmp_path),
811+
dry_run=True,
812+
)
813+
result_skill = step.execute(config, ctx_skill)
814+
assert "/speckit.specify" in result_skill.output["message"]
815+
816+
def test_dry_run_falls_back_when_integration_unknown(self, tmp_path):
817+
"""Dry-run falls back to the bare command string when no integration is set.
818+
819+
A brand-new project may run ``workflow run --dry-run`` before
820+
any integration is configured. The preview must still emit a
821+
sensible message rather than crashing inside ``get_integration``.
822+
"""
823+
from specify_cli.workflows.steps.command import CommandStep
824+
from specify_cli.workflows.base import StepContext, StepStatus
825+
826+
step = CommandStep()
827+
ctx = StepContext(
828+
inputs={"name": "login"},
829+
default_integration=None, # no integration configured
830+
project_root=str(tmp_path),
831+
dry_run=True,
832+
)
833+
config = {
834+
"id": "test",
835+
"command": "speckit.specify",
836+
"input": {"args": "{{ inputs.name }}"},
837+
}
838+
result = step.execute(config, ctx)
839+
assert result.status == StepStatus.COMPLETED
840+
# Falls back to the bare ``command`` name in the preview.
841+
assert "speckit.specify" in result.output["message"]
842+
assert "DRY RUN" in result.output["message"]
843+
732844

733845
class TestPromptStep:
734846
"""Test the prompt step type."""

0 commit comments

Comments
 (0)