Skip to content

Reject unknown ProcessContainerUi settings values - #1478

Merged
Gudge (MGudgin) merged 1 commit into
mainfrom
user/gudge/reject_unknown_systemsettings
Oct 13, 2026
Merged

Gudge (MGudgin) merged 1 commit into
mainfrom
user/gudge/reject_unknown_systemsettings

Conversation

@MGudgin

@MGudgin Gudge (MGudgin) commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

This change amends the system_settings field of ProcessContainerUi in the 1.1.0 dev contract to be a string_enum, rather than a String. Values other than 'none', 'parameters', 'display' or 'all' will be rejected. For the stable contracts, there is no change, unrecognized values will be treated as if the value was 'none'.

Details

  • In the 1.1.0 dev contract, add a string_enum!, ProcessContainerUiSystemSettings.
  • Amend the type of the system_settings field of the 1.1.0 ProcessContainerUi struct from OptionalField to
    OptionalField.
  • In the normalization layer, add an enum for UiSystemSettings and update the type of the system_settings field of BaseProcessUi from Option to Option.
  • Update the contract adapter for 1.1.0 to handle ProcessContainerUiSystemSettings when adapting to UiSystemSettings.
  • Update the contract adapters for 0.9.0 and 1.0.0 to map from String to UiSystemSettings.
  • Various test updates

Tests

  • Existing tests pass.
  • Added tests for rejecting unknown ProcessContainerUi system_settings values in the 1.1.0 dev contract.
Microsoft Reviewers: Open in CodeFlow

@MGudgin
Gudge (MGudgin) requested review from a team and a balanced review from Copilot October 13, 2026 15:00
@MGudgin
Gudge (MGudgin) requested a review from a team as a code owner October 13, 2026 15:00
@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 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.

🟡 Changes recommended

The legacy fail-closed fallbacks lack direct regression coverage, and two documentation typos remain.

3 open findings
What changed in this PR

Introduces strict systemSettings validation for the development contract while preserving fail-closed compatibility for published contracts.

Changes:

  • Adds typed UI system-setting enums and normalization.
  • Maps unknown legacy values to deny-all behavior.
  • Updates generated artifacts and contract tests.
File Description
src/​mxc-sdk/​tests/​mxc_contract/​v1_1_0_alpha/​enums.rs Tests development-contract enum validation.
src/​mxc-sdk/​src/​core/​mxc_contract/​dev/​stable.rs Defines the strict contract enum.
src/​mxc-sdk/​src/​core/​mxc_contract/​dev/​mod.rs Exports the new enum.
src/​mxc-sdk/​src/​core/​mxc_common/​wire.rs Adds normalized UI settings type.
src/​mxc-sdk/​src/​core/​mxc_common/​config_parser.rs Normalizes typed settings to policy strings.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​v1_0/​one_shot.rs Converts v1.0 strings fail-closed.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​v1_0/​one_shot_tests/​stable_candidate.rs Updates v1.0 adapter assertions.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​v0_9/​one_shot.rs Converts v0.9 strings fail-closed.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​v0_9/​one_shot_tests/​stable_candidate.rs Updates v0.9 adapter assertions.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​dev/​one_shot.rs Converts development enum values.
src/​mxc-sdk/​src/​core/​mxc_common/​config_contract_adapters/​dev/​one_shot_tests/​stable_candidate.rs Updates development adapter assertions.
sdk/​node/​src/​generated/​v1_1_0_alpha/​wire.ts Regenerates TypeScript wire types.
schemas/​dev/​mxc-config.schema.1.1.0-alpha.json Regenerates the development schema.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/mxc-sdk/src/core/mxc_contract/dev/stable.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 13, 2026 15:09

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.

🟢 Approval recommended

The compatibility behavior and strict development validation are correctly implemented and tested; only minor documentation typos remain.

3 open findings
Previously missed (1)

In code that hasn't changed since last review

Low severity Fix misspelling in public enum variant documentation

src/​mxc-sdk/​src/​core/​mxc_contract/​dev/​stable.rs:135

Correct the misspelling in this public enum variant documentation.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 16:12
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/reject_unknown_systemsettings branch from 25af25c to b27b635 Compare October 13, 2026 16:12
@MGudgin
Gudge (MGudgin) changed the base branch from main to user/gudge/align-node-wire-conformance-v1 October 13, 2026 16:12

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.

🟡 Changes recommended

Security-sensitive compatibility and variant mappings lack direct test coverage, and public documentation contains a typo.

5 open findings

🧠 Review effort: Balanced

Comment thread src/mxc-sdk/src/core/mxc_contract/dev/stable.rs Outdated
@MGudgin
Gudge (MGudgin) added this pull request to stack #1480 October 13, 2026 16:18
Copilot AI balanced review requested due to automatic review settings October 13, 2026 16:23
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/reject_unknown_systemsettings branch from b27b635 to 7a8dbfc Compare October 13, 2026 16:23

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.

🟢 Approval recommended

The implementation consistently rejects unsupported development values while retaining fail-closed compatibility for published contracts.

0 open findings

5 resolved since last review

🧠 Review effort: Balanced

Base automatically changed from user/gudge/align-node-wire-conformance-v1 to main October 13, 2026 17:37
This change amends the system_settings field of ProcessContainerUi in the 1.1.0
dev contract to be a string_enum, rather than a String. Values other than
'none', 'parameters', 'display' or 'all' will be rejected. For the stable
contracts, there is no change, unrecognized values will be treated as if the
value was 'none'.

Details

- In the 1.1.0 dev contract, add a string_enum!,
ProcessContainerUiSystemSettings.
- Amend the type of the system_settings field of the 1.1.0 ProcessContainerUi
struct from OptionalField<String> to
OptionalField<ProcessContainerUiSystemSettings>.
- In the normalization layer, add an enum for UiSystemSettings and update the
type of the system_settings field of BaseProcessUi from Option<String> to
Option<UiSystemSettings>.
- Update the contract adapter for 1.1.0 to handle ProcessContainerUiSystemSettings when
adapting to UiSystemSettings.
- Update the contract adapters for 0.9.0 and 1.0.0 to map from String to
UiSystemSettings.
- Various test updates

Tests

- Existing tests pass.
- Added tests for rejecting unknown ProcessContainerUi system_settings values
in the 1.1.0 dev contract.
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/reject_unknown_systemsettings branch from 7a8dbfc to 6f7f1ac Compare October 13, 2026 17:37
Comment thread sdk/node/src/generated/v1_1_0_alpha/wire.ts
@MGudgin
Gudge (MGudgin) merged commit e492072 into main Oct 13, 2026
30 checks passed
@MGudgin
Gudge (MGudgin) deleted the user/gudge/reject_unknown_systemsettings branch October 13, 2026 18:42
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.

3 participants