[js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity#17700
[js] Add binding-neutral BiDi schema with cddl2ts-gated fidelity#17700titusfortner wants to merge 6 commits into
Conversation
PR Summary by QodoAdd binding-neutral BiDi schema generator with cddl2ts fidelity gate Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
14 rules 1. Ordered selector skips aliases
|
Normalize the parsed BiDi AST and project a flat, binding-neutral schema
(commands + events + types) for the generated Ruby/Java/Python clients,
so each binding consumes one explicit artifact instead of re-deriving the
awkward CDDL shapes from the raw AST.
Normalizer (normalize_bidi_ast.mjs): hoist inline string-literal unions to
named enums, canonicalize variant-union params into self-contained variant
records, hoist inline records, and flatten group composition (base types +
Extensible). Projector (project_bidi_schema.mjs): map to a small vocabulary
(record/enum/union/alias + ref/primitive/const/list/map), preserving wire
names and nullability verbatim; generation validates and fails on dangling
refs or dropped commands/events.
Fidelity is gated against cddl2ts, an independent generator over the same
AST, comparing record fields, field types (optional/nullable/array) and
enum values; only one intentional difference remains (wire-faithful
namespaceURI). Wired as Bazel targets create-bidi-src_schema (output name
derived from the target) and the bidi-schema-{tests,diff-test}.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5a7fca3 to
5ae00f7
Compare
|
Code review by qodo was updated up to the latest commit 5ae00f7 |
|
Code review by qodo was updated up to the latest commit 421fc38 |
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. |
|
Code review by qodo was updated up to the latest commit 7c315df |
|
Shouldn't this be a draft until #17657 gets a 👍? |
|
@diemol #17657 is merged. Do you mean #17701? |
|
Yes, sorry. I meant that one. Thank you for clarifying. From that point of view, then we could move forward on this one. |
…l types; gate unknown and empty-record projections
|
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 |
| 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 = |
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
|
Code review by qodo was updated up to the latest commit 439dacd |
|
Code review by qodo was updated up to the latest commit 8bc4569 |
|
Pushed |
| // 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 } | ||
| }), |
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
|
Code review by qodo was updated up to the latest commit 72a51dd |
🔗 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