mirror of
https://github.com/github/spec-kit.git
synced 2026-07-07 06:35:06 +08:00
fix(workflows): compare non-numeric strings lexicographically instead of returning False (#3323)
_safe_compare coerced both operands to int/float unconditionally for <, >, <=,
>=. any non-numeric string (an iso date, a version tag, a name) failed that
coercion and the whole comparison silently returned False -- so
`{{ inputs.d < '2026-02-01' }}` was False even when the date was earlier.
only coerce when both operands look numeric; otherwise compare the original
values, so two strings order lexicographically the way python does and two
numeric strings still compare as numbers ("10" > "9"). a number vs a
non-numeric string stays incomparable and yields False.
add a regression test covering dates, plain strings, numeric strings, and the
number-vs-string case.
This commit is contained in:
@@ -467,15 +467,34 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
|
||||
return _resolve_dot_path(namespace, expr)
|
||||
|
||||
|
||||
def _coerce_number(value: Any) -> Any:
|
||||
"""Return *value* as int/float if it is a numeric string, else unchanged."""
|
||||
if isinstance(value, str):
|
||||
try:
|
||||
return float(value) if "." in value else int(value)
|
||||
except ValueError:
|
||||
return value
|
||||
return value
|
||||
|
||||
|
||||
def _safe_compare(left: Any, right: Any, op: str) -> bool:
|
||||
"""Safely compare two values, coercing types when possible."""
|
||||
try:
|
||||
if isinstance(left, str):
|
||||
left = float(left) if "." in left else int(left)
|
||||
if isinstance(right, str):
|
||||
right = float(right) if "." in right else int(right)
|
||||
except (ValueError, TypeError):
|
||||
return False
|
||||
"""Compare two values for ordering, coercing numeric strings when possible.
|
||||
|
||||
Numeric coercion is applied only when *both* operands look numeric, so a
|
||||
pair like ``"10"`` and ``"9"`` compares as numbers (10 > 9). When either
|
||||
side is a non-numeric string, both fall back to their original values and
|
||||
are compared directly -- so ordinary strings (dates, semver-ish tags,
|
||||
names) compare lexicographically the way Python does, instead of every
|
||||
such comparison silently returning ``False`` after a failed int()/float()
|
||||
coercion. A genuinely incomparable pair (e.g. number vs non-numeric string)
|
||||
raises ``TypeError`` and yields ``False``.
|
||||
"""
|
||||
cl, cr = _coerce_number(left), _coerce_number(right)
|
||||
# Only use the coerced numbers when both converted; otherwise a numeric
|
||||
# string paired with a plain string would become an int-vs-str mismatch
|
||||
# (always False) rather than a lexicographic string comparison.
|
||||
if isinstance(cl, (int, float)) and isinstance(cr, (int, float)):
|
||||
left, right = cl, cr
|
||||
try:
|
||||
if op == ">":
|
||||
return left > right # type: ignore[operator]
|
||||
|
||||
@@ -342,6 +342,35 @@ class TestExpressions:
|
||||
assert evaluate_expression("{{ steps.plan.output.task_count > 5 }}", ctx) is True
|
||||
assert evaluate_expression("{{ steps.plan.output.task_count < 5 }}", ctx) is False
|
||||
|
||||
def test_ordering_comparison_of_non_numeric_strings(self):
|
||||
"""`<`/`>`/`<=`/`>=` between non-numeric strings must compare
|
||||
lexicographically, not silently return False.
|
||||
|
||||
`_safe_compare` used to coerce both operands to int/float unconditionally;
|
||||
a non-numeric string (date, version tag, name) failed that coercion and
|
||||
the whole comparison returned False. Ordinary strings should order the
|
||||
way Python does; numeric strings must still compare as numbers."""
|
||||
from specify_cli.workflows.expressions import evaluate_expression
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
# ISO dates compare lexicographically (correct chronological order).
|
||||
ctx = StepContext(inputs={"d": "2026-01-01"})
|
||||
assert evaluate_expression("{{ inputs.d < '2026-02-01' }}", ctx) is True
|
||||
assert evaluate_expression("{{ inputs.d > '2026-02-01' }}", ctx) is False
|
||||
|
||||
# Plain string ordering.
|
||||
ctx = StepContext(inputs={"name": "beta"})
|
||||
assert evaluate_expression("{{ inputs.name > 'alpha' }}", ctx) is True
|
||||
|
||||
# Two numeric strings still compare numerically, not lexically
|
||||
# ("10" > "9" is True as numbers; as strings it would be False).
|
||||
ctx = StepContext(inputs={"v": "10"})
|
||||
assert evaluate_expression("{{ inputs.v > '9' }}", ctx) is True
|
||||
|
||||
# A number vs a non-numeric string is genuinely incomparable -> False.
|
||||
ctx = StepContext(inputs={"n": 5})
|
||||
assert evaluate_expression("{{ inputs.n > 'abc' }}", ctx) is False
|
||||
|
||||
def test_boolean_and(self):
|
||||
from specify_cli.workflows.expressions import evaluate_expression
|
||||
from specify_cli.workflows.base import StepContext
|
||||
|
||||
Reference in New Issue
Block a user