Promote seatbelt from experimental to stable#500
Conversation
Remove the --experimental flag requirement for macOS seatbelt invocations. The binary still accepts the flag (no-op) for backward compatibility with older SDK versions and configs. Changes: - Remove seatbelt from ExperimentalBackends list in SDK types - Remove experimental gate in mxc_darwin/main.rs - Update unit tests to reflect seatbelt no longer requires experimental - Update seatbelt-backend.md CLI examples (drop --experimental) - Update 0.7.0-dev schema descriptions - Update copilot-instructions.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
567f853 to
1ce6a71
Compare
There was a problem hiding this comment.
Pull request overview
This PR promotes the macOS Seatbelt backend from experimental to stable across the Rust executor, config parsing, SDK validation, and documentation, while retaining backward compatibility for legacy experimental.seatbelt configs and the macos_sandbox alias.
Changes:
- Make Seatbelt stable by switching the canonical config section path from
experimental.seatbeltto top-levelseatbelt, and removing the macOS binary’s--experimentalruntime gate. - Extend the Rust config parser and SDK types to accept top-level
seatbelt(with precedence overexperimental.seatbelt) and relax SDK experimental gating for Seatbelt. - Update schema/docs wording and Copilot instructions to reflect Seatbelt no longer being experimental (but see review comments for remaining doc/schema inconsistencies).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/wxc_common/src/models.rs |
Updates Seatbelt backend section path to seatbelt and clarifies deprecated legacy location. |
src/core/wxc_common/src/config_parser.rs |
Accepts top-level seatbelt config, enforces precedence over experimental.seatbelt, and adds unit tests. |
src/core/mxc_darwin/src/main.rs |
Removes the --experimental requirement for running mxc-exec-mac with Seatbelt. |
sdk/tests/unit/sandbox.test.ts |
Updates unit tests to reflect Seatbelt no longer requiring SDK experimental mode on macOS. |
sdk/src/types.ts |
Removes Seatbelt from ExperimentalBackends, adds top-level seatbelt to ContainerConfig, and deprecates experimental.seatbelt. |
sdk/src/sandbox.ts |
Switches macOS config construction to populate top-level seatbelt instead of experimental.seatbelt. |
schemas/dev/mxc-config.schema.0.7.0-dev.json |
Adjusts containment description and Seatbelt schema description (needs further alignment with new top-level key). |
docs/macos-support/seatbelt-backend.md |
Removes --experimental usage in examples and updates narrative (still contains outdated config-shape/example details; see comments). |
.github/copilot-instructions.md |
Removes “(experimental)” label from the Seatbelt backend doc pointer. |
Support 'seatbelt' as a top-level config key alongside the deprecated 'experimental.seatbelt' path. The parser prefers the top-level key when both are present. Old configs using experimental.seatbelt continue to work unchanged. Changes: - Rust config_parser: add top-level 'seatbelt' field to RawConfig, merge logic that prefers top-level over experimental.seatbelt - Rust models: update section_path() to return 'seatbelt', update docs - SDK types.ts: add top-level seatbelt field to ContainerConfig, mark experimental.seatbelt as deprecated - SDK sandbox.ts: buildDarwinProcessConfig writes to config.seatbelt - Add tests for top-level path and precedence Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1ce6a71 to
0c97ff1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| let seatbelt = raw_exp.seatbelt.map(|raw_sb| SeatbeltConfig { | ||
| profile_override: raw_sb.profile_override, | ||
| gui_access: raw_sb.gui_access.unwrap_or(false), | ||
| launch_method: raw_sb.launch_method.unwrap_or_default(), | ||
| nested_pty: raw_sb.nested_pty.unwrap_or(true), | ||
| keychain_access: raw_sb.keychain_access.unwrap_or(false), | ||
| extra_mach_lookups: raw_sb.extra_mach_lookups.unwrap_or_default(), | ||
| }); |
There was a problem hiding this comment.
thought: I know this will be removed in the future, but I wonder if it would be possible if we just move this logic into a reusable function? E.g if we only care about passing the raw seatbelt json object and returning a SeatbeltConfig then we can just have a make_seatbelt_config function and also use it on line 1223.
| // Top-level `seatbelt` takes precedence over `experimental.seatbelt`. | ||
| // This supports the two-phase migration: new configs use the top-level key, | ||
| // old configs still work via the experimental path. | ||
| let experimental = if let Some(raw_sb) = raw.seatbelt { |
There was a problem hiding this comment.
issue: my thinking is that we should error if the user provides both. The migration would then be either one will work but you can only specify one e.g either top level or experimental but not together. I think they should be mutually exclusive from the config creators perspective while the parser allows for both. Does that make sense?
| { | ||
| "if": { | ||
| "required": ["experimental"], | ||
| "properties": { "experimental": { "required": ["seatbelt"] } } | ||
| }, | ||
| "then": { | ||
| "required": ["containment"], | ||
| "properties": { "containment": { "enum": ["seatbelt", "process"] } } | ||
| } | ||
| }, |
There was a problem hiding this comment.
issue: we need an if/then clause for seatbelt similar to what we have for line 514 to 520 for lxc now that we're moving it to the toplevel as well. In the future we would then remove line 541 to line 550 when our deprecation period is over.
|
I don't understand why this PR does not follow the promotion steps laid out in docs\versioning.md |
| other backends, with `containment` set to `"seatbelt"`. Backend-specific | ||
| settings live under `experimental.seatbelt`, and the `--experimental` | ||
| flag is required to enable the backend at runtime: | ||
| settings live under a top-level `seatbelt` key (preferred) or under |
There was a problem hiding this comment.
This PR seems to go against what is laid out in docs\versioning.md
📖 Description
Promoting Seatbelt to stable, for backwards compatibility if the --experimental flag is passed it will no-op and continue the execution.
🔗 References
🔍 Validation
ran test locally, all passed.
✅ Checklist
📋 Issue Type
Microsoft reviewers: PR builds don't auto-run (ADO policy). Comment
/azp runto start
MXC-PR-Build. See docs/pull-requests.md.Microsoft Reviewers: Open in CodeFlow