Skip to content

feat(rules): bucketed_into_experience_key mutual-exclusion audience rule (qs-09)#42

Merged
abbaseya merged 7 commits into
feat/fullstack-v12from
feat/mutual-exclusion
Jul 16, 2026
Merged

feat(rules): bucketed_into_experience_key mutual-exclusion audience rule (qs-09)#42
abbaseya merged 7 commits into
feat/fullstack-v12from
feat/mutual-exclusion

Conversation

@abbaseya

Copy link
Copy Markdown
Collaborator

Summary

Adds one additive audience rule type to the Ruby SDK — bucketed_into_experience_key — enabling declarative mutual exclusion on fullstack. The rule's value is a target experience key; it matches iff the visitor is already bucketed into that experience per the SDK's stored visitor state, and combines with negated: true to express "NOT in experiment X". Zero new application inputs (works with empty visitor attributes).

Spec: qs-04-mutual-exclusion-rule.md (Convert Ruby SDK planning folder). Implemented via /implement-feature (feature-conductor): readiness gate → plan → TDD → decision audit → code review.

What shipped

  • lib/convert_sdk/rule_manager.rbis_rule_matched gains an optional resolver: keyword, threaded unchanged through the recursive process_and → process_or_when → process_rule_item walk so a bucketed_into_experience_key leaf is evaluated in place inside ALL/ANY groups. New process_mutual_exclusion_rule branch gated strictly on rule_type (generic match_type path bit-identical). Resolver is a 3-state contract ->(key){ true | false | nil }; nil = unknown target key → warning naming the key + bucketed_raw=false; matched = negated ? !bucketed_raw : bucketed_raw. No resolver / malformed matching → fail closed to false (negation unapplied).
  • lib/convert_sdk/data_manager.rb — per-call mutual_exclusion_resolver(visitor_id) (read-only: experience_by_key + stored_bucketing_map), built in match_rules_by_field and threaded to the audience-matching walk only. Location / site-area / segments walks deliberately fall closed.
  • RBS signatures updated for both; steep green.

Contract fidelity & guarantees

  • All 8 rows of the normative cross-SDK fixture pass, table-driven.
  • Read-only (AC5): resolver never buckets the target, never writes the store, never tracks — asserted with spies.
  • Store persistence (AC3): cross-context exclusion via a shared persistent store fake (fixture row 8).
  • Generic-rule regression lock (AC7): the 3 generic rule types are bit-identical to today.
  • Unknown-target warning (AC8), empty-attributes (AC4), ALL/ANY combination (AC6) all covered.
  • Fail-closed parity with the JS oracle (rule-manager.ts:299-364) preserved for the generic path.

Non-goals (respected)

No web-boolean enablement, no bucketed_into_variation_key, no exclusion groups/layers, no UI, no backend registration (separate sibling spec), no bucketing/storage-format/event-payload change.

Tests

Full suite 1348/0 (unit + cross_sdk + integration), rubocop clean, steep: no type errors. New: spec/unit/rule_manager_mutual_exclusion_spec.rb, spec/unit/data_manager_mutual_exclusion_spec.rb, spec/support/mutual_exclusion_fixture.rb.

Stacking

