fix(group-tools,audit): read member counts from the collection that carries one

[neilmartin83]

Two bugs, one root cause

GET /v1/computer-groups answers description, id, name and smartGroup and nothing else — wire-checked against Jamf Pro 11.32.0. Two call sites read a memberCount it has never sent, and both turned the failed type assertion into a zero rather than into "unknown".

1. pro group-tools reported every group as empty. groupMemberCount asserted memberCount, then members, then return 0. So group-tools list, list --empty and analyze --unused were all wrong. On a 37-group tenant where 6 groups hold members (max 45), list reported 0 non-empty and analyze --unused named 30 of 31 groups as removal candidates.

2. pro audit's empty-smart-group check could never fire. checkEmptySmartGroups read the same absent field behind if count, ok := g["memberCount"].(float64); ok && count == 0. ok was always false, so the counter never incremented and the check returned (nil, nil) on every instance — including one with 29 genuinely empty smart groups. The check is registered and looks healthy; it just cannot produce a finding.

The two are the same missing field read two ways: one discarded the ok and lied, the other honoured it and went silent.

The fix

Counts come from the two v3 collections that carry them — membershipCount on /v3/computer-groups/smart-groups and count on /v3/computer-groups/static-groups. Both share /v1's id space, so two paginated sweeps index the whole instance.

A count that could not be read is no longer reported as zero. groupCountIndex.count returns (count, known); a summary row carries "unknown" rather than 0; --empty and --unused select only groups proved empty and say on stderr how many they left out.

analyze --unused also stops naming groups that hold members. It lists removal candidates, and a group holding 45 computers that no policy scopes is a question for an administrator, not a group to delete. Long and the flag help say so.

Auth paths

Both v3 paths are plain Jamf Pro API paths and both are served on the platform gateway, so this works unchanged on a token or oauth2 profile and on a gateway profile. Platform /v2/groups carries the same counts but is gateway-only and would have taken these two commands with it.

Cost: two extra paginated sweeps per group-tools list / analyze --unused, fixed regardless of group count — against one request per group for the smart-group-membership and Classic static-group alternatives (~500 requests on a 500-group instance versus two). pro audit is unchanged: one sweep before, one after.

Why this shipped green

The fixtures hand-fed a memberCount the real API never returns, so the broken behaviour passed. They now carry the real payload shape, plus regression tests: a count is never derived from a field on the listed group, a failed or countless sweep reads as unknown rather than empty, and the audit check now fails loudly on an instance serving only the countless collection (the old code answered (nil, nil)).

Verification

make build, go test ./..., make lint (0 issues) all pass.

Live, on a gateway profile and on an oauth2 profile against the same instance:

$ jamf-cli pro group-tools list -o json | jq -c '[.[].memberCount]|{max:(map(select(type=="number"))|max),nonzero:(map(select(type=="number" and .>0))|length),total:length}'
{"max":45,"nonzero":6,"total":37}          # matches pro groups list exactly

$ jamf-cli pro group-tools list --empty -o json | jq length
31                                          # 37 - 6

$ jamf-cli pro group-tools analyze --unused -o json | jq '[.[]|select(.memberCount!=0)]|length'
0                                           # nothing holding members is listed

$ jamf-cli pro audit -o json | jq -c '.[]|select(.name=="Empty smart groups")'
{"affected":29,"category":"hygiene","name":"Empty smart groups","severity":"INFO",...}

The static-group count field is wire-confirmed, not spec-inferred: loading 12 computers into an empty static group made it report 12, matching pro groups list, and removing them returned it to 0.

Notes for review

