mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(workflows): strip a resolved condition before the true/false check (#3883)
evaluate_condition() special-cases the strings "false"/"true" so that
`condition: "false"` behaves as a boolean, but it matches with
`result.lower()` and never strips.
The most common way a *string* reaches a condition is captured command
output, and the shell step stores stdout verbatim
(steps/shell/__init__.py:67 `"stdout": proc.stdout`). So `run: echo false`
resolves to "false\n", which matches neither branch and falls through to
`bool("false\n")` -> True:
'false' -> False
'false\n' -> True <-- bug
'false\r\n' -> True <-- bug
' false' -> True <-- bug
An `if` step therefore takes its `then` branch on a step that printed
"false", and `while`/`do-while` keep dispatching their body.
A workflow author cannot work around it: the registered filters are
default/join/map/contains/from_json — there is no `trim`.
`InitStep._resolve_bool` and both catalog readers already strip before
matching boolean text. `bool(result)` still sees the raw string, so no
non-boolean text changes truthiness.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3451a21277
commit
1b3695bfb3
@@ -671,8 +671,20 @@ def evaluate_condition(condition: str, context: Any) -> bool:
|
||||
result = evaluate_expression(condition, context)
|
||||
# Treat plain "false"/"true" strings as booleans so that
|
||||
# condition: "false" (without {{ }}) behaves as expected.
|
||||
#
|
||||
# Strip before matching: the string a condition resolves to is most often
|
||||
# captured command output, and a ``shell`` step stores ``proc.stdout``
|
||||
# verbatim, so ``run: echo false`` resolves to ``"false\n"``. Without the
|
||||
# strip that trailing newline matches neither branch and falls through to
|
||||
# ``bool("false\n")`` -> True, silently taking an ``if`` step's ``then``
|
||||
# branch (and keeping a ``while``/``do-while`` looping) on a step that
|
||||
# printed "false". A workflow cannot strip it itself -- the registered
|
||||
# filters are default/join/map/contains/from_json, there is no ``trim``.
|
||||
# ``InitStep._resolve_bool`` and the catalog readers already strip before
|
||||
# matching boolean text. ``bool(result)`` below still sees the raw string,
|
||||
# so no non-boolean text changes truthiness.
|
||||
if isinstance(result, str):
|
||||
lower = result.lower()
|
||||
lower = result.strip().lower()
|
||||
if lower == "false":
|
||||
return False
|
||||
if lower == "true":
|
||||
|
||||
@@ -786,6 +786,39 @@ class TestExpressions:
|
||||
assert evaluate_condition("{{ inputs.ready }}", ctx) is True
|
||||
assert evaluate_condition("{{ inputs.missing }}", ctx) is False
|
||||
|
||||
def test_condition_strips_captured_command_output(self):
|
||||
"""A condition resolving to captured stdout must honour "false".
|
||||
|
||||
A ``shell`` step stores ``proc.stdout`` verbatim, so ``run: echo false``
|
||||
resolves to ``"false\\n"``. Without stripping, the trailing newline
|
||||
matched neither the "false" nor the "true" branch and fell through to
|
||||
``bool("false\\n")`` -> True, so an ``if`` step took its ``then`` branch
|
||||
on a step that printed "false". There is no ``trim`` filter, so a
|
||||
workflow author cannot strip it themselves.
|
||||
"""
|
||||
from specify_cli.workflows.expressions import evaluate_condition
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
ctx = StepContext(steps={"check": {"output": {"stdout": "false\n"}}})
|
||||
assert evaluate_condition("{{ steps.check.output.stdout }}", ctx) is False
|
||||
|
||||
for raw in ("false\n", "false\r\n", " false", "false ", "FALSE\n"):
|
||||
assert evaluate_condition(raw, StepContext()) is False, raw
|
||||
for raw in ("true\n", " true ", "TRUE\r\n"):
|
||||
assert evaluate_condition(raw, StepContext()) is True, raw
|
||||
|
||||
def test_condition_whitespace_only_string_stays_truthy(self):
|
||||
"""Stripping must not turn a whitespace-only string into False.
|
||||
|
||||
Only the "false"/"true" special case is stripped; everything else still
|
||||
falls through to ``bool(result)`` on the raw string.
|
||||
"""
|
||||
from specify_cli.workflows.expressions import evaluate_condition
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
assert evaluate_condition(" ", StepContext()) is True
|
||||
assert evaluate_condition("falsey", StepContext()) is True
|
||||
|
||||
def test_non_string_passthrough(self):
|
||||
from specify_cli.workflows.expressions import evaluate_expression
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
Reference in New Issue
Block a user