feat: add custom assignment rules evaluation

[joalves]

Summary

• Add evaluateRules() to AudienceMatcher supporting the new rules[].or[] payload structure with environment scoping and JsonExpr conditions
• Integrate rules evaluation into Context._assign() — rules are checked after audience filter but before audienceStrict / traffic split
• Add getEnvironment() to Client so the Context can access the environment name for rule filtering
• Add targetingRule flag to exposure events for explicit identification in analytics
• Validate rule.variant is a number; skip invalid rules instead of aborting iteration

Assignment Flags

Rule-matched assignments set assigned=true, eligible=true, overridden=true, targetingRule=true.

All possible assignment states

State	assigned	eligible	overridden	fullOn	custom	audienceMismatch	targetingRule
Not running	false	true	false	false	false	false	false
Override	false	true	true	false	false	false	false
Audience strict mismatch	false	true	false	false	false	true	false
Rule match	true	true	true	false	false	*	true
Normal (eligible)	true	true	false	false	false	false	false
Normal (not eligible)	true	false	false	false	false	false	false
Custom assignment	true	true	false	false	true	false	false
Full-on	true	true	false	true	false	false	false

* = audienceMismatch is set independently before rules, can be either value.

Engine bitmask

targetingRule uses bit 8 (value 256). The existing filter bitAnd(flags, 207) = 3 has bit 8 as 0 in the mask, so it's automatically ignored — no engine query changes needed. The engine's Exposure.java and EventForwardingManager.java will need the new field when implementing server-side support.

Test Plan

• 21 unit tests for evaluateRules in matcher
• 16 integration tests in context (flags, cache invalidation, overrides, variables)
• All 299 tests pass (29 suites)

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This change adds rule-based audience targeting with environment scoping. Client gains a public getEnvironment() method. AudienceMatcher adds evaluateRules(audienceString, environmentName, vars) to parse audience JSON, skip or evaluate rules based on rule.environments and rule.and, and return a numeric variant or null. Context captures the client environment and calls evaluateRules during experiment assignment; when a rule variant is returned the assignment is forced (assigned = true, eligible = true, variant set, overridden = true). Multiple new tests cover rule precedence, environment gating, audienceStrict interactions and exposure reporting.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• mario-silva
• calthejuggler

Poem

🐰 I nibble JSON, hop through each rule,
I check the environment, stay sharp and cool,
When a match pops up I cheer "Variant One!",
If none align I twitch and then I'm done,
Hopping through tests — targeting's fun.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat: add custom assignment rules evaluation' clearly and concisely describes the main feature addition—rule-based assignment evaluation—which is the primary focus of the changeset across matcher, context, and client modules.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/sdk-rules-implementation

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/__tests__/matcher.test.js (1)

214-218: Consider adding a test for rules with missing variant property.

The malformed rules tests cover invalid rules structure but don't test the case where a rule object lacks the variant property. This would help document the expected behaviour (and catch the issue flagged in matcher.ts).

📝 Proposed additional test

