[CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

[sergiocorral-conductorone]

Intent (verbatim from the ticket)

Extend the JAMF connector to support provisioning for all supported resources. It currently supports provisioning only for Users and User Accounts; Groups, User Groups, Roles, Sites, and Managed Devices are sync-only.

Fixes CXH-2344

What this adds

Grant/Revoke provisioning (ResourceProvisionerV2) for the Jamf resources that have an assignable
relationship:

Resource	Principal	What Grant/Revoke does
Groups (admin account groups)	userAccount (Group Access accounts only)	adds/removes the account from the group's member list
User Groups	user	adds/removes a user from a static user group (smart groups are rejected)
Roles	userAccount, group	privilege sets and individual privileges (see below)
Sites	user	adds/removes a site from the user's site list
Managed Devices	user	sets/clears a computer's or mobile device's assigned user

Behavior worth reviewing

Groups. The group is written with a minimal body (name + members); the last member is removed
with an explicit empty <members></members>. Jamf can return a member list that looks empty, so a
read with no members is never written back: Grant aborts with FailedPrecondition, and Revoke
returns a retryable Unavailable error without writing (it doesn't report the member as removed).
Writes are optimistic: Grant returns the grant after a successful PUT, without re-reading.

Roles.

• Every privilege set (Administrator, Auditor, Enrollment Only) and every individual privilege
  is grantable to accounts and groups.
• Revoking a privilege set moves the principal to Custom with an empty privilege block, which is the
  lowest Jamf allows (it always keeps Read License Information). Individual privileges can only be
  granted/revoked while the principal is Custom; granting a set again leaves Custom.
• Writes use minimal bodies (name, privilege_set, and the privilege block when Custom) and never
  send site or members, so a group role change cannot touch membership. Any transition to
  Custom sends an explicit privilege block, otherwise Jamf copies the previous set's whole privilege
  list into it.
• Accounts with Group Access get their rights from their groups and are rejected (assign the role to
  the group instead). Changing the role of a Full/Site Access account removes it from admin groups it
  was listed in; that membership does not grant such an account anything.
• Writes are optimistic (no re-read after the PUT). Entitlements come from Jamf's own privilege
  catalog, so an unknown privilege name can't reach a write; the next sync shows the real state.

User Groups. Reads the group first and skips the write when the membership already matches. Jamf
answers 409 when removing a non-member or adding an unknown user (for example, one deleted after the
sync); since membership is checked before the write, a 409 is returned as FailedPrecondition instead
of being treated as "already exists".

Sites. A user's sites are read-modify-written as a list (Jamf replaces the whole list on PUT); the
connector reads and checks, and the client only writes the list (UpdateUserSites). A 409 on the write
is returned as FailedPrecondition.
This PR also fixes how a user's sites are decoded: Jamf returns a flat list while the model expected
each entry wrapped under site, so every site id was read as 0 (sync emitted no user-to-site grants
and a second grant failed with 409). Note for the release: user-to-site grants now appear in sync.

Managed Devices.

• Computers use the Jamf Pro v4 computers-inventory API (v1 and v3 are deprecated). Grant writes
  username, name, email, position and phone from the new user's record in one PATCH (Jamf never
  fills those in for computers); Revoke clears all five.
• Mobile devices only get location.username; Jamf derives the rest from the directory user and
  clears it all when the username is cleared.
• Revoke only clears the device when the principal is still its assignee (username first; email
  when the grant only carries an email, as for assignees that aren't synced Jamf users).
• The v4 list is requested with explicit section= values; without them Jamf returns only general.
• Provisioning requires managedDevice to be selected as a synced resource type, like the sync.

Reads before writes. The SDK HTTP client caches GET responses for an hour. Every provisioning
entry point now reads with the cache bypassed (jamf.WithFreshReads) so idempotency checks and
read-modify-write bodies never use stale data.

Docs and tooling. README.md, docs/connector.mdx and docs/docs-info.md now list the Jamf privileges each capability needs, using the names verified on the tenant. Classic API denials come back as 401 and Pro API denials as 403. Managed Devices are opt-in through --sync-resource-types. The Instance URL field description now says to include https://. The mock server in test-server/ models the Jamf behavior observed on the tenant, so CI covers the same paths as the unit tests. Managed Devices are not mocked there.

How it was verified

• Unit tests for every Grant/Revoke path, including the exact request bodies.
• The connector was run end to end against a Jamf Pro 11.32.1 trial tenant (throwaway objects only):
  Grant, Revoke, idempotent repeats, deleted principals, wrong principal types and the guards above,
  checking the real state of each object after every step. The behavior that Jamf's docs leave
  undefined (merge vs replace on PUT, clearing fields, empty member lists, what a Custom set keeps)
  was observed on the tenant and is what the bodies above rely on.
• After the review changes (optimistic writes, empty-group Revoke error, 409 handling, the Sites and
  Roles refactors, device Revoke by email), the full Grant/Revoke flow was run again on the tenant,
  including Managed Devices on test computer and mobile-device records.

Known limitations (documented in docs/connector.mdx)

• A group with no members cannot receive its first member through the connector.
• Directory (LDAP) groups are not supported for membership changes.
• Group Access accounts are not supported for role changes, and roles inherited through groups are
  not shown in sync.
• Site Grant/Revoke only supports the user principal.
• Devices with no assignee do not expose the assigned entitlement.

🤖 Generated with Claude Code

[linear-code]

CXH-2344

[github-actions]

Superseded — see the current review report for commit 1a0efe47c757

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 5 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: full
View review run

Review Summary

This PR adds three kinds of Grant/Revoke:

• Static User Groups: a Classic PUT with <user_additions>/<user_deletions>. It re-fetches the group first and rejects smart groups.
• Sites (user principal only): a client-side GET+PUT read-modify-write of <sites>, with the idempotent cases handled inside the client.
• Managed Device assigned entitlement: a JSON PATCH on the Pro API, using a new doRequestWithJSONMethod helper.

It also updates the docs, README, capabilities JSON and test-server mocks. I scanned the full diff for security and correctness. There are no go.mod/go.sum changes. I applied the repo-local criteria:

• PR1/PR2 (entity sources): I read the full Grant/Revoke flows. Principal and entitlement come from the correct fields, and no code reads entitlement.Resource.ParentResourceId.
• PR3 (argument order): AddUserSite(userID, siteID) matches its signature.
• L1–L7 (logging): no new logging.
• E1–E3 (error codes): see the Suggestions below.
• BP gate: nothing breaking. Only provisioning capability was added; resource IDs and entitlement slugs are unchanged.

No live-tenant verification exists; the PR author already flags the Jamf response semantics as unverified.

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:214 — Revoke always PATCHes username: "" without checking who the device is assigned to now. If the device was reassigned to another user after sync, revoking the old user's stale grant clears the new user's assignment. (Confidence: high)

Suggestions

• New pkg/connector/managedDevice.go:147-155 — The assigned entitlement is only emitted for devices that already have an assignee. So Grant can't assign a device that is unassigned, and after a Revoke the entitlement disappears on the next sync. The README/docs say Grant "sets" a device's assigned user without mentioning this. (Confidence: medium)
• New pkg/connector/userGroup.go:148 — Every 409 is mapped to GrantAlreadyExists. The Jamf Classic API also uses 409 for general validation failures, such as an unknown user ID, so a failed grant can be reported as success. On 409, re-read the membership to confirm. (Confidence: medium)
• New pkg/connector/site.go:168 (also managedDevice.go, userGroup.go) — The Grant/Revoke validation errors (wrong principal type, non-numeric ID, smart group) are plain fmt.Errorf values with no gRPC status code (P4/E3). Wrap them with codes.InvalidArgument or codes.FailedPrecondition. (Confidence: high)
• New pkg/jamf/client.go:548 (AddUserSite/RemoveUserSite) — The read-modify-write of <sites> has no concurrency guard. Two concurrent site grants or revokes for the same user can overwrite each other, silently dropping a site. That's unavoidable with this API, but worth documenting, or re-verifying after the PUT. (Confidence: medium)
• New pkg/jamf/device_client.go — This adds new Pro API PATCH endpoints (/api/v1/computers-inventory-detail/{id}, /api/v2/mobile-devices/{id}) and Classic PUTs. Per B10, run the build-openapi-spec.md skill so spec/openapi.json covers them, if the repo keeps one. (Confidence: low)

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go`:
- Around line 204-218 (Revoke): before clearing the assignment, resolve the grant principal's username via usernameForPrincipal. Fetch the device's current assigned username: computers-inventory-detail userAndLocation.username for computers, mobile-devices location.username for mobile devices. If the current assignee is not the principal (case-insensitive), return annotations.New(&v2.GrantAlreadyRevoked{}) without PATCHing. Only PATCH username "" when the principal is still assigned. Add a test for the reassigned case.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 147-160 (Entitlements): the "assigned" entitlement is only emitted when the device has an assignee, so unassigned devices can't be granted. Either always emit the entitlement so Grant works on unassigned devices, or document in README/docs/connector.mdx that Grant only reassigns devices that are already assigned.

In `pkg/connector/userGroup.go`:
- Around line 146-150: don't treat every 409 as GrantAlreadyExists. On IsAlreadyExistsError, call GetUserGroupDetails. Return GrantAlreadyExists only if the user ID is in the group's Users; otherwise return the wrapped error.

In `pkg/connector/site.go`, `pkg/connector/managedDevice.go`, `pkg/connector/userGroup.go`:
- In Grant/Revoke: return the principal-type, invalid-ID, and smart-group rejection errors as status.Error(codes.InvalidArgument, ...) or codes.FailedPrecondition instead of plain fmt.Errorf, so the platform treats them as non-retryable.

In `pkg/jamf/client.go`:
- AddUserSite/RemoveUserSite: note in a comment that the GET+PUT read-modify-write can lose updates when two operations run concurrently on the same user. Optionally re-read after the PUT to verify the site was added or removed.

In `pkg/jamf/device_client.go`:
- The new PATCH endpoints were added: run the build-openapi-spec.md skill so spec/openapi.json (if present) covers them.

[github-actions]

Superseded — see the current review report for commit f8d0e71389b5

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 5 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: full
View review run

Review Summary

This PR adds three kinds of Grant/Revoke:

• Static User Groups: a Classic PUT with <user_additions>/<user_deletions>. It re-fetches the group first and rejects smart groups.
• Sites (user principal only): a client-side GET+PUT read-modify-write of <sites>, with the idempotent cases handled inside the client.
• Managed Device assigned entitlement: a JSON PATCH on the Pro API, using a new doRequestWithJSONMethod helper.

It also updates the docs, README, capabilities JSON and test-server mocks. I scanned the full diff for security and correctness. There are no go.mod/go.sum changes. I applied the repo-local criteria:

• PR1/PR2 (entity sources): I read the full Grant/Revoke flows. Principal and entitlement come from the correct fields, and no code reads entitlement.Resource.ParentResourceId.
• PR3 (argument order): AddUserSite(userID, siteID) matches its signature.
• L1–L7 (logging): no new logging.
• E1–E3 (error codes): see the Suggestions below.
• BP gate: nothing breaking. Only provisioning capability was added; resource IDs and entitlement slugs are unchanged.

No live-tenant verification exists; the PR author already flags the Jamf response semantics as unverified.

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:214 — Revoke always PATCHes username: "" without checking who the device is assigned to now. If the device was reassigned to another user after sync, revoking the old user's stale grant clears the new user's assignment. (Confidence: high)

Suggestions

• New pkg/connector/managedDevice.go:147-155 — The assigned entitlement is only emitted for devices that already have an assignee. So Grant can't assign a device that is unassigned, and after a Revoke the entitlement disappears on the next sync. The README/docs say Grant "sets" a device's assigned user without mentioning this. (Confidence: medium)
• New pkg/connector/userGroup.go:148 — Every 409 is mapped to GrantAlreadyExists. The Jamf Classic API also uses 409 for general validation failures, such as an unknown user ID, so a failed grant can be reported as success. On 409, re-read the membership to confirm. (Confidence: medium)
• New pkg/connector/site.go:168 (also managedDevice.go, userGroup.go) — The Grant/Revoke validation errors (wrong principal type, non-numeric ID, smart group) are plain fmt.Errorf values with no gRPC status code (P4/E3). Wrap them with codes.InvalidArgument or codes.FailedPrecondition. (Confidence: high)
• New pkg/jamf/client.go:548 (AddUserSite/RemoveUserSite) — The read-modify-write of <sites> has no concurrency guard. Two concurrent site grants or revokes for the same user can overwrite each other, silently dropping a site. That's unavoidable with this API, but worth documenting, or re-verifying after the PUT. (Confidence: medium)
• New pkg/jamf/device_client.go — This adds new Pro API PATCH endpoints (/api/v1/computers-inventory-detail/{id}, /api/v2/mobile-devices/{id}) and Classic PUTs. Per B10, run the build-openapi-spec.md skill so spec/openapi.json covers them, if the repo keeps one. (Confidence: low)

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go`:
- Around line 204-218 (Revoke): before clearing the assignment, resolve the grant principal's username via usernameForPrincipal. Fetch the device's current assigned username: computers-inventory-detail userAndLocation.username for computers, mobile-devices location.username for mobile devices. If the current assignee is not the principal (case-insensitive), return annotations.New(&v2.GrantAlreadyRevoked{}) without PATCHing. Only PATCH username "" when the principal is still assigned. Add a test for the reassigned case.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 147-160 (Entitlements): the "assigned" entitlement is only emitted when the device has an assignee, so unassigned devices can't be granted. Either always emit the entitlement so Grant works on unassigned devices, or document in README/docs/connector.mdx that Grant only reassigns devices that are already assigned.

In `pkg/connector/userGroup.go`:
- Around line 146-150: don't treat every 409 as GrantAlreadyExists. On IsAlreadyExistsError, call GetUserGroupDetails. Return GrantAlreadyExists only if the user ID is in the group's Users; otherwise return the wrapped error.

In `pkg/connector/site.go`, `pkg/connector/managedDevice.go`, `pkg/connector/userGroup.go`:
- In Grant/Revoke: return the principal-type, invalid-ID, and smart-group rejection errors as status.Error(codes.InvalidArgument, ...) or codes.FailedPrecondition instead of plain fmt.Errorf, so the platform treats them as non-retryable.

In `pkg/jamf/client.go`:
- AddUserSite/RemoveUserSite: note in a comment that the GET+PUT read-modify-write can lose updates when two operations run concurrently on the same user. Optionally re-read after the PUT to verify the site was added or removed.

In `pkg/jamf/device_client.go`:
- The new PATCH endpoints were added: run the build-openapi-spec.md skill so spec/openapi.json (if present) covers them.

---

Reviewed commit: 1a0efe47c757

[github-actions]

Superseded — see the current review report for commit f8d0e71389b5

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 1a0efe4
View review run

Review Summary

The new commits respond to the last review:

• Managed-device Revoke now reads the device's current assignee (new GetComputerInventoryDetail/GetMobileDeviceDetail client calls). It only PATCHes when that assignee is the grant's principal, and otherwise returns GrantAlreadyRevoked.
• On a 409, user-group Grant now re-reads the group to confirm the user is actually a member.
• Grant/Revoke validation errors now carry gRPC InvalidArgument/FailedPrecondition codes.
• The site read-modify-write has a documented concurrency caveat, and the README/docs describe the unassigned-device limitation.

I scanned the full PR diff for security and correctness issues. I applied the repo-local criteria to the Grant/Revoke flows:

• PR1–PR3: I read the full flows. Entity sources are correct, and nothing reads entitlement.Resource.ParentResourceId.
• E2/E3: the gRPC codes are appropriate.
• L1–L7: no new logging.
• T1–T5: no spans.
• J3: the new model uses a pointer Location.
• B10: new GET endpoints were added.

No go.mod/go.sum changes. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:284-291 / pkg/jamf/device_client.go:110: The mobile-device Revoke check calls GET /api/v2/mobile-devices/{id} and reads location.username. That endpoint returns a flat username (the same shape as the list MobileDevice model); the nested location object only comes from /api/v2/mobile-devices/{id}/detail. So Location is always nil, the current assignee reads as "", and every mobile-device Revoke returns GrantAlreadyRevoked without sending the PATCH. Access is silently not removed. The test mocks the nested shape, so it can't catch this. (Confidence: medium-high)

Suggestions

• Prior — still present (now documented) pkg/connector/managedDevice.go:147-157: The assigned entitlement is still only emitted for devices that already have an assignee. Grant still can't assign an unassigned device, and the entitlement still disappears after a Revoke. README/docs now state this limitation. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go: New Pro API endpoints (the PATCH calls plus the new detail GETs) are not covered by a spec/openapi.json; the repo has none. Per B10, run the build-openapi-spec.md skill. (Confidence: low)

Resolved prior findings

• Fixed: Revoke used to clear a reassigned device's user. managedDevice.go:215-232 now compares the current assignee to the principal's username before PATCHing, and new tests cover the reassigned and unassigned cases. This is effective for computers; the mobile path has the new bug above.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group and returns the original error unless the user is actually a member. Covered by TestUserGroupGrant_ConflictButNotMember_ReturnsError.
• Fixed: Validation errors had no gRPC code (P4/E3). site.go, userGroup.go and managedDevice.go now use status.Errorf with InvalidArgument/FailedPrecondition.
• Fixed (documented): The site read-modify-write had no concurrency guard. It is still unguarded, but the limitation is now documented in pkg/jamf/client.go:499-506 and :527-530, as suggested.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go` / `pkg/jamf/device_client.go` / `pkg/jamf/device_models.go`:
- Around managedDevice.go lines 284-291 and device_client.go line 110: GetMobileDeviceDetail calls GET /api/v2/mobile-devices/{id}. That endpoint returns a flat top-level `username`, not a nested `location.username`, so MobileDeviceDetail.Location is always nil. currentAssignedUser then returns "" and Revoke skips the PATCH, returning GrantAlreadyRevoked for every mobile device. Fix it one of two ways: (a) call GET /api/v2/mobile-devices/{id}/detail, which returns `location.username`; or (b) add a top-level `Username string `json:"username"`` field to MobileDeviceDetail and fall back to it when Location is nil or empty. Update jamfDeviceAssignHandlerWithCurrent in managedDevice_test.go to serve the real response shape for that path, and add a mobile-device Revoke test.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around lines 147-157: The `assigned` entitlement is only emitted for devices that already have an assignee, so Grant cannot assign an unassigned device. Consider emitting the entitlement for every device; the limitation is currently documented.

In `pkg/jamf/device_client.go`:
- New Pro API endpoints (computers-inventory-detail GET/PATCH, mobile-devices GET/PATCH): run the build-openapi-spec.md skill so spec/openapi.json covers them.

[github-actions]

Superseded — see the current review report for commit 83d0c17cd8a9

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 1a0efe4
View review run

Review Summary

The new commits respond to the last review:

• Managed-device Revoke now reads the device's current assignee (new GetComputerInventoryDetail/GetMobileDeviceDetail client calls). It only PATCHes when that assignee is the grant's principal, and otherwise returns GrantAlreadyRevoked.
• On a 409, user-group Grant now re-reads the group to confirm the user is actually a member.
• Grant/Revoke validation errors now carry gRPC InvalidArgument/FailedPrecondition codes.
• The site read-modify-write has a documented concurrency caveat, and the README/docs describe the unassigned-device limitation.

I scanned the full PR diff for security and correctness issues. I applied the repo-local criteria to the Grant/Revoke flows:

• PR1–PR3: I read the full flows. Entity sources are correct, and nothing reads entitlement.Resource.ParentResourceId.
• E2/E3: the gRPC codes are appropriate.
• L1–L7: no new logging.
• T1–T5: no spans.
• J3: the new model uses a pointer Location.
• B10: new GET endpoints were added.

No go.mod/go.sum changes. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:284-291 / pkg/jamf/device_client.go:110: The mobile-device Revoke check calls GET /api/v2/mobile-devices/{id} and reads location.username. That endpoint returns a flat username (the same shape as the list MobileDevice model); the nested location object only comes from /api/v2/mobile-devices/{id}/detail. So Location is always nil, the current assignee reads as "", and every mobile-device Revoke returns GrantAlreadyRevoked without sending the PATCH. Access is silently not removed. The test mocks the nested shape, so it can't catch this. (Confidence: medium-high)

Suggestions

• Prior — still present (now documented) pkg/connector/managedDevice.go:147-157: The assigned entitlement is still only emitted for devices that already have an assignee. Grant still can't assign an unassigned device, and the entitlement still disappears after a Revoke. README/docs now state this limitation. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go: New Pro API endpoints (the PATCH calls plus the new detail GETs) are not covered by a spec/openapi.json; the repo has none. Per B10, run the build-openapi-spec.md skill. (Confidence: low)

Resolved prior findings

• Fixed: Revoke used to clear a reassigned device's user. managedDevice.go:215-232 now compares the current assignee to the principal's username before PATCHing, and new tests cover the reassigned and unassigned cases. This is effective for computers; the mobile path has the new bug above.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group and returns the original error unless the user is actually a member. Covered by TestUserGroupGrant_ConflictButNotMember_ReturnsError.
• Fixed: Validation errors had no gRPC code (P4/E3). site.go, userGroup.go and managedDevice.go now use status.Errorf with InvalidArgument/FailedPrecondition.
• Fixed (documented): The site read-modify-write had no concurrency guard. It is still unguarded, but the limitation is now documented in pkg/jamf/client.go:499-506 and :527-530, as suggested.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go` / `pkg/jamf/device_client.go` / `pkg/jamf/device_models.go`:
- Around managedDevice.go lines 284-291 and device_client.go line 110: GetMobileDeviceDetail calls GET /api/v2/mobile-devices/{id}. That endpoint returns a flat top-level `username`, not a nested `location.username`, so MobileDeviceDetail.Location is always nil. currentAssignedUser then returns "" and Revoke skips the PATCH, returning GrantAlreadyRevoked for every mobile device. Fix it one of two ways: (a) call GET /api/v2/mobile-devices/{id}/detail, which returns `location.username`; or (b) add a top-level `Username string `json:"username"`` field to MobileDeviceDetail and fall back to it when Location is nil or empty. Update jamfDeviceAssignHandlerWithCurrent in managedDevice_test.go to serve the real response shape for that path, and add a mobile-device Revoke test.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around lines 147-157: The `assigned` entitlement is only emitted for devices that already have an assignee, so Grant cannot assign an unassigned device. Consider emitting the entitlement for every device; the limitation is currently documented.

In `pkg/jamf/device_client.go`:
- New Pro API endpoints (computers-inventory-detail GET/PATCH, mobile-devices GET/PATCH): run the build-openapi-spec.md skill so spec/openapi.json covers them.

---

Reviewed commit: f8d0e71389b5

[github-actions]

Blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 83d0c17cd8a9

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since f8d0e71
View review run

Review Summary

The new commit points GetMobileDeviceDetail at GET /api/v2/mobile-devices/{id}/detail (new mobileDeviceDetailUrlPath), which is the endpoint that returns nested location.username. It also updates the doc comments and adds a mobile-device "reassigned, no PATCH" Revoke test. The PATCH still goes to the plain /api/v2/mobile-devices/{id}. I scanned the full PR diff (16 files) for security and correctness and found nothing new. I applied the trusted repo-local criteria: provisioning PR1–PR3 (I re-read the full Revoke flow, and it takes entities from grant.Principal/grant.Entitlement.Resource only), E3 status codes, J3 pointer Location, and B10 for the new endpoint constant. The logging (L*) and span (T*) criteria don't apply because this commit adds no logs or spans. The incremental diff is not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New pkg/connector/managedDevice_test.go:399: The test handler matches any /mobile-devices/ GET, so the new test would still pass if the call regressed to the plain endpoint. There is also no mobile Revoke test where the PATCH is actually sent. (Confidence: medium)
• Prior — still present pkg/connector/managedDevice.go:147-157: The assigned entitlement is only emitted for devices that already have an assignee, so Grant can't assign an unassigned device. This limitation is documented in README/docs. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:16: Per B10, the new Pro API endpoint (/api/v2/mobile-devices/{id}/detail), plus the earlier PATCH and detail endpoints, have no spec/openapi.json coverage because the repo has none. Run the build-openapi-spec.md skill. (Confidence: low)

Resolved prior findings

• Fixed: Mobile-device Revoke read the wrong endpoint shape, so it always skipped the PATCH. pkg/jamf/device_client.go:16,119 now calls /api/v2/mobile-devices/{id}/detail, which returns location.username and matches MobileDeviceDetail (device_models.go:152).
• Fixed: Revoke used to clear a reassigned device's user. The current-assignee check at managedDevice.go:215-232 now works for both computers and mobile devices.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group to confirm membership before treating a 409 as already-granted.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice_test.go`:
- Around line 399: jamfDeviceAssignHandlerWithCurrent matches any GET path containing "/mobile-devices/", so tests cannot detect a regression back to GET /api/v2/mobile-devices/{id}. Match the "/mobile-devices/{id}/detail" suffix explicitly for GET, and let other mobile GETs fall through to t.Fatalf. Also add TestManagedDeviceRevoke_MobileDevice_ClearsUsername, where current == principal ("jappleseed"), asserting the PATCH body equals `{"location":{"username":""}}` + "\n".

In `pkg/connector/managedDevice.go`:
- Around lines 147-157: Entitlements only emits the "assigned" entitlement when the device already has an assignee, so Grant cannot assign an unassigned device. Consider emitting it for every device. The limitation is currently documented.

In `pkg/jamf/device_client.go`:
- Around lines 12-16: New Jamf Pro API endpoints (the computers-inventory-detail GET/PATCH, the mobile-devices PATCH, and /api/v2/mobile-devices/{id}/detail) are not covered by a spec/openapi.json. Run the build-openapi-spec.md skill to generate one.

[github-actions]

Superseded — see the current review report for commit 9402a35788a9

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since f8d0e71
View review run

Review Summary

The new commit points GetMobileDeviceDetail at GET /api/v2/mobile-devices/{id}/detail (new mobileDeviceDetailUrlPath), which is the endpoint that returns nested location.username. It also updates the doc comments and adds a mobile-device "reassigned, no PATCH" Revoke test. The PATCH still goes to the plain /api/v2/mobile-devices/{id}. I scanned the full PR diff (16 files) for security and correctness and found nothing new. I applied the trusted repo-local criteria: provisioning PR1–PR3 (I re-read the full Revoke flow, and it takes entities from grant.Principal/grant.Entitlement.Resource only), E3 status codes, J3 pointer Location, and B10 for the new endpoint constant. The logging (L*) and span (T*) criteria don't apply because this commit adds no logs or spans. The incremental diff is not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New pkg/connector/managedDevice_test.go:399: The test handler matches any /mobile-devices/ GET, so the new test would still pass if the call regressed to the plain endpoint. There is also no mobile Revoke test where the PATCH is actually sent. (Confidence: medium)
• Prior — still present pkg/connector/managedDevice.go:147-157: The assigned entitlement is only emitted for devices that already have an assignee, so Grant can't assign an unassigned device. This limitation is documented in README/docs. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:16: Per B10, the new Pro API endpoint (/api/v2/mobile-devices/{id}/detail), plus the earlier PATCH and detail endpoints, have no spec/openapi.json coverage because the repo has none. Run the build-openapi-spec.md skill. (Confidence: low)

Resolved prior findings

• Fixed: Mobile-device Revoke read the wrong endpoint shape, so it always skipped the PATCH. pkg/jamf/device_client.go:16,119 now calls /api/v2/mobile-devices/{id}/detail, which returns location.username and matches MobileDeviceDetail (device_models.go:152).
• Fixed: Revoke used to clear a reassigned device's user. The current-assignee check at managedDevice.go:215-232 now works for both computers and mobile devices.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group to confirm membership before treating a 409 as already-granted.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice_test.go`:
- Around line 399: jamfDeviceAssignHandlerWithCurrent matches any GET path containing "/mobile-devices/", so tests cannot detect a regression back to GET /api/v2/mobile-devices/{id}. Match the "/mobile-devices/{id}/detail" suffix explicitly for GET, and let other mobile GETs fall through to t.Fatalf. Also add TestManagedDeviceRevoke_MobileDevice_ClearsUsername, where current == principal ("jappleseed"), asserting the PATCH body equals `{"location":{"username":""}}` + "\n".

In `pkg/connector/managedDevice.go`:
- Around lines 147-157: Entitlements only emits the "assigned" entitlement when the device already has an assignee, so Grant cannot assign an unassigned device. Consider emitting it for every device. The limitation is currently documented.

In `pkg/jamf/device_client.go`:
- Around lines 12-16: New Jamf Pro API endpoints (the computers-inventory-detail GET/PATCH, the mobile-devices PATCH, and /api/v2/mobile-devices/{id}/detail) are not covered by a spec/openapi.json. Run the build-openapi-spec.md skill to generate one.

---

Reviewed commit: 83d0c17cd8a9

[github-actions]

No blocking issues found — see the full review report

Stale — all blocking findings resolved as of 83d0c17 (bot confirmed 0 blocking issues remain).

[github-actions]

Superseded — see the current review report for commit 9402a35788a9

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 83d0c17
View review run

Review Summary

The new commit only changes a test, pkg/connector/managedDevice_test.go:

• The mock handler's mobile-device GET case now matches only the /mobile-devices/{id}/detail path. Any other mobile GET falls through to t.Fatalf.
• A new test, TestManagedDeviceRevoke_MobileDevice_ClearsUsername, checks that Revoke sends the clearing PATCH {"location":{"username":""}} when the grant's user is still the device's current assignee.

I scanned the full PR diff (16 files) for security and correctness and found no new issues. The repo-local criteria were applied:

• Provisioning (P1–P6, PR1–PR3): Revoke reads the principal from gr.Principal and the device from gr.Entitlement.Resource. Nothing reads entitlement.Resource.ParentResourceId.
• Log levels (L1–L7), tracing (T1–T5), JSON types (J1–J4): the new commit adds no logging, spans or response structs, so these didn't apply.
• Breaking-change gate (BP1–BP5): no identifiers change, so it wasn't triggered.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:147-152: The assigned entitlement only exists for devices that already have a user assigned. Grant can therefore reassign a device but never assign an unassigned one. README and docs describe this limitation. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:13-16: Under B10, the new Pro API endpoints have no spec/openapi.json coverage because the repo has no spec file. These are the computers-inventory-detail GET/PATCH, the mobile-devices PATCH, and /api/v2/mobile-devices/{id}/detail. Run the build-openapi-spec.md skill to generate it. (Confidence: low)

Resolved prior findings

• Fixed: The test handler matched any mobile-device GET, and no mobile Revoke test actually sent the PATCH. managedDevice_test.go:399 now requires the /detail suffix, and TestManagedDeviceRevoke_MobileDevice_ClearsUsername (managedDevice_test.go:556) checks the PATCH body.
• Fixed: Revoke could clear a device's user after the device had been reassigned. managedDevice.go:223-230 now checks the current assignee before sending the PATCH.
• Fixed: Mobile-device Revoke read the wrong endpoint shape. device_client.go:16 now uses /api/v2/mobile-devices/{id}/detail, which returns location.username.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group to confirm membership before treating a 409 as already granted.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 147-152: Entitlements returns the "assigned" entitlement only when the device already has an assignee, so Grant cannot assign a user to an unassigned device. Either always emit the "assigned" entitlement (so unassigned devices can be granted), or keep the current behavior and leave it documented as a known limitation.

In `pkg/jamf/device_client.go`:
- Around line 13-16: The new Pro API endpoints have no checked-in spec/openapi.json coverage: computers-inventory-detail GET/PATCH, mobile-devices PATCH, and mobile-devices/{id}/detail GET. Run the build-openapi-spec.md skill to generate spec/openapi.json covering them.

[github-actions]

Superseded — see the current review report for commit 39cc010711da

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 83d0c17
View review run

Review Summary

The new commit only changes a test, pkg/connector/managedDevice_test.go:

• The mock handler's mobile-device GET case now matches only the /mobile-devices/{id}/detail path. Any other mobile GET falls through to t.Fatalf.
• A new test, TestManagedDeviceRevoke_MobileDevice_ClearsUsername, checks that Revoke sends the clearing PATCH {"location":{"username":""}} when the grant's user is still the device's current assignee.

I scanned the full PR diff (16 files) for security and correctness and found no new issues. The repo-local criteria were applied:

• Provisioning (P1–P6, PR1–PR3): Revoke reads the principal from gr.Principal and the device from gr.Entitlement.Resource. Nothing reads entitlement.Resource.ParentResourceId.
• Log levels (L1–L7), tracing (T1–T5), JSON types (J1–J4): the new commit adds no logging, spans or response structs, so these didn't apply.
• Breaking-change gate (BP1–BP5): no identifiers change, so it wasn't triggered.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:147-152: The assigned entitlement only exists for devices that already have a user assigned. Grant can therefore reassign a device but never assign an unassigned one. README and docs describe this limitation. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:13-16: Under B10, the new Pro API endpoints have no spec/openapi.json coverage because the repo has no spec file. These are the computers-inventory-detail GET/PATCH, the mobile-devices PATCH, and /api/v2/mobile-devices/{id}/detail. Run the build-openapi-spec.md skill to generate it. (Confidence: low)

Resolved prior findings

• Fixed: The test handler matched any mobile-device GET, and no mobile Revoke test actually sent the PATCH. managedDevice_test.go:399 now requires the /detail suffix, and TestManagedDeviceRevoke_MobileDevice_ClearsUsername (managedDevice_test.go:556) checks the PATCH body.
• Fixed: Revoke could clear a device's user after the device had been reassigned. managedDevice.go:223-230 now checks the current assignee before sending the PATCH.
• Fixed: Mobile-device Revoke read the wrong endpoint shape. device_client.go:16 now uses /api/v2/mobile-devices/{id}/detail, which returns location.username.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:157-167 now re-reads the group to confirm membership before treating a 409 as already granted.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 147-152: Entitlements returns the "assigned" entitlement only when the device already has an assignee, so Grant cannot assign a user to an unassigned device. Either always emit the "assigned" entitlement (so unassigned devices can be granted), or keep the current behavior and leave it documented as a known limitation.

In `pkg/jamf/device_client.go`:
- Around line 13-16: The new Pro API endpoints have no checked-in spec/openapi.json coverage: computers-inventory-detail GET/PATCH, mobile-devices PATCH, and mobile-devices/{id}/detail GET. Run the build-openapi-spec.md skill to generate spec/openapi.json covering them.

---

Reviewed commit: 9402a35788a9

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 39cc010711da

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 9402a35
View review run

Review Summary

The PR adds Grant/Revoke for three resource types:

• User groups (static only): Grant/Revoke use the Classic user_additions/user_deletions PUT. Grant re-reads the group before treating a 409 as already granted.
• Sites (user principal only): Grant/Revoke read the user's <sites> and write the full list back.
• Managed devices (assigned entitlement): Grant/Revoke PATCH the Pro API. Revoke first checks that the grant's user is still the device's assignee.

The new commit (39cc010) changes comments only. It removes TODO(verify-in-verify-plan) markers and references to internal planning docs in managedDevice.go, userGroup.go, device_models.go, and two test files. The "unverified against a live tenant" caveats remain, and no executable code changed. I scanned the full PR diff for security and correctness again. That covered all 16 files, and nothing was dropped or truncated.

How the repo-local criteria applied:

• PR1–PR3: Grant/Revoke read the principal from principal/gr.Principal and the target from entitlement.Resource/gr.Entitlement.Resource. Nothing reads entitlement.Resource.ParentResourceId.
• E1–E3: Provisioning validation failures return gRPC codes (InvalidArgument, FailedPrecondition). All HTTP calls go through uhttp.
• A (log levels) and C (spans): No new logging or manual spans were added, so these don't apply.
• J3: The nullable location/userAndLocation objects are pointers and are nil-checked before use.
• BP1–BP5 (breaking-change gate): Not triggered. The change adds capabilities, and no IDs or entitlement slugs changed.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:149-153: Entitlements returns no assigned entitlement when a device has no username or email. So Grant can reassign a device but can never assign one that has no user. The limitation is documented in README and docs/connector.mdx, so this is a known product gap rather than a hidden bug. The existing thread still covers it, so no new inline comment was posted. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:13-17 (B10): The PR adds new Pro API endpoints and the repo has no spec/openapi.json. The endpoints are computers-inventory-detail GET/PATCH, mobile-devices/{id} PATCH, and mobile-devices/{id}/detail GET. Run the build-openapi-spec.md skill to generate spec coverage. (Confidence: low)

Resolved prior findings

• Fixed: Revoke could clear a device's user after the device had been reassigned. managedDevice.go:223-230 now compares the device's current assignee with the grant's user and returns GrantAlreadyRevoked when they differ.
• Fixed: Mobile-device Revoke read the wrong endpoint shape. device_client.go:17 now uses /api/v2/mobile-devices/{id}/detail, which is modeled as MobileDeviceDetail.Location.
• Fixed: The test handler matched any mobile-device GET. It now requires the /detail path, and TestManagedDeviceRevoke_MobileDevice_ClearsUsername checks the PATCH body.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:154-167 now re-fetches the group and reports already-granted only when the user is actually a member. Otherwise it returns the original error.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 149-153: Entitlements returns no "assigned" entitlement when the device has no assignee, so Grant can never assign a device that has no user. If assigning such devices should be supported, always emit the assigned entitlement (Grants() still emits grants only when there is an assignee). Otherwise keep the documented limitation.

In `pkg/jamf/device_client.go`:
- Around line 13-17: The PR adds new Jamf Pro API endpoints (GET/PATCH /api/v1/computers-inventory-detail/{id}, PATCH /api/v2/mobile-devices/{id}, GET /api/v2/mobile-devices/{id}/detail) and the repo has no spec/openapi.json. Run the build-openapi-spec.md skill to generate spec/openapi.json covering these endpoints.

[github-actions]

Superseded — see the current review report for commit bc3e6201c5f2

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for User Groups, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f264ba743b6e.
Review mode: incremental since 9402a35
View review run

Review Summary

The PR adds Grant/Revoke for three resource types:

• User groups (static only): Grant/Revoke use the Classic user_additions/user_deletions PUT. Grant re-reads the group before treating a 409 as already granted.
• Sites (user principal only): Grant/Revoke read the user's <sites> and write the full list back.
• Managed devices (assigned entitlement): Grant/Revoke PATCH the Pro API. Revoke first checks that the grant's user is still the device's assignee.

The new commit (39cc010) changes comments only. It removes TODO(verify-in-verify-plan) markers and references to internal planning docs in managedDevice.go, userGroup.go, device_models.go, and two test files. The "unverified against a live tenant" caveats remain, and no executable code changed. I scanned the full PR diff for security and correctness again. That covered all 16 files, and nothing was dropped or truncated.

How the repo-local criteria applied:

• PR1–PR3: Grant/Revoke read the principal from principal/gr.Principal and the target from entitlement.Resource/gr.Entitlement.Resource. Nothing reads entitlement.Resource.ParentResourceId.
• E1–E3: Provisioning validation failures return gRPC codes (InvalidArgument, FailedPrecondition). All HTTP calls go through uhttp.
• A (log levels) and C (spans): No new logging or manual spans were added, so these don't apply.
• J3: The nullable location/userAndLocation objects are pointers and are nil-checked before use.
• BP1–BP5 (breaking-change gate): Not triggered. The change adds capabilities, and no IDs or entitlement slugs changed.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:149-153: Entitlements returns no assigned entitlement when a device has no username or email. So Grant can reassign a device but can never assign one that has no user. The limitation is documented in README and docs/connector.mdx, so this is a known product gap rather than a hidden bug. The existing thread still covers it, so no new inline comment was posted. (Confidence: medium)
• Prior — still present pkg/jamf/device_client.go:13-17 (B10): The PR adds new Pro API endpoints and the repo has no spec/openapi.json. The endpoints are computers-inventory-detail GET/PATCH, mobile-devices/{id} PATCH, and mobile-devices/{id}/detail GET. Run the build-openapi-spec.md skill to generate spec coverage. (Confidence: low)

Resolved prior findings

• Fixed: Revoke could clear a device's user after the device had been reassigned. managedDevice.go:223-230 now compares the device's current assignee with the grant's user and returns GrantAlreadyRevoked when they differ.
• Fixed: Mobile-device Revoke read the wrong endpoint shape. device_client.go:17 now uses /api/v2/mobile-devices/{id}/detail, which is modeled as MobileDeviceDetail.Location.
• Fixed: The test handler matched any mobile-device GET. It now requires the /detail path, and TestManagedDeviceRevoke_MobileDevice_ClearsUsername checks the PATCH body.
• Fixed: Every 409 was treated as GrantAlreadyExists. userGroup.go:154-167 now re-fetches the group and reports already-granted only when the user is actually a member. Otherwise it returns the original error.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 149-153: Entitlements returns no "assigned" entitlement when the device has no assignee, so Grant can never assign a device that has no user. If assigning such devices should be supported, always emit the assigned entitlement (Grants() still emits grants only when there is an assignee). Otherwise keep the documented limitation.

In `pkg/jamf/device_client.go`:
- Around line 13-17: The PR adds new Jamf Pro API endpoints (GET/PATCH /api/v1/computers-inventory-detail/{id}, PATCH /api/v2/mobile-devices/{id}, GET /api/v2/mobile-devices/{id}/detail) and the repo has no spec/openapi.json. Run the build-openapi-spec.md skill to generate spec/openapi.json covering these endpoints.

---

Reviewed commit: 39cc010711da

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit a5712f5ad4de

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 2 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: full
View review run

Review Summary

This PR adds Grant/Revoke for Groups, User Groups, Roles, Sites and Managed Devices:

• Writes: Classic API PUTs with minimal XML bodies, and Pro API JSON PATCHes for devices.
• Fresh reads: jamf.WithFreshReads bypasses the uhttp GET cache on every provisioning path.
• Sync changes: computers inventory moves from v1 to v4, and a user's sites decoding is fixed, so user-to-site grants now appear in sync.
• Head commit: replaces role.go's roleOps interface with a rolePrincipal struct. It changes no behavior.

I scanned the full PR diff for security and correctness and read the full Grant/Revoke files (PR1). The trusted repo-local criteria were applied as follows:

• Entity source (PR2): principals always come from principal.Id / gr.Principal.Id, and nothing reads entitlement.Resource.ParentResourceId.
• Logging (L1b/L3): the new logging is Debug only.
• E3: gRPC codes are kept on failure paths.
• S1–S6: no new swallowed errors in List/Entitlements/Grants.
• Dependencies: go.mod/go.sum are unchanged; uhttp.WithNoCache/WithJSONBody exist in vendored baton-sdk v0.40.0.

Coverage gaps: test files and test-server/main.go were spot-checked, not read line by line.

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:296 — Revoke silently skips externally-matched computer grants (confidence: medium-high).
  • deviceGrants (line 946) keys an unsynced assignee by email whenever the computer has one, so principalIdentityForRevoke returns ("", email).
  • assigneeMatches (lines 384-394) only compares email when the device's current username is empty.
  • Result: on a computer with both username and email, Revoke returns GrantAlreadyRevoked and never clears the assignee.
  • The existing test (TestManagedDeviceRevoke_ExternalMatchPrincipal_Patches) only covers the case where the current username is empty.
• Prior — still present pkg/connector/group.go:254-262 — when Jamf returns an empty member list, Revoke returns GrantAlreadyRevoked without writing.
  • That empty list is the known flaky response: Grants retries it 5 times (group.go:104-114), and Grant rejects it with FailedPrecondition (group.go:174-180).
  • A flaky read therefore reports a successful revoke while the account keeps its membership. The thread was resolved, but the code is unchanged.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:160-164 — Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign a device that has no user. This is a documented limitation in README and docs/connector.mdx. (Confidence: medium)
• Prior — still present B10 — the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new endpoints:
  • Jamf Pro v4 computers-inventory(-detail) GET/PATCH
  • Jamf Pro v2 mobile-devices/{id} PATCH and /detail GET
  • Classic API usergroups/id, users/id, accounts/userid and accounts/groupid PUTs

Resolved prior findings

• Fixed: README no longer says Groups and Roles are sync-only (README.md:14).
• Fixed: Managed Device Revoke checks that the principal is still the assignee before clearing (managedDevice.go:287-302).
• Fixed: mobile devices are read from the detail endpoint, which returns the nested location.username (GetMobileDeviceDetail, managedDevice.go:363-371). The test-matcher suggestion on this is obsolete for the same reason.
• Fixed: Role Revoke checks that the principal still holds the set before revoking it (role.go:504-506). Because revoking now moves the principal to Custom (role.go:508-519), revoking Enrollment Only also works.
• Fixed: a 409 on User Group Grant is no longer treated as GrantAlreadyExists. Grant checks membership first and re-reads after a 409 (userGroup.go:146-167).

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go`:
- Around line 296 (with assigneeMatches at 384-394 and deviceGrants at 946-949): an unsynced computer
  assignee's grant is keyed by email whenever the device has an email. On Revoke, principalIdentityForRevoke
  then returns ("", email), but assigneeMatches only compares emails when the current username is empty.
  For devices with both a username and an email, Revoke wrongly returns GrantAlreadyRevoked without
  clearing the assignee. Fix: when the principal identity has no username, compare against the current
  email even if the current username is set (or key ExternalResourceMatch by username first and match on
  that). Add a test where the current device has both a username and the matching email.

In `pkg/connector/group.go`:
- Around line 254-262: Revoke treats an empty members read as GrantAlreadyRevoked without writing, even
  though Grants (lines 104-114) treats that same read as a known flaky response and retries it. Retry the
  GetGroupDetails read the same way, and if members are still empty return a FailedPrecondition or
  Unavailable error instead of reporting the grant as revoked.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 160-164: Entitlements only emits the "assigned" entitlement for devices that already have
  an assignee, so unassigned devices can never be granted. Either always emit the entitlement, or keep it
  as a documented limitation.

In `spec/openapi.json` (missing):
- Run the build-openapi-spec.md skill so the spec covers the new endpoints: Jamf Pro v4
  computers-inventory(-detail) GET/PATCH, Jamf Pro v2 mobile-devices/{id} PATCH and /detail GET, and the
  Classic API usergroups/id, users/id, accounts/userid and accounts/groupid PUTs.

[github-actions]

Superseded — see the current review report for commit e080cef533fd

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 2 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: full
View review run

Review Summary

This PR adds Grant/Revoke for Groups, User Groups, Roles, Sites and Managed Devices:

• Writes: Classic API PUTs with minimal XML bodies, and Pro API JSON PATCHes for devices.
• Fresh reads: jamf.WithFreshReads bypasses the uhttp GET cache on every provisioning path.
• Sync changes: computers inventory moves from v1 to v4, and a user's sites decoding is fixed, so user-to-site grants now appear in sync.
• Head commit: replaces role.go's roleOps interface with a rolePrincipal struct. It changes no behavior.

I scanned the full PR diff for security and correctness and read the full Grant/Revoke files (PR1). The trusted repo-local criteria were applied as follows:

• Entity source (PR2): principals always come from principal.Id / gr.Principal.Id, and nothing reads entitlement.Resource.ParentResourceId.
• Logging (L1b/L3): the new logging is Debug only.
• E3: gRPC codes are kept on failure paths.
• S1–S6: no new swallowed errors in List/Entitlements/Grants.
• Dependencies: go.mod/go.sum are unchanged; uhttp.WithNoCache/WithJSONBody exist in vendored baton-sdk v0.40.0.

Coverage gaps: test files and test-server/main.go were spot-checked, not read line by line.

Security Issues

None found.

Correctness Issues

• New pkg/connector/managedDevice.go:296 — Revoke silently skips externally-matched computer grants (confidence: medium-high).
  • deviceGrants (line 946) keys an unsynced assignee by email whenever the computer has one, so principalIdentityForRevoke returns ("", email).
  • assigneeMatches (lines 384-394) only compares email when the device's current username is empty.
  • Result: on a computer with both username and email, Revoke returns GrantAlreadyRevoked and never clears the assignee.
  • The existing test (TestManagedDeviceRevoke_ExternalMatchPrincipal_Patches) only covers the case where the current username is empty.
• Prior — still present pkg/connector/group.go:254-262 — when Jamf returns an empty member list, Revoke returns GrantAlreadyRevoked without writing.
  • That empty list is the known flaky response: Grants retries it 5 times (group.go:104-114), and Grant rejects it with FailedPrecondition (group.go:174-180).
  • A flaky read therefore reports a successful revoke while the account keeps its membership. The thread was resolved, but the code is unchanged.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:160-164 — Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign a device that has no user. This is a documented limitation in README and docs/connector.mdx. (Confidence: medium)
• Prior — still present B10 — the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new endpoints:
  • Jamf Pro v4 computers-inventory(-detail) GET/PATCH
  • Jamf Pro v2 mobile-devices/{id} PATCH and /detail GET
  • Classic API usergroups/id, users/id, accounts/userid and accounts/groupid PUTs

Resolved prior findings

• Fixed: README no longer says Groups and Roles are sync-only (README.md:14).
• Fixed: Managed Device Revoke checks that the principal is still the assignee before clearing (managedDevice.go:287-302).
• Fixed: mobile devices are read from the detail endpoint, which returns the nested location.username (GetMobileDeviceDetail, managedDevice.go:363-371). The test-matcher suggestion on this is obsolete for the same reason.
• Fixed: Role Revoke checks that the principal still holds the set before revoking it (role.go:504-506). Because revoking now moves the principal to Custom (role.go:508-519), revoking Enrollment Only also works.
• Fixed: a 409 on User Group Grant is no longer treated as GrantAlreadyExists. Grant checks membership first and re-reads after a 409 (userGroup.go:146-167).

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/managedDevice.go`:
- Around line 296 (with assigneeMatches at 384-394 and deviceGrants at 946-949): an unsynced computer
  assignee's grant is keyed by email whenever the device has an email. On Revoke, principalIdentityForRevoke
  then returns ("", email), but assigneeMatches only compares emails when the current username is empty.
  For devices with both a username and an email, Revoke wrongly returns GrantAlreadyRevoked without
  clearing the assignee. Fix: when the principal identity has no username, compare against the current
  email even if the current username is set (or key ExternalResourceMatch by username first and match on
  that). Add a test where the current device has both a username and the matching email.

In `pkg/connector/group.go`:
- Around line 254-262: Revoke treats an empty members read as GrantAlreadyRevoked without writing, even
  though Grants (lines 104-114) treats that same read as a known flaky response and retries it. Retry the
  GetGroupDetails read the same way, and if members are still empty return a FailedPrecondition or
  Unavailable error instead of reporting the grant as revoked.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 160-164: Entitlements only emits the "assigned" entitlement for devices that already have
  an assignee, so unassigned devices can never be granted. Either always emit the entitlement, or keep it
  as a documented limitation.

In `spec/openapi.json` (missing):
- Run the build-openapi-spec.md skill so the spec covers the new endpoints: Jamf Pro v4
  computers-inventory(-detail) GET/PATCH, Jamf Pro v2 mobile-devices/{id} PATCH and /detail GET, and the
  Classic API usergroups/id, users/id, accounts/userid and accounts/groupid PUTs.

---

Reviewed commit: a5712f5ad4de

[github-actions]

Superseded — see the current review report for commit e080cef533fd

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: incremental since a5712f5
View review run

Review Summary

The new commits are mostly refactoring, but they also change behavior:

• Shared preamble: the Grant/Revoke setup moves into membershipIDs and membershipWriteError.
• Managed devices: the (username, email) pairs become an assignee type, and its matches now falls back to comparing emails whenever either side has no username.
• Sites: the client's AddUserSite/RemoveUserSite are replaced by a read-modify-write in site.go using UpdateUserSites.
• 409 handling: a 409 on a User Group or Site write now maps to FailedPrecondition instead of triggering a re-read.
• Group Revoke: an empty member read now returns Unavailable.
• Verification removed: every post-write re-read is gone from Role Grant/Revoke and Group Grant, along with its tests.

I scanned the full incremental diff (not partial, no dropped paths) and the full PR file list for security and correctness. go.mod/go.sum did not change.

The repo-local criteria were applied:

• PR1/PR2 (entity source): principals come from principal.Id/gr.Principal.Id only.
• S1–S6: no List/Entitlements/Grants changed in a way that swallows errors.
• L1–L7 (logging): the new logging is Debug-only (replacedPrivilegeSetAnnotation).
• E1–E3 (error codes): gRPC codes are used for the new 409 and empty-read paths.

Security Issues

None found.

Correctness Issues

• New pkg/connector/role.go:307,343,476,497: the post-write getRolePrincipal check is gone from all four Role write paths, and 4 *_NotApplied/*_NotRemoved tests were deleted with it. The removed code said Jamf answers 201 even when it silently drops a privilege name, so Grant/Revoke can now report success when nothing changed. The PR description still says "Every write is verified by re-reading". (Confidence: high that the behavior was removed; medium on how often Jamf drops writes in practice.)

Suggestions

• New pkg/connector/group.go:187: Group Grant no longer re-reads after the PUT, and its test was deleted. The PR description still says "Grant re-reads the group afterwards because Jamf silently drops unknown members". The fresh account read before the write lowers the risk. (Confidence: medium.)
• Prior — still present pkg/connector/managedDevice.go:187-190: Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign an unassigned device. This is documented as a known limitation. (Confidence: medium.)
• Prior — still present B10: the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new Jamf Pro v4 computers-inventory, v2 mobile-devices and Classic API PUT endpoints.

Resolved prior findings

• Fixed: Group Revoke on an empty member read now returns codes.Unavailable instead of GrantAlreadyRevoked, and makes no write (group.go:217-228). It is covered by TestGroupRevoke_EmptyMemberList_ReturnsRetryableErrorNoWrite.
• Fixed: Revoke now clears an externally matched, email-only computer grant even when the device also reports a username. assignee.matches falls back to email when either side has no username (managedDevice.go:371-381), and TestManagedDeviceRevoke_ExternalMatchPrincipal_EmailOnly_DeviceHasUsername_Clears covers it. This matches how deviceGrants keys unsynced assignees by email.
• Fixed (rechecked):
  • README no longer calls Groups/Roles sync-only.
  • Managed Device Revoke checks the live assignee before clearing (managedDevice.go:323).
  • Mobile devices are read through GetMobileDeviceDetail, so the test-matcher suggestion is obsolete.
  • Role Revoke checks that the principal still holds the set (role.go:468-470) and moves it to Custom, so revoking Enrollment Only works.
• Fixed: a 409 on a User Group write is no longer treated as GrantAlreadyExists. membershipWriteError maps it to FailedPrecondition (helpers.go:95-100).

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/role.go`:
- Around lines 300-348 (grantPrivilegeSet, grantIndividualPrivilege) and 466-500 (revokePrivilegeSet, revokeIndividualPrivilege): the post-write verification was removed. Jamf returns 201 even when it silently drops a privilege name, so these methods can report success without applying the change. After each successful updateRolePrincipal call, re-read with o.getRolePrincipal(ctx, <principal id>) and return status.Errorf(codes.FailedPrecondition, ...) when:
  - Grant of a set: verify.privilegeSet != target
  - Grant of an individual privilege: !verify.privileges.Contains(target)
  - Revoke of a set: verify.privilegeSet != privilegeSetCustom
  - Revoke of an individual privilege: verify.privileges.Contains(target)
  Restore the matching role_test.go cases (the *_NotApplied_ReturnsFailedPrecondition / *_NotRemoved tests). If the removal was intentional, update the PR description and docs, which still say every write is verified.

## Suggestions

In `pkg/connector/group.go`:
- Around line 187: Grant no longer re-reads the group after UpdateGroupMembers to confirm userID is a member. Either restore the GetGroupDetails check (return FailedPrecondition when the member is missing) and TestGroupGrant_PrincipalNotAppliedAfterWrite_ReturnsError, or update the PR description, which still says Grant re-reads the group.

In `pkg/connector/managedDevice.go`:
- Around lines 187-190: Entitlements only emits the "assigned" entitlement for devices that already have an assignee, so an unassigned device can never be granted. Consider always emitting it. This is currently documented as a limitation.

In `spec/openapi.json`:
- The file does not exist. Run the build-openapi-spec.md skill so it covers the Jamf Pro v4 computers-inventory(-detail) GET/PATCH, the v2 mobile-devices/{id} PATCH and /detail GET, and the Classic API usergroups/users/accounts PUTs.

[github-actions]

Superseded — see the current review report for commit a4bb9955d8ff

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: incremental since a5712f5
View review run

Review Summary

The new commits are mostly refactoring, but they also change behavior:

• Shared preamble: the Grant/Revoke setup moves into membershipIDs and membershipWriteError.
• Managed devices: the (username, email) pairs become an assignee type, and its matches now falls back to comparing emails whenever either side has no username.
• Sites: the client's AddUserSite/RemoveUserSite are replaced by a read-modify-write in site.go using UpdateUserSites.
• 409 handling: a 409 on a User Group or Site write now maps to FailedPrecondition instead of triggering a re-read.
• Group Revoke: an empty member read now returns Unavailable.
• Verification removed: every post-write re-read is gone from Role Grant/Revoke and Group Grant, along with its tests.

I scanned the full incremental diff (not partial, no dropped paths) and the full PR file list for security and correctness. go.mod/go.sum did not change.

The repo-local criteria were applied:

• PR1/PR2 (entity source): principals come from principal.Id/gr.Principal.Id only.
• S1–S6: no List/Entitlements/Grants changed in a way that swallows errors.
• L1–L7 (logging): the new logging is Debug-only (replacedPrivilegeSetAnnotation).
• E1–E3 (error codes): gRPC codes are used for the new 409 and empty-read paths.

Security Issues

None found.

Correctness Issues

• New pkg/connector/role.go:307,343,476,497: the post-write getRolePrincipal check is gone from all four Role write paths, and 4 *_NotApplied/*_NotRemoved tests were deleted with it. The removed code said Jamf answers 201 even when it silently drops a privilege name, so Grant/Revoke can now report success when nothing changed. The PR description still says "Every write is verified by re-reading". (Confidence: high that the behavior was removed; medium on how often Jamf drops writes in practice.)

Suggestions

• New pkg/connector/group.go:187: Group Grant no longer re-reads after the PUT, and its test was deleted. The PR description still says "Grant re-reads the group afterwards because Jamf silently drops unknown members". The fresh account read before the write lowers the risk. (Confidence: medium.)
• Prior — still present pkg/connector/managedDevice.go:187-190: Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign an unassigned device. This is documented as a known limitation. (Confidence: medium.)
• Prior — still present B10: the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new Jamf Pro v4 computers-inventory, v2 mobile-devices and Classic API PUT endpoints.

Resolved prior findings

• Fixed: Group Revoke on an empty member read now returns codes.Unavailable instead of GrantAlreadyRevoked, and makes no write (group.go:217-228). It is covered by TestGroupRevoke_EmptyMemberList_ReturnsRetryableErrorNoWrite.
• Fixed: Revoke now clears an externally matched, email-only computer grant even when the device also reports a username. assignee.matches falls back to email when either side has no username (managedDevice.go:371-381), and TestManagedDeviceRevoke_ExternalMatchPrincipal_EmailOnly_DeviceHasUsername_Clears covers it. This matches how deviceGrants keys unsynced assignees by email.
• Fixed (rechecked):
  • README no longer calls Groups/Roles sync-only.
  • Managed Device Revoke checks the live assignee before clearing (managedDevice.go:323).
  • Mobile devices are read through GetMobileDeviceDetail, so the test-matcher suggestion is obsolete.
  • Role Revoke checks that the principal still holds the set (role.go:468-470) and moves it to Custom, so revoking Enrollment Only works.
• Fixed: a 409 on a User Group write is no longer treated as GrantAlreadyExists. membershipWriteError maps it to FailedPrecondition (helpers.go:95-100).

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/connector/role.go`:
- Around lines 300-348 (grantPrivilegeSet, grantIndividualPrivilege) and 466-500 (revokePrivilegeSet, revokeIndividualPrivilege): the post-write verification was removed. Jamf returns 201 even when it silently drops a privilege name, so these methods can report success without applying the change. After each successful updateRolePrincipal call, re-read with o.getRolePrincipal(ctx, <principal id>) and return status.Errorf(codes.FailedPrecondition, ...) when:
  - Grant of a set: verify.privilegeSet != target
  - Grant of an individual privilege: !verify.privileges.Contains(target)
  - Revoke of a set: verify.privilegeSet != privilegeSetCustom
  - Revoke of an individual privilege: verify.privileges.Contains(target)
  Restore the matching role_test.go cases (the *_NotApplied_ReturnsFailedPrecondition / *_NotRemoved tests). If the removal was intentional, update the PR description and docs, which still say every write is verified.

## Suggestions

In `pkg/connector/group.go`:
- Around line 187: Grant no longer re-reads the group after UpdateGroupMembers to confirm userID is a member. Either restore the GetGroupDetails check (return FailedPrecondition when the member is missing) and TestGroupGrant_PrincipalNotAppliedAfterWrite_ReturnsError, or update the PR description, which still says Grant re-reads the group.

In `pkg/connector/managedDevice.go`:
- Around lines 187-190: Entitlements only emits the "assigned" entitlement for devices that already have an assignee, so an unassigned device can never be granted. Consider always emitting it. This is currently documented as a limitation.

In `spec/openapi.json`:
- The file does not exist. Run the build-openapi-spec.md skill so it covers the Jamf Pro v4 computers-inventory(-detail) GET/PATCH, the v2 mobile-devices/{id} PATCH and /detail GET, and the Classic API usergroups/users/accounts PUTs.

---

Reviewed commit: e080cef533fd

[github-actions]

Blocking issues found — see the full review report

The remaining finding is the optimistic-write approach requested in review: Role and Group writes no longer re-read after the PUT (an unknown privilege or member can't come from the synced catalog, and the next sync reflects the real state). The empty-group Revoke and the device Revoke issues are fixed, and CI is green.

[github-actions]

Superseded — see the current review report for commit a4bb9955d8ff

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: incremental since e080cef
View review run

Review Summary

The new commit is a refactor plus comment cleanup. In managedDevice.go, the assignee type now carries realname, position and phone. resolvePrincipalUser returns an assignee, and setAssignedUser builds jamf.ComputerAssignedUserFields from it. assignee.matches and empty() still look only at username and email, and nothing compares assignee values with ==, so the extra fields do not change Grant/Revoke matching. The stale comments in role.go:170 (re-read after write) and models.go:374 were updated.

I scanned the full PR diff (32 files) for security and correctness. I did not re-read all ~7.9k lines line by line: I checked the incremental hunks in full, re-checked every prior finding against the current code, and pattern-scanned the rest. The incremental diff dropped no paths. go.mod/go.sum are not changed.

How the repo-local criteria applied:

• PR1/PR2: principals still come from principal.Id and gr.Principal.Id.
• S1–S6: no List, Entitlements or Grants method was changed to swallow errors.
• L-rules: the new logging is Debug only.
• E-rules: the status.Errorf(..., %v) in helpers.go carries an explicit gRPC code, which is acceptable.
• Sections C/D/E/G: not triggered by this push.

The PR description now describes Group and Role writes as optimistic (no re-read after the PUT). That resolves the earlier mismatch between the description and the code.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:191: Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign an unassigned device. This is documented as a known limitation. The existing thread covers it. (Confidence: medium.)
• Prior — still present B10: the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new Jamf Pro v4 computers-inventory endpoint, the v2 mobile-devices endpoint and the Classic API PUT endpoints. (Confidence: high.)

Resolved prior findings

• Fixed: the README no longer says Roles are sync-only (README.md:14 lists Role Grant/Revoke).
• Fixed (description aligned): the PR description now states that writes are optimistic and verified by the next sync. This covers the missing re-read after a Group Grant (group.go:187) and after Role writes (role.go:307/342/476/497). This push also removed the stale rolePrincipal comment that claimed writes are re-read (role.go:170). The remaining risk is a deliberate design choice: entitlements come from Jamf's own privilege catalog.
• Fixed: Group Revoke on an empty member read returns codes.Unavailable without writing (group.go:218-228).
• Fixed: Managed Device Revoke only clears the device when the principal is still the live assignee (managedDevice.go:325). Role Revoke checks that the principal still holds the set and moves it to Custom, so revoking Enrollment Only works.
• Fixed: a 409 on a User Group or Site write maps to FailedPrecondition (helpers.go membershipWriteError).
• Obsolete: the mobile-device GET shape issue and the loose test-matcher suggestion. Mobile devices are now read through GetMobileDeviceDetail with location.username.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 189-193: Entitlements returns no "assigned" entitlement for devices without a current assignee, so Grant can never assign an unassigned device. Consider always emitting the entitlement and only skipping the grant when there is no assignee. Otherwise, keep the documented limitation.

In `spec/openapi.json` (missing):
- The repo has no OpenAPI spec. Run the `build-openapi-spec.md` skill to generate `spec/openapi.json` covering the Jamf Pro v4 computers-inventory, v2 mobile-devices and Classic API PUT endpoints used by the new provisioning code.

[github-actions]

Connector PR Review: [CXH-2344] - Add Grant/Revoke provisioning for Groups, User Groups, Roles, Sites, and Managed Devices

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ebfa9dc243b5.
Review mode: incremental since e080cef
View review run

Review Summary

The new commit is a refactor plus comment cleanup. In managedDevice.go, the assignee type now carries realname, position and phone. resolvePrincipalUser returns an assignee, and setAssignedUser builds jamf.ComputerAssignedUserFields from it. assignee.matches and empty() still look only at username and email, and nothing compares assignee values with ==, so the extra fields do not change Grant/Revoke matching. The stale comments in role.go:170 (re-read after write) and models.go:374 were updated.

I scanned the full PR diff (32 files) for security and correctness. I did not re-read all ~7.9k lines line by line: I checked the incremental hunks in full, re-checked every prior finding against the current code, and pattern-scanned the rest. The incremental diff dropped no paths. go.mod/go.sum are not changed.

How the repo-local criteria applied:

• PR1/PR2: principals still come from principal.Id and gr.Principal.Id.
• S1–S6: no List, Entitlements or Grants method was changed to swallow errors.
• L-rules: the new logging is Debug only.
• E-rules: the status.Errorf(..., %v) in helpers.go carries an explicit gRPC code, which is acceptable.
• Sections C/D/E/G: not triggered by this push.

The PR description now describes Group and Role writes as optimistic (no re-read after the PUT). That resolves the earlier mismatch between the description and the code.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present pkg/connector/managedDevice.go:191: Entitlements only emits assigned for devices that already have an assignee, so Grant can never assign an unassigned device. This is documented as a known limitation. The existing thread covers it. (Confidence: medium.)
• Prior — still present B10: the repo has no spec/openapi.json. Run the build-openapi-spec.md skill so it covers the new Jamf Pro v4 computers-inventory endpoint, the v2 mobile-devices endpoint and the Classic API PUT endpoints. (Confidence: high.)

Resolved prior findings

• Fixed: the README no longer says Roles are sync-only (README.md:14 lists Role Grant/Revoke).
• Fixed (description aligned): the PR description now states that writes are optimistic and verified by the next sync. This covers the missing re-read after a Group Grant (group.go:187) and after Role writes (role.go:307/342/476/497). This push also removed the stale rolePrincipal comment that claimed writes are re-read (role.go:170). The remaining risk is a deliberate design choice: entitlements come from Jamf's own privilege catalog.
• Fixed: Group Revoke on an empty member read returns codes.Unavailable without writing (group.go:218-228).
• Fixed: Managed Device Revoke only clears the device when the principal is still the live assignee (managedDevice.go:325). Role Revoke checks that the principal still holds the set and moves it to Custom, so revoking Enrollment Only works.
• Fixed: a 409 on a User Group or Site write maps to FailedPrecondition (helpers.go membershipWriteError).
• Obsolete: the mobile-device GET shape issue and the loose test-matcher suggestion. Mobile devices are now read through GetMobileDeviceDetail with location.username.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/managedDevice.go`:
- Around line 189-193: Entitlements returns no "assigned" entitlement for devices without a current assignee, so Grant can never assign an unassigned device. Consider always emitting the entitlement and only skipping the grant when there is no assignee. Otherwise, keep the documented limitation.

In `spec/openapi.json` (missing):
- The repo has no OpenAPI spec. Run the `build-openapi-spec.md` skill to generate `spec/openapi.json` covering the Jamf Pro v4 computers-inventory, v2 mobile-devices and Classic API PUT endpoints used by the new provisioning code.

---

Reviewed commit: a4bb9955d8ff

[github-actions]

No blocking issues found — see the full review report