fix: #2304 compare numeric strings by magnitude in relational operators

[bongbongcrypto]

Issue

Closes #2304

What this changes

applyBinary compared relational operands with (left as number) < (right as number), a
compile time assertion that converts nothing, so two strings took code-unit ordering.
Template resolution returns values as strings, so that is the usual pairing.

toRelationalOperand now coerces the operands of <, <=, > and >= only:

• /^\d+$/ becomes BigInt, so magnitude never limits exactness
• /^\d+\.\d+$/ becomes Number, the closest available representation, with the usual
  double precision limit
• anything else is returned untouched

Following your answer on the decimal question, decimals are in scope. A single point and
digits on both sides means semver is not a decimal, and digit-only means dates, addresses
and hashes are not numeric here either.

This also settles the BigInt-mode decimal case you described, where
applyBigIntConversion leaves a decimal string alone and StringToBigInt made < and
>= on the same pair both false. The coercion happens at the comparison site, so the
decimal becomes a Number and the comparison against a BigInt operand is well defined.

Two things deliberately not changed, per your correction to my scope:

• Equality operators. String equality is meaningful for addresses, symbols and hashes.
• + at the same site, which concatenates. "9" + "10" is still "910". It sits on the
  same cast, so it should not be described as checked-and-correct, but the visual builder
  cannot emit it and concatenation is plausibly the intent when someone writes it by hand.

Scope

One change. The coercion and its tests cannot ship apart: the tests assert the behaviour
the coercion introduces, and the coercion without them leaves the boundary unguarded.

No response shape, unit, status code or default moves. What changes is the verdict on a
digit string against another numeric value, which is the reported defect.

How it was verified

Added to tests/unit/condition-safe-eval.test.ts, matching the matrix you asked for:

case	before	after
"9" < "10"	false	true
"99" < "100"	false	true
"999999999999999" < "1000000000000000"	false	true
"9007199254740991" < "9007199254740992"	true	true
"9007199254740991" > "9007199254740992"	false	false
"9.5" < "10.2"	false	true
"9.5" >= "10.2"	true	false
10n < "10.5"	false	true
10n >= "10.5"	false	false
"1000000000000000000" < "1000000000000000001"	true	true
address === address	true	true
"apple" < "banana"	true	true
"1.2.3" < "1.10.0"	false	false
"2026-09-05" < "2026-10-01"	true	true

The MAX_SAFE_INTEGER pair is included as the actual edge rather than the digit count, and
the decimal pair appears in both regimes. Each of the first nine fails without the fix.

Honest note on the checklist below. I have not run pnpm check or pnpm type-check
locally, because I do not install and run an unfamiliar repository on the machine I work
from. What I did instead: ran the coercion predicate against the table above in a
standalone harness, and type-checked the two changed files in isolation for syntax. CI on
this PR is the real check, and I will fix whatever it reports.

Screenshots

Nothing renders.

---

• Targets staging
• Title carries the issue number, or an exemption applies
• pnpm check and pnpm type-check pass (not run locally, see the note above)
• No secrets, .env files, or credentials committed

🤖 Generated with Claude Code

[github-actions]

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

[suisuss]

What this changes

lib/workflow/nodes/condition/safe-eval.ts gains INTEGER_OPERAND_RE = /^\d+$/ at :552 and DECIMAL_OPERAND_RE = /^\d+\.\d+$/ at :558, plus toRelationalOperand at :572: a digit-only string becomes BigInt, a d+.d+ string becomes Number, everything else passes through. The four relational arms at :604, :607, :610 and :613 run both operands through it. Equality and arithmetic arms are untouched, as is the parser, the allowlist and resolveMissingOperands.

Does it match the description

Matches on scope - equality and + are correctly left alone, and the decimal case is in as agreed.

The doc comment at :568-570 is wrong, though, and it is load-bearing for the rest of the review. It says "Only digit strings move. Everything else - dates, semver, addresses, hashes, prose - keeps the behaviour it has today." That is not what happens. Once one operand is a BigInt, the other is coerced by StringToBigInt, which accepts signs, surrounding whitespace, and hex, octal and binary literals. So the untouched operand moves too - inside the engine rather than in toRelationalOperand. That single mechanism produces three of the four blockers below.

