KeeperHub / KeeperHub/keeperhub

Workflow Condition node silently string-compares numeric operands below MAX_SAFE_INTEGER

Closed
#2,304 1 comment 0 reactions 0 assignees View on GitHub
accepted confirmed
Dominant language
TypeScript
Stars
24
Forks
93
Avg merge
1d 8h
Merged PRs (30d)
266

Description

### Before filing

- [x] I searched open and closed issues for this behaviour.
- [x] I confirmed it still happens on the current `staging`, not only on an older checkout.
- [x] This is one problem, not several. (Several means several issues.)
- [x] This is not a security vulnerability (those go through private reporting).

This is the same fault class as #1930, on the surface that #1930 did not cover. That issue fixed the
numeric comparison in check-and-execute. The workflow Condition node still string-compares.

### Reason: reproduction

A workflow with a Manual trigger and a single Condition node. No web3 nodes are needed to reproduce.

```json
{
"name": "probe numeric comparison",
"nodes": [
{ "id": "trigger-1", "type": "trigger",
"data": { "type": "trigger", "label": "Trigger", "config": { "triggerType": "Manual" } } },
{ "id": "step-1", "type": "action",
"data": { "type": "action", "label": "Gate", "config": {
"actionType": "Condition",
"condition": "{{@trigger-1:Trigger.a}} < {{@trigger-1:Trigger.b}}",
"conditionConfig": { "id": "g1", "logic": "AND", "rules": [
{ "id": "r1",
"leftOperand": "{{@trigger-1:Trigger.a}}",
"operator": "<",
"rightOperand": "{{@trigger-1:Trigger.b}}" } ] } } } }
],
"edges": [ { "id": "e1", "source": "trigger-1", "target": "step-1" } ]
}
```

Then execute it five times with these inputs:

```json
{"a": "9", "b": "10"}
{"a": 9, "b": 10}
{"a": "99", "b": "100"}
{"a": "999999999999999", "b": "1000000000000000"}
{"a": "9999999999999999", "b": "10000000000000000"}
```

### Reason: what happened

`resolvedExpression` and `condition` are copied from each run's Condition step log.

| trigger input | resolvedExpression | condition |
| --- | --- | --- |
| `{"a":"9","b":"10"}` | `"9" < "10"` | false |
| `{"a":9,"b":10}` | `9 < 10` | true |
| `{"a":"99","b":"100"}` | `"99" < "100"` | false |
| `{"a":"999999999999999","b":"1000000000000000"}` | `"999999999999999" < "1000000000000000"` | false |
| `{"a":"9999999999999999","b":"10000000000000000"}` | `"9999999999999999" < "10000000000000000"` | true |

Nine is not less than ten, but sixteen digits against seventeen digits is correct. The verdict flips
at `Number.MAX_SAFE_INTEGER`, because `needsBigIntMode()` in `lib/bigint-condition-utils.ts` promotes
operands to BigInt only when one of them exceeds it. Below that boundary both operands stay strings.

### Reason: what you expected, and what told you to expect it

I expected a numeric comparison. Three things told me to expect one, in order of weight.

1. **This product already compares numerically on a sibling surface, and #1930 is why.**
`app/api/execute/_lib/condition.ts` converts with `BigInt(observedStr)` and calls `compareBigInt`.
Two comparison paths in one product now return different verdicts for identical operands and an
identical operator. #1930 described its own case as a guard that silently does not guard; the same
sentence applies here.
2. **The code states the intent.** `applyBinary` in `lib/workflow/nodes/condition/safe-eval.ts` is
written as `return (left as number) < (right as number)`. `as number` is a compile time assertion,
so nothing converts at runtime and two strings take the JavaScript string comparison path.
3. **The BigInt helper states the goal.** The header comment of `lib/bigint-condition-utils.ts` says
the point of the conversion is that "both sides of every comparison are the same type".

Nothing in the docs says a Condition compares numeric operands as text, and the visual builder gives
no indication either.

### Reason: what it costs

The verdict routes an on-chain write. A keeper approves and sends a transaction it should have
skipped, or skips one it should have sent.

The failure is asymmetric in the worst direction. Template resolution returns contract reads as
strings, so almost every condition built in the visual builder is string against string. A condition
tested against wei scale values behaves correctly, and the same condition against human unit amounts,
token counts, block counts, percentages or basis points is silently wrong. It passes review and fails
in production.

Concretely, on a workflow that repays a loan when a health factor falls below a threshold, comparing
a human unit amount of `9` against a step of `10` returns false, so the guard does not fire and the
position is left where it was.

### Where you saw it

Production (app.keeperhub.com)

### Version or commit

Production requests on 2026-09-02, 22:47 UTC. Source paths quoted above read from `staging` HEAD on
2026-09-04.

### Scope: what this covers, and what it does not

Affects the relational operators `<`, `<=`, `>`, `>=` in the workflow Condition node, when both
operands resolve to strings and neither exceeds `MAX_SAFE_INTEGER`.

Checked and found fine, so this issue does not propose changing them.

- `app/api/execute/_lib/condition.ts`, the direct execution check. Already BigInt, per #1930.
- `lib/workflow/codegen/templates/condition.ts`. Contains no comparison logic.
- Equality operators `==`, `===`, `!=`, `!==`. String equality is meaningful here for addresses,
symbols and hashes, and changing it is a different and riskier change.
- Arithmetic `-`, `*`, `/`, `%`, `**`. JavaScript already coerces numeric strings for these.
- `+`. String concatenation is legitimate and is used to build messages.

Not checked: the visual builder's own preview, if it evaluates conditions client side, and the
`For Each` body path.

One problem. A separate observation about seeded sample workflows will be filed separately.

### Plan: what should happen next

Promote string operands that are pure integers to BigInt for relational operators only, reusing the
existing BigInt path rather than adding a second numeric route. That keeps exactness at any
magnitude, leaves equality untouched, and does nothing to operands that are already BigInt. Decimal
strings cannot become BigInt, so either compare those as Number or declare them out of scope, and I
would rather you choose which.

Changes nothing a caller depends on in terms of response shape, units or status codes. It does change
verdicts, which is the point, so the risk sits entirely in the corpus of live workflows: any workflow
that today relies on lexicographic ordering of numeric strings would decide differently after the
fix. I could not construct a keeper condition that wants `"9" < "10"` to be false, but you can see
that corpus and I cannot. If such workflows exist, the alternative is a type hint at the builder
level rather than a change to the evaluator.

Tests would cover the boundary the current behaviour hides: 15 digits against 16, 16 against 17, a
decimal pair, a mixed string and number pair, and one equality case asserting that address comparison
is unchanged.

I am happy to open the pull request once this carries `accepted`.

Contributor guide

Open the contributing guide

Research direction

Start in lib/workflow/nodes/condition/safe-eval.ts at applyBinary, then read lib/bigint-condition-utils.ts and the existing comparison path in app/api/execute/_lib/condition.ts. Add regression coverage for string numeric comparisons around MAX_SAFE_INTEGER, decimals, mixed operands, and unchanged equality behavior. Done means relational conditions use the intended numeric behavior without changing equality or response shapes.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, blockchain
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.