Skip to content

Support array-valued context evaluation - #711

Open
roncohen wants to merge 6 commits into
mainfrom
feat/array-valued-context-evaluation
Open

Support array-valued context evaluation#711
roncohen wants to merge 6 commits into
mainfrom
feat/array-valued-context-evaluation

Conversation

@roncohen

@roncohen roncohen commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a type-preserving flattenContext() representation while leaving the public flattenJSON() serialization contract unchanged
  • normalize primitive array elements to strings and JSON-encode composite elements without expanding numeric paths
  • implement array intersection semantics for ANY_OF / NOT_ANY_OF and empty-array semantics for SET / NOT_SET
  • support legacy JSON-encoded arrays during the storage migration
  • explicitly reject scalar-only operators and percentage rollouts for array fields
  • add a minor changeset for @reflag/flag-evaluation

Compatibility

Scalar evaluation behavior and flattenJSON() remain unchanged. Legacy stringified arrays are accepted by the evaluator so readers can be deployed before native-array writers.

Testing

  • yarn workspace @reflag/flag-evaluation build
  • node node_modules/oxlint/bin/oxlint packages/flag-evaluation/src packages/flag-evaluation/test
  • cd packages/flag-evaluation && node node_modules/vitest/vitest.mjs run (177 tests)

Copilot AI lite review requested due to automatic review settings September 3, 2026 13:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are unresolved correctness/security issues in the new/modified evaluator code paths (prototype-pollution risk in flattenContext()’s accumulator and a runtime throw possibility in CONTAINS/NOT_CONTAINS when values is missing).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR extends @reflag/flag-evaluation to support array-valued context fields during rule evaluation, while keeping flattenJSON()’s serialization behavior unchanged and adding a new type-preserving flattenContext() for evaluation use.

Changes:

  • Add flattenContext() plus array normalization rules (primitive-to-string, composite-to-JSON-string) and legacy JSON-encoded array parsing.
  • Update rule evaluation to apply array semantics for ANY_OF / NOT_ANY_OF and SET / NOT_SET, and to reject scalar-only operators + percentage rollout for array fields.
  • Add integration/unit tests and publish a minor changeset for the package.
File summaries
File Description
packages/flag-evaluation/src/index.ts Introduces flattenContext(), legacy array parsing, and updates evaluator logic/types to support array-valued context evaluation.
packages/flag-evaluation/test/index.test.ts Adds test coverage for array-valued context behavior, legacy stringified arrays, and operator semantics.
.changeset/array-context-evaluation.md Declares a minor release describing the new array context evaluation behavior and compatibility.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/flag-evaluation/src/index.ts
Comment thread packages/flag-evaluation/src/index.ts Outdated
Copilot AI review requested due to automatic review settings September 3, 2026 13:34
@roncohen
roncohen force-pushed the feat/array-valued-context-evaluation branch 2 times, most recently from 17db64d to ebd227d Compare September 3, 2026 13:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

flattenContext() object traversal currently differs from the prior flattenJSON()-based evaluation path in a way that can change evaluation results for non-plain objects (enumerable prototype properties), and should be aligned for compatibility.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/flag-evaluation/src/index.ts
Copilot AI review requested due to automatic review settings September 3, 2026 17:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces a prototype-pollution risk in flattenContext() and the new warning rate-limiting hashes include full context, which can defeat throttling and bloat the in-memory limiter cache.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

packages/flag-evaluation/src/index.ts:243

  • flattenContext() assigns array values directly into a plain object. If a context key is "__proto__", "constructor", etc., assigning an object/array can mutate the result object's prototype (prototype pollution / broken lookups). Use a null-prototype map for the flattened output to ensure dangerous keys are treated as normal data keys.
  const result: FlattenedContext = {};

packages/node-sdk/src/client.ts:1443

  • Same rate-limiting issue for config evaluation errors: hashing context makes warnings effectively unthrottled across different callers and can bloat the rate limiter cache. Prefer hashing only stable identifiers and the error set.
        hashObject({
          flagKey: flagData.key,
          configKey: config.key,
          evaluationErrors: config.evaluationErrors,
          context,
        }),
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/node-sdk/src/client.ts Outdated
Copilot AI review requested due to automatic review settings September 3, 2026 18:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new warning rate-limit key hashes evaluationErrors as an array/object structure via hashObject(), which deterministically admits collisions for arrays and can suppress distinct warnings under the same key.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/node-sdk/src/client.ts
Copilot AI review requested due to automatic review settings September 3, 2026 19:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The Node SDK’s new config-related warning logic in _warnMissingFlagContextFields is currently unreachable from existing call sites, so config targeting diagnostics won’t be surfaced as intended.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/node-sdk/src/client.ts:1397

  • The config-path handling in _warnMissingFlagContextFields is currently unreachable: all call sites pass flag without config (see calls in _wrapRawFlag), so config is always undefined here and config targeting evaluationErrors/missingContextFields will never be warned about. This means array-operator diagnostics for config rules won’t be surfaced via warnings when users read flag.config.
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 4, 2026 07:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The missing-context-fields warning rate limiter still hashes the full context, creating high-cardinality keys that can undermine rate limiting and grow the in-memory cache within the limiter window.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread packages/node-sdk/src/client.ts
Comment thread packages/node-sdk/src/client.ts Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 08:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation aligns with the PR’s compatibility goals and is backed by targeted unit/integration tests covering new array semantics, legacy support, and SDK warning behavior.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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.

2 participants