it("should return null when rule has no variant property", () => {
  const audience = JSON.stringify({
    rules: [
      {
        or: [
          {
            name: "rule1",
            and: [{ value: true }],
            environments: [],
            // variant is missing
          },
        ],
      },
    ],
  });
  expect(matcher.evaluateRules(audience, "production", {})).toBe(null);
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/matcher.test.js` around lines 214 - 218, Add a test to
src/__tests__/matcher.test.js that asserts matcher.evaluateRules returns null
when a rule object is missing the required variant property: construct an
audience JSON string with rules containing an object that has
name/and/environments but no variant and call matcher.evaluateRules(audience,
"production", {}) expecting null; this will document the expected behaviour and
catch the case flagged in matcher.evaluateRules.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/matcher.ts`:
- Around line 36-45: The matcher currently returns rule.variant directly which
can be undefined if the rule object lacks a variant, letting callers treat it as
non-null; update the logic in the matching method (the block referencing rule,
rule.variant, conditions, and this._jsonExpr.evaluateBooleanExpr) to explicitly
validate the presence of a variant before returning it: when conditions are
absent or when evaluateBooleanExpr returns true, check that the rule has a
defined variant (e.g., using Object.prototype.hasOwnProperty or typeof
rule.variant !== 'undefined') and return that value only if present, otherwise
return null so callers (like context.ts which tests ruleVariant !== null) won't
receive undefined.

---

Nitpick comments:
In `@src/__tests__/matcher.test.js`:
- Around line 214-218: Add a test to src/__tests__/matcher.test.js that asserts
matcher.evaluateRules returns null when a rule object is missing the required
variant property: construct an audience JSON string with rules containing an
object that has name/and/environments but no variant and call
matcher.evaluateRules(audience, "production", {}) expecting null; this will
document the expected behaviour and catch the case flagged in
matcher.evaluateRules.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 517d7527-f95e-4572-89b4-24e573253ef8

📥 Commits

Reviewing files that changed from the base of the PR and between 47390ae and 5c2e5a8.

📒 Files selected for processing (5)

• src/__tests__/context.test.js
• src/__tests__/matcher.test.js
• src/client.ts
• src/context.ts
• src/matcher.ts

[coderabbitai]

🧹 Nitpick comments (1)

src/__tests__/matcher.test.js (1)

255-277: Consider adding a test with multiple objects in the rules array.

All current tests use a single object in the rules array with variation only in the or array. If the implementation iterates through multiple top-level rule groups in rules[], a test verifying that behaviour would improve coverage.

🧪 Example test case

it("should evaluate rules across multiple rule groups", () => {
    const audience = JSON.stringify({
        rules: [
            {
                or: [
                    {
                        name: "rule1",
                        and: [{ eq: [{ var: "country" }, { value: "GB" }] }],
                        environments: [],
                        variant: 1,
                    },
                ],
            },
            {
                or: [
                    {
                        name: "rule2",
                        and: [{ eq: [{ var: "country" }, { value: "US" }] }],
                        environments: [],
                        variant: 2,
                    },
                ],
            },
        ],
    });
    expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(2);
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/matcher.test.js` around lines 255 - 277, Add a new unit test
that verifies matcher.evaluateRules correctly iterates multiple top-level rule
groups in the rules array: create an audience JSON with two objects in rules[]
(first containing a GB rule with variant 1, second containing a US rule with
variant 2), call matcher.evaluateRules(audience, "production", { country: "US"
}) and assert it returns 2; name the test something like "should evaluate rules
across multiple rule groups" so it complements the existing single-group tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/__tests__/matcher.test.js`:
- Around line 255-277: Add a new unit test that verifies matcher.evaluateRules
correctly iterates multiple top-level rule groups in the rules array: create an
audience JSON with two objects in rules[] (first containing a GB rule with
variant 1, second containing a US rule with variant 2), call
matcher.evaluateRules(audience, "production", { country: "US" }) and assert it
returns 2; name the test something like "should evaluate rules across multiple
rule groups" so it complements the existing single-group tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 979d44b4-6279-4763-9d8a-833b504314e8

📥 Commits

Reviewing files that changed from the base of the PR and between 5c2e5a8 and 003dada.

📒 Files selected for processing (3)

• src/__tests__/context.test.js
• src/__tests__/matcher.test.js
• src/matcher.ts

✅ Files skipped from review due to trivial changes (1)

• src/tests/context.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• src/matcher.ts

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/context.ts`:
- Around line 550-553: The code in _assign sets assignment.assigned,
assignment.variant and incorrectly sets assignment.overridden when ruleVariant
!== null; remove or avoid setting assignment.overridden in the rule-match branch
so rule-derived assignments remain distinct from manual overrides. Update the
logic in _assign (the branch that checks ruleVariant) to only mark
assigned/variant for rule matches and leave overridden false (or introduce a
separate ruleMatched flag if needed), and ensure the existing manual override
fast-path is the only place that sets assignment.overridden to true so
override("...") will always take the explicit-override semantics.
- Around line 541-548: The cached assignment reuse logic only checks
assignment.audienceMismatch but not the actual rule-driven variant, so updates
to attributes used by rules can return a stale variant; compute the current
ruleVariant via _audienceMatcher.evaluateRules(experiment.data.audience,
this._environmentName, this._getAttributesMap()) (as already done) and compare
it to the cached assignment's rule-derived identity (e.g., a stored ruleVariant
id or serialized ruleVariant) and treat a mismatch as invalidation (in addition
to assignment.audienceMismatch), or store the ruleVariant id onto the assignment
when first created so subsequent reuse logic compares that id against the newly
computed ruleVariant and rejects reuse if they differ.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 44c23f01-bff6-485e-ba9a-bb01d4572414

📥 Commits

Reviewing files that changed from the base of the PR and between 003dada and 76d1d6e.

📒 Files selected for processing (2)

• src/__tests__/context.test.js
• src/context.ts

[joalves]

Code review

Found 2 issues (both now fixed in f32df8e):

1. return null on invalid variant aborts all rule iteration -- a matched rule with a non-numeric variant would return null, preventing subsequent valid rules from being evaluated. Changed to continue so the loop skips invalid rules and tries the next one.

javascript-sdk/src/matcher.ts

Lines 33 to 41 in 3b9ab8c

	continue;
	}
	}
	const conditions = rule.and;
	if (!conditions || (Array.isArray(conditions) && conditions.length === 0)) {
	return typeof rule.variant === "number" ? rule.variant : null;
	}
	if (Array.isArray(conditions)) {
	const result = this._jsonExpr.evaluateBooleanExpr({ and: conditions }, vars);

2. audienceMatches does not persist ruleVariant back to the cached assignment -- after re-evaluating rules and confirming the variant hasn't changed, assignment.attrsSeq was updated but assignment.ruleVariant was not. This breaks the invariant that if attrsSeq is up-to-date then ruleVariant is up-to-date, causing unnecessary re-evaluations on subsequent attribute changes.

javascript-sdk/src/context.ts

Lines 476 to 481 in 3b9ab8c

	const ruleVariant = this._audienceMatcher.evaluateRules(
	experiment.audience,
	this._environmentName,
	attrs
	);
	if (ruleVariant !== (assignment.ruleVariant ?? null)) {

[calthejuggler]

Looking really good so far 🙂
Good to put it in a separate PR, but let's not forget to add the SDK version to the payload before we publish this.

[calthejuggler]

This isn't quite what I meant 🤔 We want the exclusion rules to basically be the same as an override (and present as such to the engine). To me that would mean setting assignment.overriden here, and simply using ruleOverride for metadata later.

Suggested change

	assignment.ruleOverride = true;
	assignment.variant = ruleVariant;
	assignment.overridden = true;
	assignment.ruleOverride = true;

This way - the engine can function exactly how it does today, but we have the added benefit of being able to work out whether the override was from a direct code override, or a rule in the UI

[joalves]

We originally set overridden=true to pass the engine's bitAnd(flags, 207) = 3 filter, but that was abusing the flag just to get excluded from analysis. Now that we set assigned=false, the filter already excludes rule-matched assignments (it requires assigned=true), so we don't need to piggyback on overridden anymore.

This gives us proper semantics:

State	assigned	overridden	ruleOverride
Rule match	false	false	true
SDK override	false	true	false
Normal assignment	true	false	false

• overridden=true means "SDK developer override" (its original meaning)
• ruleOverride=true means "rule-forced from the web console"
• Both are excluded from analysis, but for different reasons (overridden via the bitmask, ruleOverride via assigned=false)

It also avoids the cache issue we had earlier — when rules set overridden=true, the override cache check (assignment.overridden && assignment.variant === this._overrides[...]) would incorrectly match rule-cached assignments. We had to add a !assignment.assigned workaround. With overridden=false on rules, that problem doesn't exist and the code stays clean.

For the variableValue check — overridden already passes that condition for SDK overrides, and ruleOverride passes it for rules. Each flag handles its own case without conflation.

[calthejuggler]

We then wouldn't need to add these checks, and the tests should pass just the same

[joalves]

True, we wouldn't need these checks here, but we'd have to add the !assignment.assigned workaround back to the override cache check instead, to prevent rule-cached assignments from colliding with SDK overrides. So we're trading one check for another, and losing the clean semantics on the way.

[calthejuggler]

Would we? 🤔 It would be checked already by the overridden flag.

[joalves]

The overridden flag alone doesn't cover the edge case where the override variant equals the rule variant.

Consider this sequence (assuming rules set overridden: true as you suggested):

1. treatment("exp") → rule matches → cached as {overridden: true, assigned: true, variant: 1}
2. override("exp", 1) → override cache check: assignment.overridden && assignment.variant === this._overrides["exp"] → true && 1 === 1 → cache hit → returns the stale rule assignment

The problem: it returns the rule-cached assignment with assigned: true, but a real override should have assigned: false. The overridden flag is true in both cases, so it can't tell them apart.

The !assignment.assigned guard broke that false cache hit: rules have assigned: true, real overrides have assigned: false, so the check becomes assignment.overridden && !assignment.assigned && variant === override — which correctly misses on rule-cached assignments and forces a recompute.

That said, this is now moot — in the current code rules don't set overridden: true anymore, so there's no collision. The ruleOverride flag is the sole identifier.