Blocking

• lib/workflow/nodes/condition/safe-eval.ts:580 - the decimal path reintroduces the precision loss the change exists to remove. "1000000000000000000.5" > "1000000000000000000": the left matches DECIMAL_OPERAND_RE and Number() rounds it to exactly 1e18, the right becomes 1000000000000000000n, and the result is false. On staging this was a string comparison and returned true, correctly. -> A formatted balance with a fractional part is exactly the wei-scale case this PR is for, and it gets the wrong answer where the old code got the right one. -> Compare integer and fraction parts as BigInts, or refuse to convert a decimal whose integer part exceeds MAX_SAFE_INTEGER instead of silently rounding it.

• safe-eval.ts:577 - a digit string against a hex string silently reverses. With "1" on the left and an address like "0x0000000000000000000000000000000000000002" on the right, the left becomes 1n, the engine parses the right as hex to 2n, and < is true. On staging "1" < "0x00..." was false. -> A condition comparing a count against an address-shaped or hash-shaped field flips, silently, with no error. Same for "10" < "0x1f". -> Convert only when both operands are recognised numerics, and compare explicitly rather than letting the engine coerce the second one.

• safe-eval.ts:577 - a digit string against a non-numeric string is now false in both directions. 9n < "apple" is false and 9n > "apple" is also false; on staging "9" < "apple" was true. "1e3" behaves the same way, since StringToBigInt rejects exponent notation. -> Worse than a wrong answer: a < b, a > b and a === b are all false at once, so an author cannot write an exhaustive set of branches. This is the trap you correctly identified for decimals in BigInt mode, newly created for the mixed case. -> Same fix as above.

• safe-eval.ts:552 - ^\d+$ does not admit a sign, and the resulting gap is asymmetric. "-5" < "-3" converts neither operand, falls back to string comparison and returns false, where the answer is true. But "-5" < "3" is correct, because the right side becomes 3n and the engine then parses "-5" properly. -> A negative operand's correctness depends on whether its counterpart happens to be a bare digit string. -> Use /^[+-]?\d+$/ and the signed decimal form. NUMERIC_LITERAL_RE at lib/workflow/nodes/condition/expression.ts:20 already has this shape and the builder uses it.

• The lint job fails as pushed. .github/workflows/pr-checks.yml:98-110 runs pnpm check unconditionally, and biome.jsonc excludes neither lib/workflow/nodes/condition/ nor tests/. The return a >\n b; form must be parenthesised, and several added expect(...).toBe(true); lines exceed the 80-column width. pnpm exec biome check --write on the two changed files fixes both.

Mechanical - actionable as-is

• Three of the five new cases assert behaviour that already holds on staging through the production path, so they do not isolate the regression. tests/unit/condition-safe-eval.test.ts:211 and the max/past pair at :194 both use operands that trip needsBigIntMode (lib/bigint-condition-utils.ts:74-100), so applyBigIntConversion already converts them at lib/workflow/executor/executor.workflow.ts:521-527; the tests call safeEvaluateCondition directly and bypass that. :235 asserts "9" < 10, also already true. The "999999999999999" / "1000000000000000" assertion at :205-210 is the one that is genuinely new coverage, and it is the right one.

• Changed branches with no test: digit against a non-numeric string, digit against a hex or address string, negative operands anywhere, leading or trailing whitespace, empty string, null/undefined, a boolean against a converted side, a decimal above MAX_SAFE_INTEGER, and the decimal path through > at :604 and <= at :613 - only < and >= are asserted at :220-233.

• Correct the comment at :568-570 once the coercion is made explicit, since the claim will still be false while the engine is doing the second conversion.

With the team

• Whether the engine and the exported SDK are allowed to disagree on relational semantics. lib/workflow/codegen/codegen.ts:917 and :1046 emit the user's condition verbatim into exported JS, which runs plain relational rules - so with this change the code we show a user no longer computes what the engine computes for these operators. I am weighing emitting a comparison helper into generated code against accepting the divergence and documenting it; the first keeps one meaning for a condition, the second is much less code. This does not block any of the fixes above - the same divergence exists today for applyBigIntConversion. I'm taking it to the core team and will come back.

