Skip to content

Commit 838e7e4

Browse files
committed
fix(workflows): require literal boolean for runtime continue_on_error
Engine was checking `continue_on_error` via truthiness (`if step_config.get("continue_on_error"):`), which let truthy non-bool values like the string `"true"` activate the flag at runtime if validation was skipped. `execute()` does not auto-validate (documented on the `load_definition` docstring), so a caller bypassing `validate_workflow()` could see the contract silently violated. Tighten the runtime check to identity comparison (`is True`) so only a literal boolean enables the behaviour. Validation continues to catch the authoring mistake at parse time; this is defense in depth for the bypass-validation path. Add `test_engine_ignores_truthy_non_bool_continue_on_error` locking the contract — feeds `continue_on_error: "true"` (string) through `execute()` directly and asserts the run halts with `RunStatus.FAILED`. Addresses Copilot review feedback on github#2663.
1 parent c827f4f commit 838e7e4

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

src/specify_cli/workflows/engine.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,17 @@ def _execute_steps(
658658
# steps can branch on it. Log a single, unambiguous
659659
# event per failure resolution — either the run
660660
# continued past it, or it halted.
661-
if step_config.get("continue_on_error"):
661+
#
662+
# Use identity comparison (`is True`) rather than
663+
# truthiness so that only a literal boolean enables
664+
# the behaviour, even if validation was skipped.
665+
# Validation rejects non-bool values at parse time,
666+
# but `execute()` does not auto-validate (see the
667+
# `load_definition` docstring), so a caller passing
668+
# an unvalidated definition could otherwise see
669+
# truthy non-bool values like the string `"true"`
670+
# silently change run semantics.
671+
if step_config.get("continue_on_error") is True:
662672
state.append_log(
663673
{
664674
"event": "step_continue_on_error",

tests/test_workflows.py

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2410,6 +2410,44 @@ def test_validation_accepts_bool_continue_on_error(self):
24102410
errors = validate_workflow(definition)
24112411
assert errors == [], errors
24122412

2413+
def test_engine_ignores_truthy_non_bool_continue_on_error(self, project_dir):
2414+
"""Defense-in-depth: even if a caller bypasses
2415+
`validate_workflow()` and feeds the engine a definition with
2416+
`continue_on_error: "true"` (a string), the engine must NOT
2417+
honour the flag — only a literal boolean enables the
2418+
behaviour. `execute()` does not auto-validate (the
2419+
`load_definition` docstring documents this), so the engine
2420+
guards against truthy non-bool values itself via an identity
2421+
check rather than truthiness.
2422+
"""
2423+
from specify_cli.workflows.engine import WorkflowDefinition, WorkflowEngine
2424+
from specify_cli.workflows.base import RunStatus
2425+
2426+
# Bypass `validate_workflow()` — execute() is what would
2427+
# be called by a caller that skipped validation.
2428+
definition = WorkflowDefinition.from_string("""
2429+
schema_version: "1.0"
2430+
workflow:
2431+
id: "string-coe"
2432+
name: "String COE"
2433+
version: "1.0.0"
2434+
steps:
2435+
- id: fail-step
2436+
type: shell
2437+
run: "exit 1"
2438+
continue_on_error: "true"
2439+
- id: should-not-run
2440+
type: shell
2441+
run: "echo should-not-run"
2442+
""")
2443+
engine = WorkflowEngine(project_dir)
2444+
state = engine.execute(definition)
2445+
2446+
# String "true" is truthy but not a literal boolean, so the
2447+
# engine must treat the step as a halting failure.
2448+
assert state.status == RunStatus.FAILED
2449+
assert "should-not-run" not in state.step_results
2450+
24132451

24142452
# ===== State Persistence Tests =====
24152453

0 commit comments

Comments
 (0)