feat(protect): add unified logging filter sets (jamfprotect-go-sdk v0.8.0)

[neilmartin83]

What

Bumps jamfprotect-go-sdk v0.7.1 → v0.8.0 and adds CLI support for the three capabilities that release introduces.

SDK addition	CLI surface
UnifiedLoggingFilterSet (full CRUD + list)	new protect unified-logging-filter-sets (ulfs)
UnifiedLoggingFilter.sets back-reference	surfaced in ulf list/get output
Plan.unifiedLoggingFilterSets + PlanInput	plans export/apply + flattened output

A filter set groups unified logging filters so they can be assigned to a plan as a unit — the ULF analogue of analytic-sets, and modelled on it directly.

Commands

protect unified-logging-filter-sets  (ulfs)
  list  get  apply  delete  export  add-filter  remove-filter

apply upserts by name, add-filter/remove-filter are idempotent read-modify-write, delete carries jamf:destructive. export uses filter names rather than UUIDs so files stay portable across tenants, and deliberately omits the server-side plans back-reference.

Attach to a plan by name:

name: My Plan
unifiedLoggingFilterSets:
  - My Filter Set

Notable

ULF get/apply moved from protect.PrintOne to printResult. The filter now carries a nested sets array, which renders badly in table output. This also brings ULF in line with the get/apply convention CLAUDE.md already documents, so it's a drive-by consistency fix rather than a new pattern.

SDK moved ULF operations from /graphql to /app. Worth knowing that the two endpoints serve different schemas — updatePlan doesn't exist on /graphql at all. Verified reads and mutations both work on the new path.

Verification

Exercised end-to-end against a live sandbox tenant, then cleaned up — full CRUD, granular mutation idempotency, name resolution, plan attach/detach, and the complete flag matrix from docs/solutions/conventions/output-flag-matrix (json/yaml/csv/plain/table, --field, --select, --compact, --quiet, NO_COLOR=1, --out-file).

Two API behaviours confirmed by probe, both handled by clear server-side errors rather than client guards:

• Deleting a set still used by a plan is refused.
• Deleting a filter cascades it out of any sets.

16 new tests, all green (go test ./... exit 0, 28 packages), make lint 0 issues, verify-generated and verify-site pass. Coverage spans the conversion/flatten functions (including empty-collection omission), resolver found/not-found/caching, and the structural allowlists in protect_test.go that verify wiring. The wiring tests were mutation-checked: unwiring AddCommand fails all three.

Pre-existing limitation documented, not changed

While cleaning up I found that protect apply cannot clear a collection field — if len(x) > 0 plus the SDK's nil check means an omitted key and an explicit [] produce the same request, so the server keeps its existing membership. This affects exceptionSets and analyticSets identically; it is not new here.

I made the new field match that shape rather than diverge, since two collections on the same input behaving differently would be worse than a uniform documented limitation, and the remove-* subcommands already cover the real use case. Written up in docs/solutions/conventions/protect-apply-cannot-clear-collections-2026-08-07.md with the shape a proper fix would need.

Happy to make apply clear-capable across all collection fields as a follow-up if you'd rather — it needs *[]string (or raw-map key presence) plus a matching SDK change.

🤖 Generated with Claude Code

[neilmartin83]

@ktn-jamf Addressed all three blocking findings plus the doc/README nice-to-haves:

