Commit 2da3a1e5 authored by Sam Wiskow's avatar Sam Wiskow
Browse files

fix: address adversarial review blockers 

MR-1: Cast callable limit/period to Integer to prevent type confusion
when lambdas return non-integer values (string env vars, floats).
Raises TypeError at check-time rather than silently failing open.

MR-2: Scenario A test now calls check() between the two object_id
reads, so it would catch a regression that recreates Evaluator inside
check().

MR-3: Scenario H description was incorrect ("no logging" when
logger.info is called); assert the actual no-match info log is emitted.

MR-4: Add assertion that the lambda return value is used as the
operative limit (not just that the lambda was called).

Co-Authored-By: default avatarClaude Sonnet 4.6 <noreply@anthropic.com>
parent baadb280

lib/labkit/rate_limit/evaluator.rb

+2 −2
Original line number Diff line number Diff line
@@ -48,8 +48,8 @@ module Labkit 


      def evaluate_rule(rule, identifier)

        redis_key = build_redis_key(rule, identifier)

        resolved_limit = resolve_value(rule.limit)

        resolved_period = resolve_value(rule.period)

        resolved_limit = Integer(resolve_value(rule.limit))

        resolved_period = Integer(resolve_value(rule.period))



        count = incr_with_ttl(redis_key, resolved_period)

        exceeded = count > resolved_limit

spec/labkit/rate_limit/limiter_spec.rb

+21 −3
Original line number Diff line number Diff line
@@ -26,10 +26,12 @@ RSpec.describe Labkit::RateLimit::Limiter do 


  # Scenario A: Limiter reuse - same Evaluator instance across calls

  describe "Scenario A: Evaluator reuse across calls" do

    it "uses the same Evaluator object_id on every check call" do

    it "uses the same Evaluator object_id before and after a check call" do

      lim = limiter

      eval_ids = Array.new(2) { lim.instance_variable_get(:@evaluator).object_id }

      expect(eval_ids.uniq.size).to eq(1)

      id_before = lim.instance_variable_get(:@evaluator).object_id

      lim.check({ user: 1 })

      id_after = lim.instance_variable_get(:@evaluator).object_id

      expect(id_before).to eq(id_after)

    end

  end
@@ -82,6 +84,22 @@ RSpec.describe Labkit::RateLimit::Limiter do 
      lim.check({ user: 2 })

      expect(call_count).to eq(2)

    end



    it "uses the lambda return value as the operative limit" do

      r = rule(limit: -> { 5 }, action: :block)

      allow(redis).to receive(:incr).and_return(5)



      result = described_class.new(name: "rack_request", rules: [r], redis: redis, logger: logger)

        .check({ user: 1 })



      expect(result.exceeded?).to be(false)



      allow(redis).to receive(:incr).and_return(6)

      result2 = described_class.new(name: "rack_request", rules: [r], redis: redis, logger: logger)

        .check({ user: 1 })



      expect(result2.exceeded?).to be(true)

    end

  end



  # Scenario L: rule with empty match matches any identifier

spec/labkit/rate_limit_spec.rb

+2 −1
Original line number Diff line number Diff line
@@ -115,12 +115,13 @@ RSpec.describe Labkit::RateLimit do 
  end



  describe "Scenario H: empty rules array" do

    it "returns a no-match Result without writing to Redis or logging" do

    it "returns a no-match Result without writing to Redis and without a warning" do

      result = limiter(rules: []).check({ user: 42 })



      expect(result.matched?).to be(false)

      expect(result.exceeded?).to be(false)

      expect(logger).not_to have_received(:warn)

      expect(logger).to have_received(:info).with(hash_including(matched: false))

    end

  end