[js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity#17700
Open
titusfortner wants to merge 4 commits into
Open
[js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity#17700titusfortner wants to merge 4 commits into
titusfortner wants to merge 4 commits into
Conversation
selenium-ci
added
C-nodejs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Summary by QodoAdd binding-neutral BiDi schema generator with cddl2ts fidelity gate Description
Diagram
High-Level Assessment
Files changed (7)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
14 rules 1. Orphaned synthetic variant defs
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
titusfortner
force-pushed
the
project_bidi_schema
branch
from
5a7fca3 to
5ae00f7
Compare
Contributor
|
Code review by qodo was updated up to the latest commit 5ae00f7 |
Contributor
|
Code review by qodo was updated up to the latest commit 421fc38 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a binding-neutral WebDriver BiDi schema artifact (commands/events/types) generated from shared CDDL AST/model data, plus Bazel wiring and tests that gate schema fidelity against cddl2ts to prevent silent field/type loss across bindings.
Changes:
- Introduces a BiDi AST normalizer and schema projector that emit a flat schema with referential-integrity + completeness validation.
- Extends the Bazel BiDi generation pipeline to produce a shared
*_schema.jsonartifact with cross-binding visibility. - Adds mocha tests, including a differential “oracle” check comparing the generated schema to
cddl2tsoutput.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| javascript/selenium-webdriver/project_bidi_schema.mjs | Projects normalized AST + model into a flat schema and validates it (integrity + completeness). |
| javascript/selenium-webdriver/project_bidi_schema_test.mjs | Unit tests for schema projection and validation helpers. |
| javascript/selenium-webdriver/private/generate_bidi.bzl | Adds schema-generation step to the Bazel BiDi pipeline and exposes schema to other bindings. |
| javascript/selenium-webdriver/normalize_bidi_ast.mjs | Normalizes raw CDDL AST into canonical forms (hoisted enums/records, flattened unions/composition). |
| javascript/selenium-webdriver/normalize_bidi_ast_test.mjs | Unit tests for AST normalizer transforms. |
| javascript/selenium-webdriver/BUILD.bazel | Adds project_bidi_schema_script and a mocha test target for schema tooling + fidelity gate. |
| javascript/selenium-webdriver/bidi_schema_diff_test.mjs | Differential test comparing generated schema to cddl2ts as an oracle. |
Contributor
|
Code review by qodo was updated up to the latest commit 7c315df |
2 tasks
Member
|
Shouldn't this be a draft until #17657 gets a 👍? |
Member
Author
|
@diemol #17657 is merged. Do you mean #17701? |
Member
|
Yes, sorry. I meant that one. Thank you for clarifying. From that point of view, then we could move forward on this one. |
Member
Author
|
Working with this code to generate Ruby code surfaced several real projector type-loss bugs (LocalValue date/regexp arms, js-int/js-uint ranges, the ConsoleLogEntry composition drop) which are now fixed and gated at build time |
Comment on lines
+77
to
+81
| function projectRef(type) { | ||
| const all = typeList(type) | ||
| const entries = all.filter((e) => !isNullAlt(e)) | ||
| if (entries.length === 0) return { primitive: 'null', nullable: true } // the type is only `null` | ||
| const node = |
Contributor
There was a problem hiding this comment.
1. projectref() null-only untested 📘 Rule violation ▣ Testability
projectRef() now treats a null/nil-only type as { primitive: 'null', nullable: true }, but
the new behavior is not covered by an automated test, risking regressions in schema fidelity. This
violates the requirement that new bug fixes include corresponding tests with assertions.
Agent Prompt
## Issue description
`projectRef()` has new behavior for types that are only `null`/`nil`, but there is no regression test asserting the projected schema contains `primitive: 'null'` (not `unknown`) and `nullable: true`.
## Issue Context
This is a schema-fidelity bugfix path; without a targeted test it can regress without being noticed.
## Fix Focus Areas
- javascript/selenium-webdriver/project_bidi_schema.mjs[73-88]
- javascript/selenium-webdriver/project_bidi_schema_test.mjs[83-162]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 439dacd |
Contributor
|
Code review by qodo was updated up to the latest commit 8bc4569 |
Member
Author
|
Pushed |
Comment on lines
+284
to
+290
| // No shared discriminator: dispatch by required-field presence, in spec order. | ||
| return { | ||
| ordered: variants.map((ref) => { | ||
| const t = types[ref] | ||
| const requires = t?.kind === 'record' ? t.fields.filter((f) => f.required).map((f) => f.name) : [] | ||
| return { ref, requires } | ||
| }), |
Contributor
There was a problem hiding this comment.
1. Ordered selector skips aliases 🐞 Bug ≡ Correctness
unionSelector() computes selector.ordered[].requires only when the variant is a direct record; alias (and other non-record) variants get requires: [], which makes them match trivially and can cause incorrect structural-union dispatch.
Agent Prompt
### Issue description
`unionSelector()` falls back to a structural selector `{ ordered: [...] }` when no discriminator key works. In that branch it derives `requires` only for variants whose projected kind is `record`; if a variant is an `alias` to a record (or a nested `union`), it gets `requires: []`, which makes that variant always “selectable” and can cause the structural dispatch to pick the wrong arm.
### Issue Context
The same file already treats alias-to-record as a normal part of the union graph (`unionLeaves()` follows `kind: 'alias'`), and tagged-discriminator derivation (`tagContribution`) also follows aliases. The structural selector should be consistent with those paths.
### Fix Focus Areas
- javascript/selenium-webdriver/project_bidi_schema.mjs[200-211]
- javascript/selenium-webdriver/project_bidi_schema.mjs[264-292]
- javascript/selenium-webdriver/project_bidi_schema.mjs[406-427]
- javascript/selenium-webdriver/project_bidi_schema_test.mjs[114-124]
- javascript/selenium-webdriver/project_bidi_schema_test.mjs[234-249]
### Suggested fix
1. When building the structural selector, resolve each immediate variant to its leaf records (reuse `unionLeaves(ref, types)`), and build `ordered` entries from those leaf records (preserving spec order by iterating variants in-order, then their leaves in-order).
2. Compute `requires` from the resolved leaf record’s required fields.
3. Add a unit test where a structural union includes an alias variant (e.g. `x.Alias` is a single-ref alias to `x.A`), and assert the `requires` for that ordered entry reflects `x.A` (not `[]`).
4. (Optional but recommended) Strengthen `checkSelector()` to flag an `ordered` selector entry with `requires: []` when it would shadow later variants (e.g., an empty-requires entry that is not last), since that makes subsequent variants unreachable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 72a51dd |
titusfortner
force-pushed
the
project_bidi_schema
branch
from
72a51dd to
09f7fa7
Compare
Comment on lines
+162
to
166
| # Step 2: parse the merged CDDL once into the reusable AST artifact. Internal | ||
| # input to the schema and the JS generator; not consumed by other bindings. | ||
| ast_target = name + "_ast" | ||
| ast_out = name + "_ast.json" | ||
| js_run_binary( |
Contributor
There was a problem hiding this comment.
1. Ast/model targets made private 📘 Rule violation ≡ Correctness
generate_bidi_library() now makes the generated AST/model artifacts package-private by omitting visibility = _ARTIFACT_VISIBILITY, while still exposing the new schema target. This is a backward-incompatible change for any cross-binding code that depended on those previously-visible artifacts.
Agent Prompt
## Issue description
The PR changes the Bazel visibility contract of the generated BiDi artifacts: `_ast` and `_json` are now package-private (no explicit `visibility`), while `_schema` remains exposed to other bindings. This can break existing (or soon-to-be-written) cross-binding dependencies.
## Issue Context
The file defines `_ARTIFACT_VISIBILITY` for `//java`, `//py`, and `//rb` subpackages, and applies it to the new schema generator output. The AST/model generation steps no longer apply the same visibility, effectively removing them from the cross-binding API surface.
## Fix Focus Areas
- javascript/selenium-webdriver/private/generate_bidi.bzl[162-194]
- javascript/selenium-webdriver/private/generate_bidi.bzl[196-216]
- javascript/selenium-webdriver/private/generate_bidi.bzl[5-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+77
to
+120
| const typeList = (t) => (Array.isArray(t) ? t : t === undefined || t === null ? [] : [t]) | ||
| const isLiteral = (e) => e && typeof e === 'object' && e.Type === 'literal' | ||
| const isRef = (e) => e && typeof e === 'object' && e.Type === 'group' && typeof e.Value === 'string' | ||
|
|
||
| // A `null` keyword or a `nil` prelude ref in a union means the value may be null. | ||
| const isNullAlt = (e) => | ||
| e === 'null' || (e && typeof e === 'object' && e.Type === 'group' && PRELUDE[e.Value] === 'null') | ||
|
|
||
| function projectRef(type) { | ||
| const all = typeList(type) | ||
| const entries = all.filter((e) => !isNullAlt(e)) | ||
| if (entries.length === 0) return { primitive: 'null', nullable: true } // the type is only `null` | ||
| const node = | ||
| entries.length > 1 | ||
| ? entries.every(isLiteral) | ||
| ? { enum: entries.map((e) => e.Value) } | ||
| : { union: entries.map(projectEntry) } | ||
| : projectEntry(entries[0]) | ||
| if (entries.length < all.length) node.nullable = true // a `null` alternative means the value may be null | ||
| return node | ||
| } | ||
|
|
||
| function projectEntry(e) { | ||
| if (typeof e === 'string') return { primitive: PRIMITIVES[e] ?? e } | ||
| if (!e || typeof e !== 'object') return { primitive: 'unknown' } | ||
| // A control operator (`.ge` / `.default` / `.le` …) wraps the real type as | ||
| // `{ Type: <innerType>, Operator: {...} }`; the constraint does not change the | ||
| // type, so project the inner type. | ||
| if (e.Type && typeof e.Type === 'object') return projectEntry(e.Type) | ||
| if (e.Type === 'literal') return { const: e.Value } | ||
| if (e.Type === 'group' && e.Value) return e.Value in PRELUDE ? { primitive: PRELUDE[e.Value] } : { ref: e.Value } | ||
| if (e.Type === 'group' && Array.isArray(e.Properties)) { | ||
| // An inline group that only wraps anonymous ref(s) — e.g. a union arm | ||
| // `{ DateLocalValue }` — is that ref (or a union of them), not a record. | ||
| const refs = unionMemberRefs(e) | ||
| if (refs) return refs.length === 1 ? { ref: refs[0] } : { union: refs.map((r) => ({ ref: r })) } | ||
| return { | ||
| record: e.Properties.flat() | ||
| .filter((p) => p?.Name) | ||
| .map(projectField), | ||
| } | ||
| } | ||
| if (e.Type === 'array') return { list: projectRef(e.Values?.[0]?.Type) } | ||
| if (e.Type === 'map') return { map: projectRef(e.ValueType ?? e.Values?.[0]?.Type), extensible: true } |
Contributor
There was a problem hiding this comment.
3. Undefined types project to null 🐞 Bug ☼ Reliability
projectRef() returns { primitive: 'null', nullable: true } when given an empty type list; because
array/map projection passes potentially-undefined inputs (e.Values?.[0]?.Type, `e.ValueType ??
...`) into projectRef, missing element/value types are mis-projected as null instead of failing
closed. This can let malformed/unhandled AST shapes produce valid-looking schema output and evade
checkSchema’s unknown-type guard.
Agent Prompt
### Issue description
`projectRef()` treats an empty `typeList()` as “null-only type” and returns `{ primitive: 'null', nullable: true }`. Because array/map projection calls `projectRef(e.Values?.[0]?.Type)` / `projectRef(e.ValueType ?? e.Values?.[0]?.Type)`, any missing/empty element/value type is silently mapped to `null` rather than to `unknown` or an error.
### Issue Context
This undercuts the schema gate’s stated “fail-closed” behavior, since `checkSchema()` only flags `primitive === 'unknown'`.
### Fix Focus Areas
- javascript/selenium-webdriver/project_bidi_schema.mjs[77-97]
- javascript/selenium-webdriver/project_bidi_schema.mjs[119-120]
- javascript/selenium-webdriver/project_bidi_schema.mjs[386-415]
Suggested approach:
- Make `projectRef(undefined)` (and `projectRef(null)` when it indicates “missing”, not a CDDL null alternative) project to `{ primitive: 'unknown' }` (or throw) rather than the null-only type.
- One option: change `typeList` to return a sentinel for undefined, or add an explicit guard at the start of `projectRef`:
- `if (type === undefined) return { primitive: 'unknown' }`
- In the `array`/`map` cases, explicitly validate the presence of an element/value type and project `unknown` (so `checkSchema` fails) when absent.
- Add a unit test asserting `projectRef(undefined)` is treated as `unknown` (or that an empty array/map element type fails schema validation) to prevent regression.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 09f7fa7 |
titusfortner
force-pushed
the
project_bidi_schema
branch
from
09f7fa7 to
a87d7b9
Compare
Comment on lines
+332
to
+357
| const memberRefs = detected.branches.map((branch, i) => { | ||
| const entry = branchType(branch) | ||
| if (!entry) return null | ||
| const { fields, label, supersedes } = variantRecord(detected.commonFields, entry, defMap, i) | ||
| const memberLabel = trimRedundantPrefix(owner.local, label) | ||
| const localName = `${owner.local}_${memberLabel}` | ||
| const synthName = alloc(owner.domain ? `${owner.domain}.${localName}` : localName) | ||
| created.push({ | ||
| Type: 'group', | ||
| Name: synthName, | ||
| IsChoiceAddition: false, | ||
| Properties: fields, | ||
| Comments: [], | ||
| 'x-selenium-synthetic': true, | ||
| 'x-selenium-owner': def.Name, | ||
| 'x-selenium-label': memberLabel, | ||
| }) | ||
| if (supersedes) { | ||
| superseded.add(supersedes) | ||
| supersededBy.set(supersedes, synthName) | ||
| } | ||
| return groupRef(synthName) | ||
| }) | ||
|
|
||
| if (memberRefs.some((r) => r === null)) continue // unexpected branch shape: leave def untouched | ||
|
|
Contributor
There was a problem hiding this comment.
1. Orphaned synthetic variant defs 🐞 Bug ≡ Correctness
canonicalizeVariantParams() appends synthesized variant-record defs to created before confirming all choice branches are supported, so a later bailout (continue) can leave behind unreferenced synthetic defs while keeping the original def unchanged. This makes the normalized AST (and therefore the projected schema) depend on branch order and can introduce unexpected extra types.
Agent Prompt
### Issue description
`canonicalizeVariantParams()` mutates output even when it decides not to canonicalize a detected inline-variant record. It pushes synthesized variant defs into `created` during the per-branch map, but if any branch is unsupported it bails out (`continue`) and leaves the original def untouched. This results in orphan synthetic defs being appended to the output AST.
### Issue Context
The current control flow creates defs before validating that *all* branches are supported.
### Fix Focus Areas
- javascript/selenium-webdriver/normalize_bidi_ast.mjs[327-367]
### Suggested fix
Refactor to be atomic per `def`:
1. First, pre-validate and compute all `branchType()` entries for `detected.branches`.
2. If any entry is null/unsupported, do not create any synthesized defs and do not modify `def`.
3. Otherwise, build synthesized defs into a temporary local array (e.g., `newDefs`) and only then append them to `created` and rewrite `def` to `Type='variable'`.
4. Add a unit test covering an inline-variant record where one branch is unsupported, asserting that **no** synthesized defs are added to the output.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit a87d7b9 |
Contributor
|
Code review by qodo was updated up to the latest commit eb5ef4f |
Contributor
|
I think this is the right direction. Building a Java CDDL generator off of this is much better. Looks good to me. Once the bot comments are addressed, let's merge it. |
pujagani
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Builds on #17657 (shared BiDi CDDL ast/model artifacts).
💥 What does this PR do?
Generates a single, binding-neutral BiDi schema (commands + events + types) from the shared CDDL artifacts, so the non-JS bindings can create client generators that consume one explicit, normalized artifact instead of each re-deriving the native CDDL shapes from the raw AST.
That re-derivation is where the bindings currently diverge and silently drop data — inline string-literal enums, variant-union params, composed-in base-type fields, optional/nullable fields. The schema normalizes all of these once (hoists inline enums to named types, canonicalizes variant unions into self-contained records, flattens group composition, preserves wire names and nullability verbatim) and the generation step fails the build if anything is dropped or dangling.
It lands the artifact and its fidelity gate only; no binding consumes it yet.
🔧 Implementation Notes
(A | B) & commonintersection form, because the non-TS bindings can't express TS-style intersections cleanly. This duplicates common fields across variants but keeps each variant complete and the discriminator-conditional rule structural.{ synthetic: true, owner, label }—owneris the type the construct was lifted out of,labelthe member name within it — so a binding can keep the flat name or nest it (Owner::Label) without parsing the synthetic def name. Generation fails the build if a syntheticownerdoesn't resolve.bidi-ast.json/bidi-model.jsonare made package-private (dropping the cross-binding visibility added in [js] Expose BiDi CDDL ast and model as shared artifacts #17657), since the schema folds in the model and supersedes both for bindings. They remain internal inputs to the schema and to the existing JS generation. Nothing consumed the old visibility yet, so this changes a not-yet-used contract.🤖 AI assistance
💡 Additional Considerations
javascript/selenium-webdrivertojavascript/bidi-support, the additional wiring seemed out of scope for this PR🔄 Types of changes