feat: add opt-in string coercion to List value matching by smandegar · Pull Request #48 · rewind-community/eight_ball

smandegar · 2026-07-22T15:09:15Z

Summary

Adds an opt-in coerce boolean to the List condition. By default List#satisfied? matches by exact type (values.include?(value)), so a list authored with integers never matches a string caller and vice versa. When coerce: true, both the list values and the tested input are compared as strings (values.map(&:to_s).include?(value.to_s)), making the match type-tolerant.

Behavior

Default unchanged: coerce defaults to nil and matching stays exact-type, so existing definitions and consumers behave identically.
Backward-compatible wire format: coerce is serialized only when true (nil/false is omitted), so existing serialized blobs are untouched and older gem versions simply ignore the field.
coerce participates in condition equality.

Tests

Exact-type default preserved; coerce matches across Integer/String in both directions; non-members still excluded.
JSON round-trip: coerce: true persists and rebuilds enabled; a default list omits the key (never emits coerce: false).
Equality distinguishes two lists that differ only in coerce.

Minor version bump 3.3.3 to 3.4.0.

🤖 Generated with Claude Code

Add an optional `coerce` boolean to the List condition. When true, the list values and the tested input are compared as strings, so an integer-authored list matches a string caller (and vice versa). Defaults to exact-type matching, keeping existing definitions fully backward compatible; the flag is serialized only when enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

diff-vader-bot

🟢 Risk: LOW — no substantive findings

✅ Diff Vader · Approved

Council reviewed 6 files · 12 reviewers · 0 findings · $0.35

✅ blast‑radius
✅ correctness
✅ cost
❓ github‑actions — skipped: filter: not applicable to changed files
❓ iac — skipped: filter: not applicable to changed files
❓ migration‑safety — skipped: filter: not applicable to changed files
✅ performance
✅ revertibility
✅ security‑tenant‑isolation
❓ supply‑chain — skipped: no external packages changed in diff
✅ supportability
✅ test‑adequacy

Why: All approval gates passed.

