Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/reference/workflows.md
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,19 @@ pauses: the named stored input is reset to `""`. A later resume therefore
prompts or pauses again until another verdict is supplied. Approve, abort, and
skip outcomes leave the input unchanged.

Because of that reset, a verdict input used with `on_reject: retry` must accept
`""`. If it declares an `enum`, include the empty string — otherwise the reset
value violates the input's own `enum` and the run can no longer be resumed with
any input. `specify workflow add` reports this as a validation error.

```yaml
inputs:
spec_verdict:
type: string
enum: ["", approve, reject]
default: ""
```

## FAQ

### What happens when a workflow hits a gate step?
Expand Down
65 changes: 47 additions & 18 deletions src/specify_cli/workflows/engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,16 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]:
errors.append("Workflow has no steps defined.")

seen_ids: set[str] = set()
# ``input_names`` is the set of declared workflow input names — used by
# ``_validate_steps`` to cross-reference gate ``verdict_input`` bindings.
# ``None`` means the inputs block itself is malformed (already reported
# above); the cross-check is then disabled so one authoring mistake does
# not cascade into N spurious "undeclared" errors.
input_names: set[str] | None = (
set(definition.inputs) if isinstance(definition.inputs, dict) else None
# ``input_defs`` maps declared workflow input names to their definitions —
# used by ``_validate_steps`` to cross-reference gate ``verdict_input``
# bindings (both that the name exists and that its ``enum`` permits the
# reset sentinel). ``None`` means the inputs block itself is malformed
# (already reported above); the cross-check is then disabled so one
# authoring mistake does not cascade into N spurious "undeclared" errors.
input_defs: dict[str, Any] | None = (
dict(definition.inputs) if isinstance(definition.inputs, dict) else None
)
_validate_steps(definition.steps, seen_ids, errors, input_names)
_validate_steps(definition.steps, seen_ids, errors, input_defs)

return errors

Expand All @@ -325,15 +326,15 @@ def _validate_steps(
steps: list[dict[str, Any]],
seen_ids: set[str],
errors: list[str],
input_names: set[str] | None = None,
input_defs: dict[str, Any] | None = None,
inside_fan_out: bool = False,
) -> None:
"""Recursively validate a list of steps.

``input_names`` is the set of declared workflow input names (or ``None``
when the inputs block is malformed). ``inside_fan_out`` is threaded
through nested control-flow steps so gate verdict bindings can be rejected
anywhere inside a fan-out template.
``input_defs`` maps declared workflow input names to their definitions (or
is ``None`` when the inputs block is malformed). ``inside_fan_out`` is
threaded through nested control-flow steps so gate verdict bindings can be
rejected anywhere inside a fan-out template.
"""
from . import STEP_REGISTRY

Expand Down Expand Up @@ -440,11 +441,39 @@ def _validate_steps(
f"Gate step {step_id!r}: 'verdict_input' is not "
"supported inside fan-out templates."
)
elif input_names is not None and verdict_input not in input_names:
elif input_defs is not None and verdict_input not in input_defs:
errors.append(
f"Gate step {step_id!r}: 'verdict_input' references "
f"undeclared input {verdict_input!r}."
)
elif input_defs is not None:
# ``on_reject: retry`` resets the bound input to "" before
# pausing, and every later resume re-resolves the persisted
# inputs through ``_coerce_input``. If the input declares an
# ``enum`` that omits "", that reset value is instantly
# illegal: the run pauses fine, but the next resume that
# supplies any input raises "value '' not in allowed
# values", and no verdict can be routed through the gate
# again. Require the enum to admit the sentinel so the
# retry cycle the field advertises is actually reachable.
verdict_def = input_defs.get(verdict_input)
enum_values = (
verdict_def.get("enum")
if isinstance(verdict_def, dict)
else None
)
if (
step_config.get("on_reject") == "retry"
and isinstance(enum_values, list)
and "" not in enum_values
):
errors.append(
f"Gate step {step_id!r}: on_reject='retry' resets "
f"verdict input {verdict_input!r} to '' when the "
f"gate is rejected, but that input's 'enum' does "
f"not allow ''. Add '' to the enum or use "
f"on_reject='abort'/'skip'."
)

# Recursively validate nested steps
for nested_key in ("then", "else", "steps"):
Expand All @@ -454,7 +483,7 @@ def _validate_steps(
nested,
seen_ids,
errors,
input_names,
input_defs,
inside_fan_out=inside_fan_out,
)

Expand All @@ -467,7 +496,7 @@ def _validate_steps(
case_steps,
seen_ids,
errors,
input_names,
input_defs,
inside_fan_out=inside_fan_out,
)

Expand All @@ -478,7 +507,7 @@ def _validate_steps(
default,
seen_ids,
errors,
input_names,
input_defs,
inside_fan_out=inside_fan_out,
)

Expand All @@ -491,7 +520,7 @@ def _validate_steps(
[fan_step],
set(),
fan_errors,
input_names,
input_defs,
inside_fan_out=True,
)
errors.extend(fan_errors)
Expand Down
143 changes: 143 additions & 0 deletions tests/test_workflows.py
Original file line number Diff line number Diff line change
Expand Up @@ -4673,6 +4673,149 @@ def test_malformed_verdict_input_no_duplicate_error(self):
# No undeclared-input error (123 is not a string, so cross-check skips)
assert not any("undeclared input" in e for e in errors)

def test_retry_verdict_enum_must_allow_reset_sentinel(self):
# on_reject: retry resets the bound input to "" before pausing, and
# every resume re-resolves persisted inputs through _coerce_input. An
# enum that omits "" makes that reset value instantly illegal, so the
# next resume supplying any input dies with "value '' not in allowed
# values" and no verdict can reach the gate again.
errors = self._errors("""
workflow:
id: wf
name: wf
version: "1.0.0"
inputs:
spec_verdict:
type: string
enum: [approve, reject]
steps:
- id: review
type: gate
message: "Review?"
options: [approve, reject]
on_reject: retry
verdict_input: spec_verdict
""")
assert any(
"on_reject='retry' resets verdict input 'spec_verdict'" in e
for e in errors
), errors

def test_retry_verdict_enum_including_sentinel_passes(self):
errors = self._errors("""
workflow:
id: wf
name: wf
version: "1.0.0"
inputs:
spec_verdict:
type: string
enum: ["", approve, reject]
default: ""
steps:
- id: review
type: gate
message: "Review?"
options: [approve, reject]
on_reject: retry
verdict_input: spec_verdict
""")
assert not any("on_reject='retry'" in e for e in errors), errors

def test_verdict_enum_without_sentinel_passes_when_not_retry(self):
# abort/skip never reset the input, so the enum need not admit "".
for on_reject in ("abort", "skip"):
errors = self._errors(f"""
workflow:
id: wf
name: wf
version: "1.0.0"
inputs:
spec_verdict:
type: string
enum: [approve, reject]
steps:
- id: review
type: gate
message: "Review?"
options: [approve, reject]
on_reject: {on_reject}
verdict_input: spec_verdict
""")
assert not any("on_reject='retry'" in e for e in errors), (
on_reject,
errors,
)

def test_retry_verdict_without_enum_passes(self):
# No enum means _coerce_input accepts "" — the documented shape.
errors = self._errors("""
workflow:
id: wf
name: wf
version: "1.0.0"
inputs:
spec_verdict:
type: string
default: ""
steps:
- id: review
type: gate
message: "Review?"
options: [approve, reject]
on_reject: retry
verdict_input: spec_verdict
""")
assert not any("on_reject='retry'" in e for e in errors), errors

def test_retry_verdict_enum_wedge_is_reachable_end_to_end(self, tmp_path):
"""The validation error above guards a real, unrecoverable run state.

Without the guard this workflow installs and runs fine, then wedges:
the retry reset writes "" into the persisted inputs, and the next
resume that supplies *any* input re-resolves them and dies on the
enum. Only a resume with no inputs at all still works, so the bound
verdict can never be delivered.
"""
import pytest
import yaml as _yaml

from specify_cli.workflows.engine import WorkflowEngine

definition_data = {
"schema_version": "1.0",
"workflow": {"id": "wf", "name": "WF", "version": "1.0.0"},
"inputs": {
"spec_verdict": {"type": "string", "enum": ["approve", "reject"]},
"note": {"type": "string", "default": "a"},
},
"steps": [
{
"id": "review",
"type": "gate",
"message": "Review?",
"options": ["approve", "reject"],
"on_reject": "retry",
"verdict_input": "spec_verdict",
}
],
}
wf_dir = tmp_path / ".specify" / "workflows" / "wf"
wf_dir.mkdir(parents=True)
(wf_dir / "workflow.yml").write_text(
_yaml.safe_dump(definition_data), encoding="utf-8"
)

engine = WorkflowEngine(tmp_path)
definition = engine.load_workflow("wf")
state = engine.execute(definition, inputs={"spec_verdict": "reject"})
assert state.status.value == "paused"
# The retry reset persisted a value the input's own enum forbids.
assert state.inputs["spec_verdict"] == ""

with pytest.raises(ValueError, match="not in allowed values"):
engine.resume(state.run_id, inputs={"note": "b"})

def test_verdict_input_in_switch_case(self):
# Recursion coverage: bad reference inside a switch case must surface.
errors = self._errors("""
Expand Down