From 23e79b4fa9380c82f8489f837d7eabc8dcf19365 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 20:11:36 +0500 Subject: [PATCH] fix(workflows): strip the resolved value before matching switch cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../workflows/steps/switch/__init__.py | 17 +++++-- tests/test_workflows.py | 49 +++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/workflows/steps/switch/__init__.py b/src/specify_cli/workflows/steps/switch/__init__.py index 690df0f19a..93145e870d 100644 --- a/src/specify_cli/workflows/steps/switch/__init__.py +++ b/src/specify_cli/workflows/steps/switch/__init__.py @@ -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 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: 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): diff --git a/tests/test_workflows.py b/tests/test_workflows.py index afd70adecf..fa24c894d6 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3128,6 +3128,55 @@ def test_validate_accepts_missing_else(self): 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