Enforce per-field schema version availability at parse time - #738
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| const firstAppearance = (path) => timeline.find((t) => t.paths.has(path)); | ||
| const atLabel = (label) => timeline.find((t) => t.label === label); | ||
|
|
||
| for (const record of declared) { |
There was a problem hiding this comment.
checkAvailability only iterates declared (records that already carry an x-mxc-since / x-mxc-until). A brand-new field added after the floor with no annotation produces no record, so it is never checked?
| ); | ||
| } else { | ||
| for (const path of record.paths) { | ||
| if (!at.paths.has(path)) { |
There was a problem hiding this comment.
This only confirms the field exists at the until version; it never checks that the field is absent from every later frozen schema. A field still present in a frozen 0.7 schema can declare x-mxc-until:"0.6" and pass here after which the runtime wrongly rejects valid 0.7 configs that use it?
| throw new Error(`schema-version.json: 'min' (${schemaVer.min}) is not a version`); | ||
| } | ||
|
|
||
| const stable = readdirSync(stableDir) |
There was a problem hiding this comment.
discoverTimeline reads schemas/stable/* from the HEAD worktree (readdirSync + readJson off disk), so a PR that edits or adds a stable schema shifts the very timeline the since / until claims are validated against. This is asymmetric with #732/#730, which read the comparison baseline from the base commit via git-base?
| "field": "version", | ||
| "declaredVersion": "", | ||
| "since": null, | ||
| "until": null, |
There was a problem hiding this comment.
nit: for a missing/empty version, the SDK path emits "since": null, "until": null here, while the parser path emits since: MIN_SUPPORTED / until: MAX_SUPPORTED for the equivalent error in config_parser.rs?
| { path: "experimental", why: "declared no properties before 0.8, so anything under it validated vacuously" }, | ||
| { path: "phase", why: "carried by state-aware requests declaring 0.6 since before the schema described it" }, | ||
| { path: "sandboxId", why: "carried by state-aware requests declaring 0.6 since before the schema described it" }, | ||
| { path: "correlationVector", why: "carried by state-aware requests declaring 0.6 since before the schema described it" }, |
There was a problem hiding this comment.
question (non- blocking): for these, the why contains version numbers. Does this mean that we must always update this file ever release? might need to document this somewhere. Might have to update this later, so we don't have to remember to update these everytime.
| ### The consequence: breaking changes are additive | ||
|
|
||
| Because one dev schema has to validate configs declaring *every* supported | ||
| version, the schema cannot be the thing that retires a field — any surface a | ||
| supported version can use has to stay in it. So a breaking change is made | ||
| **additively**: | ||
|
|
||
| 1. Keep the old field, and mark it `until` the last version it was valid in. | ||
| 2. Add the new shape alongside it, marked `since` the version that introduced it. | ||
| 3. Let the declared version decide which one a given config may use. | ||
|
|
||
| This is also why the dev-schema compatibility gate | ||
| (`check-dev-schema-compat.js`) has no per-field escape hatch: a removal is | ||
| always the wrong shape for a change, not an exception to be waived. Deleting an | ||
| `until`-marked field is legitimate only once the supported floor rises past its | ||
| `until` value. | ||
|
|
There was a problem hiding this comment.
note (non- blocking): I think having this
This is also why the dev-schema compatibility gate (
check-dev-schema-compat.js) has no per-field escape hatch: a removal is always the wrong shape for a change, not an exception to be waive
where we talk about removal being wrong and then saying this right after
Deleting an
until-marked field is legitimate only once the supported floor rises past itsuntilvalue.
is confusing, at least to me. I'd just keep the second part and remove the first since future folks when reading this part will care more about the how does versioning work rather than why do we do versioning this way.
| A derive is required rather than `#[schemars(extend(...))]`: the schemars | ||
| attributes sit behind the `schema-gen` feature, which only `mxc_schema_gen` | ||
| enables, so they are invisible to the parser. An annotation nothing enforces is | ||
| documentation, not a contract. |
There was a problem hiding this comment.
note (non- blocking): probably can remove. Doesn't provide good value like the two paragraphs above. In the future I think we need to separate this doc into 2. One for why versioning is done this way and how to update version. First one would be good for us and new devs coming to the project. and second one would be good for quickly understanding what we need to do at a glance to publish a new version.
| **Put the range on the containing field, not inside a shared struct.** A | ||
| struct reached from two places is *one* node, so a range on its fields applies | ||
| to every path that reaches it. `Seatbelt`, for example, is both the top-level | ||
| `seatbelt` section and `experimental.seatbelt`; the range therefore lives on | ||
| `MxcConfig::seatbelt`. The oracle gate refuses any range that would leak onto | ||
| the `experimental` surface this way. |
There was a problem hiding this comment.
thought (non- blocking): re-read this a few times to wrap my head around it but still not quite sure I understand what happens for fields in experimental.seatbelt since there is only one rust struct MxcConfig::seatbelt.
|
|
||
| | Gate | What it protects | | ||
| |---|---| | ||
| | `check-version-availability.js` | Each `since` matches the field's true first appearance across the frozen 0.6 / 0.7 and dev schemas; each `until` names a version the field really existed in. Fail-closed under `experimental`. | |
There was a problem hiding this comment.
note (non- blocking): for this I think the wording should be updated to be more general e.g maybe instead of "across the frozen 0.6 / 0.7 and dev schemas" we just say "across the stable schemas".
|
|
||
| [Fact] | ||
| public void Spawn_MalformedPolicy_ThrowsMalformedRequest() | ||
| public void Spawn_VersionlessPolicy_ThrowsVersionIncompatible() |
There was a problem hiding this comment.
note (non-blocking): test seems like it's a duplicate of the Run_VersionlessPolicy_ThrowsVersionIncompatible test in sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcSandboxTests.cs
| // Regression (review F2): the darwin builder synthesises a top-level | ||
| // `seatbelt` block purely as a marker. That block carries a `since: 0.7` | ||
| // range natively, so emitting it for a 0.6 policy made the executor reject a | ||
| // field the caller never supplied — every 0.6 macOS policy failed. | ||
| // | ||
| // Asserted through the exported predicate because buildDarwinProcessConfig | ||
| // only runs on darwin; testing it only there is what let the bug through. |
There was a problem hiding this comment.
note (non-blocking): looks like this comment is still referring to a local AI code review perhaps?
| // Previously this surfaced as the generic `MalformedRequest`, because | ||
| // `build_request` wrapped every loader error. Version problems now carry | ||
| // their own code and structured details so a caller can act on them without | ||
| // parsing the message. |
There was a problem hiding this comment.
note (non-blocking): comments here can be removed since it's talking about what the test did in the past.
| // Preserve the loader's typed error: flattening to `malformed_request` | ||
| // would deny the code and details to every one-shot SDK/FFI/C# caller. |
There was a problem hiding this comment.
note (non-blocking): 50/50 on whether this one is a helpful or not to future folks. It tells us why we're changing it from malformed_request but that's it.
From reading the code I can see that what is being returned by load_request_from_value as an error will get converted to an mxc error and we'll early return from the function. That tells me already we want to preserve the error from load_request_from_value so wouldn't need to have that comment.
| ? "" | ||
| : ` (dev line moved ${baseVersions.devSchemaFile} -> ${headVersions.devSchemaFile})`; | ||
|
|
||
| const findings = detectBreaking(baseSchema, headSchema); |
There was a problem hiding this comment.
This gate advertises a floor-bump escape hatch it doesn't implement. detectBreaking(baseSchema, headSchema) compares the complete schemas; neither baseVersions.min nor headVersions.min scopes it, so a removal is blocked unconditionally- even in a PR that raises the floor.
Fixes:
• A (now): make the message honest- drop "or move the supported-availability range in the same change" from L98 and correct the L17–20 comment to say removals are always blocked and window-based retirement isn't implemented yet.
• B (real fix): when headVersions.min > baseVersions.min , scope the comparison to surface still supported after the new floor — prune base nodes whose x-mxc-until < headMin before detectBreaking (watch $ref -shared definitions so a retired field sharing a type with a live one doesn't over-prune), plus the integration test that raises min and removes retired surface. Note there's no x-mxc-until surface today, so B needs a synthetic fixture to exercise it.
5bf77ac to
bafbd1f
Compare
|
sigh, used the new gh stack commands to try fixing the merge conflicts but looks like it "merged" this into the stack itself. Will see how to undo. It's not actually merged, just merged into what Github considered the stack. Also I guess the github stack feature some how overrides our ruleset which is slightly concerning that a simple "gh stack push" could cause this. |
Important
Superseded by #775. While resolving a merge conflict using rebase with the new gh stack workflow, this branch was briefly pushed with no commits beyond its stack base, so GitHub recorded this PR as merged. #775 restores the original change as the fourth PR in the stack.
Summary
This PR adds per-field schema version availability: a wire field can declare the range of config schema versions it is valid in, and the parser rejects any use outside that range.
The governing requirement is that a breaking config-schema change must be possible without dropping support for an earlier version, where "support" means shape only — an older config keeps parsing and is enforced with today's semantics. #732 blocks the dev schema from accepting less than it did, which forces breaking changes to be additive; this PR supplies the other half. Without parse-time enforcement, an availability annotation is documentation that nothing honours.
Details
mxc_version_deriveproc-macro crate.#[derive(VersionAvailability)]lifts#[mxc_version(since = "0.8")]/untiloffwire.rsinto metadata normal builds carry, so one declaration feeds both the parser and schema generation (published asx-mxc-since/x-mxc-until). It has to be a derive:#[schemars(extend(...))]sits behind theschema-genfeature, which onlymxc_schema_genenables, so it can annotate the schema but can never be consulted by the parser.flatten, splitrename/rename_all, unrecognisedrename_allrules, data-carrying variants, malformed literals), and a conformance test cross-checks all 32 wire types against the property namesschemarsderives independently from the same attributes.convert_wire_configmoves fields out of the config and the document can no longer be checked as a whole. State-aware requests are gated on the original document, not the experimental-masked copy.versionis now required. It selects the legal field surface, so an absent one would silently opt out of every range rather than defaulting to something safe.version_incompatibleerror code across all five coupled surfaces (RustMxcErrorCode, engineErrorCode, TS, C#,MXC_STATUS_VERSION_INCOMPATIBLE = 13) carrying structureddetails: { field, declaredVersion, since, until }. The supported-range error migrates onto it, so one code covers both classes.seatbeltsince 0.7,processContainer.captureDenialsandprocessContainer.learningModesince 0.8.check-version-availability.jsoracle gate derives each field's true first appearance from the frozen 0.6/0.7 and dev schemas and fails when a declaredsincedisagrees.Invoke-StateAware*helper rather than at ~20 call sites), and the SDK config builders.Tests
cargo fmt --all -- --check,cargo check --workspace --all-targets,cargo clippy --workspace --all-targets -- -D warnings, and the per-package test suites all clean on the rebased tip.wxc_common{schema-gen, microvm},mxc_ffi{dotnetsdk},mxc_engine{isolation_session},wxc{isolation_session, microvm, tier2_bfs, wslc, hyperlight}.SUPPORTED_VERSIONunchanged at>=0.6, <=0.8.Notes for reviewers
Annotation is opt-in, and "unannotated" means no claim, not "since 0.6". An unannotated field is unbounded, which today is indistinguishable from
since: 0.6, until: 0.8because the supported-range check already rejects everything outside that window — but it becomes observable when the range moves, and unbounded is what lets fields keep working as a new dev line opens.The most important part of this PR is what is deliberately not annotated. A field's first appearance in the JSON Schema is only a lower bound on how long it has been accepted:
experimentaldeclared no properties before 0.8, so anything under it validated vacuously and has always been accepted.0.6.0-alphawhile carryingphase/sandboxId/correlationVector, which the schema only described from 0.8.Deriving bounds from schema data alone would have approved
since: 0.8onphaseand rejected every state-aware request ever sent. Measured: 66 properties are unannotated yet absent from the 0.6 schema — 8 are covered transitively by an annotated ancestor (the walker checks a field's range before descending, so a rejected parent is never traversed into), and the rest legitimately carry none. The oracle gate is therefore fail-closed on those surfaces: it refuses a declaration it cannot justify rather than checking it against a bound that would be wrong.corpus_parses.rsis the behavioural counter-check the oracle structurally cannot provide.Put the range on the containing field, never inside a shared struct.
Seatbeltis reachable from both the top-levelseatbeltsection andexperimental.seatbelt— it is one node, so a range on its inner fields would leak onto the unconstrained experimental surface. The oracle gate catches this class automatically.One deliberate observable break: migrating the supported-range error onto
version_incompatiblechanges an existing error's shape, so a consumer string-matching the old "older/newer than supported" message is affected. This was flagged and accepted in review.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-targetsis clean formxc_engine,wxc_commonandmxc-sdk, including the newcfg(target_os = "macos")regression tests — but it has not been run.mxc_darwincannot be cross-checked at all, for the pre-existing reason in #735.Microsoft Reviewers: Open in CodeFlow