fix: resolve an identifier to exactly one record before acting on it

[ktn-jamf]

Why

Several commands resolve a name, serial or UDID to a record before a destructive action. They picked the wrong record when two records matched, or when the lookup failed. Each case below was reproduced against a loopback fake:

• School devices. All seven device actions, including erase, unenroll and clear-activation-lock, checked device names before serials and UDIDs, and the last duplicate won. An iPad renamed to another device's serial received that device's wipe.
• --group on device actions (27 commands through deviceTarget). The XML converter turned a numeric-looking group name into a float, so --group 14 resolved to the group named "14.2". The Classic fallback ran on any smart-group error, including the server's own "multiple groups found" refusal, and then took the first case-insensitive match. flush-commands --group, pro bulk send-command --group and add-to-group / remove-from-group had the same first-match rule.
• Package upload. The filename went into an RSQL filter unescaped, and the lookup asked for one result. A crafted name such as Nope",fileName=="GlobalSecurityAgent.pkg replaced a different package. Two packages that shared a filename resolved to one, silently.
• Protect computers. A hostname shadowed a serial, and the last duplicate won. delete, set-plan and update acted on the wrong computer. The other Protect name resolvers also let the last duplicate win.
• Protect apply, all 13. Any lookup error, such as a 403, a 5xx, a GraphQL error or a timeout, took the create branch with no confirmation. A failed read became a duplicate. For api-clients, that duplicate was a new credential.
• Classic scope add/remove/set --name. These used the server's /name/ endpoint, which picks one record when names collide.

Found by the 2026-09-29 security scan. These are the name-resolution findings: School device resolver, numeric --group coercion, package upload RSQL, Protect computer resolver and plans apply resolver error. Each one was verified, and several reached further than the scan said.

What changed

• internal/pickone is a small helper that takes ordered tiers, for example exact ID, then serial, then name. The first tier with any match decides. More than one match in that tier is refused, and the error names the candidates. School, Protect and the Classic group scan use it.
• School devices match UDID, then serial, then name. The UDID fallback applies only to a not-found result and an argument shaped like a UDID. Confirmations show the resolved device.
• Group resolution reads the raw <name> text. A unique exact-case match wins, then a unique case-insensitive one, and anything else is refused. The Classic fallback runs only on an empty result or a 404. flush-commands, pro bulk and the blueprint group lookup now resolve from the collection, and the blueprint lookup refuses more than one match.
• RSQL. resolve.EscapeRSQL escapes \ and ", which the quoted value needs. Every lookup that feeds a write reads the requested value back off the result. Package upload asks for two results, refuses more than one, and requires an exact fileName match. pro_blueprints.go now uses the shared escaper.
• Protect. Every name resolver refuses a duplicate. apply and import create only on protect.ErrNotFound.
• Scope --name resolves by listing the collection for every scopeable resource. A non-200 status is an error.
• Also tightened.
  • pro setup role and integration lookups read back displayName.
  • set-auto-admin-password resolves exactly one LAPS account.
  • The serial lookup reads the serial back, and moves on to other lookups only on a 404.
  • The Protect insights restore refuses a duplicate label.

Verification

go test ./internal/pickone/ ./internal/resolve/ ./internal/protect/ ./internal/school/ ./internal/scope/ -count=1
go test ./internal/commands/ -count=1

The first commit adds the repro tests alone, and they fail. For example, erase VICTIMSN01 sent POST /api/devices/ATTACKER-UDID-0002/wipe, and --group "14" resolved to "14.2". A hostile package name resolved to package 1, and a failed Protect lookup sent createPlan. The later commits make them pass.

TestProGroupAction_DuplicateGroupNameIsRefusedThroughTheRoot drives the shipped erase --group and flush-commands --group commands through NewRootCmd. It asserts a refusal that names both group IDs, with zero writes, and a positive control checks that a unique name sends exactly one. A table runs all 13 Protect apply commands against five lookup failures plus a duplicate.

Existing tests changed, with approval:

• TestFetchScope_ByNameUsesTheNameEndpoint now asserts the collection lookup, followed by a write by ID.
• Eight tests gained a collection mock entry, and none of their assertions changed. These are two in resolve_test.go and six in pro_command_flush_test.go.
• One stale /proclassic/{}/name/{} entry was removed from gateway_handwritten_paths_test.go, as that test's own guard requires.

Review ran security twice and test quality once. The second security round closed every first-round finding.

