fix(workflows): strip the resolved value before matching switch cases (#4143)
`SwitchStep.execute` matched with `str(value)` and no strip. The values a
switch dispatches on are overwhelmingly captured command output, and
`ShellStep` stores `proc.stdout` verbatim, so `run: echo approve` resolves
to "approve\n" — which matches no `approve:` case:
stdout stored : 'approve\n'
matched_case : '__default__' <-- silently wrong
next steps : ['fallback']
The switch falls through to `default:` (or dispatches nothing at all) while
still reporting COMPLETED. A workflow author cannot fix it themselves: the
registered filters are default/join/map/contains/from_json — there is no
`trim`.
spec-kit already treats exactly this as a bug wherever else it matches a
resolved string against declared literals — `evaluate_condition` strips for
this same shell-newline reason, and `InitStep._resolve_bool` does
`resolved.strip().lower()`. Switch case keys are such literals, and this was
the only site not stripping.
`expression_value` still reports the raw value, so nothing downstream loses
information, and a genuine mismatch ("approve-later") still falls through.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -12,7 +12,8 @@ class SwitchStep(StepBase):
|
||||
"""Multi-branch dispatch on an expression.
|
||||
|
||||
Evaluates ``expression:`` once, matches against ``cases:`` keys
|
||||
(exact match, string-coerced). Falls through to ``default:`` if
|
||||
(exact match; the resolved value is string-coerced and stripped of
|
||||
surrounding whitespace first). Falls through to ``default:`` if
|
||||
no case matches.
|
||||
"""
|
||||
|
||||
@@ -22,8 +23,18 @@ class SwitchStep(StepBase):
|
||||
expression = config.get("expression", "")
|
||||
value = evaluate_expression(expression, context)
|
||||
|
||||
# String-coerce for matching
|
||||
str_value = str(value) if value is not None else ""
|
||||
# String-coerce for matching, stripping surrounding whitespace first.
|
||||
# The value a switch dispatches on is most often captured command
|
||||
# output, and a ``shell`` step stores ``proc.stdout`` verbatim, so
|
||||
# ``run: echo approve`` resolves to ``"approve\n"`` and matches no
|
||||
# ``approve:`` case -- the switch silently falls through to ``default:``
|
||||
# while still reporting COMPLETED. A workflow cannot strip it itself:
|
||||
# the registered filters are default/join/map/contains/from_json, there
|
||||
# is no ``trim``. ``evaluate_condition`` and ``InitStep._resolve_bool``
|
||||
# already strip before matching a resolved string against declared
|
||||
# literals, and case keys are exactly such literals. ``expression_value``
|
||||
# below still reports the raw value, so nothing downstream loses it.
|
||||
str_value = str(value).strip() if value is not None else ""
|
||||
|
||||
cases = config.get("cases", {})
|
||||
if not isinstance(cases, dict):
|
||||
|
||||
@@ -3128,6 +3128,55 @@ class TestIfThenStep:
|
||||
class TestSwitchStep:
|
||||
"""Test the switch step type."""
|
||||
|
||||
def test_execute_matches_case_ignoring_surrounding_whitespace(self):
|
||||
"""A shell step's stdout keeps its trailing newline; the case must match.
|
||||
|
||||
`ShellStep` stores `proc.stdout` verbatim, so `run: echo approve`
|
||||
resolves to "approve" plus a newline. Unstripped, that matched no
|
||||
`approve:` case and the switch silently fell through to `default:`
|
||||
while still reporting COMPLETED. There is no `trim` filter, so a
|
||||
workflow author cannot strip it themselves.
|
||||
"""
|
||||
from specify_cli.workflows.steps.switch import SwitchStep
|
||||
from specify_cli.workflows.base import StepContext, StepStatus
|
||||
|
||||
config = {
|
||||
"id": "route",
|
||||
"expression": "{{ steps.check.output.stdout }}",
|
||||
"cases": {
|
||||
"approve": [{"id": "approved", "type": "command", "command": "echo"}],
|
||||
"reject": [{"id": "rejected", "type": "command", "command": "echo"}],
|
||||
},
|
||||
"default": [{"id": "fallback", "type": "command", "command": "echo"}],
|
||||
}
|
||||
for raw in ("approve\n", "approve\r\n", " approve ", "approve"):
|
||||
ctx = StepContext(steps={"check": {"output": {"stdout": raw}}})
|
||||
result = SwitchStep().execute(config, ctx)
|
||||
assert result.status == StepStatus.COMPLETED
|
||||
assert result.output["matched_case"] == "approve", repr(raw)
|
||||
assert [s["id"] for s in result.next_steps] == ["approved"], repr(raw)
|
||||
# The raw value is still reported unchanged.
|
||||
assert result.output["expression_value"] == raw
|
||||
|
||||
def test_execute_still_falls_through_for_a_genuine_mismatch(self):
|
||||
"""Stripping must not make unrelated values match."""
|
||||
from specify_cli.workflows.steps.switch import SwitchStep
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
config = {
|
||||
"id": "route",
|
||||
"expression": "{{ steps.check.output.stdout }}",
|
||||
"cases": {
|
||||
"approve": [{"id": "approved", "type": "command", "command": "echo"}]
|
||||
},
|
||||
"default": [{"id": "fallback", "type": "command", "command": "echo"}],
|
||||
}
|
||||
ctx = StepContext(steps={"check": {"output": {"stdout": "approve-later\n"}}})
|
||||
result = SwitchStep().execute(config, ctx)
|
||||
|
||||
assert result.output["matched_case"] == "__default__"
|
||||
assert [s["id"] for s in result.next_steps] == ["fallback"]
|
||||
|
||||
def test_execute_matches_case(self):
|
||||
from specify_cli.workflows.steps.switch import SwitchStep
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
Reference in New Issue
Block a user