Skip to content

fix: reject non-numeric --*-min/--*-max alert flags instead of silently dropping them - #577

Open
batuhankocyigit wants to merge 3 commits into
nansen-ai:mainfrom
batuhankocyigit:batuhankocyigit-patch-1
Open

fix: reject non-numeric --*-min/--*-max alert flags instead of silently dropping them#577
batuhankocyigit wants to merge 3 commits into
nansen-ai:mainfrom
batuhankocyigit:batuhankocyigit-patch-1

Conversation

@batuhankocyigit

Copy link
Copy Markdown

What

nansen alerts create/update numeric range flags (--usd-min/max, --market-cap-min/max,
--fdv-min/max, --token-amount-min/max, --token-age-min/max, and all
--inflow/outflow/netflow-*-min/max flags) went through plain Number(val).

A non-numeric value becomes NaN, and JSON.stringify(NaN) silently serializes to null:

buildCommonTokenTransferData({ 'usd-min': 'abc', 'usd-max': '5000' })
→ JSON sent to API: {"usdValue":{"min":null,"max":5000}}

So a typo'd flag doesn't error — the alert is just created without the filter the user
asked for, and there's no signal anywhere that anything went wrong. This module had no
test coverage at all before this PR.

Fix

Added parseFiniteNumber(raw, name) and wired it into buildRange() and the standalone
token-age fields. Non-numeric input now throws a clear NansenError (Invalid --<flag> "<value>": must be a number) instead of silently becoming null. Negative values are
still allowed where they're meaningful (e.g. --netflow-1h-min -5000 for a net-outflow
filter) — this only rejects genuinely non-numeric input, not the sign.

Testing

npm test output for the new file:

 RUN  v4.1.9
 ✓ src/__tests__/alerts.test.js (9 tests) 9ms
 Test Files  1 passed (1)
      Tests  9 passed (9)

Full suite: 2626 passed / 5 failed / 2 skipped. The 5 failures (all in doctor.test.js,
chmod/permission-based checks) reproduce identically on a clean main with no changes —
confirmed by stashing this PR's changes and re-running — so they're a pre-existing
CI/sandbox artifact (running as root skips permission enforcement), not something this
PR introduces.

npm run lint — clean.

Checklist

  • npm test passes for the new/changed code (full-suite failures pre-exist on main)
  • npm run lint passes
  • New code paths have tests
  • No console.log, no hardcoded secrets
  • Error messages are actionable
  • Changeset added
  • No src/schema.json change needed (no new commands/options, just validation)

Add tests for numeric range validation in alerts commands.
Fixes silent dropping of numeric range filters in alerts commands by rejecting non-numeric input with clear error messages.
@nansen-pr-reviewer

Copy link
Copy Markdown

pr-reviewer Summary for #3c214a1

No issues found

The code review completed successfully with no findings.

Review effort: 2/5 (Simple)

Summary

This PR correctly fixes a real silent-failure bug: Number('abc') returns NaN, JSON.stringify(NaN) produces null, and the API accepted it — so the user's filter was quietly dropped. The fix is minimal, well-targeted, and correctly uses Number.isFinite (which also rejects Infinity/-Infinity, not just NaN). Error messages are actionable and follow the repo's style.

No issues found. A few things worth noting were verified and confirmed clean:

  • The remaining Number() calls on lines 627–628 (--offset/--limit for alerts list) are pre-existing, out of scope for this PR, and don't feed the API payload directly.
  • buildSmTokenFlowsData only handles token-age-max (not token-age-min) — this asymmetry pre-dates this PR and matches the main branch, so it's not a regression introduced here.
  • The new parseFiniteNumber doc-comment in alerts.js (lines 111–118) explains the why (the NaN → null footgun) rather than restating the code — this is a good comment.
  • The changeset is correctly typed as patch, the package name is correct, and the description is accurate.
  • Tests are unit-only, mock no network, and cover both the happy path and all the error paths introduced.

Token usage: 17 input, 2,761 output, 348,787 cache read, 39,427 cache write | Usage Guide

New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment @nansen-pr-reviewer re-review.

@nansen-pr-reviewer nansen-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved

This PR was automatically approved because:

  • Claude recommends approval
  • Claude assessed this as a moderate effort change
  • The effort level is within the auto-approval threshold of 2
  • No high or critical issues were detected

If you have any concerns, please request a manual review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant