From 231dc85344bdf4d6450a75db20c5fa956408d90d Mon Sep 17 00:00:00 2001
From: John Nunemaker <nunemaker@gmail.com>
Date: Wed, 29 Jul 2026 17:11:15 -0400
Subject: [PATCH] Add empty group detection and pruning to Expression, guard
 enables
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

An expression containing an empty All group evaluates true for every
actor, so persisting one silently enables the feature for everyone.

- Expression#empty_groups? detects empty Any/All groups anywhere in
  the tree, #prune_empty_groups removes them wherever removal cannot
  broaden the expression, and #always_true? reports expressions that
  are guaranteed to match every actor.
- Feature#enable_expression and #add_expression raise ArgumentError
  when the expression contains empty groups. flipper-api already
  rescues ArgumentError into the expression_invalid error response,
  so API clients get a 422 instead of persisting the footgun.
- Feature#remove_expression of the last condition now disables the
  expression instead of persisting an empty group (previously it
  left {"Any":[]} — or worse, {"All":[]}, enabled-for-everyone —
  in the adapter).
- Includes a seeded property spec verifying against the evaluator
  that pruning never broadens an expression.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 lib/flipper/expression.rb                     | 68 +++++++++++++++
 lib/flipper/feature.rb                        | 42 +++++++--
 spec/flipper/dsl_spec.rb                      |  2 +-
 .../expression_pruning_property_spec.rb       | 85 +++++++++++++++++++
 spec/flipper/expression_spec.rb               | 78 +++++++++++++++++
 spec/flipper/feature_spec.rb                  | 45 ++++++++--
 6 files changed, 306 insertions(+), 14 deletions(-)
 create mode 100644 spec/flipper/expression_pruning_property_spec.rb

diff --git a/lib/flipper/expression.rb b/lib/flipper/expression.rb
index 8d08988c0..10032f9c5 100644
--- a/lib/flipper/expression.rb
+++ b/lib/flipper/expression.rb
@@ -50,12 +50,80 @@ def eql?(other)
     end
     alias_method :==, :eql?
 
+    # Public: Returns true if this expression contains an Any/All group with
+    # no arguments anywhere in its tree. An empty All evaluates to true for
+    # every actor and an empty Any to false for every actor, so persisting
+    # one is almost always a mistake.
+    def empty_groups?
+      return true if group? && args.empty?
+
+      args.any? { |arg| arg.is_a?(Expression) && arg.empty_groups? }
+    end
+
+    # Public: Returns a copy of this expression with empty Any/All groups
+    # removed wherever removal cannot broaden the expression, or nil when no
+    # conditions remain. An empty All can be removed from an All parent (it
+    # contributes a constant true) and an empty Any from an Any parent (a
+    # constant false), but an empty Any inside an All is kept because
+    # removing it would broaden the expression from never-true.
+    def prune_empty_groups
+      prune_empty_groups_with_identity.first
+    end
+
+    # Public: Returns true when this expression is guaranteed to match every
+    # actor, e.g. an empty All group reaches an Any group or the root.
+    def always_true?
+      constant_group_value == true
+    end
+
     def value
       {
         name => args.map(&:value)
       }
     end
 
+    protected
+
+    def prune_empty_groups_with_identity
+      return [self, nil] unless group?
+      return [nil, all?] if args.empty?
+
+      pruned_args = args.map do |arg|
+        arg.is_a?(Expression) ? arg.prune_empty_groups_with_identity : [arg, nil]
+      end
+      kept = pruned_args.map(&:first).compact
+
+      # No condition remains. Preserve this group's constant value for an
+      # enclosing group, which lets an empty All disappear from All without
+      # treating it as the unsafe empty-Any-in-All case.
+      return [nil, constant_group_value] if kept.empty?
+
+      if all? && pruned_args.any? { |value, identity| value.nil? && identity == false }
+        return [self, nil]
+      end
+
+      [build(name => kept), nil]
+    end
+
+    def constant_group_value
+      return nil unless group?
+      return all? if args.empty?
+
+      values = args.map do |arg|
+        arg.is_a?(Expression) ? arg.constant_group_value : nil
+      end
+
+      if all?
+        return false if values.include?(false)
+        return true if values.all?(true)
+      else
+        return true if values.include?(true)
+        return false if values.all?(false)
+      end
+
+      nil
+    end
+
     private
 
     def call_with_context?
diff --git a/lib/flipper/feature.rb b/lib/flipper/feature.rb
index 2b44cf655..8b9468388 100644
--- a/lib/flipper/feature.rb
+++ b/lib/flipper/feature.rb
@@ -124,8 +124,10 @@ def enabled?(*actors)
     # expression - an Expression or Hash that can be converted to an expression.
     #
     # Returns result of enable.
+    # Raises ArgumentError if the expression contains empty Any/All groups,
+    # because an empty All matches every actor.
     def enable_expression(expression)
-      enable Expression.build(expression)
+      enable validate_expression!(Expression.build(expression))
     end
 
     # Public: Add an expression for a feature.
@@ -133,12 +135,18 @@ def enable_expression(expression)
     # expression_to_add - an expression or Hash that can be converted to an expression.
     #
     # Returns result of enable.
+    # Raises ArgumentError if the resulting expression contains empty Any/All
+    # groups, because an empty All matches every actor.
     def add_expression(expression_to_add)
-      if (current_expression = expression)
-        enable current_expression.add(expression_to_add)
+      expression_to_add = Expression.build(expression_to_add)
+
+      new_expression = if (current_expression = expression)
+        current_expression.add(expression_to_add)
       else
-        enable expression_to_add
+        expression_to_add
       end
+
+      enable validate_expression!(new_expression)
     end
 
     # Public: Enables a feature for an actor.
@@ -191,14 +199,21 @@ def disable_expression
     end
 
     # Public: Remove an expression from a feature. Does nothing if no expression is
-    # currently enabled.
+    # currently enabled. Removing the last condition disables the expression
+    # rather than persisting an empty group.
     #
     # expression - an Expression or Hash that can be converted to an expression.
     #
-    # Returns result of enable or nil (if no expression enabled).
+    # Returns result of enable or disable or nil (if no expression enabled).
     def remove_expression(expression_to_remove)
       if (current_expression = expression)
-        enable current_expression.remove(expression_to_remove)
+        remaining = current_expression.remove(expression_to_remove)
+
+        if remaining.group? && remaining.args.empty?
+          disable_expression
+        else
+          enable remaining
+        end
       end
     end
 
@@ -427,6 +442,19 @@ def gate_for(actor)
 
     private
 
+    # Private: Raises if an expression about to be enabled contains empty
+    # Any/All groups. An empty All evaluates to true for every actor, so
+    # persisting one silently enables the feature for everyone.
+    #
+    # Returns the expression.
+    def validate_expression!(expression)
+      if expression.is_a?(Expression) && expression.empty_groups?
+        raise ArgumentError, "#{expression.value.inspect} contains empty Any/All groups and cannot be enabled (an empty All matches every actor). Remove the empty groups or add conditions to them."
+      end
+
+      expression
+    end
+
     # Private: Wrap the actors passed to enabled? in a single pass to avoid
     # intermediate array allocations on the hot path. Arrays are flattened one
     # level; the explicit is_a?(Array) check (instead of flatten) avoids the
diff --git a/spec/flipper/dsl_spec.rb b/spec/flipper/dsl_spec.rb
index 6b93f2a20..f978365e4 100644
--- a/spec/flipper/dsl_spec.rb
+++ b/spec/flipper/dsl_spec.rb
@@ -226,7 +226,7 @@
       expect(subject[:stats].expression).to eq(any_expression)
 
       subject.remove_expression(:stats, expression)
-      expect(subject[:stats].expression).to eq(Flipper.any)
+      expect(subject[:stats].expression).to be(nil)
     end
   end
 
diff --git a/spec/flipper/expression_pruning_property_spec.rb b/spec/flipper/expression_pruning_property_spec.rb
new file mode 100644
index 000000000..12a2d4364
--- /dev/null
+++ b/spec/flipper/expression_pruning_property_spec.rb
@@ -0,0 +1,85 @@
+require 'flipper/expression'
+
+# Property-based check that empty group detection and pruning honor their
+# contracts, verified against the evaluator itself. For seeded random trees
+# (biased toward empty Any/All groups, including multi-key hashes where only
+# the first key counts):
+#
+#   - expressions without empty groups prune to themselves
+#   - pruning never broadens: no context excluded by the original may be
+#     included by the pruned expression
+#   - pruning to nil only happens when the original is a constant
+#   - always_true? implies the expression evaluates true in every context
+#
+# The seed is fixed for determinism; override with FUZZ_SEED/FUZZ_TREES for
+# exploratory runs.
+RSpec.describe Flipper::Expression do
+  SEED = (ENV["FUZZ_SEED"] || 20260728).to_i
+  TREES = (ENV["FUZZ_TREES"] || 1500).to_i
+
+  PLANS = %w[basic pro enterprise].freeze
+
+  CONTEXTS = PLANS.flat_map do |plan|
+    (0..50).step(10).map { |age| {properties: {"plan" => plan, "age" => age}} }
+  end.freeze
+
+  def random_leaf(rng)
+    case rng.rand(4)
+    when 0 then {"Equal" => [{"Property" => ["plan"]}, PLANS[rng.rand(3)]]}
+    when 1 then {"NotEqual" => [{"Property" => ["plan"]}, PLANS[rng.rand(3)]]}
+    when 2 then {"GreaterThan" => [{"Property" => ["age"]}, rng.rand(50)]}
+    else        {"LessThan" => [{"Property" => ["age"]}, rng.rand(50)]}
+    end
+  end
+
+  def random_tree(rng, depth)
+    return random_leaf(rng) if depth <= 0 || rng.rand < 0.25
+
+    operator = rng.rand < 0.5 ? "Any" : "All"
+    children = Array.new(rng.rand(4)) { random_tree(rng, depth - 1) }
+    tree = {operator => children}
+
+    # Multi-key hashes must behave as if only the first key exists, matching
+    # Flipper::Expression.build.
+    if rng.rand < 0.05
+      other = operator == "Any" ? "All" : "Any"
+      tree[other] = Array.new(rng.rand(3)) { random_tree(rng, depth - 1) }
+    end
+
+    tree
+  end
+
+  def evaluate_all(expression)
+    CONTEXTS.map { |context| !!expression.evaluate(context) }
+  end
+
+  it "prunes without ever broadening the expression" do
+    rng = Random.new(SEED)
+
+    TREES.times do
+      tree = random_tree(rng, 4)
+      expression = described_class.build(tree)
+      failure = "seed=#{SEED} tree=#{tree.inspect}"
+
+      values = evaluate_all(expression)
+      pruned = expression.prune_empty_groups
+
+      if expression.always_true?
+        expect(values).to all(be(true)), "#{failure} always_true? but not always true"
+      end
+
+      unless expression.empty_groups?
+        expect(pruned).to eq(expression), "#{failure} pruning changed a clean expression"
+        next
+      end
+
+      if pruned.nil?
+        expect(values.uniq.size).to eq(1), "#{failure} pruned to nil but original is not constant"
+      else
+        evaluate_all(pruned).zip(values).each do |after, before|
+          expect(after && !before).to be(false), "#{failure} pruning broadened the expression"
+        end
+      end
+    end
+  end
+end
diff --git a/spec/flipper/expression_spec.rb b/spec/flipper/expression_spec.rb
index 54f9f72ee..f4c86f784 100644
--- a/spec/flipper/expression_spec.rb
+++ b/spec/flipper/expression_spec.rb
@@ -185,4 +185,82 @@
       expect(expression == other).to be(false)
     end
   end