• One expected merge conflict with the --page-size work in fix!: --all pages at the endpoint's maximum, not a hard-coded 100 #389 (issue Computer-Inventory --all doesn't respect --page-size #385), in checkEmptySmartGroups: that branch changes the /v1/computer-groups fetch line's page size, this branch deletes the line. Resolution is take-this-side. Trial-merged both together: build green, full suite green, lint clean, all behaviours confirmed live on the combined binary.
• Merge order: fix!: --all pages at the endpoint's maximum, not a hard-coded 100 #389 before this PR is slightly preferable, though either order works. With PageSizeFromPath already on main, rebasing this branch resolves the conflict above and changes groupCountIndex.sweep's literal 100 in the same pass — one resolution, no dangling follow-up. The other order leaves that literal on main until someone remembers it. fix(blueprints): render a YAML export as the document JSON renders #388 is independent of both.
• One follow-up line. groupCountIndex.sweep passes a literal 100; it should become PageSizeFromPath once that constant exists. Left as 100 here so the branch compiles standalone against main. That path has no declared page-size maximum, so the constant resolves to 2000 there — safe, but do not raise it past that.
• No CHANGELOG.md entry, to avoid churn against the same in-flight branch. This does warrant one before merge: memberCount can now be the string "unknown", --unused returns fewer rows, and pro audit gains a finding that never appeared.
• group-tools members <name> still returns bare {"id": ...} rows. Names would cost one /v4/computers-inventory/{id} per member, or an RSQL filter whose URL length is unbounded in member count. Separate change.

🤖 Generated with Claude Code

[neilmartin83]

Rebased onto main (now at 64671e9, after #389 and #388 merged) and force-pushed: 5a5d417 → 448b021.

Conflict resolution — one hunk, checkEmptySmartGroups in internal/commands/pro_audit.go, resolved take-this-side as recorded above. #389 changed that /v1/computer-groups fetch's page-size argument; this branch deletes the line in favour of the /v3/computer-groups/smart-groups sweep, so there was nothing of #389's to preserve there. pro_group_tools.go auto-merged, and the five other PageSizeFromPath call sites #389 added to pro_audit.go are untouched.

Follow-up applied in the same pass, as planned: groupCountIndex.sweep now takes PageSizeFromPath instead of the literal 100, with a comment on why it must not go past 2000 on those paths. Both v3 collections are {totalCount, results} endpoints with no declared maximum, so it resolves to the wire-verified 2000 — and 2000 being the ceiling itself is what makes FetchAllPaginated's short-page termination sound there (see #385).

CHANGELOG entry added, covering the three script-visible changes: memberCount can now be the string "unknown", --empty / --unused" return fewer rows because an unknown count is not an empty group, and pro auditgains anEmpty smart groupsfinding that previously could never fire. The static-groupcount` field is described as wire-confirmed rather than spec-inferred.

Checks: go build ./..., go test ./..., make lint (0 issues) and make verify-generated all clean on the rebased tree. TestCheckEmptySmartGroups_DoesNotReadTheCountlessCollection and TestGroupMemberCountIsNeverDerivedFromTheListedGroup both pass.

Live check on a second tenant (platform-mockingbird, gateway auth), independent of the numbers measured on platform-nmjspp: group-tools list returns 25 computer groups whose id set and every member count match /v2/groups' COMPUTER rows exactly — 5 non-zero, max 5, zero unreadable — and --empty returns the complementary 20, all with memberCount: 0. pro audit reports Empty smart groups with affected: 9.

One correction to the acceptance criteria above: group-tools list equalling pro groups list is tenant-specific, not an invariant. pro groups list reads /v2/groups, which carries mobile device groups too — 25 COMPUTER + 20 MOBILE = 45 here against group-tools' 25. The two agree exactly on the computer subset, which is the comparison worth making.

analyze --unused could not be checked live on this tenant: it panics, on main as well as on this branch, so the crash is pre-existing and unrelated to this PR. Filing separately. Its membership gating is covered by the unit tests in this PR.

🤖 Generated with Claude Code

[neilmartin83]

Adding the analyze --unused number, now that #390 makes that command runnable on a gateway tenant.

With #390's guard applied on top of this branch, against platform-mockingbird:

	rows from analyze --unused
main (counts always 0)	23
this branch	19
rows with a non-zero memberCount	0

The 4-row difference is the clearest statement of what this PR does: four groups that actually hold members are no longer offered as removal candidates. On main every count read as 0, so --unused could not distinguish an empty group from a populated one and listed both.

For the avoidance of doubt about the earlier note: those 23 rows on main are not evidence of anything about this branch. main's counts are uniformly 0, so "zero rows with a non-zero memberCount" is trivially true there and only becomes a real assertion once counts are readable — which is what this PR fixes.

#390 is independent of this PR (pre-existing crash, reproduces on main) and the two auto-merge — verified by cherry-pick, no conflict in pro_group_tools.go despite both touching it.

🤖 Generated with Claude Code