Skip to content

Block breaking changes to the dev schema at pull-request time - #732

Open
Gudge (MGudgin) wants to merge 1 commit into
user/gudge/versioning_phase7c_detector_libfrom
user/gudge/versioning_phase7d_dev_schema_gate
Open

Block breaking changes to the dev schema at pull-request time#732
Gudge (MGudgin) wants to merge 1 commit into
user/gudge/versioning_phase7c_detector_libfrom
user/gudge/versioning_phase7d_dev_schema_gate

Conversation

@MGudgin

@MGudgin Gudge (MGudgin) commented Aug 1, 2026

Copy link
Copy Markdown
Member

Stacked on #731. Base is user/gudge/versioning_phase7c_detector_lib; review the top commit only.

Summary

This PR adds a CI gate that compares the dev schema at the pull-request base against the dev schema at HEAD and fails when the new one rejects an instance the old one accepted.

Every other breaking-change guard compares RELEASED stable schemas, and only at release time. The surface a pull request actually edits -- the dev schema -- is unguarded, so a change can delete a stable field, regenerate the schema and the SDK types, migrate the config corpus, and merge green. PR #676 did exactly that, and was reverted by hand.

Details

  • scripts/versioning/check-dev-schema-compat.js resolves the base commit with the fail-closed helper, reads both dev schemas out of git, and reports every structural restriction the compatibility detector finds.
  • Each side is read at its own declared devSchemaFile. Opening a new dev line copies the outgoing one, so the documents stay the same lineage and the comparison holds across that transition. Skipping the comparison when the line moves would let a change disable the gate by editing one line of schemas/schema-version.json.
  • A missing or unparsable schema on either side fails. The gate is only useful if it cannot succeed vacuously.
  • There is no per-field escape hatch. The supported-version window is what allows surface to end, so until a change moves that window, a config declaring an already-supported version has to keep parsing.
  • Documented in .github/copilot-instructions.md alongside the other schema gates, including how to make a breaking change additively, since this gate is what a contributor meets when they try to remove surface.
  • Runs ahead of corpus validation, because a change that removes a field also migrates the corpus; validation then passes and the removal is what needs reporting.

Tests

  • 8 end-to-end tests drive the real CLI against throwaway repositories and assert on its exit code: unchanged and additive schemas pass; a removed property, a narrowed type, a missing schema and an unparsable schema all exit 1; a compatible new dev line passes and reports the move; and an incompatible new dev line is still blocked.
  • Replayed against PR Bring network wire schema to full GA spec (wire.rs + config fixtures only) #676: the gate exits 1 and names all six removed network fields.
  • Run against the repository as it stands, the gate passes, as do check-schema-versions.js and corpus validation across 195 configs.
  • Full versioning suite: 71 tests.

Note for reviewers

The gate blocks the dev schema accepting less than it did, which is not the same as blocking breaking changes. Because one dev schema validates configs declaring every supported version, surface a supported version can use has to remain in it. A breaking change is made additively -- keep the old fields, add the new shape alongside -- and the supported-version window (a later phase) governs which may be used at which version. Deleting becomes legitimate only once min rises past the surface being dropped.

Microsoft Reviewers: Open in CodeFlow

@MGudgin
Gudge (MGudgin) requested a review from a team as a code owner August 1, 2026 19:08
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI review requested due to automatic review settings August 1, 2026 19:15
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase7d_dev_schema_gate branch from 62679f7 to 8977c18 Compare August 1, 2026 19:15

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

Adds a CI gate to prevent the development schema from becoming structurally more restrictive.

Changes:

  • Adds the compatibility gate and eight integration tests.
  • Runs the gate before corpus validation.
  • Documents the additive schema-evolution policy.
Show a summary per file
File Description
scripts/versioning/check-dev-schema-compat.js Implements base-to-HEAD comparison.
scripts/versioning/tests/dev-schema-gate-integration.test.js Adds end-to-end gate coverage.
scripts/versioning/package.json Exposes the gate as an npm script.
.github/workflows/Versioning.Checks.Job.yml Adds the CI check.
.github/copilot-instructions.md Documents compatibility requirements.

Review details

  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

? ""
: ` (dev line moved ${baseVersions.devSchemaFile} -> ${headVersions.devSchemaFile})`;

const findings = detectBreaking(baseSchema, headSchema);
Copilot AI review requested due to automatic review settings August 3, 2026 20:11
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase7d_dev_schema_gate branch from 8977c18 to 04ca0fe Compare August 3, 2026 20:11
Gudge (MGudgin) pushed a commit that referenced this pull request Aug 3, 2026
This PR adds per-field schema version windows: a wire field can declare the
range of config schema versions it is valid in, and the parser rejects any use
outside that range. It is what makes shape-only support for older schema
versions real — until now a window annotation would have been documentation
that nothing honoured.

Details

* New `mxc_version_derive` proc-macro crate. `#[derive(VersionWindows)]` lifts
  `#[mxc_version(since = "0.8")]` / `until` off `wire.rs` into metadata that
  normal builds carry, so one declaration feeds both the parser and schema
  generation (published as `x-mxc-since` / `x-mxc-until`). A derive is required
  rather than `#[schemars(extend(...))]`, which sits behind the `schema-gen`
  feature and is invisible to the parser.
* The mechanism fails **open** if a derived JSON name ever disagrees with what
  serde accepts — the window simply never fires — so that case is guarded twice:
  the macro compile-errors on every serde construct it cannot model exactly
  (`flatten`, split `rename`/`rename_all`, unknown `rename_all`, data-carrying
  variants), and a conformance test cross-checks all 32 wire types against the
  names `schemars` independently derives.
* The gate runs immediately after deserialisation in every entry point, because
  `convert_wire_config` moves fields out of the config; state-aware requests are
  gated on the original document, not the experimental-masked copy.
* `version` is now **required** — it selects the legal field surface, so an
  absent one would silently opt out of every window.
* New `version_incompatible` code across all five surfaces (Rust `MxcErrorCode`,
  engine `ErrorCode`, TS, C#, `MXC_STATUS_VERSION_INCOMPATIBLE = 13`) carrying
  `details: { field, declaredVersion, since, until }`. The supported-range error
  migrates onto it. NOTE: this changes an existing error's observable shape — a
  consumer string-matching the old range message is affected.
* Three annotations, each checked against real corpus usage first: `seatbelt`
  since 0.7, `processContainer.captureDenials` / `learningMode` since 0.8. The
  central subtlety is what is deliberately **not** annotated: schema-first-
  appearance is only a lower bound on accepted surface. `experimental` was an
  open block before 0.8, and state-aware requests declare 0.6 while carrying
  `phase` / `sandboxId` / `correlationVector` — annotating those from schema
  data would reject configs that have always worked.
* New `check-version-windows.js` oracle gate derives each field's true first
  appearance from the frozen 0.6/0.7 and dev schemas and fails on disagreement.
  It is fail-closed on the roots above, which also catches windows that would
  leak onto the permissive `experimental` surface via a shared type.
* Corpus and callers migrated: 61 configs versioned (state-aware to 0.6.0-alpha,
  matching what the SDK emits; one-shot to 0.8.0-alpha), ~200 Rust test
  literals, the PowerShell lifecycle helpers (stamped centrally), and the SDK
  builders, which no longer synthesise a top-level `seatbelt` marker below 0.7.

Tests

* On the squashed tip: `cargo fmt --all -- --check`,
  `cargo check --workspace --all-targets`,
  `cargo clippy --workspace --all-targets -- -D warnings` and
  `cargo test --workspace` all clean.
* Feature-gated builds covering every flag this diff can reach, all clean:
  `wxc_common` {schema-gen, microvm}, `mxc_ffi` {dotnetsdk}, `mxc_engine`
  {isolation_session}, `wxc` {isolation_session, microvm, tier2_bfs, wslc,
  hyperlight}.
* `wxc_common` 594 unit tests plus a new corpus test asserting all 195 configs
  declare a version and still parse, with the out-of-range fixture pinned as a
  negative case by exact code and bounds. Versioning gate tests 71 → 94.
* Node SDK build + 223 tests; C# SDK 35 tests; ErrorCode parity 17 codes;
  bindings codegen OK. All 10 CI gates pass.
* Non-regression: the PR #676 replay still yields exactly 6 findings naming
  `allowLocalNetwork`, `allowedHosts`, `blockedHosts`, `defaultPolicy`,
  `enforcementMode`, `proxy`; detector baselines hold (dev vs dev = 0,
  0.6→0.7 = 6, 0.7→dev = 12); #732's gate passes; `SUPPORTED_VERSION` unchanged
  at `>=0.6, <=0.8`.
* Converged through a 2-round adversarial review (14 findings; 12 fixed, 1
  pushback accepted, 1 pre-existing). Two blockers were genuine test failures an
  earlier verification pass had masked with a faulty grep.
* **Not executed on this host** (Windows): the macOS Seatbelt paths, the Windows
  Sandbox and IsolationSession PowerShell lifecycle suites, and the host-gated
  MicroVM / Hyperlight E2E configs. The macOS code does **cross-compile** —
  `cargo check --target aarch64-apple-darwin --all-targets` is clean for
  `mxc_engine`, `wxc_common` and `mxc-sdk`, including the new
  `cfg(target_os = "macos")` regression tests — but it has not been run.
  (`mxc_darwin` fails that cross-check on a pre-existing `build.rs` issue
  unrelated to this change: it embeds Windows version info unconditionally.
  Reproduced at the base commit.)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 21bb36ae-131a-4ab6-b062-a830ba488428
Generated-with: claude-opus-5

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.

Review details

Suppressed comments (2)

scripts/versioning/check-dev-schema-compat.js:89

  • The gate never reads or compares versions.min, so advancing the supported floor does not make an expired field removable: detectBreaking will still report that deletion and fail. This contradicts the remediation printed below (and the documented policy that deletion becomes legitimate after min passes the field's window), leaving no working retirement path. Please make the comparison version-window-aware and add an integration case where min advances past an x-mxc-until field, or stop claiming that moving the window permits deletion.
const findings = detectBreaking(baseSchema, headSchema);

.github/copilot-instructions.md:143

  • This new blanket rule conflicts with the existing experimental-schema contract in docs/versioning.md:120-125 and src/core/wxc_common/src/wire.rs:430-434, which says the experimental surface is in flux and may iterate without a stable compatibility promise. Because the gate compares the complete schema, narrowing a known experimental field now fails CI. Either scope compatibility enforcement to the stable surface or update the versioning policy and contributor documentation to explicitly establish the stronger experimental guarantee.
- **Dev schema compatibility**: `scripts/versioning/check-dev-schema-compat.js` is a CI gate that compares the dev schema at the pull-request base against the dev schema at HEAD and **fails on any structural restriction** — a removed property, a new `required`, a narrowed `type`, a tightened bound. There is no per-field escape hatch. Because one dev schema validates configs declaring every supported version, surface a supported version can use has to stay in it. Make a breaking change **additively**: keep the old fields, add the new shape alongside them, and let the supported-version window govern which may be used. Deleting is legitimate only once `min` in `schemas/schema-version.json` rises past the surface being dropped.
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 3, 2026 21:06
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase7d_dev_schema_gate branch from 04ca0fe to 2b42c50 Compare August 3, 2026 21:06

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.

Review details

Suppressed comments (1)

scripts/versioning/check-dev-schema-compat.js:99

  • Raising min cannot actually make this remediation succeed. The gate passes only the two schema documents to detectBreaking; it never reads either min, and the detector explicitly ignores x-mxc-until. Therefore deleting an expired field still produces the same finding even if this change raises the supported-version window (and a follow-up PR also compares against a base schema that still contains it), so the new gate permanently prevents the documented deletion path. Please make the comparison account for fields whose until is below the new minimum, or define another safe baseline-rotation mechanism, and cover that lifecycle with an integration test.
    `Configs declaring an already-supported version must keep parsing. Add ` +
      `surface instead of removing it, or move the supported-version window ` +
      `in the same change.`,
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

This PR adds a CI gate that compares the dev schema at the pull-request base
against the dev schema at HEAD and fails when the new one rejects an instance
the old one accepted.

Every other breaking-change guard compares RELEASED stable schemas, and only at
release time. The surface a pull request actually edits -- the dev schema -- is
unguarded, so a change can delete a stable field, regenerate the schema and the
SDK types, migrate the config corpus, and merge green. PR #676 did exactly
that, and was reverted by hand.

Details

* `scripts/versioning/check-dev-schema-compat.js` resolves the base commit with
  the fail-closed helper, reads both dev schemas out of git, and reports every
  structural restriction the compatibility detector finds.
* Each side is read at its own declared `devSchemaFile`. Opening a new dev line
  copies the outgoing one, so the documents stay the same lineage and the
  comparison holds across that transition. Skipping the comparison when the
  line moves would let a change disable the gate by editing one line of
  `schemas/schema-version.json`.
* A missing or unparsable schema on either side fails. The gate is only useful
  if it cannot succeed vacuously.
* There is no per-field escape hatch. The supported-version window is what
  allows surface to end, so until a change moves that window, a config
  declaring an already-supported version has to keep parsing.
* Documented in `.github/copilot-instructions.md` alongside the other schema
  gates, including how to make a breaking change additively, since this gate is
  what a contributor meets when they try to remove surface.
* Runs ahead of corpus validation, because a change that removes a field also
  migrates the corpus; validation then passes and the removal is what needs
  reporting.

Tests

* 8 end-to-end tests drive the real CLI against throwaway repositories and
  assert on its exit code: unchanged and additive schemas pass; a removed
  property, a narrowed type, a missing schema and an unparsable schema all
  exit 1; a compatible new dev line passes and reports the move; and an
  incompatible new dev line is still blocked.
* Replayed against PR #676: the gate exits 1 and names all six removed
  `network` fields.
* Run against the repository as it stands, the gate passes, as do
  `check-schema-versions.js` and corpus validation across 195 configs.
* Full versioning suite: 71 tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cd48fff2-bde9-487a-ab67-012e9bbc0796
Generated-with: claude-opus-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants