Skip to content

Commit 220e6fc

Browse files
jawwad-aliclaude
andauthored
fix(workflows): fan-in validate() rejects non-mapping output (#3349)
FanInStep.validate() only checked wait_for, so a non-mapping 'output' (a list or scalar) validated clean; execute() then silently coerces it to {}, so the author's declared aggregation keys vanish with COMPLETED status and no diagnostic. Reject a non-mapping output at validation, mirroring the command-step (#3262) non-mapping fix. execute()'s defensive coercion is left in place for unvalidated callers. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent fb796c2 commit 220e6fc

2 files changed

Lines changed: 30 additions & 0 deletions

File tree

src/specify_cli/workflows/steps/fan_in/__init__.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,4 +58,13 @@ def validate(self, config: dict[str, Any]) -> list[str]:
5858
f"Fan-in step {config.get('id', '?')!r}: "
5959
f"'wait_for' must be a non-empty list of step IDs."
6060
)
61+
output = config.get("output")
62+
if output is not None and not isinstance(output, dict):
63+
# execute() silently coerces a non-mapping output to {}, so the
64+
# author's declared aggregation keys would vanish with no error.
65+
# Reject at validation, mirroring the command-step (#3262) fix.
66+
errors.append(
67+
f"Fan-in step {config.get('id', '?')!r}: 'output' must be a "
68+
f"mapping of key -> expression, got {type(output).__name__}."
69+
)
6170
return errors

tests/test_workflows.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2202,6 +2202,27 @@ def test_validate_wait_for_not_list(self):
22022202
errors = step.validate({"id": "test", "wait_for": "not-a-list"})
22032203
assert any("non-empty list" in e for e in errors)
22042204

2205+
@pytest.mark.parametrize("bad_output", [["{{ fan_in.results }}"], "{{ x }}", 42])
2206+
def test_validate_rejects_non_mapping_output(self, bad_output):
2207+
"""A non-mapping 'output' must be rejected: execute() would otherwise
2208+
silently coerce it to {} and drop the declared aggregation keys."""
2209+
from specify_cli.workflows.steps.fan_in import FanInStep
2210+
2211+
step = FanInStep()
2212+
errors = step.validate(
2213+
{"id": "j", "wait_for": ["a"], "output": bad_output}
2214+
)
2215+
assert any("'output' must be a mapping" in e for e in errors)
2216+
2217+
def test_validate_accepts_mapping_or_absent_output(self):
2218+
from specify_cli.workflows.steps.fan_in import FanInStep
2219+
2220+
step = FanInStep()
2221+
assert step.validate(
2222+
{"id": "j", "wait_for": ["a"], "output": {"joined": "{{ x }}"}}
2223+
) == []
2224+
assert step.validate({"id": "j", "wait_for": ["a"]}) == []
2225+
22052226

22062227
class TestFanOutConcurrency:
22072228
"""Fan-out honors max_concurrency (WorkflowEngine._run_fan_out)."""

0 commit comments

Comments
 (0)