fix(pro): resolve serials in bulk group/send-command --from-file

[neilmartin83]

Summary

• pro bulk add-to-group/remove-from-group/send-command accept --from-file with "one computer ID or serial per line" (per the existing --help text), but the underlying resolveComputerTargets/readIDsFromFile in pro_bulk.go never resolved anything — every line was passed straight through as the Classic API numeric <id>. A serial number sent this way 409s (Error: Unable to match computer), reported in Jamf's internal #jamf-api Slack channel.
• internal/resolve already implements the correct pattern (isNumericID + serial-vs-ID resolution against /v3/computers-inventory), and it's already used by the pro computers/pro mobile-devices device-action commands (erase, restart, ddm-sync, etc. via deviceTarget). The bulk group/command helpers predate that package (54b4a93, before 9536a7c introduced internal/resolve) and were never migrated onto it.
• resolveComputerTargets's --from-file branch now delegates to resolve.ResolveComputersFromFile, which resolves each line (serial or numeric ID) to a real computer before it's ever used in a Classic API request. Deleted the now-dead local readIDsFromFile.
• --group (source group membership) was not affected — those IDs already come straight from a real server response, not user input — so it's left as-is.
• Updated send-command's help text, which previously (accurately) said ID-only; it now also supports serials.

Root cause / verification

Reproduced live against a Jamf Pro test tenant before the fix:

$ jamf-cli pro bulk add-to-group --target-group "Example Static Computer Group" --from-file serials.txt --yes
...
<-- 409 409 Conflict
Error: Unable to match computer

After the fix, the same file produces a 201 Created (serial resolved to the real numeric ID first).

Test plan

• go test ./... — all packages pass
• make lint — 0 issues
• Added TestAddToGroup_FromFile regression coverage that captures the outgoing PUT body and asserts the resolved numeric ID appears (<id>5</id>) and the raw serial never leaks into the Classic API payload (<id>FVFC41HCLYWP</id>) — this test fails against the pre-fix code.
• Live-tested against a real Jamf Pro tenant: add-to-group/remove-from-group/send-command all correctly resolve a serial via --from-file; verified via -v that a /v3/computers-inventory RSQL lookup happens before the Classic API call, and that the previously-reported 409 no longer occurs (201 Created instead). Cleaned up the test group membership afterward.

🤖 Generated with Claude Code

[ktn-jamf]

Warning

⚠️ Needs changes — Fixes pro bulk add-to-group/remove-from-group/send-command --from-file sending raw serials as Classic API IDs (409s), by delegating to the existing internal/resolve resolver.
Blocking: (1), (2). See collapsed section for 1 nice-to-have suggestion.

Rating: 3/5

• Would be a 5 with partial-failure handling for from-file resolution errors (1) and a send-command --from-file regression test (2).
• Coverage: security-reviewer, usability-reviewer, and devil-advocate have never run on this PR — the rating reflects the dimensions that were searched, not the whole diff.

Findings

🟧 (1) (correctness/design, c=100, via silent-failure-hunter+test-quality-reviewer) — internal/commands/pro_bulk.go:354-357: --from-file resolution failure silently defeats --allow-partial-failure

resolveComputerTargets's new --from-file branch delegates to resolve.ResolveComputersFromFile, which resolves file entries one at a time and returns nil, err on the first unresolvable entry (internal/resolve/resolve.go:124-143), discarding every already-resolved device. resolveComputerTargets propagates that error immediately (line 356), and both callers — runGroupMutation (pro_bulk_groups.go:84-87) and runSendCommand (pro_bulk_commands.go:101-104) — return it before the mutation loop where successCount/failCount/finishBatch/--allow-partial-failure (pro_bulk.go:21-33) apply. This is a new asymmetry inside the very same function: the --group branch a few lines below (pro_bulk.go:369-391) already warns-and-continues on a per-member resolution failure, and batchResolveComputers (resolve.go:482-493) does the same for --group elsewhere — so --group already tolerates one bad member even for destructive commands, but --from-file now aborts the whole batch on one bad line.

Failure scenario: bulk add-to-group --from-file big.txt --target-group Quarantine --allow-partial-failure --yes where big.txt has 499 valid serials and 1 typo'd/decommissioned one → the command aborts with zero PUTs issued for any of the 499 valid computers, and --allow-partial-failure has no effect because the mutation loop is never entered. No test in pro_bulk_test.go exercises --allow-partial-failure combined with a from-file batch that mixes valid and invalid entries.

Suggested fix: match the --group path's existing soft-fail pattern in ResolveComputersFromFile (and ResolveMobileDevicesFromFile, for the same reason on the mobile side) — this brings --from-file back in line with --group's already-accepted behavior for the same destructive commands, so it introduces no new risk to pro_device_actions.go's erase/restart callers:

 	var results []*DeviceIdentifiers
 	for _, entry := range entries {
 		var d *DeviceIdentifiers
 		if isNumericID(entry) {
 			d, err = ResolveComputer(ctx, client, "", "", entry)
 		} else {
 			d, err = ResolveComputer(ctx, client, entry, "", "")
 		}
 		if err != nil {
-			return nil, fmt.Errorf("resolving %q: %w", entry, err)
+			_, _ = fmt.Fprintf(os.Stderr, "  warning: could not resolve %q: %v\n", entry, err)
+			continue
 		}
 		results = append(results, d)
 	}
 	return results, nil

Fixed when: an unresolvable --from-file entry warns to stderr and the batch continues with the remaining entries, so --allow-partial-failure covers resolution failures the same way it already covers mutation failures, and a test asserts "N valid + 1 invalid entry → N mutations attempted, 1 warning logged" for at least one of add-to-group/send-command.

---

🟧 (2) (test-quality, c=75, via test-quality-reviewer) — internal/commands/pro_bulk_commands.go:101,138: send-command --from-file has no test that a resolved entry reaches the MDM command endpoint

The PR's bug report names three broken surfaces — add-to-group, remove-from-group, send-command — but only add-to-group got a rewritten regression test (TestAddToGroup_FromFile) that resolves a mixed serial+ID file and asserts the resolved ID reaches the outgoing request. Every send-command --from-file test (TestSendCommand_DestructiveRequiresConfirm, TestSendCommand_DestructiveWithBothFlags, TestSendCommand_DestructiveRequiresYesToo, TestSendCommand_InvalidCommandName) uses /dev/null or an empty/comment-only file — only the zero-entries error path. TestSendCommand_YesDispatches/TestSendCommand_PartialFailure do exercise the POST to /JSSResource/computercommands/command/{cmd}/id/{id} (pro_bulk_commands.go:138), but only via --group, never --from-file.

Failure scenario: a future change that swaps t["id"] for t["name"] (or otherwise breaks the from-file→POST wiring) at pro_bulk_commands.go:138 would go undetected, since no test combines --from-file with a resolved entry and a mutating send-command POST.

Suggested fix: add TestSendCommand_FromFile mirroring TestAddToGroup_FromFile's mock setup (a file with a serial + numeric ID, mocked /v3/computers-inventory responses, --yes), asserting the resolved IDs — not the raw serial — appear in the outgoing POST paths.

Fixed when: a test runs send-command --from-file <file-with-a-serial> --yes and asserts the resolved ID reaches the POST.

Nice-to-have suggestions (1 item)

🟩 (3) (test-quality, c=75, via test-quality-reviewer) — internal/commands/pro_bulk.go:360-363: name-fallback branch (if name == "" { name = d.ID }) is untested

Every mock fixture in pro_bulk_test.go supplies a non-empty general.name, so this fallback never runs in any test. Blast radius is cosmetic only — d.ID still reaches the Classic API payload correctly; only the preview table / mutation-log computer_name column would render blank instead of the ID for a computer with no inventory name.

Fixed when: a from-file test case uses a mock response with an empty general.name and asserts the target's name falls back to the ID.

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

Review coverage

• Design and architecture: consolidates duplicate serial/ID resolution onto the existing internal/resolve package, matching the precedent already used by pro_device_actions.go — good direction; the --group/--from-file asymmetry it surfaces is finding (1)
• Correctness: traced resolveComputerTargets → resolve.ResolveComputersFromFile → isNumericID dispatch → XML body construction; confirmed the fix eliminates the reported 409 (raw serial no longer reaches <id>); found the partial-failure interaction gap (1)
• Security: computer IDs used in staticGroupAddComputerXML/staticGroupRemoveComputerXML are now validated/resolved server-side before use rather than passed through raw — a hardening, not a new injection surface; no new trust-boundary risk
• Performance: no new loop/query pattern — the diff reuses internal/resolve's existing per-entry sequential resolution unchanged; not introduced or altered by this PR
• Test coverage: reviewed via test-quality-reviewer plus mutation spot-checks; regression test for add-to-group confirmed genuine (fails against pre-fix code); gaps captured in (2) and (3)
• Reliability: traced error propagation through both callers; (1) is the reliability-relevant gap
• Code quality: clean removal of dead code (readIDsFromFile), clear comment explaining the delegation rationale
• Simplification: removes a duplicate helper in favor of the already-established shared resolver
• [na] Frontend concerns: CLI-only change, no UI
• Documentation currency: checked docs/site/examples.json and CLAUDE.md's "Where to Make Changes" table — no new construct introduced (bug fix reusing an existing pattern), so no doc update is needed beyond the help-text change already in the diff
• [na] Cross-repo contracts: no wire keys, enums, or shared contracts crossing a repo boundary — internal Classic/v3 Jamf Pro API calls only
• [na] Project rules compliance: no .claude/rules/ directory in this repo