1. Doc scope — CLAUDE.md and the solutions doc now scope the "apply cannot clear a collection" claim to plans apply's cross-resource reference fields, and explicitly call out that ulfs apply/analytic-sets apply clear their own membership list when the field is omitted. Added a matching --help (Long) warning to plans apply.
2. flattenULF field parity — restored uuid, description, tags (kept created/updated off, matching the fix's own acceptance criteria) so get/apply table/csv/plain output isn't missing columns vs. json/yaml.
3. Row-stable columns — flattenULF's sets and flattenULFSet's filters/plans are now always present (empty string when unpopulated) instead of conditionally omitted, so table/csv column sets and plain field counts don't depend on the first row.
4. Added TestULFSetExportToInput_ResolvesFilterNames / TestPlanExportToInput_ResolvesULFSetNames (plus unresolvable-name cases) to kill the two mutation survivors.
5. README alias table now lists ulfs; CLAUDE.md's Protect overview call count updated to 17.

Left (5) — the resolver-error/not-found conflation in apply — as a follow-up per your own note, since fixing it only for ulfs/ulf-sets would diverge from every sibling apply; a package-wide protect.ErrNotFound sentinel is the right scope for that.

go build ./..., go vet ./..., golangci-lint run ./internal/commands/..., and the full go test ./... all pass.

[neilmartin83]

Round 3 fixes pushed (fb6b6ca).

#	State	Notes
(1) 🟧 flattenPlan gates unifiedLoggingFilterSets	✅ Fixed	Key now unconditional with the same // always present: table/csv columns come from row 0 comment as the two siblings. TestFlattenPlan_OmitsEmptyUnifiedLoggingFilterSets inverted to TestFlattenPlan_EmptyUnifiedLoggingFilterSetsPresent — asserts present-and-empty, mirroring TestFlattenULFSet_OmitsEmptyCollections.
(2) 🟩 flattenULFSet omits uuid	✅ Fixed	uuid added; TestFlattenULFSet_FiltersAndPlans asserts it.
(3) 🟩 lost-update race on add-filter/remove-filter	💬 Follow-up	Pre-existing package-wide GET-then-PUT pattern shared with analytic-sets; fixing only for ulfs would diverge. Tracking with (5) from round 1.
(4) 🟩 nil-response panic after concurrent delete	💬 Follow-up	Same reasoning — identical unchecked-nil pattern in every sibling Get* call site. Tracking with (3)/(5).

go build ./... clean, go test ./internal/commands/... ./internal/protect/... green.

[ktn-jamf]

Tip

✅ Merge-ready — Adds protect unified-logging-filter-sets (ulfs) plus the jamfprotect-go-sdk v0.8.0 bump, surfacing sets/unifiedLoggingFilterSets back-references on ULF and plans. All findings from rounds 1 and 2 are fixed.

Rating: 5/5

• Clean — the fix commit (fb6b6ca) closes round 2's one blocking finding and its uuid nice-to-have with no regressions; the two remaining nice-to-haves are author-acknowledged, package-wide follow-ups, not defects in this PR.

Prior review status

Round 1 (5d8d912): 3 blocking findings (stale doc scope, flattenULF dropped fields, row-0 column instability) + 3 nice-to-have — all fixed in 216baa7. Round 2 (bae861f): 1 new blocking finding (flattenPlan reintroduced the same row-0 bug) + 1 actionable nice-to-have (flattenULFSet missing uuid) — both fixed in fb6b6ca, verified below. The two reliability nice-to-haves from round 2 (lost-update race on add-filter/remove-filter; nil-response panic after concurrent delete) remain 💬 Dismissed — author left them as package-wide follow-ups matching the identical pattern in every sibling apply/Get*, which the round-2 review agreed avoids diverging the fix from the rest of the package.

What's done well

✅ Three rounds in, the fix commits stay disciplined rather than papering over findings. fb6b6ca applies the exact // always present: table/csv columns come from row 0 fix to flattenPlan that rounds 1's fix already proved out on flattenULF/flattenULFSet, with the test assertion inverted the same way (TestFlattenPlan_EmptyUnifiedLoggingFilterSetsPresent). No shortcut, no new pattern — the third sibling now matches the other two exactly.

Review coverage

• Design and architecture: fb6b6caed extends the already-vetted analytic-sets/flattenULF pattern to flattenPlan; no divergence, no new abstraction
• Correctness: traced flattenPlan's unconditional unifiedLoggingFilterSets assignment through printTable/printCSV/printPlain's row-0 column derivation (internal/output/output.go) — confirmed the column no longer disappears when the first plan has no sets; confirmed flattenULFSet's new uuid field matches the sibling flattenULF
• Security: no new surface in the fix commit; SDK boundary was already resolved at source in rounds 1–2 (jamfprotect-go-sdk@v0.8.0 module cache read directly)
• [na] Performance: no DB/query/cache/hot-path files touched by the fix commit or the whole-PR diff
• Test coverage: TestFlattenPlan_EmptyUnifiedLoggingFilterSetsPresent and the uuid assertion in TestFlattenULFSet_FiltersAndPlans verified against the code change; go test ./internal/commands/... ./internal/protect/... green at current head (47d7acc)
• Reliability: the two dismissed nice-to-haves (lost-update race, nil-response panic) remain open only as cross-package follow-ups, unchanged in shape from round 2
• Code quality: fix commit's comment style and naming match the established convention
• [na] Simplification: no new abstraction introduced by the fix commit
• [na] Frontend concerns: no UI in this repo
• Documentation currency: re-verified CLAUDE.md:283 ("Protect overview makes ~17") and README.md:227 (ulfs alias row) both still match current code
• Cross-repo contracts: no new SDK surface touched since round 2's source-level verification
• Project rules compliance: no .claude/rules/ directory; fix commit adds no credential flags, touches no generated code

Scope of review

• This round's diff: whole-PR diff unchanged since round 2 except the fix commit fb6b6caed (3 files, +15/−9) plus an intervening main-merge commit (47d7accd7) that pulls in unrelated changes now on main (PR #316/#318) — confirmed not part of this PR's contribution via git diff --stat origin/main...origin/feat/protect-unified-logging-filter-sets, which still shows the original 18 files / 1017+/13− against current main.
• Fix commit read in full (git show fb6b6caed) and independently traced against internal/output/output.go's row-0 derivation, rather than taken on the author's description.
• Build and tests re-run at current head (47d7acc): go build ./... clean; go test ./internal/commands/... ./internal/protect/... green.
• Specialists not re-dispatched this round. All 4 eligible lanes (security-reviewer, usability-reviewer, devil-advocate, test-quality-reviewer) are already in ever from rounds 1–2. The fix commit introduces no new high-risk-regardless-of-size shape — the removed if len() > 0 is a display-formatting guard (which map key exists for table/csv output), not a security, auth, or business-logic branch, and it's a mechanical repeat of the identical guard already removed (and specialist-reviewed) in flattenULF/flattenULFSet during round 1. Per the "lane already in ever" rule, re-spawning is warranted only for new high-risk changes; none are present.
• Never-run lanes (unchanged from rounds 1–2): performance-reviewer — no DB/query/cache/hot-path files; scope-reviewer — single-concern change; fidelity-reviewer — no linked ticket/issue/branch-name key.

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