Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds three Jamf Pro 11.23 PPPC service keys (BluetoothAlways, SystemPolicyAppBundles, SystemPolicyAppData) across the app’s data model, UI, fixtures, and tests, and also includes broader workflow/tooling and deprecation-related updates.
Changes:
- Register 3 new PPPC services in
PPPCServices.json, model enums/properties, and wire them into the main UI. - Update unit/UI tests and mobileconfig fixtures to cover import/export and legacy profile behavior.
- Migrate file panel filtering to
UniformTypeIdentifiersand add Spec Kit tooling/configuration files.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/002-add-pppc-keys/tasks.md | Task breakdown for adding 3 PPPC keys + tests/fixtures. |
| specs/002-add-pppc-keys/spec.md | Feature spec + acceptance scenarios/requirements. |
| specs/002-add-pppc-keys/research.md | Research summary for Apple MDM keys + behaviors. |
| specs/002-add-pppc-keys/quickstart.md | Build/test quickstart for the PPPC key work. |
| specs/002-add-pppc-keys/plan.md | Implementation plan and project structure mapping. |
| specs/002-add-pppc-keys/data-model.md | Data model changes for new services/properties. |
| specs/002-add-pppc-keys/clarifications.md | Clarification log for tests/fixtures strategy. |
| specs/002-add-pppc-keys/checklists/requirements.md | Spec completeness checklist for 002. |
| specs/001-fix-deprecations/tasks.md | Task list for deprecation warning removals. |
| specs/001-fix-deprecations/spec.md | Feature spec for deprecation cleanup. |
| specs/001-fix-deprecations/research.md | Deprecation inventory + UTType migration notes. |
| specs/001-fix-deprecations/quickstart.md | Verification steps for deprecation fixes. |
| specs/001-fix-deprecations/plan.md | Implementation plan for deprecation work. |
| specs/001-fix-deprecations/checklists/requirements.md | Spec completeness checklist for 001. |
| Source/View Controllers/TCCProfileViewController.swift | Adds new outlets/wiring + UTType-based panels and accessibility identifiers. |
| Source/View Controllers/SaveViewController.swift | Uses allowedContentTypes and addresses KVO isolation warning. |
| Source/View Controllers/OpenViewController.swift | Uses allowedContentTypes for executable picker. |
| Source/TCCProfileImporter/TCCProfileConfigurationPanel.swift | Uses allowedContentTypes for import panel filters. |
| Source/Model/TCCProfile.swift | Adds new ServicesKeys cases for 3 services. |
| Source/Model/Executable.swift | Adds 3 new KVC Policy properties for new MDM keys. |
| Resources/TestTCCUnsignedProfile.mobileconfig | Updates bundled UI-test profile with new service entries. |
| Resources/PPPCServices.json | Registers BluetoothAlways/AppBundles/AppData service metadata. |
| PPPC UtilityUITests/AppLaunchTests.swift | Adds UI assertions for popup existence/order under -UITestMode. |
| PPPC UtilityTests/TCCProfileImporterTests/TestTCCUnsignedProfile.mobileconfig | Updates importer fixture with 3 new service entries. |
| PPPC UtilityTests/TCCProfileImporterTests/TestTCCUnsignedProfile-allLower.mobileconfig | Adds lowercase-key entries for new services. |
| PPPC UtilityTests/TCCProfileImporterTests/TestTCCProfileForJamfProAPI.txt | Updates expected Jamf Pro API XML payload content. |
| PPPC UtilityTests/TCCProfileImporterTests/TCCProfileTests.swift | Adjusts serialization tests for new services included by builder. |
| PPPC UtilityTests/TCCProfileImporterTests/TCCProfileImporterTests.swift | Adds legacy-import defaults test + adjusts service-count expectation. |
| PPPC UtilityTests/ModelTests/PPPCServicesManagerTests.swift | Updates service count and asserts new services load with names. |
| PPPC UtilityTests/ModelTests/ModelTests.swift | Adds parameterized round-trip test for new keys and refactors policyFromString tests. |
| PPPC UtilityTests/ModelTests/ExecutableTests.swift | Updates defaults test to assert new policy properties default to dash. |
| PPPC UtilityTests/Helpers/TCCProfileBuilder.swift | Ensures builder includes new keys in generated policies. |
| CLAUDE.md | Adds Swift/test/UI conventions used by the repo. |
| .specify/templates/tasks-template.md | Adds Spec Kit tasks template. |
| .specify/templates/spec-template.md | Adds Spec Kit spec template. |
| .specify/templates/plan-template.md | Adds Spec Kit plan template. |
| .specify/templates/constitution-template.md | Adds constitution template. |
| .specify/templates/checklist-template.md | Adds checklist template. |
| .specify/templates/agent-file-template.md | Adds agent context template. |
| .specify/scripts/bash/setup-plan.sh | Adds plan setup script. |
| .specify/scripts/bash/check-prerequisites.sh | Adds consolidated prereq checker script. |
| .specify/memory/constitution.md | Adds/updates project constitution (quality gates + UI ordering rule). |
| .specify/integrations/speckit.manifest.json | Adds speckit integration manifest metadata. |
| .specify/integrations/claude/scripts/update-context.sh | Adds wrapper script for context updates. |
| .specify/integrations/claude/scripts/update-context.ps1 | Adds PowerShell wrapper for context updates. |
| .specify/integrations/claude.manifest.json | Adds Claude integration manifest metadata. |
| .specify/integration.json | Selects active integration and script wiring. |
| .specify/init-options.json | Stores Spec Kit initialization options. |
| .specify/feature.json | Records active feature directory. |
| .specify/extensions/git/scripts/powershell/initialize-repo.ps1 | Git extension repo init script (PS). |
| .specify/extensions/git/scripts/powershell/git-common.ps1 | Git extension shared utils (PS). |
| .specify/extensions/git/scripts/powershell/auto-commit.ps1 | Git extension auto-commit hook (PS). |
| .specify/extensions/git/scripts/bash/initialize-repo.sh | Git extension repo init script (Bash). |
| .specify/extensions/git/scripts/bash/git-common.sh | Git extension shared utils (Bash). |
| .specify/extensions/git/scripts/bash/auto-commit.sh | Git extension auto-commit hook (Bash). |
| .specify/extensions/git/README.md | Documents git extension behavior/config. |
| .specify/extensions/git/git-config.yml | Git extension configuration defaults. |
| .specify/extensions/git/extension.yml | Git extension manifest. |
| .specify/extensions/git/config-template.yml | Template config for git extension installs. |
| .specify/extensions/git/commands/speckit.git.validate.md | Git extension command doc: validate. |
| .specify/extensions/git/commands/speckit.git.remote.md | Git extension command doc: remote detection. |
| .specify/extensions/git/commands/speckit.git.initialize.md | Git extension command doc: initialize. |
| .specify/extensions/git/commands/speckit.git.feature.md | Git extension command doc: feature branch creation. |
| .specify/extensions/git/commands/speckit.git.commit.md | Git extension command doc: auto-commit. |
| .specify/extensions/.registry | Registers enabled extensions and metadata. |
| .specify/extensions.yml | Hook wiring for extensions across Spec Kit commands. |
| .claude/skills/speckit-taskstoissues/SKILL.md | Adds Claude skill definition for tasks→issues. |
| .claude/skills/speckit-tasks/SKILL.md | Adds Claude skill definition for tasks generation. |
| .claude/skills/speckit-plan/SKILL.md | Adds Claude skill definition for planning. |
| .claude/skills/speckit-implement/SKILL.md | Adds Claude skill definition for implementation. |
| .claude/skills/speckit-git-validate/SKILL.md | Adds Claude skill for git validate. |
| .claude/skills/speckit-git-remote/SKILL.md | Adds Claude skill for git remote. |
| .claude/skills/speckit-git-initialize/SKILL.md | Adds Claude skill for git init. |
| .claude/skills/speckit-git-feature/SKILL.md | Adds Claude skill for git feature. |
| .claude/skills/speckit-git-commit/SKILL.md | Adds Claude skill for git auto-commit. |
| .claude/skills/speckit-constitution/SKILL.md | Adds Claude skill for constitution updates. |
| .claude/skills/speckit-clarify/SKILL.md | Adds Claude skill for clarification workflow. |
| .claude/skills/speckit-analyze/SKILL.md | Adds Claude skill for cross-artifact analysis. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func testServicePopupsExistInExpectedOrder() { | ||
| app.terminate() | ||
| app.launchArguments = ["-UITestMode"] | ||
| app.launch() | ||
|
|
||
| let expectedPopUpOrder = [ | ||
| "AccessibilityPopUp", | ||
| "AdminFilesPopUp", | ||
| "AppBundlesPopUp", | ||
| "AppDataPopUp", | ||
| "BluetoothAlwaysPopUp", | ||
| "CalendarPopUp", | ||
| "CameraPopUp", | ||
| "AddressBookPopUp", | ||
| "DesktopFolderPopUp", | ||
| "DocumentsFolderPopUp", | ||
| "DownloadsFolderPopUp", | ||
| "FileProviderPresencePopUp", | ||
| "AllFilesPopUp", | ||
| "ListenEventPopUp", | ||
| "MediaLibraryPopUp", | ||
| "MicrophonePopUp", | ||
| "NetworkVolumesPopUp", | ||
| "PhotosPopUp", | ||
| "PostEventsPopUp", | ||
| "RemindersPopUp", | ||
| "RemovableVolumesPopUp", | ||
| "ScreenCapturePopUp", | ||
| "SpeechRecognitionPopUp" | ||
| ] | ||
|
|
||
| var previousY = -CGFloat.greatestFiniteMagnitude | ||
| for identifier in expectedPopUpOrder { | ||
| let popUp = app.popUpButtons[identifier] | ||
| XCTAssertTrue(popUp.waitForExistence(timeout: 5), "\(identifier) should exist") | ||
| let currentY = popUp.frame.origin.y | ||
| XCTAssertGreaterThan(currentY, previousY, "\(identifier) should appear below the previous popup") | ||
| previousY = currentY | ||
| } |
There was a problem hiding this comment.
The new UI test verifies a hard-coded vertical order of many popups using frame coordinates. This is brittle (layout changes, window sizing, OS differences) and diverges from the spec’s stated intent for TR-006 (minimal UI test verifying the service/column count increased). Consider replacing this with a count-based assertion (or a small set of presence checks for the 3 new services) rather than enforcing full ordering via Y-position comparisons.
| let expectedPopUpOrder = [ | ||
| "AccessibilityPopUp", | ||
| "AdminFilesPopUp", | ||
| "AppBundlesPopUp", | ||
| "AppDataPopUp", | ||
| "BluetoothAlwaysPopUp", | ||
| "CalendarPopUp", | ||
| "CameraPopUp", | ||
| "AddressBookPopUp", | ||
| "DesktopFolderPopUp", | ||
| "DocumentsFolderPopUp", | ||
| "DownloadsFolderPopUp", | ||
| "FileProviderPresencePopUp", | ||
| "AllFilesPopUp", | ||
| "ListenEventPopUp", | ||
| "MediaLibraryPopUp", | ||
| "MicrophonePopUp", | ||
| "NetworkVolumesPopUp", | ||
| "PhotosPopUp", | ||
| "PostEventsPopUp", | ||
| "RemindersPopUp", | ||
| "RemovableVolumesPopUp", | ||
| "ScreenCapturePopUp", | ||
| "SpeechRecognitionPopUp" | ||
| ] |
There was a problem hiding this comment.
The expected popup order list is not alphabetical (e.g., AddressBook appears after Camera, AllFiles after FileProviderPresence), which conflicts with the documented UI convention that service rows must be alphabetical (CLAUDE.md:48 and .specify/memory/constitution.md:54). Either the list should be updated to alphabetical ordering (to enforce the convention) or the test should avoid asserting ordering altogether.
| @Test("Legacy profile without new keys imports with dash defaults") | ||
| func legacyProfileImportsWithDashDefaults() async throws { | ||
| let tccProfileImporter = TCCProfileImporter() | ||
| let resourceURL = try getResourceProfile(fileName: "TestTCCUnsignedProfile-Legacy") | ||
| let tccProfile = try tccProfileImporter.decodeTCCProfile(fileUrl: resourceURL) | ||
|
|
||
| // when | ||
| let model = Model() | ||
| await model.importProfile(tccProfile: tccProfile) | ||
|
|
||
| // then | ||
| for executable in model.selectedExecutables { | ||
| #expect(executable.policy.BluetoothAlways == "-", "BluetoothAlways should default to dash for legacy profiles") | ||
| #expect(executable.policy.SystemPolicyAppBundles == "-", "SystemPolicyAppBundles should default to dash for legacy profiles") | ||
| #expect(executable.policy.SystemPolicyAppData == "-", "SystemPolicyAppData should default to dash for legacy profiles") | ||
| } |
There was a problem hiding this comment.
This legacy import test will pass vacuously if model.selectedExecutables is empty (the loop won’t execute). Add an assertion that at least one executable was imported (or assert on a specific expected executable count) before checking policy defaults, so the test fails if import breaks.
| let tccProfile = try tccProfileImporter.decodeTCCProfile(fileUrl: resourceURL) | ||
| #expect(!tccProfile.content.isEmpty) | ||
| #expect(!tccProfile.content[0].services.isEmpty) | ||
| #expect(tccProfile.content[0].services.count >= 24, "Profile should contain at least 24 services including 3 new keys") | ||
| } |
There was a problem hiding this comment.
The test only asserts services.count >= 24, which can still pass if one of the required services is missing but the profile contains additional (newer) services. Since the goal is to ensure the 3 new keys are present, assert explicitly that BluetoothAlways, SystemPolicyAppBundles, and SystemPolicyAppData exist in services (and optionally that the expected total is 24 for this fixture).
| #expect(content.version == 1) | ||
|
|
||
| // then verify the services key | ||
| #expect(content.services.count == 2) | ||
| let allFiles = content.services["SystemPolicyAllFiles"] | ||
| #expect(allFiles?.count == 1) | ||
| allFiles?.forEach { policy in | ||
| #expect(policy.identifier == "policy id") | ||
| #expect(policy.identifierType == "policy id type") | ||
| #expect(policy.codeRequirement == "policy code req") | ||
| #expect(policy.allowed == nil) | ||
| #expect(policy.authorization == .allowStandardUserToSetSystemService) | ||
| #expect(policy.comment == "policy comment") | ||
| #expect(policy.receiverIdentifier == "policy receiver id") | ||
| #expect(policy.receiverIdentifierType == "policy receiver id type") | ||
| #expect(policy.receiverCodeRequirement == "policy receiver code req") | ||
| } | ||
| #expect(content.services.count == 5) | ||
| } |
There was a problem hiding this comment.
These serialization tests were previously validating that at least one service entry (e.g., SystemPolicyAllFiles) round-trips with the expected policy fields. After the change they only assert content.services.count == 5, which significantly reduces coverage and may miss regressions in parsing/serialization. Consider restoring the detailed assertions for an existing key (and add a minimal check for one of the new keys) rather than only checking the dictionary count.
| <key>bluetoothalways</key> | ||
| <array> | ||
| <dict> | ||
| <key>allowed</key> | ||
| <true/> | ||
| <key>coderequirement</key> | ||
| <string>identifier "io.brackets.appshell" and anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] /* exists */ and certificate leaf[field.1.2.840.113635.100.6.1.13] /* exists */ and certificate leaf[subject.OU] = JQ525L2MZD</string> | ||
| <key>comment</key> | ||
| <string></string> | ||
| <key>identifier</key> | ||
| <string>io.brackets.appshell</string> | ||
| <key>identifiertype</key> | ||
| <string>bundleID</string> | ||
| </dict> | ||
| </array> |
There was a problem hiding this comment.
This fixture is named "allLower" and the surrounding entries consistently use lowercase formatting/values (e.g., bundleid, subject.ou) and indentation. The newly added bluetoothalways block is unindented and includes mixed-case values (bundleID, subject.OU), making the fixture inconsistent and harder to maintain. Please align the added block with the existing lowercase/formatting pattern in this file.
There was a problem hiding this comment.
The PR description is scoped to adding new PPPC keys, but this change set also introduces a large amount of Spec Kit tooling/configuration (new .specify scripts, templates, git extension, etc.). Consider splitting these workflow/tooling additions into a separate PR so the PPPC key feature can be reviewed and released independently.
Added new keys:
#133