Scope of review

Full diff (3 files, +83/−60) read directly; also read internal/resolve/resolve.go (ResolveComputersFromFile, batchResolveComputers, ResolveComputerGroup), internal/commands/pro_device_actions.go (confirms ResolveComputersFromFile/ResolveMobileDevicesFromFile are pre-existing, shared with erase/restart/etc.), and internal/commands/root.go:806 (--allow-partial-failure flag). Ran go build ./..., go test ./internal/commands/... ./internal/resolve/... (pass), and golangci-lint run (0 issues) at head a928a0a. Checked for prior reviews/comments on the PR (none found) and for a .claude/rules/ directory (none).

Medium risk (diff modifies data-transformation/error-handling logic in existing source, <150 lines changed). Dispatched silent-failure-hunter and test-quality-reviewer as the two highest-signal specialists under the medium-risk cap of 2, both independently converging on finding (1). Never-run lanes:

• security-reviewer — eligible (input parsing feeding API calls), not dispatched under the cap
• usability-reviewer — eligible (CLI help text, user-facing --from-file behavior), not dispatched under the cap
• devil-advocate — eligible (business-logic change to target resolution), not dispatched under the cap
• fidelity-reviewer — inapplicable, no linked spec/issue/Jira ticket (PR description cites an internal Slack report, not a formal linked issue)
• performance-reviewer — inapplicable, no DB/loop/caching changes; delegates to a pre-existing resolver unchanged by this PR
• scope-reviewer — inapplicable, small single-concern PR

What's done well

✅ The rewritten TestAddToGroup_FromFile is a genuine regression test, not just a passing test — confirmed via mutation testing that it fails against the pre-fix readIDsFromFile-based code (leaking <id>FVFC41HCLYWP</id> into the PUT body) and asserts the raw serial never appears in the payload.

✅ Deleting readIDsFromFile in favor of the already-established internal/resolve pattern (used by pro computers/pro mobile-devices device actions) is the right consolidation — it removes a second, buggy implementation of the same concern instead of patching it in place.

Generated by pr-review v1.29.0, a Jamf Claude Code skill

[neilmartin83]

@ktn-jamf — all three findings addressed in 5052d11 (pushed). Ready for re-review.

(1) 🟧 --from-file resolution failure silently defeats --allow-partial-failure — fixed in internal/resolve/resolve.go. ResolveComputersFromFile and ResolveMobileDevicesFromFile now warn to stderr and continue on an unresolvable entry instead of return nil, err, exactly matching batchResolveComputers's existing per-member soft-fail on the --group path. Applied to the mobile variant too, as suggested, so pro mobile-devices device actions get the same behavior. The zero-resolvable-entries case was already handled downstream (len(targets) == 0 → "No target computers found").

(2) 🟧 send-command --from-file has no test that a resolved entry reaches the MDM command endpoint — added TestSendCommand_FromFile, mirroring TestAddToGroup_FromFile's mock setup (file with a serial + a numeric ID, mocked /v3/computers-inventory lookups, --yes). Asserts both POSTs land on /JSSResource/computercommands/command/BlankPush/id/5 and .../id/7, and that the raw serial never appears in the POST path.

(3) 🟩 name-fallback branch untested — added TestAddToGroup_FromFile_NameFallsBackToID, whose mock returns a computer with an empty general object; asserts the mutation log labels it by its ID.

Also added TestAddToGroup_FromFile_PartialResolutionFailure for (1): a 3-line file where one entry has no inventory record → 2 PUTs still issued with <id>5</id>/<id>7</id>, and <id>999</id> never reaches the Classic API.

Verification: go test ./internal/commands/ ./internal/resolve/ pass, make lint 0 issues.

Note on the never-run lanes you flagged (security-reviewer, usability-reviewer, devil-advocate) — happy for those to run on the updated diff if you want the coverage before merge.

[ktn-jamf]

Warning

⚠️ Needs changes — Round 2. All three round-1 findings are fixed; the soft-fail fix introduced new gaps, and the never-run lanes surfaced a request-volume regression.
Blocking: (1), (2), (3). See collapsed section for 1 nice-to-have suggestion.

Rating: 3/5

• Would be a 5 with resolution failures folded into the batch tally/exit code ((1), (2)) and numeric-ID lines no longer round-tripped through the v3 API (3).
• Coverage: security-reviewer, usability-reviewer, devil-advocate, and performance-reviewer had never run on this PR before this round — all four ran now, so the rating reflects the whole diff.

Prior findings status (3 items)

#	Location	State	Notes
(1) c=100	internal/commands/pro_bulk.go:354-357	✅ Fixed	5052d11 changed both ResolveComputersFromFile/ResolveMobileDevicesFromFile to warn-and-skip; TestAddToGroup_FromFile_PartialResolutionFailure asserts the two resolvable entries still mutate. The fix is correct as specified — findings (1) and (2) below are residual gaps it did not close, not a re-open.
(2) c=75	internal/commands/pro_bulk_commands.go:101,138	✅ Fixed	TestSendCommand_FromFile added; asserts /id/5 and /id/7 reach the POST path and the raw serial never does.
(3) c=75	internal/commands/pro_bulk.go:360-363	✅ Fixed	TestAddToGroup_FromFile_NameFallsBackToID covers the empty-general.name branch.

Findings

🟧 (1) (correctness, via devil-advocate, c=75) — internal/resolve/resolve.go:140-146: an all-unresolvable --from-file exits 0