+
+  describe "#empty_groups?" do
+    it "returns true for an empty group" do
+      expect(described_class.build({"Any" => []}).empty_groups?).to be(true)
+      expect(described_class.build({"All" => []}).empty_groups?).to be(true)
+    end
+
+    it "returns true for a nested empty group" do
+      expression = described_class.build({
+        "Any" => [{"Equal" => [{"Property" => ["plan"]}, "basic"]}, {"All" => []}],
+      })
+      expect(expression.empty_groups?).to be(true)
+    end
+
+    it "returns false for groups with conditions" do
+      expression = described_class.build({
+        "Any" => [{"All" => [{"Equal" => [{"Property" => ["plan"]}, "basic"]}]}],
+      })
+      expect(expression.empty_groups?).to be(false)
+    end
+
+    it "returns false for a condition" do
+      expression = described_class.build({"Equal" => [{"Property" => ["plan"]}, "basic"]})
+      expect(expression.empty_groups?).to be(false)
+    end
+  end
+
+  describe "#prune_empty_groups" do
+    let(:condition) { {"Equal" => [{"Property" => ["plan"]}, "basic"]} }
+
+    it "returns nil when no conditions remain" do
+      expect(described_class.build({"Any" => []}).prune_empty_groups).to be(nil)
+      expect(described_class.build({"All" => []}).prune_empty_groups).to be(nil)
+      expect(described_class.build({"Any" => [{"All" => []}]}).prune_empty_groups).to be(nil)
+    end
+
+    it "removes an empty All from an All parent" do
+      expression = described_class.build({"All" => [condition, {"All" => []}]})
+      expect(expression.prune_empty_groups).to eq(described_class.build({"All" => [condition]}))
+    end
+
+    it "removes an empty Any from an Any parent" do
+      expression = described_class.build({"Any" => [condition, {"Any" => []}]})
+      expect(expression.prune_empty_groups).to eq(described_class.build({"Any" => [condition]}))
+    end
+
+    it "keeps an empty Any inside an All because removal would broaden" do
+      expression = described_class.build({"All" => [condition, {"Any" => []}]})
+      expect(expression.prune_empty_groups).to eq(expression)
+    end
+
+    it "returns a condition untouched" do
+      expression = described_class.build(condition)
+      expect(expression.prune_empty_groups).to eq(expression)
+    end
+  end
+
+  describe "#always_true?" do
+    it "returns true for an empty All" do
+      expect(described_class.build({"All" => []}).always_true?).to be(true)
+    end
+
+    it "returns true for an empty All inside an Any" do
+      expression = described_class.build({
+        "Any" => [{"Equal" => [{"Property" => ["plan"]}, "basic"]}, {"All" => []}],
+      })
+      expect(expression.always_true?).to be(true)
+    end
+
+    it "returns false for an empty Any" do
+      expect(described_class.build({"Any" => []}).always_true?).to be(false)
+    end
+
+    it "returns false for conditional expressions" do
+      expression = described_class.build({"Equal" => [{"Property" => ["plan"]}, "basic"]})
+      expect(expression.always_true?).to be(false)
+    end
+  end
 end
diff --git a/spec/flipper/feature_spec.rb b/spec/flipper/feature_spec.rb
index 3a9b26ff2..3cea74cf6 100644
--- a/spec/flipper/feature_spec.rb
+++ b/spec/flipper/feature_spec.rb
@@ -911,6 +911,39 @@ def actor.nil?
     end
   end
 
+  describe '#enable_expression with empty groups' do
+    it "raises for an empty root group" do
+      expect {
+        subject.enable_expression(Flipper.any)
+      }.to raise_error(ArgumentError, /empty Any\/All groups/)
+      expect(subject.expression).to be(nil)
+    end
+
+    it "raises for a nested empty group" do
+      expect {
+        subject.enable_expression({"Any" => [{"Equal" => [{"Property" => ["plan"]}, "basic"]}, {"All" => []}]})
+      }.to raise_error(ArgumentError, /empty Any\/All groups/)
+      expect(subject.expression).to be(nil)
+    end
+  end
+
+  describe '#add_expression with empty groups' do
+    it "raises when the added expression contains an empty group" do
+      expect {
+        subject.add_expression(Flipper.all)
+      }.to raise_error(ArgumentError, /empty Any\/All groups/)
+      expect(subject.expression).to be(nil)
+    end
+
+    it "raises when adding to an expression would keep an empty group" do
+      subject.enable_expression Flipper.property(:plan).eq("basic")
+
+      expect {
+        subject.add_expression(Flipper.any(Flipper.all))
+      }.to raise_error(ArgumentError, /empty Any\/All groups/)
+    end
+  end
+
   describe '#remove_expression' do
     context "when nothing enabled" do
       context "with Expression instance" do
@@ -946,10 +979,10 @@ def actor.nil?
       end
 
       context "with Expression instance" do
-        it "changes expression to Any and removes Expression if it matches" do
+        it "disables the expression when removing the last condition" do
           new_expression = Flipper.property(:plan).eq("basic")
           subject.remove_expression new_expression
-          expect(subject.expression).to eq(Flipper.any)
+          expect(subject.expression).to be(nil)
         end
 
         it "changes expression to Any if Expression doesn't match" do
@@ -985,9 +1018,9 @@ def actor.nil?
       end
 
       context "with Expression instance" do
-        it "removes Expression if it matches" do
+        it "disables the expression when removing the last condition" do
           subject.remove_expression condition
-          expect(subject.expression).to eq(Flipper.any)
+          expect(subject.expression).to be(nil)
         end
 
         it "does nothing if Expression does not match" do
@@ -1038,9 +1071,9 @@ def actor.nil?
       end
 
       context "with Expression instance" do
-        it "removes Expression if it matches" do
+        it "disables the expression when removing the last condition" do
           subject.remove_expression condition
-          expect(subject.expression).to eq(Flipper.all)
+          expect(subject.expression).to be(nil)
         end
 
         it "does nothing if Expression does not match" do