Skip to content

[rb] validate nullable-constant BiDi params outbound - #17818

Merged
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:rb-bidi-outbound-const-validation
Jul 24, 2026
Merged

[rb] validate nullable-constant BiDi params outbound#17818
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:rb-bidi-outbound-const-validation

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

Closes item 3 (outbound local validation) of the low-level WebDriver BiDi behavioral contract proposed in #17786. Schema signal it relies on was added in #17700.

💥 What does this PR do?

  • The internal BiDi layer now rejects an invalid value for a nullable-constant command param — emulation.setScriptingEnabled's enabled and browsingContext.setBypassCSP's bypass — with a local ArgumentError, instead of serializing an off-spec value onto the wire for the remote end to reject on a round-trip.
  • Valid calls are unchanged: the spec literal or an explicit null still serialize exactly as before.
  • This makes Ruby fully conformant with the contract's outbound-validation item (enum, required-field, unknown-property, and now const-literal membership).

🔧 Implementation Notes

  • The shared schema already carried the {const, nullable} descriptor for these fields (since [js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity #17700); the Ruby generator was consuming it only to type the RBS and dropping it from the runtime spec. This threads it through: the generator emits const: and the runtime Record validates it — no schema change needed.
  • Non-nullable consts stay baked discriminators (auto-populated, unchanged); only nullable consts — which are settable literal-or-null — gain validation, so item 2 settability is preserved.
  • enabled's const is false (the spec lets you disable scripting or clear the override, not force it on), so the check handles a falsy literal via sentinels rather than truthiness.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the runtime const check in record.rb, the generator threading in bidi_generate.rb, the regenerated protocol files, and the test
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Jul 24, 2026
@selenium-ci

Copy link
Copy Markdown
Member

Thank you, @titusfortner for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Validate nullable const-literals for Ruby BiDi outbound params

🐞 Bug fix ✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Reject non-literal values for nullable-const BiDi params before sending them over the wire.
• Thread schema const through the Ruby generator into runtime Record validation.
• Add unit coverage for literal-or-null param enforcement and update lint/signature metadata.
Diagram

graph TD
  G["bidi_generate.rb"] --> P["Protocol param records"] --> R["Serialization::Record"] --> V["validate_const"] --> E["ArgumentError (local)"]
  T["serialization_spec.rb"] --> V --> E
  S["serialization.rbs"] --> V
  C[".rubocop.yml"] --> R
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Command-layer validation per API method
  • ➕ Error messages could be tailored to each command and parameter name in the public API surface
  • ➕ Avoids adding more validation paths inside the generic Record layer
  • ➖ Duplicates logic across commands and languages/domains
  • ➖ Easy to miss future nullable-const fields added to the schema
  • ➖ Harder to keep aligned with the shared schema contract
2. Full JSON-schema validation on outbound payloads
  • ➕ Generalizes beyond const/enum to richer constraints if adopted later
  • ➕ Keeps validation rules declarative and centralized
  • ➖ Higher runtime overhead and additional dependency surface
  • ➖ Requires bridging schema format to a validator; more moving parts than needed for const-literals

Recommendation: The PR’s approach (thread const metadata from the schema into generated Record specs and validate in the shared Serialization::Record) is the best fit: it is schema-driven, applies uniformly across all generated commands, and prevents off-spec wire traffic without duplicating checks in command code. Per-command validation and full schema validation were plausible but would either drift from the shared schema or add unnecessary complexity for this targeted contract item.

Files changed (7) +65 / -18

Enhancement (2) +38 / -11
bidi_generate.rbThread schema const-literals into generated Record specs +36/-11

Thread schema const-literals into generated Record specs

• Extends the generator IR to include 'const', emits 'const:' in 'Record.define' specs when present, and adds 'leaf_const' to resolve const-literals through alias chains so runtime validation has the correct literal.

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

serialization.rbsAdd RBS signature for const validation helper +2/-0

Add RBS signature for const validation helper

• Adds an RBS method signature for 'validate_const' to match the new runtime validation hook.

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

Bug fix (3) +19 / -7
browsing_context.rbMark bypass param as nullable const-literal +1/-1

Mark bypass param as nullable const-literal

• Updates 'BrowsingContext::SetBypassCSPParameters' to include 'const: true' alongside 'nullable: true', enabling runtime enforcement that the only non-null value is the spec literal.

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

emulation.rbMark enabled param as nullable const-literal (false) +1/-1

Mark enabled param as nullable const-literal (false)

• Updates 'Emulation::SetScriptingEnabledParameters' to include 'const: false' alongside 'nullable: true', ensuring outbound validation correctly handles a falsy literal constant.

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

record.rbAdd runtime validation for nullable const fields +17/-5

Add runtime validation for nullable const fields

• Extends Record field metadata to carry 'const', threads it through field construction, and adds 'validate_const' invoked during outbound validation to raise 'ArgumentError' when a provided value is neither the literal nor null.

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

Tests (1) +7 / -0
serialization_spec.rbTest rejection of non-literal values for literal-or-null params +7/-0

Test rejection of non-literal values for literal-or-null params

• Adds a unit test asserting that 'bypass: false' (must be true or null) and 'enabled: true' (must be false or null) raise 'ArgumentError' before serialization occurs.

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

Other (1) +1 / -0
.rubocop.ymlExclude BiDi Record from ModuleLength metrics +1/-0

Exclude BiDi Record from ModuleLength metrics

• Adds 'lib/selenium/webdriver/bidi/serialization/record.rb' to the RuboCop 'Metrics/ModuleLength' exclusion list to avoid lint failures after the added validation logic.

rb/.rubocop.yml

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

Copilot AI 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.

Pull request overview

This PR tightens the Ruby BiDi serialization layer to locally validate nullable-const outbound parameters (literal-or-null) so off-spec values are rejected with an ArgumentError before any wire round-trip, aligning Ruby with the outbound-validation behavioral contract referenced in #17786.

Changes:

  • Thread { const, nullable } schema information through the Ruby BiDi generator into runtime Record.define specs (const: metadata).
  • Add runtime validation in Serialization::Record to enforce const membership for non-nil outbound values.
  • Add a unit regression spec covering the two affected nullable-const command params.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rb/spec/unit/selenium/webdriver/bidi/serialization_spec.rb Adds regression coverage ensuring invalid literal-or-null values raise locally.
rb/sig/lib/selenium/webdriver/bidi/serialization.rbs Updates RBS signatures to include the new validate_const helper.
rb/lib/selenium/webdriver/bidi/support/bidi_generate.rb Extends generator IR to carry and emit const: metadata for runtime validation.
rb/lib/selenium/webdriver/bidi/serialization/record.rb Implements outbound const validation in Record construction.
rb/lib/selenium/webdriver/bidi/protocol/emulation.rb Regenerates protocol to include const: false for enabled on SetScriptingEnabledParameters.
rb/lib/selenium/webdriver/bidi/protocol/browsing_context.rb Regenerates protocol to include const: true for bypass on SetBypassCSPParameters.
rb/.rubocop.yml Adjusts RuboCop exclusions for the updated record.rb.

@titusfortner
titusfortner merged commit 5548a8a into SeleniumHQ:trunk Jul 24, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants