fix(scope)!: resolve --computer/--mobile-device to an ID and send <id> alone

[neilmartin83]

Problem

scope add/remove --computer <UDID> answered 409 Unable to match computer on every computer-scoped resource, although --help advertised UDIDs. There were two defects:

1. udidRe matched only the 40-hex UDID older iOS devices use, so a Mac's UUID-shaped UDID was sent as <name>.
2. Once a UDID was matched, NamedItem.Name had no omitempty, so the body carried <name></name><udid>…</udid>.

Wire behaviour (platform-nmjspp, 2026-10-01, raw PUTs, targets and exclusions)

• <id>, <name>, <udid> and <serial_number> each resolve on their own, case-insensitively.
• Computer matcher: reads id → name → udid and refuses at the first element present that doesn't match. An empty <name>, an empty or stale <id>, or id=0 ahead of a good <udid> all answer 409. A good <id> wins over a bad name or UDID.
• Mobile-device matcher: accepts the item if any element matches, which is why mobile UDIDs already worked.
• Names aren't unique, and the server's own name match picks one duplicate silently.

<id> alone is the only form that is unambiguous and resolves on both matchers.

Fix

• resolve.ResolveComputerIdentifier / ResolveMobileDeviceIdentifier: one inventory request that ORs id (when numeric), name, UDID and serial number. Endpoints are /v4/computers-inventory and /v2/mobile-devices/detail, both published on the gateway.
  • A numeric value that is some device's ID wins.
  • Results are re-checked exactly, because RSQL == treats * as a wildcard.
  • A value matching several devices is refused with the ids listed.
  • Not-found is the typed NoDeviceMatchError.
• Scope add/remove resolve --computer / --mobile-device first and send <id> alone.
  • The duplicate check, removal and read-back verification compare IDs, so remove by serial number now works. Before, it could never match: the scope GET carries no serial.
  • remove falls back to the literal value only on no-match, so a member whose inventory record is gone can still be removed.
• Messages show the resolution, e.g. Added computer "fwwt058jg5" (id 107, ARMADA-058JG5) to exclusion scope of id 5717.
• parseMobileDevice now reads general.udid and hardware.serialNumber, which is where /detail puts them.
• Help text names the accepted identifiers and the extra read privilege.

Breaking

• A device name shared by more than one device is refused instead of the server picking one; pass the ID.
• Adding or removing a device needs Read Computers / Read Mobile Devices on the API client.

Verification

• Wire matrix, platform-nmjspp: 8 resource × device-kind combinations (policies, macOS profiles, mac apps, restricted software, ebooks × computer; mobile apps, mobile profiles, ebooks × mobile device), targets and exclusions. Each adds by id, name, UDID and serial number and removes by a different kind each time: 128 operations, all OK. Every PUT body carried <id> only, and every scope ended byte-identical to where it started. On main, the same matrix failed every computer UDID add with a 409.
• pro-nmartin: a duplicate name (computers 4 and 31) is refused with both ids listed, and a not-found value gets a clean message.
• Tests: unit tests for the resolver (each identifier kind, OR filter, id precedence, ambiguity, wildcard re-check, lookup failure vs no-match, mobile /detail shape) and for target resolution (<id>-only body, remove by serial, remove fallback, non-device flags untouched). Mutation-checked. go test ./... passes.

🤖 Generated with Claude Code

[ktn-jamf]

Warning

⚠️ Needs changes. Resolves --computer / --mobile-device to a numeric ID before any scope write and sends <id> alone. The design is sound and well verified on the wire.
Blocking: (1). Two nice-to-haves sit in the collapsed section.

Rating: 3/5

• Would be a 5 with the CHANGELOG entry in (1).
• Coverage: no specialist lane ran (reviewed inline); the rating reflects the dimensions I searched, not a lane panel.

Findings

⚠️ IMPORTANT (1) (conventions) — CHANGELOG.md: a self-declared breaking change adds no entry

The title is fix(scope)!: and the body has a Breaking section (shared device names refused, new Read Computers / Read Mobile Devices requirement). CHANGELOG.md opens by saying it exists to record breaking changes and "what the migration is", and the other recent ! PRs (#407, fe2a3e6f, f012e1c5) each added an entry under ## Unreleased. This diff touches none.

Failure scenario: a script that runs scope add --computer <name> with an API client lacking Read Computers, or on a name shared by two devices, starts failing after upgrade, and the only place that explains it is the PR body, which release notes do not carry.

Suggested fix: add a ### Breaking — ... entry under ## Unreleased covering the two behaviour changes, the migration (pass the numeric ID; grant Read Computers / Read Mobile Devices), and the fact that remove by serial number now works.

Fixed when: CHANGELOG.md carries the entry in this PR.

Nice-to-have suggestions (2 items)

💡 NICE-TO-HAVE (2) (correctness) — internal/scope/scope.go:345, :463, internal/scope/verify.go:114: after resolution name is a numeric ID, but AddToScope, removeNamedItem and itemPresent still also match it against item.Name / item.UDID. A different member whose name is the same digits (a device renamed 107) makes add report "already in scope" and remove delete both. The PR text says these now "compare IDs"; for a resolved device they could compare item.ID only. Promote to IMPORTANT when: device names that are all digits occur in a tenant using this command.

💡 NICE-TO-HAVE (3) (usability) — internal/resolve/identifier.go:103: a failed lookup (for example the 403 when the client lacks Read Computers) surfaces as looking up computer "5": HTTP 403. The new requirement is documented in --help, but the error does not name the privilege, and a plain numeric ID that worked before now fails the same way. A hint on 401/403 naming Read Computers / Read Mobile Devices would make the breaking change self-explaining.

This covers all findings — addressing (1) gets this PR to merge-ready.

Review coverage and scope

• Correctness: resolver OR-filter, numeric-ID precedence, wildcard re-check, ambiguity refusal, remove fallback, <id>-only body (NamedItem.Name omitempty). Finding (2)
• Test coverage: resolver and target-resolution tests present; go test ./internal/scope/ ./internal/resolve/ passes at the reviewed head (run with JAMF_URL=https://nonexistent.invalid).
• Conventions: CHANGELOG policy, finding (1). Output routing, flag rules and generated-code boundary not touched.
• Security: no credential handling; user value is RSQL-escaped (EscapeRSQL) and URL-escaped.
• [na] Performance (one extra GET per device flag), frontend, migrations, dependencies.

Diff: 8 files, +559/−25, head 14fbe40. Lanes run: none, reviewed inline; the gateway and wire claims in the PR body (platform-nmjspp, 128 operations) were not re-run against a tenant. Never run: all specialist lanes. Conventions: root CLAUDE.md and the three imported rules files, read from the PR worktree.

_{🤖 Generated by the pr-review:review skill · reviewed head 14fbe40}

[neilmartin83]

Thanks. All three are addressed in 7b6edb98 and 24fe0f99. 61c094ff merges main (#402), which touched the CHANGELOG too.

(1) CHANGELOG. There is now a ### Breaking entry under ## Unreleased. It covers:

• The refusal of a name shared by more than one device.
• The new Read Computers / Read Mobile Devices requirement.
• That remove by serial number now works.
• The migration: pass the ID, and grant the read privilege.

(2) Resolved ID matched against names. A resolved target now matches item.ID only, through ScopeTarget.matches. This applies in four places:

• add's duplicate check (AddTargetToScope).
• remove (RemoveTargetFromScope).
• The verification read (targetPresent).
• DiffScope's exclusion of the touched member.

An unresolved literal (remove's no-match fallback) still matches by name, ID or UDID. Tests cover the helpers and both commands, with a computer 31 named 107 beside computer 107. Four mutations were checked: reverting the matcher, either call site, or the verify call each fails a test.

(3) 403 hint. A 401 or 403 on the inventory lookup keeps its exit code. It adds a hint naming Read Computers or Read Mobile Devices, and keeps any hint the client already attached. A 5xx gets no privilege hint. Mutation-checked.

Wire, platform-nmjspp, on a disposable policy (5809, since deleted):

Command	Result
add by serial	Added computer "FWWT058JG5" (id 107, ARMADA-058JG5)
add exclusion by UDID	id 106 added
add by name, already present	no-op, already in target scope
remove by name	removed
remove exclusion by serial	removed
add an unknown value	refused, exit 1

The scope ended empty, as it started. go test ./internal/... passes and make lint reports 0 issues.

[ktn-jamf]

Tip

✅ Merge-ready. All three round-1 findings are fixed in 7b6edb98 and 24fe0f99. Resolved devices now match by ID alone, a 401/403 on the lookup names the missing privilege, and the CHANGELOG carries the breaking entry.

Rating: 5/5

• Every prior finding is fixed in code; nothing new found in the incremental diff.
• Coverage: no specialist lane ran (reviewed inline); the rating reflects the dimensions I searched, not a lane panel.

Prior findings status

#	Location	Rounds	State	Notes
(1)	CHANGELOG.md	1	✅ Fixed	### Breaking — scope add/remove resolve --computer and --mobile-device to an ID first under ## Unreleased: shared-name refusal, Read Computers / Read Mobile Devices requirement, remove by serial, migration. The "exit 5" claim matches exitcode.PermissionDenied.
(2)	internal/scope/scope.go, verify.go	1	✅ Fixed	ScopeTarget.matches compares item.ID only when Device is set. Used by AddTargetToScope, RemoveTargetFromScope, targetPresent and DiffScope. The literal remove fallback still matches name, ID or UDID.
(3)	internal/resolve/identifier.go	1	✅ Fixed	lookupError adds a hint naming readPrivilege on exit codes 3 and 5, keeps any hint the client attached, and leaves other errors unwrapped.

Review coverage and scope

• Correctness: the new matcher at all four call sites; no remaining caller of the old name-matching path for a resolved device.
• Test coverage: go test ./internal/scope/ ./internal/resolve/ passes at the reviewed head (run with JAMF_URL=https://nonexistent.invalid). The PR reports a computer named 107 beside computer 107, and four mutation checks.
• Conventions: CHANGELOG policy satisfied.
• [na] Security, performance, frontend, migrations, dependencies: unchanged since round 1.

Diff since 14fbe40: the PR's own changes are CHANGELOG.md, internal/resolve/identifier.go, internal/scope/{commands,scope,types,verify}.go and tests. The rest of the range is the main merge (#402, MCP run_command), which is not part of this PR's change. Lanes run: none. Never run: all specialist lanes. The wire results the author posted (platform-nmjspp) were not re-run; no tenant was called.

_{🤖 Generated by the pr-review:review skill · reviewed head 24fe0f9}