Repository navigation
Support devicectl JSON version 5 - #193
Merged
Merged
Conversation
Parse the new properties dictionary with compatibility fallbacks for deprecated fields, and cover the new JSON shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Nested v5 lookups, CPU fallback handling, and mixed-format test coverage need correction.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates devicectl parsing for JSON version 5 while preserving deprecated-field compatibility.
Changes:
- Parses the new nested
propertiesstructure. - Adds compatibility fallbacks for deprecated fields.
- Adds version 5 parser test coverage.
File summaries
| File | Description |
|---|---|
Xamarin.MacDev/DeviceCtlOutputParser.cs |
Adds v5 property parsing and compatibility fallbacks. |
tests/DeviceCtlOutputParserTests.cs |
Adds coverage for the v5 JSON structure. |
Review details
Suppressed comments (2)
Xamarin.MacDev/DeviceCtlOutputParser.cs:62
- The compatibility path is not exercised by the tests: the legacy case has no
propertiesobject, while the v5 case has no deprecated fields. If a v5 payload omits one field but still supplies it throughdeviceProperties,hardwareProperties, orconnectionProperties, a regression in this per-field fallback ordering would pass both tests. Add a mixed-format case that verifies the deprecated value is used for a missing new field.
info.Name = GetString ("name", newDeviceProperties, stateProperties, properties, deviceProperties);
info.BuildVersion = GetString ("osBuildUpdate", stateProperties, newDeviceProperties, properties, deviceProperties);
info.OSVersion = GetString ("osVersionNumber", stateProperties, newDeviceProperties, properties, deviceProperties);
info.Udid = GetString ("udid", newHardwareProperties, properties, hardwareProperties);
tests/DeviceCtlOutputParserTests.cs:100
- The new test fixture is not the Xcode 27 v5 shape: it puts
osBuildUpdateandosVersionNumberdirectly underproperties.state, but v5 nests them underproperties.softwareasosBuildVersions.buildVersion.nameandosVersionNumber.stringValue. It therefore passes against the current incorrect lookups and would not catch the missing v5 support; update the fixture to the real nested shape (and putnameunderstate).
""state"": {
""osBuildUpdate"": ""23B85"",
""osVersionNumber"": ""18.1"",
""pairingState"": ""paired""
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dalexsoto
approved these changes
Sep 16, 2026
Read build and OS version values from the nested software objects emitted by Xcode 27 while retaining the legacy compatibility fallbacks. Resolve CPU architecture by the first non-empty name so numeric-only v5 cpuType objects do not hide the deprecated named value. Update the v5 fixture to reflect the observed payload and exercise mixed-format fallback behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
CPU architecture parsing still depends on deprecated data for v5 devices.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Translate the Mach CPU type and subtype values in the Xcode 27 properties payload into the architecture names expected by PhysicalDeviceInfo. Preserve named architecture fallback for legacy and unknown payloads, and cover both arm64e and arm64 without deprecated CPU data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 891a4a77-04cb-4497-8c29-83759604e389
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Handle unsigned 64-bit CPU subtypes and add a regression test before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Read Xcode 27 CPU subtype values as UInt64 so sign-extended ARM64e values retain their low-byte architecture discriminator. Update the v5 regression fixture to use the exact unsigned value emitted by devicectl. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 891a4a77-04cb-4497-8c29-83759604e389
dalexsoto
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parse the new properties dictionary with compatibility fallbacks for deprecated fields, and cover the new JSON shape.
Ref: dotnet/macios#26482.