Known leniency. A pro setup result with no displayName is still accepted. So is a serial result with no serial. Fixtures on main have neither field, and a result that carries a different value is refused.

🤖 Generated with Claude Code

[neilmartin83]

Caution

❌ Needs changes. Resolves names, serials and UDIDs to exactly one record before destructive actions across School, Pro, Protect and Classic scope.
Blocking: (1), (2). Both are regressions confirmed against a live Jamf Pro 11.x tenant. The unit fakes return response shapes the server does not send, so the suite stays green.

Rating: 2/5

• The design is sound, and the scope --name fix is confirmed on the wire. Fixing (1) and (2), and pointing their fixtures at the real wire shapes, would make this a 4/5 or 5/5.

Findings

❌ CRITICAL (1) (correctness) — internal/commands/pro_device_resolve.go:34: a serial or name passed to pro device now fails at the ID lookup and never reaches the serial or name search

tryDeviceByID now returns errNoDeviceWithID only on a 404. Jamf Pro answers a non-numeric ID on /v4/computers-inventory-detail/{x} with 400 INVALID_ID ("id field must be string of positive numeric value or -1"). Every serial and every name is non-numeric, so resolution stops at step 1.

Wire-checked on pro-nmartin. main resolves pro device FVFC41HCLYWP. The PR exits 1 with looking up "FVFC41HCLYWP" as a device ID: request failed (HTTP 400). pro classic-computer-app-usage --serial and --name take the same path and fail the same way, while --udid still works.

Failure scenario: jamf-cli pro device <serial> or pro classic-computer-app-usage --serial X --last 7d → HTTP 400 at the ID probe → exit 1. Both commands worked on main.

Suggested fix: probe by ID only when isNumericID(identifier). Alternatively, treat a 400 INVALID_ID like a 404. Keep the stricter handling for 401, 403 and 5xx, which is the intent here.

Fixed when: the mocks in pro_device_resolve_test.go answer a non-numeric ID with the real 400 INVALID_ID body instead of 404, and serial and name resolution pass against that mock.

❌ CRITICAL (2) (correctness) — internal/resolve/resolve.go:491: every mobile --serial lookup is refused by the new readback

resolveMobileByFilter queries /v2/mobile-devices/detail with no section parameter. That endpoint nests the serial at hardware.serialNumber and returns "hardware": null unless HARDWARE is requested. parseMobileDevice reads serialNumber from the top level only, so SerialNumber is always empty and confirm rejects every match.

Wire-checked: -n pro mobile-devices restart --serial VGP6T0HP95 --yes resolves to id 10 on main. The PR answers no mobile device found with serial number "VGP6T0HP95" (the server returned 10, whose serial number is ""). --name still works, because general.displayName is populated by default. Every pro mobile-devices action that takes --serial is affected, erase included.

The fixture mobileV2Response (resolve_test.go:92) has the flat shape of the list endpoint (top-level serialNumber, udid, displayName). It does not match the detail endpoint the code calls.

Failure scenario: jamf-cli pro mobile-devices erase --serial <real serial> → the record comes back with hardware: null → readback compares "" to the serial → "no mobile device found" → exit 1.

Suggested fix: add &section=GENERAL&section=HARDWARE to the detail query, and read hardware.serialNumber (and general.udid) in parseMobileDevice as fallbacks.

Fixed when: mobileV2Response mirrors the real /detail shape (mobileDeviceId, general{displayName,udid,managementId}, hardware{serialNumber}), and the serial lookup test passes against it.

⚠️ IMPORTANT (3) (security, pre-existing) — internal/commands/pro_platform_devices.go:64: resolveDeviceIDDirect still takes the first match with no readback

This function is outside the diff. It is the same class of defect the PR closes everywhere else, and it is the one serial resolver left untouched. serialNumber==%q uses Go quoting, not EscapeRSQL, so * stays a wildcard and " or \ mis-escape. The function returns list[0].ID with no count check and no readback, and pro platform devices delete confirms with the typed argument rather than the device it resolved.

Failure scenario: jamf-cli pro platform devices delete 'C02*' --yes → several devices match → the first in server order is deleted.

Suggested fix: use resolve.EscapeRSQL, keep only results whose serial EqualFolds the argument, refuse zero or more than one match, and name the resolved device in the confirmation. A follow-up PR is fine, but it should be named in this PR's description, because the description claims the whole class is fixed.

Fixed when: a wildcard or multi-match serial is refused, and the delete prompt names the resolved device.

Nice-to-have suggestions (5 items)

💡 NICE-TO-HAVE (4) (correctness) — internal/commands/pro_blueprints.go:1644: the readback GroupName != groupName is case-exact, but /v2/groups filters case-insensitively (wire-checked: groupName=="excluded" returns Excluded). A wrong-case name used to resolve and now reports "no group found". Every other readback in this PR uses EqualFold.

💡 NICE-TO-HAVE (5) (security) — internal/commands/pro_device_resolve.go:47: serial == "" || EqualFold(...) accepts a wildcard match whose record has no serial. Now that HARDWARE is requested, require a non-empty serial that matches.

💡 NICE-TO-HAVE (6) (consistency) — internal/scope/scope.go:200: resolveNameToID matches on case folding alone. pickClassicGroup tries an exact match first, so Foo beside foo resolves for --group but is refused for scope --name. Put both on pickone.One with the same tiers.

💡 NICE-TO-HAVE (7) (security) — internal/resolve/resolve.go:110: a smart group shadows a static group of the same name, because the Classic list is read only on errGroupNotFound. Since the listing holds both kinds, a refusal on a cross-type collision costs nothing extra.

💡 NICE-TO-HAVE (8) (dead-code) — internal/scope/types.go:24: nothing reads ResolveByList any more. Remove the field in the generator template instead of documenting it as dead.

This covers all findings — addressing the above gets this PR to merge-ready.

Review coverage and scope

• Correctness: every resolver in the diff. (1) and (2) were found by running main and PR binaries against live tenants
• Security: a specialist lane, plus (3), (5) and (7)
• Wire verification on pro-nmartin and school-nmartin, with both binaries:
  • Computer --serial, --name and --udid, case-insensitive serials, and a name collision (ids 4 and 31) all match main
  • Smart and static --group, flush-commands --group, and every School device form all match main
  • Scope get --name on two policies sharing a name (4972/5499): main picked one, the PR refuses ✅
• Tests: go test on the six packages the PR names passes, and go vet is clean. The fixtures behind (1) and (2) do not match the wire
• [?] Protect resolvers: reviewed in code only, with no Protect profile probed
• [na] Performance: one more list call per name lookup, which the CHANGELOG records

Diff: 53 files, +2934/−802, head 3343877. Lanes run: security. Conventions: root CLAUDE.md, plus the three .claude/rules/*.md files, from the main checkout. No prior reviews.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 3343877}

[neilmartin83]

Caution

⚠️ Needs changes. No commits since round 2, so (1) to (3) stand as they were. This round ran the lanes that had never looked at the PR and found one more regression from main: pro packages upload no longer replaces a package whose stored filename differs only in case. It now tries to create a second package, and the server refuses that with DUPLICATE_FIELD.
Blocking: (1), (2), (3), (4). Round 1's four NICE-TO-HAVEs reached their third round and are closed under Dispositions: three are dismissed and one is retired.

Rating: 3/5

• Fixing (1) to (4) makes this 5/5. The rating is unchanged from round 2. The new (4) is IMPORTANT, the same severity as (1) to (3), and the four dispositions removed only NICE-TO-HAVEs.
• Coverage: performance-reviewer, usability-reviewer, devil-advocate and scope-reviewer are still eligible and have never run as specialists. This round could not dispatch subagents, so I covered their dimensions myself and left them out of ever (see the coverage block).

Findings

⚠️ IMPORTANT (1) (correctness, open 2 rounds) — internal/commands/pro_device_resolve.go:81: pro device stops at the ID probe when given an all-digit serial or name that is not a Jamf ID. On main, the lookup went on to the serial and name searches.

client.Do never returns a 404 or 400 as a response. It turns both into errors (httpStatusError, internal/client/client.go:571), so the StatusNotFound/StatusBadRequest branches at lines 86–96 never run, and resolveDeviceByIdentifier stops at the error. Wire-checked on pro-nmartin in round 2: main gives no device found matching "99999" (exit 1, after the serial and name searches), and this PR gives resource not found (HTTP 404): GET /api/v4/computers-inventory-detail/99999 (exit 4). deviceResolveMockClient returns every status as a response with a nil error, which hides this.

Failure scenario: a fleet that names computers by asset tag runs jamf-cli pro device 1042. No computer has ID 1042, so the command exits 4 without ever searching general.name=="1042".

Suggested fix: client.Do(registry.WithAllowedStatuses(ctx, http.StatusNotFound, http.StatusBadRequest), "GET", ...), or match *exitcode.Error with Code == exitcode.NotFound.

Fixed when: the mock fails any non-allowed status the way client.Do does, a test where 99999 gets a 404 on the probe and then resolves by name passes, and pro device 99999 on a live tenant behaves as it does on main.

⚠️ IMPORTANT (2) (correctness, silent-failure, open 2 rounds) — internal/resolve/resolve.go:613: the smart-group → static-group fallback on a 404 never fires.

errors.As(err, &status) looks for resolve.httpStatusError, but only fetchInventoryPage creates that type, and only for a non-200 2xx. A real 404 comes back as client.Do's own error, so the fallback that resolve.go:607 describes never runs. mockClient answers unmatched paths with (resp{404}, nil), so the tests run a path that production never takes.

Failure scenario: a server or gateway without /v3/computer-groups/smart-groups answers 404, and --group <static group> fails with "searching for group" instead of resolving the static group.

Suggested fix: pass registry.WithAllowedStatuses(ctx, http.StatusNotFound) on that call, or match *exitcode.Error/exitcode.NotFound and drop the local type.

Fixed when: with a mock that returns an error for any status not covered by registry.StatusAllowed, a smart-group 404 reaches the static lookup.

⚠️ IMPORTANT (3) (test-coverage, open 2 rounds) — internal/resolve/resolve.go:383, :451, :630: three of the exactly-one guards survive mutation. These are deviceQuery.confirm's read-back, the mobile total > 1 refusal in resolveMobileByFilter, and the group-name read-back in resolveGroupIDByName. Every mock echoes the requested value, and the mobile side has no MultipleMatches test.

Failure scenario: a later refactor drops one of these lines and the suite stays green. --serial 'C02*' then acts on whichever record the server's wildcard returned.

Suggested fix: add mock cases whose returned serialNumber, general.name, udid, managementId and group name differ from the request, and assert both the refusal and that no action request follows. Add TestResolveMobileDevice_MultipleMatches.

Fixed when: each of the three mutations fails at least one test.

⚠️ IMPORTANT (4) (correctness, regression, via orchestrator covering devil-advocate) — internal/commands/pro_packages_upload.go:236: the new read-back compares fileName case-sensitively, but Jamf Pro matches and enforces package file names case-insensitively. So an existing package whose name differs only in case is reported as absent, and the create that follows is refused.

Wire-checked on pro-nmartin with a disposable package, deleted afterwards:

• fileName=="GEN-PKG-DATAJAR-REISSUE-FV-IMAGES.PKG" returns the package stored as gen-pkg-datajar-reissue-fv-images.pkg. RSQL == is case-insensitive.
• POST /v1/packages with zz-pr408-probe.pkg, while ZZ-PR408-Probe.pkg exists, answers 400 DUPLICATE_FIELD fileName. The uniqueness check is case-insensitive.
• pro packages upload --file …/zz-pr408-probe.pkg with this PR's binary prints "Checking for existing package... Creating package record..." and fails with that 400.

On main, page-size=1 plus totalCount == 1 returned that package's id, so the upload replaced its binary, as the command's help says ("If a package with the same filename already exists, its binary and hash metadata are updated"). No test covers a case difference.

Failure scenario: an AutoPkg or CI pipeline re-uploads Firefox-134.0.pkg to a tenant where someone created the record as firefox-134.0.pkg. On main the binary is replaced. On this PR the upload exits 1 with a raw DUPLICATE_FIELD body, and it fails the same way on every retry.

Suggested fix: compare with strings.EqualFold(name, fileName). The server forbids two names that differ only in case, so a case-insensitive exact match still identifies one record, and the totalCount > 1 refusal still catches a * in the name. If the replace should stay case-exact on purpose, refuse with a message that names the stored filename, not with the server's 400.

Fixed when: a test where the search returns {"fileName":"Foo.pkg"} for an upload of foo.pkg resolves to that package's id, and the live upload above replaces the existing package.

Nice-to-have suggestions (4 items, carried from round 2 unchanged)

💡 NICE-TO-HAVE (5) (silent-failure, open 2 rounds) — internal/resolve/resolve.go:786-810, internal/commands/pro_device_actions.go:183,201: --group drops every member whose lookup fails, 403 and 5xx included, with only a stderr warning, and hard-codes 0 unresolved. So erase --group G acts on the rest and exits 0. Return the skipped count from batchResolve* and pass it to unresolvedTargetsErr.

💡 NICE-TO-HAVE (6) (silent-failure, open 2 rounds) — internal/resolve/resolve.go:680: members, _ := detail[membersKey].([]any) turns a wrong key or a different envelope into an empty group with a nil error. The unwrap loop at :673-678 takes the first map-typed value, in random map order.

💡 NICE-TO-HAVE (7) (test-coverage, open 2 rounds) — internal/resolve/group_numeric_name_repro_test.go:~118: TestResolveComputerGroup_SmartAmbiguityIsNotResolvedByFallback still passes when errors.Is(err, errGroupNotFound) is reverted to err != nil, because its Classic mock also lists two "Lab" groups. Give the Classic list a single "Lab".

💡 NICE-TO-HAVE (8) (dead-code, open 2 rounds) — the same unreachable non-2xx pattern as (1) and (2), with cosmetic effect only: resolveComputerByID/resolveMobileByID (resolve.go:397, :469), pickClassicGroup's 404 branch (:709-714), scope.resolveNameToID's status check, and the non-200 checks at :546 and :861. Fixing the shared mocks per (1) exposes these too.

Dispositions

Disposition (round 3) — dismissed. Was (4), internal/commands/pro_device_resolve.go:49, open 3 rounds. The PR description lists this as a known leniency, and the reason holds. The query requests HARDWARE, so a wildcard hit always carries the serial it matched, and a different serial is already refused. Only a record with no serial at all gets through, and that cannot be a wildcard match on serial.

Disposition (round 3) — dismissed. Was (5), internal/scope/scope.go:200, open 3 rounds. The inconsistency fails safe. Two records whose names differ only in case are refused, the refusal names both ids, and it says to pass one as <id>. Nothing resolves to the wrong record.

Disposition (round 3) — dismissed. Was (6), internal/resolve/resolve.go:110, open 3 rounds. A smart group and a static group cannot share a name. On pro-nmartin, creating a static group named after the existing smart group "Active Directory Not Bound" answers 409 Error: Duplicate name. There is no static group for a smart group to shadow.

Disposition (round 3) — retired, out of scope. Was (7), internal/scope/types.go:24, open 3 rounds. The field is dead, and the PR's comment says so. Removing it means a generator template edit and a make generate across the generated trees, which would add generated-code churn to a security fix. It belongs in a separate cleanup PR.

PR decomposition

Keep as one PR. All 57 files serve one concern: resolve an identifier to exactly one record, or refuse. They share pickone and resolve.EscapeRSQL. Splitting by product (School, Protect, Pro groups, packages, scope) would mean landing the shared helper first and four dependent PRs after it, after two review rounds have already been spent on the combined diff. (Covered by the orchestrator, not by scope-reviewer.)

Review coverage and scope

• Gate A: the diff is unchanged since round 2 (f644279). The round ran anyway, because four eligible lanes had never run (A3) and four round-1 findings were due for a disposition (A5)
• Dispositions: round 1's (5) to (8) reached three rounds. Two dismissals rest on wire evidence (the duplicate-name 409 on pro-nmartin) and on the PR's own stated leniency
• Performance (orchestrator): the Protect resolvers keep the per-Resolver listing cache. pro bulk, flush-commands and add/remove-from-group each make one collection list, as main did. Package upload asks for page-size=2 instead of 1. Scope --name lists the collection instead of calling /name/ once, an extra list per invocation that the description records. No loop gained calls
• Usability (orchestrator): refusal messages name the candidate ids and a remedy. (1) and (4) are the user-visible regressions
• Devil-advocate (orchestrator): package upload, Protect apply's create-on-ErrNotFound, and the group fallbacks, read against wire behaviour. That produced (4)
• Wire: pro-nmartin. RSQL fileName== case and * behaviour (read-only). A disposable package record created, its case-variant create and the PR binary's upload tried, then the record deleted (fileName=="zz-pr408*" now returns []). A smart/static duplicate-name create was refused before anything was stored
• Merge with current main (cc702a40, #404): git merge-tree is clean
• [?] School resolver and internal/pickone: not re-read this round, because the tool environment refused that read. Round 1's security lane and round 2's lanes covered them
• [?] Protect resolvers: still checked in code only. No Protect profile was probed
• [na] Fidelity: no linked issue or ticket

Lanes: this run could not dispatch subagents, so ran= is empty. I covered the four never-run lanes' dimensions myself and left them out of ever, so a later round still dispatches them as specialists. (4) has no verifier score, so it is self-scored under the orchestrator carve-out, backed by the wire reproduction. Conventions: root CLAUDE.md and the three .claude/rules/*.md files. Head graded: f644279.

Prior review status

Round 2 (1), (2) and (3) open 2 rounds, still (1) to (3). Round 2 (8) to (11) open 2 rounds, now (5) to (8). Round 2 (4), (5) and (6) are dismissed and (7) is retired, all at 3 rounds (see Dispositions). No commits, replies or description edits since round 2.

This covers all findings. Fixing (1) to (4) gets this PR to merge-ready.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head f644279}

[neilmartin83]

Caution

⚠️ Needs changes. Since round 3 the branch has a main merge (#404, reviewed on its own PR, merge-tree clean) and two test-fixture commits. No production file changed: pro_device_resolve.go, resolve.go and pro_packages_upload.go are byte-identical to f644279. So (1) to (4) stand. I re-ran (1) on the wire at the new head and it still reproduces. (1), (2) and (3) reach their third round and are promoted below as merge blockers.
Blocking: (1), (2), (3), (4).

Rating: 3/5

• Unchanged from round 3. Fixing (1) to (4) makes this 5/5.

Findings

⚠️ IMPORTANT (1) (correctness, open 3 rounds, promoted) — internal/commands/pro_device_resolve.go:81: an all-digit serial or name that is not a Jamf ID stops at the ID probe

Re-checked on pro-nmartin with the binary built at 826c4d1:

$ jamf-cli -p pro-nmartin pro device 99999 -o json
"message": "looking up \"99999\" as a device ID: resource not found (HTTP 404): GET /api/v4/computers-inventory-detail/99999"

On main this goes on to the serial and name searches and reports no device found matching "99999". 826c4d1c changes the fixtures' probe answer from 404 to the server's real 400 INVALID_ID, which is more faithful. But deviceResolveMockClient still returns that status as a response with a nil error, and client.Do returns an *exitcode.Error for both 400 and 404. So the branches at :86–:96 are still unreachable in production, and the suite still cannot see it. A non-numeric name resolves correctly on the wire, as the fixtures now model. The all-digit case is the one that breaks.

Merge impact: a fleet that names computers by asset tag (pro device 1042) loses name lookup, compared with main.

Fix and acceptance: as in round 3. Pass registry.WithAllowedStatuses(ctx, http.StatusNotFound, http.StatusBadRequest), or match exitcode.NotFound/Usage. Make the mock return an error for any status not allowed, the way client.Do does. Done when pro device 99999 on a live tenant behaves as it does on main.

⚠️ IMPORTANT (2) (correctness, silent-failure, open 3 rounds, promoted) — internal/resolve/resolve.go:613: the smart-group → static-group fallback on a 404 still never fires, because errors.As targets a type client.Do never returns. The mocks still answer (resp{404}, nil). Merge impact: --group <static group> fails on any server that 404s the smart-groups path, where resolve.go:607 says it falls back. The fix and acceptance are as in round 3.

⚠️ IMPORTANT (3) (test-coverage, open 3 rounds, promoted) — internal/resolve/resolve.go:383, :451, :630: the three exactly-one read-back guards still survive mutation. f76f432f corrects mobileV2Response to the detail shape (general/hardware nesting). That makes the fixture honest, but it still echoes the requested serial, so deleting the read-back stays green. There is still no TestResolveMobileDevice_MultipleMatches. Merge impact: these guards are the PR's whole claim ("resolve to exactly one record before acting"), and nothing holds them. The fix and acceptance are as in round 3.

⚠️ IMPORTANT (4) (correctness, regression, open 2 rounds) — internal/commands/pro_packages_upload.go:236: name == fileName is still case-sensitive. The server matches fileName== case-insensitively and refuses a case-variant create with 400 DUPLICATE_FIELD. That was wire-checked in round 3. Use strings.EqualFold. The fix and acceptance are as in round 3.

Dispositions

Disposition (round 4) — promoted. (1), (2) and (3), open 3 rounds. Each one is above, with its merge impact stated. Each one is a regression from main, or leaves the PR's central guarantee untested. All three were reproduced, two of them on the wire.

Disposition (round 4) — retired, follow-up. Was (5), internal/resolve/resolve.go:786: --group drops members whose lookup fails, with a warning, and reports 0 unresolved. main behaves the same, so this PR is not a regression there. Worth an issue.

Disposition (round 4) — retired, follow-up. Was (6), internal/resolve/resolve.go:680: a Classic group with the wrong envelope reads as an empty group. Fails safe, because an empty group acts on nothing. Follow-up.

Disposition (round 4) — retired, superseded. Was (7), group_numeric_name_repro_test.go:118, a weak fallback-ambiguity test, and (8), the unreachable non-2xx branches at resolve.go:397/:469/:709/:546/:861. Both are consequences of the mock not modelling client.Do's errors. The mock change that (1) and (2) require exposes them, so they are tracked through those two findings.

Review coverage and scope

• Delta since f644279: 9800986e merges main (#404, prestage owned-record ids, reviewed on #404). f76f432f and 826c4d1c are test fixtures only. The three production files cited by (1) to (4) are unchanged (git diff --quiet f644279 HEAD)
• Tests: ./internal/resolve, ./internal/commands -run 'Resolve|Device|Mobile' and ./internal/commands/pro/generated pass at 826c4d1. git merge-tree with current main is clean
• Wire (read-only, pro-nmartin, binary at 826c4d1): pro device 99999 still fails at the ID probe, which is (1). pro device <name> resolves. Nothing was created
• Project rules: root CLAUDE.md and the three .claude/rules/*.md, from the session load. The merged generated files came from generator/parser/generator.go via #404, as the generated-code rule requires
• [na] Fidelity: no linked ticket. Platform and School profiles were not probed, because the delta touches neither

Lanes: no specialist dispatched. The delta is 22 lines of test fixtures plus an already-reviewed merge. Head graded: 826c4d1.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 826c4d1}

[ktn-jamf]

Round 4 fixes. Each one is proven by a mutation or revert: the named test fails without the fix and passes with it.

The shared mocks mockClient (internal/resolve/resolve_test.go) and deviceResolveMockClient (internal/commands/pro_device_resolve_test.go) now return client.StatusError for any status of 400 or above that the caller did not allow with registry.WithAllowedStatuses. That is what client.Do does.

1. pro device 99999 (d249f040). The ID probe passes WithAllowedStatuses(ctx, 404, 400), so its not-found branches run. A 400 that is not INVALID_ID returns client.StatusError. New test: TestResolveDeviceByIdentifier_NumericNonIDGoesOnToSerialAndName. 99999 gets a 404 on the probe, runs the serial and name searches, and reports no device found matching "99999". An all-digit name (1042) resolves by name. TestResolveDeviceByIdentifier_ForbiddenIDProbeStops checks that a 403 still stops resolution. Revert: the new test and TestTryDeviceByID_InvalidIDIsNoDevice fail.
2. Smart-group 404 fallback (ec1948dd). resolveGroupIDByName passes WithAllowedStatuses(ctx, 404). Revert: TestResolveComputerGroup and the new TestResolveMobileDeviceGroup_SmartSearch404FallsBackToStatic fail. TestResolveComputerGroup_SmartSearch403DoesNotFallBack checks that a 403 does not fall back.
3. Guards hold (3959310b). exactly_one_guard_test.go answers with records whose serial, name, UDID, management ID or group name differ from the request, and adds TestResolveMobileDevice_MultipleMatches. Mutations if false && … at the confirm read-back, the mobile total > 1 check and the group-name read-back each fail a test.
4. Package case variant (903a394f). strings.EqualFold replaces ==. Two records with the same name are still refused. Revert: TestFindPackageByFileName_CaseVariantResolvesToTheStoredPackage fails.

Changed existing tests:

• internal/commands/mcp_blueprint_profile_redaction_test.go:34 (a93510a8): added "groupName":"G" to the /v2/groups fixture of classicProfileClient. That fixture came from main with fix(mcp): judge run_command on the command and flags cobra resolves #402 and had no groupName. This PR's resolveGroupPlatformID checks the returned groupName before it accepts a match, and the real endpoint always returns one. So TestImportProfile_SendsTheRealPayloadInAnMCPChild failed at the merge commit itself. Its assertions are unchanged. I did not make production accept an empty groupName, because that would weaken the read-back.
• No other existing assertion changed. With the corrected mocks, every other existing test passes as it was once the fixes are in.

Merge of origin/main (#406, #409, merge 32f4d371): no textual conflicts. The merge needed one edit in this PR's own test file: protect_apply_lookup_error_test.go renames its helper newFakeProtectServer to newFakeProtectLookupServer, because main's mcp_protect_report_client_secret_test.go now declares that name. Only the name changed.

Gates (at a93510a8): go test ./... has no FAIL lines, make lint prints 0 issues. and make verify-generated prints Generated code is up to date.

[neilmartin83]

Tip

✅ Merge-ready. All four promoted findings are fixed in d249f040..a93510a8. The shared mocks now return client.StatusError for any non-allowed status of 400 or above, the way client.Do does. That is the root cause behind (1), (2) and the retired dead-branch item. I wire-checked (1) on pro-nmartin. Wire-checking (4) also turned up a correction to my own round 3, below.

Rating: 5/5

• (1) to (3) are fixed and pinned by tests, and both production fixes fail a test when reverted, which I confirmed myself. (4) is fixed as asked. The case-variant upload still fails at the server, but it fails exactly as it does on main, so it does not block. It is a nice-to-have, below.

Findings

1 nice-to-have suggestion

💡 NICE-TO-HAVE (5) (usability) — internal/commands/pro_packages_upload.go:94: a case-variant upload finds the package and then fails at the upload endpoint with a raw INVALID_FILE_NAME body

Correction to round 3's (4). I wrote that on main the upload "replaced its binary". That was read from the code and never run. On the wire it is not true. On pro-nmartin I created disposable package 1570 as ZZ-PR408-Probe.pkg, since deleted, and then ran upload --file …/zz-pr408-probe.pkg --yes:

binary	lookup	upload
main (247f14d)	finds 1570	400 INVALID_FILE_NAME, "The filename does not correspond to the filename linked to the package ID."
this PR at round 3	finds nothing, then a create	400 DUPLICATE_FIELD fileName
this PR at a93510a	finds 1570	400 INVALID_FILE_NAME, the same as main

The record was unchanged after the failed upload: 2048 bytes and the original hash. So the failure is clean, and this PR no longer differs from main here. The EqualFold fix was right, but the server checks the multipart filename against the stored one with exact case. So a case-variant replace cannot succeed as long as the local name is what gets sent.

Suggested fix: when the matched record's fileName differs from the local name only in case, either send the stored fileName as the multipart filename, or refuse before uploading with a message that names the stored filename and says to rename the local file. Either is better than the raw 400 body. Fine as a follow-up.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/pro_device_resolve.go:81	4	✅ Fixed	d249f040: the probe allows 404/400, and any other 4xx returns client.StatusError. Wire: pro device 99999 now answers no device found matching "99999" (exit 1), the same as main, where round 4 got exit 4 at the ID probe. Mutation: removing WithAllowedStatuses fails ResolveDeviceByIdentifier/TryDeviceByID
(2)	internal/resolve/resolve.go:613	4	✅ Fixed	ec1948dd: WithAllowedStatuses(ctx, 404) on the smart-group search, and a 403 does not fall back. Mutation: removing it fails the group tests. Not wire-checked, because no tenant here 404s that path
(3)	internal/resolve/resolve.go:383	4	✅ Fixed	3959310b: exactly_one_guard_test.go answers with mismatched serial, name, UDID, management ID and group name, and adds TestResolveMobileDevice_MultipleMatches. The author reports that each of the three if false && mutations fails a test
(4)	internal/commands/pro_packages_upload.go:236	3	✅ Fixed as asked	903a394f: strings.EqualFold, and two records are still refused. Round 3's premise is corrected in (5)

Review coverage and scope

• Delta since 826c4d1: merge 32f4d371 (#406, #409; the helper rename newFakeProtectServer → newFakeProtectLookupServer is the only edit) and five commits, 10 files and +207/−15 after the merge
• Changed existing test: classicProfileClient's /v2/groups fixture gains "groupName":"G". That is correct. The real endpoint always returns one, and Keaton was right not to loosen the production read-back to make the old fixture pass
• Mock fidelity: both shared mocks now return a status error for any non-allowed 4xx/5xx. This was the root cause behind (1), (2) and the retired (7)/(8)
• Gates: go test ./internal/... reports no failures at a93510a. Two of my own mutation reverts were each caught. CI ci passes. git merge-tree with current main is clean
• Wire, on pro-nmartin: pro device 99999, and the package probe above, run with both this PR's binary and main's. Package 1570 was created and deleted, and fileName=="zz-pr408*" now returns []
• Project rules: root CLAUDE.md and the three .claude/rules/*.md, from the session load
• [na] Platform and School profiles: the delta does not touch either

Grading: no lanes dispatched, since the delta is targeted fixes plus tests. ever is carried. Head graded: a93510a.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head a93510a}