[rb] validate BiDi outbound ref fields against their declared type

[titusfortner]

🔗 Related Issues

#17786

💥 What does this PR do?

• Ruby BiDi now validates outbound field's declared type before sending it, as required by the proposed ADR
• Adds scalar-value labels to the shared BiDi schema so a union's bare-scalar arm literals (e.g. input.Origin's "viewport"/"pointer") can be validated across bindings

🔧 Implementation Notes

• The new outbound checks mirror the existing inbound readers one-to-one, reusing the file's convention that outbound raises ArgumentError (caller error) and inbound raises Error::WebDriverError (wire error).
• Most of the change is the hand-written serialization runtime (serialization/record.rb, serialization/union.rb) used by the generated protocol classes.
• The scalar-arm literals are carried end-to-end: the schema emits scalarValues, the generator turns it into a scalar_values DSL call (failing at generation for an unmodeled non-object_only union), and protocol/*.rb is regenerated.
• The current BiDi implementation only uses this code for navigation currently when websocket url is enabled, nothing else is impacted

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: the ref-validation methods, the Union.valid_outbound? predicate, tests, and RBS signatures
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

PR Summary by Qodo

Ruby BiDi: validate outbound ref fields (incl. union scalar literals) before sending

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Validate outbound BiDi ref-typed fields against their declared record/union types.
• Propagate union bare-scalar literals from schema to Ruby generator for strict validation.
• Add unit coverage for record refs, union variants, scalar-arm maps, and ref lists.

Diagram

graph TD
  A["BiDi schema (JS)"] --> B["Ruby generator"] --> C["Generated protocol (Ruby)"]
  B --> G["Ruby templates"] --> C
  C --> D["Serialization::Record"] --> E["Outbound validation"]
  C --> F["Serialization::Union"] --> E
  E --> H["Ruby unit tests"]
  A --> I["Schema tests"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Validate only at top-level command boundary

• ➕ Less validation code in shared serialization runtime
• ➕ Potentially simpler control flow
• ➖ Easy to miss nested refs (lists, maps, nested records)
• ➖ Duplicates logic across commands and domains

2. Rely on browser-side validation (no outbound checks)

• ➕ Zero local complexity
• ➕ Avoids keeping local rules in sync
• ➖ Caller errors become wire round-trips with harder-to-debug failures
• ➖ Does not meet ADR requirement for outbound type validation

3. Encode unions as tagged Ruby types only (no bare scalars)

• ➕ Eliminates scalar-arm ambiguity entirely
• ➕ More uniform union handling
• ➖ Breaking/awkward API for spec shapes like input.Origin
• ➖ Still needs schema awareness to preserve wire compatibility

Recommendation: Keep the current approach: validating in the shared serialization runtime (Record/Union) provides consistent coverage for nested shapes (lists/maps) and avoids per-command duplication, while threading scalarValues from schema→generator→DSL enables the tightest possible check for bare-scalar union arms (rejecting wrong literals like "banana" before the wire).

Files changed (9) +257 / -8

Enhancement (6) +122 / -8

project_bidi_schema.mjsDerive union scalarValues from const scalar arms +9/-2

Derive union scalarValues from const scalar arms

• Documents and emits a new 'scalarValues' signal on union type refs. The normalizer now collects '{ const }' arm literals and attaches them to the union node for downstream consumers.

javascript/selenium-webdriver/project_bidi_schema.mjs

input.rbDeclare Input::Origin scalar_values literals +1/-0

Declare Input::Origin scalar_values literals

• Adds 'scalar_values 'viewport', 'pointer'' to the Origin union. This enables outbound union validation to accept only the schema-declared scalar literals for the bare-scalar arm.

rb/lib/selenium/webdriver/bidi/protocol/input.rb

record.rbValidate outbound ref-typed fields (records, unions, lists, scalar-arm maps) +51/-1

Validate outbound ref-typed fields (records, unions, lists, scalar-arm maps)

• Introduces outbound ref validation mirroring inbound readers: record refs must be instances of the declared class, and union refs must be accepted by the union. Adds support for validating ref lists, scalar-arm map entries ([key,value]) including primitive checking for bare keys, raising ArgumentError for caller mistakes.

rb/lib/selenium/webdriver/bidi/serialization/record.rb

union.rbAdd scalar_values DSL and valid_outbound? union predicate +33/-0

Add scalar_values DSL and valid_outbound? union predicate

• Adds a 'scalar_values' declaration for non-object_only unions with bare-scalar arms, and implements 'valid_outbound?' to determine whether a value matches any variant or an allowed scalar literal. Introduces internal helpers to recurse into nested unions and to cache variant refs/classes.

rb/lib/selenium/webdriver/bidi/serialization/union.rb

bidi_generate.rbThread schema scalarValues into generated Ruby unions and fail fast on unsupported shapes +25/-5

Thread schema scalarValues into generated Ruby unions and fail fast on unsupported shapes

• Extends the union IR to carry 'scalar_values', renders it via helper methods, and populates it from the schema for alias-to-union cases. Adds a generator-time guard that rejects non-object_only unions without scalar_values, ensuring unsupported union shapes fail during generation rather than at runtime.

rb/lib/selenium/webdriver/bidi/support/bidi_generate.rb

module.rb.erbRender scalar_values declarations in generated unions +3/-0

Render scalar_values declarations in generated unions

• Updates the Ruby protocol template to emit 'scalar_values ...' when the generated union type carries pinned scalar literals. Keeps existing 'object_only' emission behavior unchanged.

rb/lib/selenium/webdriver/bidi/support/templates/module.rb.erb

Tests (2) +115 / -0

project_bidi_schema_test.mjsAdd coverage for union scalarValues emission +3/-0

Add coverage for union scalarValues emission

• Extends schema tests to assert that unions like Origin report both 'scalar' and 'scalarValues'. Ensures bindings can reject wrong scalar literals, not just wrong primitives.

javascript/selenium-webdriver/project_bidi_schema_test.mjs

serialization_spec.rbAdd unit tests for outbound ref validation scenarios +112/-0

Add unit tests for outbound ref validation scenarios

• Adds a comprehensive test suite covering record ref type enforcement, union variant acceptance and cross-union rejection, object_only union scalar rejection, non-object_only union scalar literal acceptance/rejection, raw Hash rejection, ref list element validation, and scalar-arm map entry validation.

rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb

Other (1) +20 / -0

serialization.rbsAdd RBS signatures for outbound ref and union validation APIs +20/-0

Add RBS signatures for outbound ref and union validation APIs

• Adds method signatures for Record outbound ref validation helpers and Union scalar_values/valid_outbound? and related private helpers. Keeps type annotations aligned with the expanded runtime behavior.

rb/sig/lib/selenium/webdriver/bidi/serialization.rbs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Scalar arm too permissive ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

Union.scalar_arm? accepts any String/Numeric/boolean for all non-object_only unions, so a ref to
Input::Origin can incorrectly accept primitives like 1/true outbound and bypass the PR’s
declared-type validation. This can produce invalid wire payloads (caller error) that should be
rejected locally with ArgumentError.

Code

rb/lib/selenium/webdriver/bidi/serialization/union.rb[R102-104]

+            def scalar_arm?(value)
+              value.is_a?(::String) || value.is_a?(::Numeric) || value == true || value == false
+            end

Evidence

valid_outbound? accepts any value that satisfies scalar_arm? for non-object_only unions when no
variant matches; scalar_arm? currently returns true for any String/Numeric/boolean.
Input::PointerMoveAction.origin is a ref to Input::Origin (a non-object_only union), and the
spec demonstrates the intended bare-scalar usage as a String ('viewport'), but the current
implementation would also accept numeric/boolean primitives.

rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-95]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[99-104]
rb/lib/selenium/webdriver/bidi/serialization/record.rb[167-171]
rb/lib/selenium/webdriver/bidi/protocol/input.rb[210-216]
rb/lib/selenium/webdriver/bidi/protocol/input.rb[254-259]
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[462-477]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Union.valid_outbound?` falls back to `scalar_arm?` for non-`object_only` unions when no variant matches. The current `scalar_arm?` treats *any* String/Numeric/boolean as acceptable, which can let invalid primitives through for unions whose scalar arm is more specific (e.g., `Input::Origin` is exercised in tests with a String scalar arm like `'viewport'`).

### Issue Context
This method is used by outbound ref validation (`Record#validate_ref_value`) to decide whether a union-typed ref value is acceptable.

### Fix approach
Add per-union scalar-arm metadata and use it in `valid_outbound?`:
1. Introduce a small DSL on `Union` (e.g., `scalar_types(*types)` or `scalar_primitives(*primitives)`) that stores allowed scalar Ruby classes (or protocol primitive names).
2. Change `scalar_arm?` to consult that metadata (and return `false` if the union has no scalar arm declared).
3. Update union subclasses that truly have a scalar arm (e.g., `Input::Origin`) to declare it (String).
4. Add a regression spec ensuring `Input::PointerMoveAction.new(... origin: 1)` and `origin: true` are rejected.

### Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-105]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[462-479]
- rb/sig/lib/selenium/webdriver/bidi/serialization.rbs[146-147] (only if new DSL methods are added)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Union accepts invalid Hash ✓ Resolved 🐞 Bug ≡ Correctness

Description

Union.valid_outbound? treats any non-Record value as an acceptable scalar for non-object_only
unions, so a plain Hash that matches no variant passes outbound ref validation. This allows invalid
objects (e.g., Input::Origin) to be serialized and sent, defeating the PR’s declared type checks.

Code

rb/lib/selenium/webdriver/bidi/serialization/union.rb[R92-95]

+              return true if variant_refs.any? { |ref| variant_accepts?(ref, value) }
+
+              !@object_only && !value.is_a?(Record::Serializable)
+            end

Evidence

Inbound unions treat non-Hash payloads as the bare scalar arm (when not object_only), but treat Hash
payloads as objects that must match a variant; outbound currently accepts Hash as a scalar fallback.
Record outbound ref validation delegates union checking to Union.valid_outbound?, so this permissive
fallback defeats the new ref typing guarantees for non-object-only unions like Input::Origin (used
by PointerMoveAction.origin).

rb/lib/selenium/webdriver/bidi/serialization/union.rb[54-67]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-95]
rb/lib/selenium/webdriver/bidi/serialization/record.rb[167-172]
rb/lib/selenium/webdriver/bidi/protocol/input.rb[210-259]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Serialization::Union.valid_outbound?` currently falls back to accepting any value that is not a `Record::Serializable` when the union is not `object_only`. This incorrectly treats a plain `Hash` (that matches no variant) as a valid scalar-arm value.

Because `Record::Deserializer#validate_ref_value` uses `klass.valid_outbound?(value)` to validate union-typed refs, this bug bypasses the new outbound ref-type validation and permits invalid wire payloads.

### Issue Context
Inbound `Union.from_json` explicitly treats `Hash` as an object payload that must match a declared variant (or it raises). Outbound should mirror this and reject `Hash` values unless they are instances of an allowed record/union variant.

Add a regression spec demonstrating that a non-object-only union ref (e.g., `Input::Origin`) rejects an unmatched Hash, e.g.:
- `Input::PointerMoveAction.new(x: 0, y: 0, origin: {'bad' => 'shape'})` should raise `ArgumentError`.

### Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-95]
- rb/lib/selenium/webdriver/bidi/serialization/record.rb[167-172]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[420-471]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit e9d21d1

Results up to commit f440258 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Union accepts invalid Hash ✓ Resolved 🐞 Bug ≡ Correctness

Description

Union.valid_outbound? treats any non-Record value as an acceptable scalar for non-object_only
unions, so a plain Hash that matches no variant passes outbound ref validation. This allows invalid
objects (e.g., Input::Origin) to be serialized and sent, defeating the PR’s declared type checks.

Code

rb/lib/selenium/webdriver/bidi/serialization/union.rb[R92-95]

+              return true if variant_refs.any? { |ref| variant_accepts?(ref, value) }
+
+              !@object_only && !value.is_a?(Record::Serializable)
+            end

Evidence

Inbound unions treat non-Hash payloads as the bare scalar arm (when not object_only), but treat Hash
payloads as objects that must match a variant; outbound currently accepts Hash as a scalar fallback.
Record outbound ref validation delegates union checking to Union.valid_outbound?, so this permissive
fallback defeats the new ref typing guarantees for non-object-only unions like Input::Origin (used
by PointerMoveAction.origin).

rb/lib/selenium/webdriver/bidi/serialization/union.rb[54-67]
rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-95]
rb/lib/selenium/webdriver/bidi/serialization/record.rb[167-172]
rb/lib/selenium/webdriver/bidi/protocol/input.rb[210-259]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Serialization::Union.valid_outbound?` currently falls back to accepting any value that is not a `Record::Serializable` when the union is not `object_only`. This incorrectly treats a plain `Hash` (that matches no variant) as a valid scalar-arm value.

Because `Record::Deserializer#validate_ref_value` uses `klass.valid_outbound?(value)` to validate union-typed refs, this bug bypasses the new outbound ref-type validation and permits invalid wire payloads.

### Issue Context
Inbound `Union.from_json` explicitly treats `Hash` as an object payload that must match a declared variant (or it raises). Outbound should mirror this and reject `Hash` values unless they are instances of an allowed record/union variant.

Add a regression spec demonstrating that a non-object-only union ref (e.g., `Input::Origin`) rejects an unmatched Hash, e.g.:
- `Input::PointerMoveAction.new(x: 0, y: 0, origin: {'bad' => 'shape'})` should raise `ArgumentError`.

### Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/serialization/union.rb[87-95]
- rb/lib/selenium/webdriver/bidi/serialization/record.rb[167-172]
- rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb[420-471]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 9740ccf

[qodo-code-review]

Code review by qodo was updated up to the latest commit e9d21d1