mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): fail fan-out loudly on a truthy non-mapping step template (#3537)
* fix(workflows): fail fan-out loudly on a truthy non-mapping step template
A fan-out step whose `step:` is a truthy scalar or list (an authoring mistake) passed execute and reached the engine, which calls template.get("id", ...) in _run_fan_out — raising AttributeError and taking down the whole run. validate already rejects a non-mapping step, but the engine does not auto-validate, so an unvalidated run crashed.
Guard execute to FAIL the step (with a clear error and normalized empty output) instead, mirroring the existing non-list items guard and the switch non-dict cases guard. Add the matching test_execute_non_dict_step_fails_loudly covering the execute-path guard (validate was already covered).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(workflows): reject explicit fan-out `step: null` in validate()
The runtime guard in execute() rejects a truthy non-mapping step, but
`config.get("step", {})` only substitutes the `{}` default for an *absent*
key — an explicit `step: null` reaches the guard as None and FAILS the step.
validate() previously exempted None (`step is not None and ...`), so such a
workflow passed validation and then failed during execution.
Align validate() with the runtime guard: a present-but-non-mapping `step`
(including `None`) is an authoring mistake and is now rejected up front.
Extend the validate and execute regression cases to cover None.
Addresses Copilot review feedback on #3537.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -25,6 +25,33 @@ class FanOutStep(StepBase):
|
||||
max_concurrency = config.get("max_concurrency", 1)
|
||||
step_template = config.get("step", {})
|
||||
|
||||
# The engine does not auto-validate step config (see
|
||||
# ``WorkflowEngine.load_workflow``). On a COMPLETED fan-out it reads the
|
||||
# ``step_template`` back out and, when it is truthy, calls
|
||||
# ``template.get("id", ...)`` in ``_run_fan_out``. A truthy non-mapping
|
||||
# ``step`` (a scalar or list authoring mistake) would crash the whole
|
||||
# run with AttributeError there — the engine invokes ``execute`` and
|
||||
# ``_run_fan_out`` with no surrounding try/except. ``validate`` already
|
||||
# rejects a non-mapping ``step``; fail this step loudly on an
|
||||
# unvalidated run instead, mirroring the ``items`` guard below. An empty
|
||||
# or absent ``step`` defaults to ``{}`` (falsy) and the engine's
|
||||
# ``if template and items`` skips fan-out, so it stays valid here.
|
||||
if not isinstance(step_template, dict):
|
||||
return StepResult(
|
||||
status=StepStatus.FAILED,
|
||||
error=(
|
||||
f"Fan-out step {config.get('id', '?')!r}: 'step' must be a "
|
||||
f"mapping (nested step template), got "
|
||||
f"{type(step_template).__name__}."
|
||||
),
|
||||
output={
|
||||
"items": [],
|
||||
"max_concurrency": max_concurrency,
|
||||
"step_template": {},
|
||||
"item_count": 0,
|
||||
},
|
||||
)
|
||||
|
||||
if not isinstance(items, list):
|
||||
# A non-list here is a wiring error (the expression did not
|
||||
# resolve to a collection); silently fanning out over zero
|
||||
@@ -66,8 +93,13 @@ class FanOutStep(StepBase):
|
||||
f"Fan-out step {config.get('id', '?')!r} is missing "
|
||||
f"'step' field (nested step template)."
|
||||
)
|
||||
step = config.get("step")
|
||||
if step is not None and not isinstance(step, dict):
|
||||
elif not isinstance(config["step"], dict):
|
||||
# A present-but-non-mapping ``step`` (including an explicit
|
||||
# ``step: null``) is an authoring mistake. ``config.get("step", {})``
|
||||
# in ``execute`` only substitutes the ``{}`` default for an *absent*
|
||||
# key, so an explicit ``None`` reaches the runtime guard and FAILS
|
||||
# the step. Reject it here too so a workflow cannot pass validation
|
||||
# and then fail during execution.
|
||||
errors.append(
|
||||
f"Fan-out step {config.get('id', '?')!r}: 'step' must be a mapping."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user