mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): fail while/do-while steps on non-list steps instead of crashing (#3519)
`WhileStep.validate()` and `DoWhileStep.validate()` already reject a non-list `steps` body, but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run the body is returned as `next_steps`, and the engine feeds it straight into `_execute_steps`, which iterates it as step mappings. A non-list `steps` — a single mapping or scalar authoring mistake — was iterated element-wise (a dict yields its string keys, a str its characters) and raised `AttributeError` on `.get()`, taking down the whole run; the engine invokes `step_impl.execute()` with no surrounding try/except. Guard both `execute` paths to return a FAILED StepResult naming the type error instead, mirroring the if/switch non-list-branch and fan-out non-list `items` handling. The do-while body always dispatches on the first call, so its guard is unconditional; the while body only dispatches when the condition is truthy, so its guard fires only then — a false condition leaves a non-list `steps` benign and the step completes, unchanged. The condition/expression is still evaluated first, so its result is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -27,6 +27,30 @@ class DoWhileStep(StepBase):
|
||||
nested_steps = config.get("steps", [])
|
||||
condition = config.get("condition", "false")
|
||||
|
||||
# The engine does not auto-validate step config (see
|
||||
# ``WorkflowEngine.load_workflow``) and feeds ``next_steps`` straight
|
||||
# into ``_execute_steps``, which iterates them as step mappings. A
|
||||
# non-list ``steps`` (a single mapping or scalar authoring mistake)
|
||||
# would otherwise be iterated element-wise — a dict yields its string
|
||||
# keys, a str its characters — and crash the whole run with
|
||||
# AttributeError on ``.get()``. ``validate`` already rejects a non-list
|
||||
# ``steps``; fail this step loudly on an unvalidated run instead,
|
||||
# mirroring the if/switch/fan-out steps. The body always runs on the
|
||||
# first call, so unlike the while step this guard is unconditional.
|
||||
if not isinstance(nested_steps, list):
|
||||
return StepResult(
|
||||
status=StepStatus.FAILED,
|
||||
output={
|
||||
"condition": condition,
|
||||
"max_iterations": max_iterations,
|
||||
"loop_type": "do-while",
|
||||
},
|
||||
error=(
|
||||
f"Do-while step {config.get('id', '?')!r}: 'steps' must be "
|
||||
f"a list of steps, got {type(nested_steps).__name__}."
|
||||
),
|
||||
)
|
||||
|
||||
# Always execute body at least once; the engine layer evaluates
|
||||
# `condition` after each iteration to decide whether to loop.
|
||||
return StepResult(
|
||||
|
||||
@@ -26,6 +26,32 @@ class WhileStep(StepBase):
|
||||
nested_steps = config.get("steps", [])
|
||||
|
||||
result = evaluate_condition(condition, context)
|
||||
|
||||
# The engine does not auto-validate step config (see
|
||||
# ``WorkflowEngine.load_workflow``) and feeds ``next_steps`` straight
|
||||
# into ``_execute_steps``, which iterates them as step mappings. A
|
||||
# non-list ``steps`` (a single mapping or scalar authoring mistake)
|
||||
# would otherwise be iterated element-wise — a dict yields its string
|
||||
# keys, a str its characters — and crash the whole run with
|
||||
# AttributeError on ``.get()``. ``validate`` already rejects a non-list
|
||||
# ``steps``; fail this step loudly on an unvalidated run instead,
|
||||
# mirroring the if/switch/fan-out steps. The guard fires only when the
|
||||
# body would actually be dispatched (condition truthy). The condition is
|
||||
# still evaluated first, so its result is surfaced for downstream context.
|
||||
if result and not isinstance(nested_steps, list):
|
||||
return StepResult(
|
||||
status=StepStatus.FAILED,
|
||||
output={
|
||||
"condition_result": True,
|
||||
"max_iterations": max_iterations,
|
||||
"loop_type": "while",
|
||||
},
|
||||
error=(
|
||||
f"While step {config.get('id', '?')!r}: 'steps' must be a "
|
||||
f"list of steps, got {type(nested_steps).__name__}."
|
||||
),
|
||||
)
|
||||
|
||||
if result:
|
||||
return StepResult(
|
||||
status=StepStatus.COMPLETED,
|
||||
|
||||
Reference in New Issue
Block a user