Both FromFile resolvers now continue past every failure and end at return results, nil, so when every line fails they hand back (nil, nil). resolveComputerTargets (internal/commands/pro_bulk.go:354) passes that empty slice to runSendCommand/runGroupMutation, which take the len(targets) == 0 branch and return nil (pro_bulk_commands.go:105-108); pro_device_actions.go:113,130 → executeAction does the same. Before this PR the same input hard-errored. The --group symmetry argument does not carry here: group member IDs come from a server response, whereas a --from-file list is exactly the input that goes wholly stale (yesterday's decommission list against a replaced fleet).

Failure scenario: a CI step runs jamf-cli pro bulk send-command --command BlankPush --from-file quarantine.txt --yes (or pro comp erase --from-file offboard.txt --yes --confirm-destructive) where every serial is stale → every line warns, the command prints "No target computers found." and exits 0, and the pipeline's next step proceeds as though the batch ran. Reachable from the PR as shipped.

Suggested fix:

+	if len(results) == 0 {
+		return nil, fmt.Errorf("none of the %d entries in %s could be resolved", len(entries), path)
+	}
 	return results, nil

Fixed when: a --from-file whose every line fails resolution returns a non-nil error (non-zero exit), with a test feeding an all-unresolvable file into send-command or add-to-group and asserting the error.

---

🟧 (2) (correctness/reliability, via devil-advocate+usability-reviewer, c=100) — internal/commands/pro_bulk.go:347-366: dropped entries are invisible to the tally, the exit code, and the summary line

The count of entries dropped during resolution is not returned by resolve.ResolveComputersFromFile, so it never reaches successCount/failCount, finishBatch (pro_bulk.go:25-33), or exitcode.PartialOrPropagate. Every downstream number — the "Applying … to %d computers" banner, the "complete: %d succeeded, %d failed" line (pro_bulk_groups.go:116,132; pro_bulk_commands.go:125,150) — is computed from the post-filter slice. --allow-partial-failure and exit 7 therefore cover mutation failures only, never resolution failures, even though both are "some entries in my file didn't work". The lone stderr warning emitted earlier is the only trace, and it is structurally disconnected from the completion line the user reads for the outcome.

Failure scenario: a 10-serial file where 3 are stale → 3 warnings scroll past, 7 PUTs succeed, finishBatch(stderr, …, 7, 0, nil) is called with failed=0, PartialOrPropagate returns nil, and the run ends "Group update complete: 7 succeeded, 0 failed." at exit 0 — reading as a clean full success for all 10 requested computers. Reachable from the PR as shipped.

Suggested fix:

-func ResolveComputersFromFile(ctx context.Context, client registry.HTTPClient, path string) ([]*DeviceIdentifiers, error) {
+// Returns the resolved devices and the number of entries that could not be resolved.
+func ResolveComputersFromFile(ctx context.Context, client registry.HTTPClient, path string) ([]*DeviceIdentifiers, int, error) {

then thread the skipped count through resolveComputerTargets into finishBatch's failed tally (and into executeAction's counters for the device-action callers), so --allow-partial-failure and exit 7 govern it.

Fixed when: a --from-file batch with N resolution failures and M mutation successes reports exit 7 — or is silenced by --allow-partial-failure — the same way N mutation failures do, the completion line names the skipped count, and a test asserts the exit code on a mixed resolve-failure/mutation-success run.

---

🟧 (3) (performance, via performance-reviewer, c=100) — internal/resolve/resolve.go:127-147: --from-file now costs one sequential request per line, including numeric IDs that previously cost zero

The loop resolves entries one at a time with no batching and no concurrency. A serial costs one RSQL filtered list query (resolveComputerByFilter); a numeric ID costs one GET /v3/computers-inventory/{id} (resolveComputerByID) purely to re-derive the ID the file already supplied, plus a display name. The pre-PR readIDsFromFile path made zero resolution calls, so an ID-only file goes from N requests to 2N. /v3/computers-inventory accepts RSQL and this same file already has fetchAllPages, so an =in= batched filter is available.

Failure scenario: pro bulk send-command --from-file computers.txt --command BlankPush --yes against a 2,000-line file of numeric IDs → 2,000 sequential GETs before the first POST; near the tenant's Pro API rate limit a fraction 429 and each retries up to 3 times with Retry-After backoff (internal/client/client.go doWithRetry), adding minutes before any mutation. Reachable from the PR as shipped.

Suggested fix: short-circuit the numeric case and batch the serials.

-	for _, entry := range entries {
-		if isNumericID(entry) {
-			d, err = ResolveComputer(ctx, client, "", "", entry)
-		} else {
-			d, err = ResolveComputer(ctx, client, entry, "", "")
-		}
+	// Numeric entries are already the identifier the Classic endpoints want —
+	// pass them through without a lookup. Resolve the serials in one chunked
+	// RSQL `hardware.serialNumber=in=(…)` query via fetchAllPages.

If the per-ID GET is wanted for validation and display names, keep it but gate it behind an explicit flag rather than making it the default.

Fixed when: an N-line numeric-ID file makes zero resolution requests, an N-serial file makes O(N/batch) requests instead of N, and a mock-client call count in the TestAddToGroup_FromFile-style tests asserts both.

Nice-to-have suggestions (1 item)

🟩 (4) (usability, via usability-reviewer, c=75) — internal/commands/pro_bulk_commands.go:46,65 and pro_bulk_groups.go:44,58: help text documents the input format but not two behavior changes

--from-file correctly gained "or serial", but neither help text mentions that an unresolvable line is now skipped with a warning rather than fatal, nor that a blank/comment-only file now hard-errors "file … contains no entries" (internal/resolve/resolve.go:638-640) where it previously printed "No target computers found." at exit 0 — the flip the PR's own TestSendCommand_DestructiveWithBothFlags edit encodes. Add both to the Long text for add-to-group, remove-from-group, and send-command.

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

Review coverage

• Design and architecture: consolidating onto internal/resolve remains the right call; the open question the soft-fail raises is whether a shared resolver should decide failure policy for callers with different trust in their input — findings (1), (2)
• Correctness: traced resolveComputerTargets → ResolveComputersFromFile → isNumericID dispatch → Classic XML//id/ path for both the partial-drop and total-drop cases, and through the second pre-existing caller pro_device_actions.go:113,130 → executeAction; findings (1), (2)
• Security: security-reviewer ran on the whole diff. Routing file lines through the resolver is a hardening — the Classic <id> and MDM command path now receive a server-confirmed numeric ID instead of a raw file line. The one candidate finding (EscapeRSQL at resolve.go:600-602 not escaping \) scored c=0 and was dropped: those lines are unchanged by this PR and pro_device_actions.go already routed --from-file content through the same function before it
• Performance: performance-reviewer ran; the added per-line request volume is finding (3)
• Test coverage: the three tests added in 5052d11 are genuine (the partial-resolution test fails if the drop is removed); the gap that remains is an exit-code assertion for the all-unresolvable and partial-resolution cases, named in (1) and (2)
• Reliability: finishBatch/exitcode.PartialOrPropagate traced end to end — finding (2)
• Code quality: dead readIDsFromFile cleanly removed; the delegation comment at pro_bulk.go:348-352 explains the why
• Simplification: the diff removes a duplicate helper rather than patching it
• [na] Frontend concerns: CLI-only
• Documentation currency: README.md and docs/site/examples.json --from-file references checked — no new construct, so no doc update beyond the help text, which is finding (4). No docs/solutions/ entry matches pro_bulk or internal/resolve
• [na] Cross-repo contracts: internal Jamf Pro Classic/v3 API calls only; no wire key, enum, or config name another repo reads
• [na] Project rules compliance: no .claude/rules/ directory; CLAUDE.md checked — the credential-input and generated-code boundaries are untouched by this diff

Scope of review

Round 2. Baseline review at head a928a0a; incremental diff a928a0a..3c66ea6 is one PR commit (5052d11) plus a merge of main (which carried unrelated protect work — reviewed under #317, not re-reviewed here). Whole-PR diff (4 files, +235/−55) re-read at head 3c66ea6. Also read internal/resolve/resolve.go in full, internal/commands/pro_device_actions.go (deviceTarget, runDeviceAction, executeAction), pro_bulk_groups.go, pro_bulk_commands.go, internal/exitcode/exitcode.go, internal/client/client.go (doWithRetry), and internal/resolve/resolve_test.go. Ran go test ./internal/commands/... ./internal/resolve/... at head — pass.

Medium risk (data-transformation/error-handling logic in existing source), but the soft-fail edit is a guard change in a shared function with a second pre-existing caller, which the dispatch rules tier as high risk regardless of line count — so the round dispatched four lanes, all of them never-run on this PR: security-reviewer, usability-reviewer, devil-advocate, performance-reviewer (the last is exempt from the medium-risk cap as a never-run backfill). silent-failure-hunter and test-quality-reviewer ran in round 1 and were not re-dispatched; the fix commit is squarely in silent-failure-hunter's dimension, and the orchestrator covered that ground directly instead — findings (1) and (2) are its output.

Never-run lanes after this round: fidelity-reviewer — inapplicable, no linked spec, issue, or Jira key in the title, body, or branch name (the PR cites an internal Slack report). scope-reviewer — inapplicable, single-concern PR well under the size threshold.

One 🟩 candidate was dropped by the confidence pass at c=50: the two new warnings write to os.Stderr directly rather than through cmd.ErrOrStderr(), so they escape --quiet and the test harness's cmd.SetErr capture. Dropped because the surrounding batchResolveComputers/batchResolveMobileDevices use the identical pattern — it is a package-wide convention question, not something this PR introduced. Worth a separate cleanup ticket if the --quiet contract is meant to bind these.

What's done well

✅ Round 1's fix was implemented exactly as specified rather than approximately — including the mobile-side twin (ResolveMobileDevicesFromFile), which no finding forced and which would have left the two FromFile resolvers with divergent failure policy had it been skipped.

✅ TestAddToGroup_FromFile_PartialResolutionFailure asserts the negative too (<id>999</id> never reaches the Classic API), so it fails if the drop silently turns into a pass-through — the failure mode that started this PR.

_{🤖 Generated by the pr-review:review skill v1.29.0 · reviewed head 3c66ea6}

[neilmartin83]

@ktn-jamf — round-2 findings addressed in 6712ae0 (pushed). One deviation from the suggested fix for (3), explained below.

(1) 🟧 all-unresolvable --from-file exits 0 — fixed. resolveEntriesFromFile returns fmt.Errorf("none of the %d entries in %s could be resolved", …) when nothing resolves, for both the computer and mobile variants, so the empty slice never reaches the len(targets) == 0 → return nil branch. Verified live: an all-stale file now exits 1 with that message and issues no mutation.

(2) 🟧 dropped entries invisible to the tally / exit code / summary — fixed, threaded exactly as suggested. ResolveComputersFromFile/ResolveMobileDevicesFromFile now return ([]*DeviceIdentifiers, int, error); resolveComputerTargets passes the count on, runGroupMutation/runSendCommand seed failCount with it (so finishBatch → PartialOrPropagate treats a resolution failure exactly like a mutation failure, including being silenced by --allow-partial-failure), and the completion line gains a (N unresolved) suffix. Same threading into executeAction for the device-action callers. Live run of your failure scenario:

Group update complete: 2 succeeded, 1 failed (1 unresolved).
{"error":"partial_failure","exitCode":7,…,"message":"1 of 3 group membership operations failed"}

(3) 🟧 one sequential request per line — fixed by batching, not by the suggested numeric pass-through. The pass-through would break the other caller: ResolveComputersFromFile is shared with pro_device_actions.go, and blank-push (batchByManagementID), ddm-sync, and renew-mdm hard-fail on an empty d.ManagementID/d.UDID (pro_device_actions.go:284,319,354) — values only an inventory lookup supplies. A numeric-ID file would resolve to ID-only records and every one of those actions would error.

So entries are now resolved in chunked RSQL =in= queries instead: one request per identifier kind per 100 entries. Your 2,000-line numeric file goes from 2,000 sequential GETs to ~20, and an N-serial file from N to O(N/100) — not the zero you asked for on the ID path, but without regressing the device actions. Details worth flagging:

• Verified live before implementing, against a tenant on 11.30: id=in=(…) and hardware.serialNumber=in=(…) are both honored on /v3/computers-inventory (quoted and unquoted), a 200-value list is accepted, and a partially-matching list returns just the matches. Chunk size is 100 to keep URLs short. serialNumber/mobileDeviceId are likewise filterable on /v2/mobile-devices/detail per its spec, and the mobile resolver batches identically.
• Serial matching is case-insensitive server-side (probed: an uppercased query matched a lowercase stored serial), so the result index is keyed case-folded — otherwise a file entry differing only in case would come back as a false "not found".
• An entry that can't be quoted into a shared list (a comma, quote, paren, whitespace — isRSQLListSafe) is resolved on its own rather than interpolated into a filter shared with other entries.
• A batch lookup's HTTP failure is now fatal rather than being reported per-entry as "these entries don't exist" — a 401/429 must not read as a stale file. Test asserts this.
• Duplicate lines are deduped for querying but still produce one target each, as before.
• Ambiguity is still detected: a serial matching multiple records warns and skips that entry.

The --group path (batchResolveComputers) still resolves per member ID and has the same shape of cost. Left alone as out of scope for this PR — happy to file a follow-up.

(4) 🟩 help text — added to add-to-group, remove-from-group, and send-command: serials are resolved first; an unmatched line is reported and skipped and counts as a failure in the summary and exit code (tolerable with --allow-partial-failure); a file with no resolvable lines, or no entries at all, fails without changing anything.

Tests — new: TestAddToGroup_FromFile_BatchesLookups (3-ID file → exactly 1 inventory request, and no /v3/computers-inventory/{id} call at all), TestAddToGroup_FromFile_PartialResolutionFailure extended to assert exitcode.PartialFailure and (1 unresolved) in the summary, TestAddToGroup_FromFile_PartialResolutionAllowed (--allow-partial-failure silences it), TestAddToGroup_FromFile_AllUnresolvableFails, TestSendCommand_FromFile_UnresolvedCountsAsFailure (exit 7 assertion on the command path), plus resolver-level tests for batching, case-insensitive serials, the skipped count, the all-unresolvable error, the unbatchable-entry isolation, and the fatal-lookup-error case. The bulk mock gained filter-aware response matching so the two batched queries sharing one base path can be told apart.

Mutation-checked the two new blocking-finding tests: reverting the failCount seeding fails TestAddToGroup_FromFile_PartialResolutionFailure, and removing the zero-resolved guard fails both TestAddToGroup_FromFile_AllUnresolvableFails and TestResolveComputersFromFile_AllUnresolvableErrors.

Verification — go test ./... all pass, make lint 0 issues. Live against a Jamf Pro tenant through the platform gateway, on a scratch static group (created and deleted afterwards): batched single-request lookup confirmed via -v, mixed file → exit 7 with the unresolved count, all-stale file → exit 1 with no mutation, and pro comp blank-push --from-file still sends resolved managementIds (confirming why the pass-through was declined).

On the dropped 🟩 about os.Stderr vs cmd.ErrOrStderr() in the resolver warnings — agreed it's a package-wide convention issue rather than this PR's; the new warnUnresolvedTargets/unresolvedNote summary output does go through cmd.ErrOrStderr(), so the total is --quiet-respecting and test-capturable even while the per-entry warnings aren't. Happy to file that cleanup separately.

[ktn-jamf]

Round 3 review — re-review of the round-2 fixes

Scope: 6712ae0 "fix(pro): batch --from-file lookups; fold unresolved entries into the tally", plus the 6c46426 merge of main. Head reviewed: 6c46426. The round-2 review is at #pullrequestreview-4938491525.

Note on reading the diff: git diff 3c66ea6...6c46426 also shows the generated-code deletions from #322, which merged into main in between. Those are not part of this PR.

Verdict: ✅ All three round-2 findings are fixed. Rating 5/5.

Round-2 findings

#	Round-2 finding	Status
(1)	All-unresolvable --from-file returned no error and exited 0	✅ Fixed
(2)	Dropped entries were absent from the tally, exit code, and summary	✅ Fixed
(3)	--from-file cost one sequential request per line	✅ Fixed

(1) resolveEntriesFromFile (resolve.go:187) now returns an error when no entry resolves. Both the bulk path and the pre-existing pro comp erase --from-file caller inherit the fix, because both go through the same two exported functions.

(2) Both resolvers return a skipped count. runGroupMutation seeds failCount with it (pro_bulk_groups.go:130), names it in the completion line, and passes it to finishBatch, so --allow-partial-failure and exit 7 now govern resolution failures. The device-action path folds the count into executeAction (pro_device_actions.go:593) and into unresolvedTargetsErr for the single-device and batch-endpoint returns.

(3) Entries are partitioned into numeric IDs, list-safe serials, and unbatchable entries, then resolved in chunked RSQL =in= queries of 100 (resolve.go:150-260). A 2,000-line file costs about 20 requests. The author did not take the suggested shortcut of passing numeric IDs straight through, and gives a correct reason: ResolveComputersFromFile is shared with the device actions, where blank-push, ddm-sync, and renew-mdm need the ManagementID and UDID that only an inventory lookup returns.

The fix also handles three cases the review did not ask for: serials match case-insensitively, an entry that cannot be quoted into a shared list is resolved on its own, and an HTTP failure of a batch lookup stays fatal instead of being reported as "these entries do not exist".

Verification

• go test ./internal/resolve/... ./internal/commands/ — both packages pass.
• Mutation check on finding (1): replacing the len(devices) == 0 guard with if false fails TestResolveComputersFromFile_AllUnresolvableErrors and ..._UnbatchableEntryIsolated.
• Mutation check on finding (3): setting batchChunkSize to 1 fails TestResolveComputersFromFile_PartialFailureCounted, because the mock serves only the batched path.
• Each finding has a named test: _BatchesIDs, _SerialCaseInsensitive, _PartialFailureCounted, _AllUnresolvableErrors, _UnbatchableEntryIsolated, _LookupErrorIsFatal.

The worktree was left clean after both mutations.

New observations — all minor, none blocking

🟩 (a) A matched device with no serial in its record reports "not found". parseComputerInventory (resolve.go:~640) returns an error only when id is missing, so a record whose hardware.serialNumber is absent or blank parses with an empty SerialNumber. resolveEntries keys such a device under "" in bySerial, so the entry that the server matched reports no computer found with serial number "X" and counts as skipped. This needs an inventory record that matches a serial filter but returns no serial value — for example when the HARDWARE section is withheld by section-level permissions. Consider keying the result by the requested entry when exactly one record comes back for a single-term chunk.

🟩 (b) A parse failure is reported as "not found". In the ID and serial loops, a record that fails spec.parse is skipped with continue or err == nil. The entry then reports no <label> found with ID x, which points the user at their input file rather than at the malformed response. Consider recording the parse error and surfacing it in the per-entry warning.

🟩 (c) The device-action path exits 1, not 7. unresolvedTargetsErr and the per-device loop return a plain error, so pro comp erase --from-file with one unresolved line of three exits 1, and those commands have no --allow-partial-failure. The bulk group and send-command paths use finishBatch and exit 7. This asymmetry predates the PR — mutation failures behaved the same way — so it is not a regression. It is more visible now that resolution failures are counted. Worth a separate ticket if the exit-code contract is meant to be uniform.

🟩 (d) The empty-file message counts entries that do not exist. A file that holds only blanks and comments produces none of the 0 entries in <path> could be resolved. The behaviour is correct and documented in the help text. Only the wording is odd. Consider a distinct message when the file holds no entries.

Not findings

• Dry-run and preview paths warn about unresolved entries and then exit 0. Nothing is mutated on those paths, so this reads as intended.
• The multi-match error for a duplicated serial matches what resolveComputerByFilter already did with page-size=2, so it is not a behaviour change.
• EscapeRSQL is applied to serials that isRSQLListSafe has already restricted to alphanumerics, -, _, and .. The escape is redundant but harmless, and it keeps the call correct if the predicate is ever loosened.

[ktn-jamf]

Approving on the round-3 re-review: all three round-2 findings are fixed, and the batching and tally changes are mutation-verified.

The four remaining 🟩 items are diagnostic-quality follow-ups. None blocks this merge.