Skip to content

Commit 804f08d

Browse files
adpa-msCopilot
andauthored
Stop the dev schema advertising a stop/deprovision config for IsolationSession (#683)
* Stop the dev schema advertising a stop/deprovision config for IsolationSession `wire::IsolationSession` reused a single `IsolationSessionPhase` for all four per-phase state-aware slots, but the backend's `StatefulSandboxBackend` impl declares `StopConfig`/`DeprovisionConfig`/`ExecConfig` as `()`. The generated dev schema and SDK wire types therefore advertised an optional `user` payload for `stop`/`deprovision` that `deserialize_config` rejects at dispatch. Drop the two fields from the wire model and regenerate both artifacts. The SDK never emitted those slots (it lifts `version` to the envelope top level), so only a hand-authored raw-JSON caller reading the schema could be misled; there is no behavior change. Add two regression tests in the iso backend, where both halves of the contract are visible: one pins the wire model's per-phase key set (built field-by-field, so a newly added field breaks the build instead of silently regenerating), the other pins that the `()` config phases reject a payload. The existing codegen gate only proves the artifacts match the wire model, not that the wire model matches the backend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f3b1916-3f39-4dcc-a420-2ce5db5898bd * Address review O1/O2: precise exec wording, pin the wire->config direction O1 — the doc comment on `wire::IsolationSession` grouped `exec` with `stop` and `deprovision` as "invoked via the top-level `phase` field with `sandboxId`". That is correct for stop/deprovision and incomplete for exec, which also requires the top-level `process` block: `validate_exec_common` rejects an empty `process.commandLine` as `malformed_request`, and the dev schema root carries no `required` array, so this description is the only in-schema guidance a hand-authored caller gets. Separate the two cases and regenerate both artifacts, since the text is copied verbatim into each. O2 — the parity tests pinned only the negative direction (the advertised key set, and that the `()` phases reject a payload), while the section comment claimed they pinned both halves. Add `phases_with_a_config_accept_the_wire_payload`, which derives its payload from `wire::IsolationSessionPhase` rather than a JSON literal and asserts the user bundle survives into `ProvisionConfig` and `StartConfig`. That closes a real gap rather than restating existing coverage: on the state-aware path the wire model is never constructed — the dispatcher deserializes raw JSON straight into the config types — so the `From<crate::wire::IsolationUser>` compile-time guard protects only the one-shot path. A rename of the wire `user` key would leave both config types (`#[serde(default)]`, no `deny_unknown_fields`) silently dropping the bundle, provisioning a local sandbox for a caller who asked for an Entra one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f3b1916-3f39-4dcc-a420-2ce5db5898bd --------- Co-authored-by: adpa-ms <> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f3b1916-3f39-4dcc-a420-2ce5db5898bd
1 parent b2c438a commit 804f08d

4 files changed

Lines changed: 101 additions & 38 deletions

File tree

schemas/dev/mxc-config.schema.0.8.0-dev.json

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -260,19 +260,8 @@
260260
"type": "object"
261261
},
262262
"IsolationSession": {
263-
"description": "IsolationSession backend config. Carries the one-shot `user` field and the per-phase state-aware nesting (`provision` / `start` / `stop` / `deprovision`).",
263+
"description": "IsolationSession backend config. Carries the one-shot `user` field and the per-phase state-aware nesting for the phases that take config (`provision` / `start`). `stop`, `deprovision`, and `exec` take no per-phase config payload: `stop` and `deprovision` are invoked with only the top-level `phase` and `sandboxId`, and `exec` additionally carries the top-level `process` block.",
264264
"properties": {
265-
"deprovision": {
266-
"anyOf": [
267-
{
268-
"$ref": "#/definitions/IsolationSessionPhase"
269-
},
270-
{
271-
"type": "null"
272-
}
273-
],
274-
"description": "State-aware deprovision-phase configuration."
275-
},
276265
"provision": {
277266
"anyOf": [
278267
{
@@ -295,17 +284,6 @@
295284
],
296285
"description": "State-aware start-phase configuration."
297286
},
298-
"stop": {
299-
"anyOf": [
300-
{
301-
"$ref": "#/definitions/IsolationSessionPhase"
302-
},
303-
{
304-
"type": "null"
305-
}
306-
],
307-
"description": "State-aware stop-phase configuration."
308-
},
309287
"user": {
310288
"anyOf": [
311289
{

sdk/node/src/generated/wire.ts

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,9 @@ export interface Filesystem {
107107
}
108108

109109
/**
110-
* IsolationSession backend config. Carries the one-shot `user` field and the per-phase state-aware nesting (`provision` / `start` / `stop` / `deprovision`).
110+
* IsolationSession backend config. Carries the one-shot `user` field and the per-phase state-aware nesting for the phases that take config (`provision` / `start`). `stop`, `deprovision`, and `exec` take no per-phase config payload: `stop` and `deprovision` are invoked with only the top-level `phase` and `sandboxId`, and `exec` additionally carries the top-level `process` block.
111111
*/
112112
export interface IsolationSession {
113-
/**
114-
* State-aware deprovision-phase configuration.
115-
*/
116-
deprovision?: IsolationSessionPhase | null;
117113
/**
118114
* State-aware provision-phase configuration.
119115
*/
@@ -122,10 +118,6 @@ export interface IsolationSession {
122118
* State-aware start-phase configuration.
123119
*/
124120
start?: IsolationSessionPhase | null;
125-
/**
126-
* State-aware stop-phase configuration.
127-
*/
128-
stop?: IsolationSessionPhase | null;
129121
/**
130122
* Optional Entra cloud-agent user bundle (one-shot).
131123
*/

src/backends/isolation_session/common/src/state_aware.rs

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,100 @@ mod tests {
314314
);
315315
}
316316

317+
// ====== Wire-model / backend config parity ======
318+
319+
// The generated JSON schema (`schemas/dev/`) and the SDK wire types
320+
// (`sdk/node/src/generated/wire.ts`) are both emitted from
321+
// `wxc_common::wire::IsolationSession`, while the phases that actually
322+
// accept a config are the associated types on the impl above. On the
323+
// state-aware path the wire model is never constructed — the dispatcher
324+
// deserializes raw JSON straight into those associated types — so nothing
325+
// couples the two at compile time. The tests below pin that contract from
326+
// both directions: the key set the wire model advertises, that the `()`
327+
// phases reject a payload, and that the phases which do take one still
328+
// accept the payload the wire model describes.
329+
330+
#[test]
331+
fn wire_model_nests_config_only_for_phases_that_take_one() {
332+
// Field-by-field construction is deliberate: adding a per-phase field
333+
// to the wire struct breaks this test's compilation, forcing a
334+
// decision about whether the backend honors it.
335+
let wire = wxc_common::wire::IsolationSession {
336+
user: None,
337+
provision: None,
338+
start: None,
339+
};
340+
let value = serde_json::to_value(&wire).unwrap();
341+
let mut keys: Vec<&str> = value
342+
.as_object()
343+
.unwrap()
344+
.keys()
345+
.map(String::as_str)
346+
.collect();
347+
keys.sort_unstable();
348+
assert_eq!(
349+
keys,
350+
["provision", "start", "user"],
351+
"wire model nests a per-phase config for a phase the backend takes none for"
352+
);
353+
}
354+
355+
#[test]
356+
fn phases_without_a_config_reject_a_payload() {
357+
type StopConfig = <IsolationSessionRunner as StatefulSandboxBackend>::StopConfig;
358+
type DeprovisionConfig =
359+
<IsolationSessionRunner as StatefulSandboxBackend>::DeprovisionConfig;
360+
type ExecConfig = <IsolationSessionRunner as StatefulSandboxBackend>::ExecConfig;
361+
362+
// These are `()`, which deserializes only from null, so any object in
363+
// the slot is a hard error at dispatch.
364+
let payload = serde_json::json!({ "user": { "upn": "a@b.com", "wamToken": "t" } });
365+
assert!(
366+
serde_json::from_value::<StopConfig>(payload.clone()).is_err(),
367+
"stop accepted a config payload"
368+
);
369+
assert!(
370+
serde_json::from_value::<DeprovisionConfig>(payload.clone()).is_err(),
371+
"deprovision accepted a config payload"
372+
);
373+
assert!(
374+
serde_json::from_value::<ExecConfig>(payload).is_err(),
375+
"exec accepted a config payload"
376+
);
377+
}
378+
379+
#[test]
380+
fn phases_with_a_config_accept_the_wire_payload() {
381+
type ProvisionConfig = <IsolationSessionRunner as StatefulSandboxBackend>::ProvisionConfig;
382+
type StartConfig = <IsolationSessionRunner as StatefulSandboxBackend>::StartConfig;
383+
384+
// Derive the payload from the wire type instead of a JSON literal: the
385+
// wire model is only the schema source on this path, so a serde rename
386+
// on either side would go unnoticed. Both config types are
387+
// `#[serde(default)]` with no `deny_unknown_fields`, so a renamed key
388+
// does not error — it drops the bundle and provisions a local sandbox
389+
// for a caller who asked for an Entra one.
390+
let phase = wxc_common::wire::IsolationSessionPhase {
391+
user: Some(wxc_common::wire::IsolationUser {
392+
upn: "alice@contoso.com".to_string(),
393+
wam_token: "tok".to_string(),
394+
}),
395+
};
396+
let payload = serde_json::to_value(&phase).unwrap();
397+
398+
let provision: ProvisionConfig = serde_json::from_value(payload.clone()).unwrap();
399+
let u = provision
400+
.user
401+
.expect("provision dropped the wire user bundle");
402+
assert_eq!(u.upn, "alice@contoso.com");
403+
assert_eq!(u.wam_token, "tok");
404+
405+
let start: StartConfig = serde_json::from_value(payload).unwrap();
406+
let u = start.user.expect("start dropped the wire user bundle");
407+
assert_eq!(u.upn, "alice@contoso.com");
408+
assert_eq!(u.wam_token, "tok");
409+
}
410+
317411
fn request_with_filesystem_policy() -> ExecutionRequest {
318412
ExecutionRequest {
319413
policy: ContainerPolicy {

src/core/wxc_common/src/wire.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -484,8 +484,11 @@ pub enum TransportProtocol {
484484
}
485485

486486
/// IsolationSession backend config. Carries the one-shot `user` field and
487-
/// the per-phase state-aware nesting (`provision` / `start` / `stop` /
488-
/// `deprovision`).
487+
/// the per-phase state-aware nesting for the phases that take config
488+
/// (`provision` / `start`). `stop`, `deprovision`, and `exec` take no
489+
/// per-phase config payload: `stop` and `deprovision` are invoked with only
490+
/// the top-level `phase` and `sandboxId`, and `exec` additionally carries
491+
/// the top-level `process` block.
489492
#[derive(Debug, Clone, Serialize, Deserialize)]
490493
#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))]
491494
#[serde(rename_all = "camelCase")]
@@ -496,10 +499,6 @@ pub struct IsolationSession {
496499
pub provision: Option<IsolationSessionPhase>,
497500
/// State-aware start-phase configuration.
498501
pub start: Option<IsolationSessionPhase>,
499-
/// State-aware stop-phase configuration.
500-
pub stop: Option<IsolationSessionPhase>,
501-
/// State-aware deprovision-phase configuration.
502-
pub deprovision: Option<IsolationSessionPhase>,
503502
}
504503

505504
/// Per-phase IsolationSession configuration (state-aware lifecycle).

0 commit comments

Comments
 (0)