Verdict

Changes requested - the decimal path loses precision at wei scale, and three operand shapes change answers because the engine coerces the operand toRelationalOperand leaves alone.

The diagnosis is right and the fix is in the right place. (left as number) < (right as number) really was a compile-time assertion converting nothing, and doing the coercion at the comparison site really does settle the BigInt-mode decimal case. All four blockers come from one decision - relying on JS's implicit BigInt-versus-string coercion for the second operand - and converting only when both sides match a numeric shape, then comparing explicitly, fixes them together.

[bongbongcrypto]

Thank you - the diagnosis in the review is right, and it is one decision rather than four. toRelationalOperand converted one operand and left the other to the engine, and StringToBigInt then converted the one I thought I was leaving alone. Every blocker follows from that.

Pushed 7b9300c. Neither operand moves unless both are decimals, and the comparison is made in compareRelational rather than by the operator. A pair that is not recognised is handed back and the arm evaluates exactly as it does on staging.

• Decimal precision. No Number anywhere on the path. Fractions are padded to a common width and compared as BigInt, so "1000000000000000000.5" > "1000000000000000000" is true, and "0.30000000000000004" > "0.3" is too.
• Hex and address shapes. Not decimals, so the pair is not recognised: "1" < "0x00...02" is false and "10" < "0x1f" is false, as on staging.
• Non-numeric strings. Also not a pair, so "9" < "apple" is true, "9" > "apple" is false, and an exhaustive set of branches is writable again. "1e3" behaves the same way.
• Signs. The grammar is NUMERIC_LITERAL_RE from expression.ts, as suggested, so "-5" < "-3" is true and no longer depends on what the other operand looks like.
• Lint. biome check is clean on both files with the repo config and ultracite 6.5.1 resolved. It fails on the previous revision, so the check is real.

A number operand is only converted when it is a safe integer, which is the range whose decimal form is exact. Outside that it is left alone, because JavaScript already orders a Number against a BigInt exactly - 10n < 10.5 is true either way.

On the tests: dropped the three that already held on staging through applyBigIntConversion and kept the 15-to-16 digit assertion. Added coverage for what this actually changes - signs on either side, decimals past the double precision limit, all four relational operators, hex, exponent notation, whitespace, the empty string, words, null, booleans, and a non-integral Number.

I ran the suite against three trees to check the tests bite rather than just pass: staging fails three, the previous revision fails four (the blockers above), this revision passes 41.

On the SDK divergence: understood, and I have not touched codegen.ts. If the team prefers a helper emitted into generated code I am happy to write it in a follow-up against whatever shape you settle on.

[suisuss]

What this changes

One commit. INTEGER_OPERAND_RE and DECIMAL_OPERAND_RE collapse into NUMERIC_OPERAND_RE = /^[+-]?\d+(\.\d+)?$/ (lib/workflow/nodes/condition/safe-eval.ts:559), and the four relational arms route through a new compareRelational that pads both fractions to a common width and compares scaled BigInts, with no Number() anywhere on the path. An operand pair where either side fails to parse falls back to the exact staging expression.

All five blocking items land, and I ran them differentially against staging and the previous head rather than reading them: the wei-scale decimal case reverses correctly at head and not at b610489; "1" < "0x00…02" and "9" < "apple" are back to their staging answers; "-5" < "-3" is true at head and false on both earlier trees. The three already-passing tests are gone and 3 of 8 relational cases now fail on staging, which is the point. biome format and biome lint are clean on both files under the repo's own config.

Does it match the description

Matches.

Blocking

• lib/workflow/nodes/condition/safe-eval.ts:630 against the untouched equality arms at :650 and :654 - ordering is now numeric while equality is still textual, so the trichotomy hole has moved rather than closed. For two operands that are numerically equal and textually different, <, >, === and == are now all false at once. Verified: "1" vs "1.0", "1000000000000000000" vs "1000000000000000000.0", "007" vs "7", "0.5" vs "0.50", "-0" vs "0", "+1" vs "1" - every one returns [false,false,false,false] at head where staging returned true on <. -> A Condition branching balance < threshold / balance > threshold / balance === threshold, where the template resolves to a formatter's "1000000000000000000.0" and the author typed the integer, takes no branch at all. That is a formatted-balance against an integer literal, which is the pair this change exists to serve, and it is the same "an author cannot write an exhaustive set of branches" defect the first round named. -> Route ===, ==, !== and != through compareRelational as well, so the three predicates agree on one notion of equality.

Mechanical - actionable as-is

• lib/workflow/nodes/condition/safe-eval.ts:604 - no length guard on the operand before BigInt(...). Measured on a single comparison: 100k digits 0.17 ms on staging against 49.5 ms at head, 1M digits 2.71 ms against 269.6 ms, and a 2M-digit operand runs 665 ms with no cap and no throw. The padEnd allocation is sized by the other operand's fraction. I did not trace whether any upstream node output is capped before template resolution, so mark the reachability UNSURE - but refusing to convert past a few hundred digits and falling through to the staging path costs nothing.

• lib/workflow/nodes/condition/safe-eval.ts:593 - the comment says a non-safe-integer number is "left to the relational operator, where JavaScript already orders numbers and BigInts exactly". True for number-against-number and number-against-bigint; false when the other operand is a string, which the fallback puts through ToNumber - "9007199254740993" > 9007199254740992 is false. Not a regression, and needsBigIntMode (lib/bigint-condition-utils.ts:76-88) promotes both sides before safeEvaluateCondition is reached, so it is unreachable through the executor. The comment is what needs correcting. Same misattribution on tests/unit/condition-safe-eval.test.ts:263-264, which attaches that rationale to a string-against-number case.

• tests/unit/condition-safe-eval.test.ts - the wei-scale digit-string assertion was deleted this round and nothing replaced it. :203 is a 15-to-16-digit pair, well inside MAX_SAFE_INTEGER, so no test now pins 1e18 against 1e18 + 1 as digit strings - the case the issue is about. Also uncovered: leading zeros (which is the blocking case above), two-component version strings like "1.10" against "1.9" (these do match the grammar and are compared as decimals; the note at :274 covers only three-part semver), "Infinity" and "NaN" as strings, and undefined.

• lib/workflow/nodes/condition/safe-eval.ts:590 - every bigint operand is round-tripped through toString() and BigInt() on each compare, which in BigInt mode replaces a native compare with two string parses. Cheap to special-case.

With the team

• Whether the exported SDK should share the engine's comparison. lib/workflow/codegen/sdk.ts:913 and :1043 still emit the condition verbatim, so exported code runs plain relational rules. This was already deferred, but the deferral was priced against a narrower gap: before this PR the two agreed on plain numeric strings and diverged only where applyBigIntConversion fired; at head "9" < "10" is true in the engine and false in the code we hand the user. I'm weighing emitting a comparison shim into the generated code against documenting the divergence - the tradeoff is generated-code weight versus a silent disagreement between what runs and what we show. I'm taking it back to the core team with the wider number. Nothing here is blocked on you.

Verdict

Changes requested - ordering became numeric while equality stayed textual, so a formatted decimal against an integer literal now satisfies none of the three comparisons.

[bongbongcrypto]

All of it is in, and the blocking one took me further than the instruction, so that part is first.

Blocking: one notion of equality

===, !==, == and != now ask compareRelational the same question the four ordering operators ask. A pair it does not recognise keeps the equality it has always had.

That alone did not close the hole. Doing only what you asked left "1.5" === 1.5 answering false with < and > false beside it, because asDecimalOperand admitted a number only when it was a safe integer. That pair is not exotic: wrapOperand in lib/workflow/nodes/condition/expression.ts:49 emits a value the author typed as a bare number whenever it matches NUMERIC_LITERAL_RE, and template resolution hands the other side over as a string. So a number operand is now read through its own decimal form, toString being the shortest decimal that reads back as the same double, and the grammar decides whether it counts.

Run through the real generator and the real executor, an "equals" rule against a typed number:

author types	expression the builder emits	at 7b9300c7	now
100	{{...}} === 100	false	true
1.5	{{...}} === 1.5	false	true
0.5	{{...}} === 0.5	false	true
1000000000000000000	{{...}} === 1000000000000000000	false	true

Reading a number that way also settles "9007199254740993" > 9007199254740992, which is now true rather than a comment about ToNumber.

What it cost, which you should look at

Three assertions changed that were written on purpose, and the operator dropdown moved with them.

• tests/unit/condition-safe-eval.test.ts asserted "0" === 0 is false, titled "distinguishes loose == from strict === across types". Numbers are no longer what separates the two operators. What still does is every pair that is not two numbers, and the test now pins that instead: "" == 0 against "" === 0, a boolean against 1, null against undefined.
• tests/unit/condition-executor.test.ts asserted the same thing through evaluateConditionExpression.
• tests/unit/condition-executor.test.ts:203 asserted that a BigInt value does not match the same number quoted, with a comment telling authors to unquote the literal. That case is the defect: BigInt mode promotes the left side and leaves the quoted right side a string, so <, > and === were all false, and the workaround in the comment was the only way out. It now answers true, and I added the two cases that still answer false, a different number and a non-numeric string.
• OPERATOR_METADATA described === as "Strict equality, no type coercion (a === b)". That is no longer true of a numeric pair, and a dropdown hint that lies is worse than a wrong one, so it now reads "Same value; numbers by magnitude". lib/workflow/nodes/condition/builder-utils.ts:31 is the only place either operator is described.

If you would rather keep strict === across types and accept that "1" === 1 stays false while < and > are false too, say so and I will restrict the routing to same-typed operands. It closes every pair you listed and leaves the builder's own shape open, which is why I did not choose it.

Mechanical

• Cap, safe-eval.ts:593. An operand past 256 digits is refused and the pair falls back. A uint256 is 78 digits and the same value formatted with 18 decimals is 97 characters, so nothing a read produces comes near it. Measured here on one comparison of two 1,000,000-digit operands: 2,644 ms uncapped, 0.01 ms capped. I did not trace reachability either, so it is a bound on the cost rather than a fix for a known path. Past the cap the comparison is still a total order, which the test asserts, so refusing does not reopen the branch problem.
• BigInt round trip, safe-eval.ts:677. Two BigInts are compared as they are. That is the shape applyBigIntConversion produces, so it is the common case in BigInt mode.
• The comment at the old :593. Rewritten, and the case it described is fixed rather than documented. Same for the misattribution in the test, which had "JavaScript already orders a Number against a BigInt exactly" sitting on a string-against-number assertion.
• Tests. The 1e18 against 1e18 + 1 digit-string pair is back, as its own case with <, >, >= and ===. Added: exactly one of <, > and === answers true, over nine pairs equal by magnitude and different as text, including all six you listed, and five pairs that are not equal. Added leading zeros, "1.10" against "1.9" (two components do match the grammar, three do not), "Infinity" and "NaN" as strings against the number NaN, undefined on either side, and the cap boundary.

Evidence

Same test file against four trees, the runner being a minimal describe/it/expect so the real source file is what runs:

tree	result
staging	38 passed, 7 failed
b610489 first revision	37 passed, 8 failed
7b9300c7 the revision you read	41 passed, 4 failed
this one	45 passed, 0 failed

At 7b9300c7 the trichotomy case fails with 1,false,false,false, which is the [false,false,false,false] you measured.

Beyond that file: the other ten condition test files pass (417 tests across the eleven). A full tests/unit run on a 4 GB box gave 21,971 passed and 18 failed; the 18 are auth, captcha, DB pool and route-handler cases plus one <50ms timing assertion, and re-running those files with and without this commit under the same conditions moves failures in both directions, so they are environment. biome check is clean on the four changed files. I could not run tsgo on that box, it needs more memory than the box has, so the typecheck job is one I have not seen pass; it is sitting in action_required with the rest of the workflow, as it did for the two earlier pushes.

On the SDK divergence

The gap you are taking to the core team is wider now than when you measured it. It was < and >; === is in it too, so exported code computes "0" === 0 as false where the engine says true. Nothing here depends on how that lands, but the number should be the current one.