_{💡 /diff-vader-review <name> adds a one-shot reviewer. Beyond this PR's council, you can request: backup-correctness, loop-design-system.}

Copilot

Pull request overview

Adds an opt-in coerce flag to the EightBall::Conditions::List condition so list matching can be made type-tolerant by comparing both sides as strings, while keeping default exact-type matching and preserving backwards-compatible JSON serialization.

Changes:

Add coerce support to List#satisfied?, include it in wire fields, and incorporate it into equality state.
Add specs covering default behavior, coercion behavior, JSON round-tripping, and serialization omission when not enabled.
Bump gem version to 3.4.0 and document the change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
spec/marshallers/json_spec.rb	Adds JSON serialization + round-trip coverage for `coerce` on list conditions.
spec/conditions/list_spec.rb	Adds behavioral tests for default exact matching vs `coerce: true`, plus equality coverage.
lib/eight_ball/version.rb	Bumps version from 3.3.3 to 3.4.0.
lib/eight_ball/conditions/list.rb	Implements `coerce` flag, wire serialization field, and equality participation.
Gemfile.lock	Updates local gem version to 3.4.0.
CHANGELOG.md	Documents the new opt-in `coerce` behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

smandegar · 2026-07-22T16:59:25Z

      options ||= {}

      @values = Array(options[:values])
+      @coerce = options[:coerce] || nil


Fixed in a70ee9c: coerce is now normalized to a canonical true/nil (options[:coerce] ? true : nil), so a non-boolean can never reach the wire. The gem's contract is still that callers pass a real boolean (the admin casts before constructing); this just guarantees the serialized form is always valid.

smandegar · 2026-07-22T16:59:27Z

    def satisfied?(value)
+      return values.map(&:to_s).include?(value.to_s) if @coerce
+
      values.include? value
    end


Fixed in a70ee9c: satisfied? now uses values.any? { |v| v.to_s == string_value } with the tested value stringified once, so no intermediate array is allocated.

smandegar · 2026-07-22T16:59:28Z

    def state
-      super + [@values.sort]
+      super + [@values.sort, @coerce]
    end


Fixed in a70ee9c: state now sorts via sort_by(&:to_s), so a mixed-type list (which coerce makes a first-class case) no longer raises ArgumentError in ==/hash. Both operands transform identically, so equality is preserved.

- normalize coerce to a canonical true/nil so the wire never carries a stray non-boolean value - evaluate coerce allocation-free (any? instead of map(&:to_s)) - sort state by string form so a mixed-type list does not raise in ==/hash Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

diff-vader-bot

🟢 Risk: LOW — no substantive findings

✅ Diff Vader · Approved

Council reviewed 7 files · 12 reviewers · 0 findings · $0.38

✅ blast‑radius
✅ correctness
✅ cost
❓ github‑actions — skipped: filter: not applicable to changed files
❓ iac — skipped: filter: not applicable to changed files
❓ migration‑safety — skipped: filter: not applicable to changed files
✅ performance
✅ revertibility
✅ security‑tenant‑isolation
❓ supply‑chain — skipped: no external packages changed in diff
✅ supportability
✅ test‑adequacy

Why: All approval gates passed.

_{💡 /diff-vader-review <name> adds a one-shot reviewer. Beyond this PR's council, you can request: backup-correctness, loop-design-system.}

diff-vader-bot · 2026-07-22T17:08:04Z

+    end
  end

  describe '==' do


pr-reviewer-test-adequacy — confidence 88% (advisory)

The satisfied? method now has a fast-path for @coerce (returning early via return) and a fallback values.include? value. The tests exercise the coerce=true path thoroughly, but there is no test that verifies the exact-type path still returns true for an integer list tested against an identical integer value when coerce is nil — only the negative case (satisfied?('1') returns false) is checked. A regression that inadvertently broke the default include? path for matching values would go undetected. Consider adding: expect(EightBall::Conditions::List.new(values: [1]).satisfied?(1)).to eq true in the 'should not coerce types by default' block.

React with 👍 / 👎 / 😕 to help us calibrate — why.

Added in 7ec9a72. The should not coerce types by default example now also asserts the positive case, List.new(values: [1]).satisfied?(1) is true, alongside the two existing negative assertions. A regression that broke the default include? path for a matching value would now fail here instead of going undetected. Suite 163/0.

kmgoswami99

Clean and well-scoped, and the specs cover the coerce paths. Three things worth a look before merge: coercion is pure string equality, so it isn't the numeric equivalence the doc implies (1.0 won't match [1], and nil/empty collapse together); the wire field is locked once this ships and a pre-3.4.0 reader silently falls back to exact matching; and the 'no stray value' test never actually marshals. None blocking.

kmgoswami99 · 2026-07-22T17:36:31Z

    def satisfied?(value)
+      if @coerce
+        string_value = value.to_s
+        return values.any? { |v| v.to_s == string_value }


Coercion here is pure string equality (v.to_s == value.to_s), which is narrower and wider than the int/string example in the doc:

1.0 won't match a [1] list ("1.0" != "1"), so it's not numeric equivalence.

nil.to_s is "", so a nil or empty-string caller matches a list containing nil/"" (and vice versa). any_satisfied? guards nil callers but not empty strings.

Neither is exploitable (flag JSON is trusted admin config), but both are easy to trip over. Worth a doc line naming the float/nil behavior, a satisfied?(nil) test to pin it, and a call on whether empty/nil should match at all.

Good catch, both pinned and documented in cb3d329. Added a spec asserting the exact-string semantics: satisfied?(1.0) against [1] is false (not numeric equivalence), satisfied?(nil) is false, and a [nil] list matches an empty-string caller (the nil/"" collapse). Kept the behavior as-is rather than special-casing nil/empty, since coerce targets scalar id/name lists where those are out of contract, and added a README line naming the float and nil/empty behavior.

kmgoswami99 · 2026-07-22T17:36:31Z


    def wire_fields
-      %i[values parameter]
+      %i[values parameter coerce]


Two wire-contract notes now that coerce is on the wire:

The field is effectively locked once this ships. A bare boolean won't extend - the next ask (case-insensitive, numeric-tolerant) needs a second flag, where a named mode (match: "string") would've left room. Not worth reworking for v1, just flagging that the shape is committed.

And the backward-compat story has a quiet edge: a pre-3.4.0 reader ignores coerce:true and silently falls back to exact matching, so the same flag JSON evaluates differently across gem versions with no signal. The description frames older readers ignoring the field as clean; the trip-wire is that it flips the answer. Worth stating in the README compat note.

Both flagged, thanks. On the field shape: agreed the boolean is committed once this ships; I weighed match: "string" (named mode, room to extend) against a boolean during design and went boolean for YAGNI, since the only near-term need is string coercion. A future case-insensitive/numeric mode can add its own field. Not reworking for v1, per your read.

On backward-compat: you are right that "ignored" undersells it, since it flips the result. Added a README compat note in cb3d329 stating a pre-coerce reader silently exact-matches a coerce:true list, so the same flag can evaluate differently across gem versions until every reader is upgraded.

kmgoswami99 · 2026-07-22T17:36:31Z

+        expect(EightBall::Conditions::List.new(values: [1, 2], coerce: true).satisfied?('3')).to eq false
+      end
+
+      it 'should normalize a truthy value to a canonical boolean so the wire carries no stray value' do


This test name promises the wire carries no stray value, but it only asserts the in-memory .coerce attr for coerce: 1 and coerce: false - it never marshals them. The json_spec only exercises coerce: true and the default, so the actual serialized output for a truthy non-boolean or an explicit false is never verified end-to-end. Marshal a coerce: 1 list and assert the output to make the name's claim real.

Fixed in cb3d329. Renamed the in-memory test to drop the "wire" claim, and added end-to-end wire assertions in the marshaller spec: a coerce: 1 list marshals to "coerce":true (never "coerce":1), and an explicit coerce: false is omitted from the wire entirely. So the normalization is now verified through the serializer, not just the attr.

… the TS port - README: note coercion is exact-string (1.0 != 1, nil/"" share a form) and that a pre-coerce reader silently exact-matches (cross-version behavior flip) - pin nil / float / empty-string coerce behavior with a spec - assert the marshalled wire for a truthy non-boolean (canonical true) and an explicit false (omitted), so the normalization is verified end to end - emit `coerce` before `parameter` so the List wire matches eight-ball-ts byte for byte Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

kmgoswami99

Fixes from the last round all landed - the string-semantics specs, the compat note, and the end-to-end marshal assertions read well. One gap left: cb3d329's byte-order invariant against the TS port isn't actually tested (see inline). Also, diff-vader's nudge to add a positive default-path assertion is already covered by the existing value accepted test at list_spec.rb:20, so no action needed there.

kmgoswami99 · 2026-07-22T19:07:53Z

+      coerced = [
+        EightBall::Feature.new('Coerced', [EightBall::Conditions::List.new(values: [1], parameter: 'param1', coerce: true)])
+      ]
+      expect(marshaller.marshall(coerced)).to include '"coerce":true'


cb3d329's whole point - emit coerce before parameter so the wire stays byte-identical with eight-ball-ts - isn't actually pinned. include '"coerce":true' passes no matter where the key lands, so reverting wire_fields to values, parameter, coerce stays green. One exact-substring assertion ("values":[1],"coerce":true,"parameter":"param1") would lock the ordering so it can't silently regress.

Fixed in 7ec9a72. Replaced the position-agnostic include '"coerce":true' with the exact substring '"values":[1],"coerce":true,"parameter":"param1"', so the ordering is locked.

I checked it is actually load-bearing rather than just longer: reverting wire_fields back to values, parameter, coerce now fails this example (expected "...\"values\":[1],\"parameter\":\"param1\",\"coerce\":true..." to include "\"values\":[1],\"coerce\":true,\"parameter\":\"param1\""), where before it stayed green. Restored, suite 163/0.

kmgoswami99 · 2026-07-22T19:07:53Z

+      @coerce = options[:coerce] ? true : nil
      self.parameter = options[:parameter]
    end



Unrelated to coerce, but you're editing right below it: this @example doesn't run - new EightBall::Conditions::List.new [1, 'a'] has a stray new and passes a positional array where initialize expects an options hash. Should be List.new(values: [1, 'a']). Cheap to fix while you're in here.

Fixed in 7ec9a72: the example is now EightBall::Conditions::List.new(values: [1, 'a']), dropping the stray new and the positional array. Thanks for catching it while I was in the neighbourhood.

@example

- assert the exact marshalled substring so the coerce-before-parameter order that keeps the wire byte-identical with eight-ball-ts cannot silently regress - cover the positive exact-type match, so a break in the non-coerce path is caught - fix the List @example, which had a stray `new` and passed a positional array Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

diff-vader-bot approved these changes Jul 22, 2026

View reviewed changes

diff-vader Bot added the diff-vader:auto-approved label Jul 22, 2026

smandegar requested a review from Copilot July 22, 2026 15:15

Copilot started reviewing on behalf of smandegar July 22, 2026 15:16 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

smandegar and others added 2 commits July 22, 2026 12:58

docs: document opt-in List value coercion in the README

101b99a

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

smandegar marked this pull request as ready for review July 22, 2026 17:07

smandegar requested a review from a team as a code owner July 22, 2026 17:07

diff-vader-bot approved these changes Jul 22, 2026

View reviewed changes

smandegar self-assigned this Jul 22, 2026

kmgoswami99 reviewed Jul 22, 2026

View reviewed changes

kmgoswami99 approved these changes Jul 22, 2026

View reviewed changes

smandegar merged commit e253e28 into main Jul 28, 2026
1 check passed

smandegar deleted the list-value-coercion branch July 28, 2026 18:02

Uh oh!

Conversation

smandegar commented Jul 22, 2026

Summary

Behavior

Tests

Uh oh!

diff-vader-bot left a comment

Choose a reason for hiding this comment

✅ Diff Vader · Approved

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diff-vader-bot left a comment

Choose a reason for hiding this comment

✅ Diff Vader · Approved

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmgoswami99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmgoswami99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants