fix(node): reject leading-zero semver values in local evaluation#3643
Merged
Conversation
Per semver 2.0.0 §2, numeric identifiers must not include leading zeros. Values like "1.07.3" are not valid semver and should not match targeting conditions. Both override values and flag values are validated; invalid inputs surface as InconclusiveMatchError-equivalent so the condition does not match.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/node/src/__tests__/feature-flags.spec.ts:3648-3654
**Misleading test name — "across operators" tests only `semver_eq`**
The test description says "rejects leading zeros in override values across operators," but every assertion inside it uses `operator: 'semver_eq'`. Since the override value (person property) goes through `parseSemver` regardless of operator, the rejection behaviour is the same for `semver_gt`, `semver_neq`, etc. — but the test name implies coverage that doesn't exist. This should either be renamed (e.g. `rejects leading zeros in override value`) or expanded to actually cover multiple operators, ideally with `it.each`.
### Issue 2 of 2
packages/node/src/__tests__/feature-flags.spec.ts:3657-3673
**Prefer `it.each` for parameterised rejection tests**
Per the project's review guidelines, parameterised tests are always preferred. This block has five independent `expect(…).toThrow` assertions over different `(operator, bad-value)` pairs — a natural fit for `it.each`. Using `it.each` would also make it easy to add `semver_lt`, `semver_lte`, `semver_gte`, and `semver_neq` (not currently tested in flag-value position) without further repeated boilerplate.
Reviews (1): Last reviewed commit: "fix(node): reject semver values with lea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
dmarticus
changed the title
Contributor
|
Size Change: +520 B (0%) Total Size: 16.1 MB
ℹ️ View Unchanged
|
6 tasks
Address review feedback: the previous "across operators" test used only semver_eq, which is misleading. Converted to it.each so each comparison operator (semver_eq/neq/gt/gte/lt/lte/tilde/caret) and each wildcard pattern is exercised independently. Added the missing changeset for posthog-node.
gustavohstrassburger
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Per semver 2.0.0 §2, numeric identifiers MUST NOT include leading zeros. Values like
1.07.3or01.2.3are not valid semver — posthog-node's local feature flag evaluator currently parses them silently (viaparseInt("07", 10) → 7), which means a person property of1.07.3would incorrectly match asemver_eqcondition against1.7.3.This PR makes
posthog-node's parser reject leading zeros in numeric identifiers. Both override values and flag values are validated; invalid inputs throwInconclusiveMatchErrorso the condition simply does not match.Scope: changes are limited to
packages/node/. No other packages in this monorepo are affected.This mirrors the equivalent fixes in posthog-python #601 and posthog-go #200.
Changes
parseSemvernow delegates numeric-identifier parsing to aparseSemverNumericIdentifierhelper that rejects strings with leading zeros (except literal"0").computeWildcardBoundsuses the same helper, so01.*and1.07.*are rejected.01.02.03parsed/matched as1.2.3— they now correctly reject.semver_eq,semver_gt,semver_caret,semver_tilde, andsemver_wildcardfor both override values and flag values, plus tests confirming literal-zero components (0.1.0,1.0.0,0.0.0) still parse.Test plan
pnpm --filter posthog-node test:unit -- --testPathPattern feature-flags— 315 passedpnpm --filter posthog-node lintcleanpackages/node/