Stacked PR — base is feat/experiment-preview (qs-03, open PR #41), not main. Part of the Ruby SDK feature stack (feat/fullstack-v12 ← feat/ruby-sdk-cache-level ← feat/experiment-preview ← feat/mutual-exclusion).

Compatibility (release-note material)

Old SDK versions receiving this rule evaluate it false with negation unapplied (fail-closed). Recommend combining the exclusion audience under ALL; state the minimum SDK version in the guide.

🤖 Generated with Claude Code

abbaseya and others added 6 commits July 15, 2026 18:35
…tests (RED)

Beads: ai-driven-product-dev-dyrh
Agent: sdk-ruby

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…implementation (GREEN)

Threads optional resolver through is_rule_matched walk; new leaf branch gated on
rule_type, 3-state resolver (true/false/nil->warn), negation applied. Generic path
bit-identical (AC7). RBS + steep green.

Beads: ai-driven-product-dev-dyrh
Agent: sdk-ruby

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…exclusion — tests (RED)

AC2/AC3/AC4/AC5/AC6/AC8 integration coverage over the qs-04 fixture; shared
MutualExclusionFixture builder; RedisStore-backed cross-context persistence.

Beads: ai-driven-product-dev-oy0p
Agent: sdk-ruby

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ion (GREEN)

Per-call mutual_exclusion_resolver(visitor_id) built in match_rules_by_field,
threaded through match_audiences -> matched_audiences -> is_rule_matched. Read-only
(experience_by_key + stored_bucketing_map). Location/site-area walks unchanged
(fall closed). RBS + steep green. Full suite 1345/0.

Beads: ai-driven-product-dev-oy0p
Agent: sdk-ruby

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ion in mutual-exclusion specs

Comment/description-only; describes merged GREEN behavior. No assertion changed.
30/0, rubocop clean.

Beads: ai-driven-product-dev-oy0p

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lusion leaf fails closed (no TypeError)

Guard rule['matching'].is_a?(Hash) in process_mutual_exclusion_rule before dig;
malformed/missing matching returns false (negation unapplied), resolver never
invoked — restores FR22 fail-closed per-leaf. Adds row-5 distinct-experience
fixture example. Generic path unchanged. Full suite 1348/0, steep green.

Beads: ai-driven-product-dev-5v8j
Agent: sdk-ruby

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request implements the bucketed_into_experience_key mutual-exclusion rule (qs-04) for the Ruby SDK. It introduces a mutual_exclusion_resolver in DataManager that is threaded down to RuleManager to evaluate whether a visitor has already been bucketed into a target experience. It also adds comprehensive unit tests and updates RBS type signatures. The feedback suggests optimizing the resolver by caching the visitor's stored bucketing map outside of the lambda block to prevent redundant store lookups during evaluation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lib/convert_sdk/data_manager.rb
@abbaseya abbaseya self-assigned this Jul 15, 2026
@abbaseya
abbaseya requested a review from a team July 15, 2026 17:08
Base automatically changed from feat/experiment-preview to feat/fullstack-v12 July 15, 2026 23:08

@JosephSamirL JosephSamirL 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.

This PR is solid — the strongest of the three qs-mutual-exclusion SDK PRs I've looked at

I reviewed it against the spec of record (ai-driven-product-dev#86), the JS SDK's feat/mutual-exclusion-rule branch, and the iOS/Android siblings. I found no correctness defect. Ruby gets right the two things the mobile SDKs each got wrong, and the findings below are traceability and test-coverage — nothing that changes behavior.

Specifically, checked rather than taken on trust:

The id-keyed resolution is correct — and this is exactly where Android had to depart from the spec, so I traced it the same way. bucket_fresh sets experience_id = experience["id"].to_s, and persist_bucketing writes { experience_id => variation_id }. So the stored map genuinely is id-keyed and stored_bucketing_map(visitor_id).key?(target["id"].to_s) matches the normative algorithm exactly. Ruby follows the spec literally, Android correctly diverged — both right for their own storage. That's the spec's wording being JS-shaped, not either SDK erring.

matching_options.audiences is honoredaudiences_verdict? requires matched.length == to_check.length under ALL, matching JS (data-manager.ts:418-421). This matters: it's the one thing the spec's Compatibility section ("The guide must recommend ALL") depends on, and it's not implemented on Android or iOS. Across the fleet: JS and Ruby honor it; the two mobile SDKs silently degrade ALL to ANY.

The rule-tree traversal is correctprocess_and requires every OR_WHEN block to pass; process_or_when returns on the first matching leaf. And the fixture builder encodes the distinction properly (:all → each leaf its own AND-member; :any → all leaves in one OR_WHEN).

The three-state resolver draws the right distinction, which is the subtle part: no resolver injected → false with negation unapplied (capability absent, mirroring the JS fall-through); resolver present but target unresolvable → warn naming the key + negation applied on top of bucketed_raw = false (rows 6/7). Those are genuinely different cases and both are handled correctly.

Also verified: the 8-row fixture matches the spec row-for-row; AC5 holds (the lambda reads only frozen config + the store — no BucketingManager, no merge_visitor_data, no tracking); the leaf is evaluated in place in the walk (a legitimate choice — the spec explicitly frees the seam: "extend literal-mode resolution…, wrap data in a provider, or pre-resolve — implementer's choice"); RBS sigs are updated alongside; and CI is 12/12 green including Steep, JRuby, and the Puma-cluster fork smoke.

Findings (neither is a behavior bug)

  1. The spec citation points at a real, unrelated spec — inline on spec/support/mutual_exclusion_fixture.rb. Worse than a dead link, because qs-04 exists in this folder and is something else.
  2. The config shape the guide will recommend is untested — inline on the config builder. Ruby supports two audiences under matching_options: ALL; nothing proves it for the exclusion rule.

Plus two nits inline (generic_key_value isn't a real wire rule_type; the resolver re-reads the store per leaf).

Reference implementation: follow convertcom/javascript-sdk#416

Reviewing the sibling PRs together settled an ambiguity in AC6, and JS has the reading its own spec text supports — JS #416 is the reference to follow. Its model: a dedicated exclusion audience carrying exactly one bucketed_into_experience_key rule, composed with other audiences via matching_options.audiences: ALL/ANY, resolved before the tree reaches the rule engine. The spec's cross-references back it: Verified Facts cite matching_options.audiences supports ALL/ANY; AC2 says "a transient audience (matching_options: ALL)"; Compatibility says "Under ANY-combination with another matching audience". All audience-level.

Ruby is the closest of the four to that reference already — it's one of only two SDKs that honors matching_options.audiences (JS being the other; Android and iOS ignore it entirely), so unlike the mobile SDKs there's no blocker here. The gap is only in the tests:

  • Finding 2 above is the alignment: the two-audience matching_options example I'm asking for is exactly what JS #416's AC6 does ([GENERIC_AUDIENCE_ID, EXCLUSION_AUDIENCE_ID] driven under matchingOptions: 'all' | 'any').
  • Correspondingly, rule_tree(mode: :all/:any)'s mixed-leaf trees test an axis the reference model never emits. They're correctly built for what they are — unlike iOS #43, which gets that exact tree distinction backwards — but AC6's ALL/ANY belongs on matching_options, not on the tree shape.

The is_rule_matched seam itself doesn't need to move: under the dedicated-single-rule-audience contract, in-place and pre-resolve give identical answers.

Nothing here blocks — leaving as a comment.

Comment thread spec/support/mutual_exclusion_fixture.rb Outdated
Comment thread spec/support/mutual_exclusion_fixture.rb
Comment thread spec/support/mutual_exclusion_fixture.rb Outdated
Comment thread lib/convert_sdk/data_manager.rb
@abbaseya abbaseya changed the title feat(rules): bucketed_into_experience_key mutual-exclusion audience rule (qs-04) feat(rules): bucketed_into_experience_key mutual-exclusion audience rule (qs-09) Jul 16, 2026
…ience matching_options coverage

Addresses Joseph's PR #42 (qs-09 mutual-exclusion) review feedback:
- Fix stale qs-04/planning-artifacts citations to the real qs-09
  implementation-artifacts spec path across the fixture + both spec files.
- Fixture's generic_leaf now uses the real served wire rule_type
  (generic_text_key_value) instead of the JS-parity-fixture convention.
- Add MutualExclusionFixture.config_two_audiences + AC6 audience-level
  (matching_options ALL/ANY) coverage exercising audiences_verdict? via two
  separately-attached audiences (exclusion + generic), per JS SDK PR #416's
  model — additive to the existing intra-audience-tree AC6 coverage.

Full suite 1356/0, steep clean, rubocop clean, cross_sdk 274/0,
full_chain 9/0.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@abbaseya
abbaseya merged commit 36fb956 into feat/fullstack-v12 Jul 16, 2026
12 checks passed
@abbaseya
abbaseya deleted the feat/mutual-exclusion branch July 16, 2026 14:59
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