feat(protect): backup, restore, and the tenant overlay on Jamf-managed analytics

[neilmartin83]

Summary

Adds protect backup and protect restore, plus protect analytics overrides — and fixes seven export/apply bugs found by actually cloning one tenant into another.

Important

Breaking output change. groups, users, api-clients and exception-sets now export cross-resource references as names rather than server IDs. Anything that parses those four commands' output needs updating — see Output shape changes. Input stays backwards compatible in both directions; only the emitted shape moved.

Jamf Pro has had backup and diff for a while. Protect had neither, and no product in this repo had a restore, so cloning or rebuilding a Protect tenant meant a hand-rolled shell loop over fifteen export commands in the right order.

What's new

protect analytics overrides (list/get/set/clear/export/apply). Jamf publishes analytics centrally — a stock tenant returns ~156, all jamf: true, and the server refuses updateAnalytic on any of them:

This mutation may only be used for custom analytics.

What a tenant can change is an overlay: the severity it reports at and the actions it triggers, written by a separate updateInternalAnalytic mutation. The SDK has exposed it since v0.8.0; nothing in the CLI called it. Since the definitions are identical in every tenant, that overlay is the only part of a Jamf-managed analytic worth capturing — and it was the part being dropped.

protect backup / protect restore. Backup writes each object to its own file under a per-resource directory. Restore walks it in dependency order — members before the sets naming them, sets before the plans binding them, roles before groups before users — resolving every reference by name against the target as it goes. Existing objects are updated, absent ones created; nothing is ever deleted. Both take --resources (allowlist) and --exclude (denylist), which compose; restore adds --dry-run and --include-defaults.

The Order field in protectResources() is the correctness argument, so a test asserts the dependency chain rather than the numbers, and every resource must either be restorable or carry a reason it is not.

Deliberately not replayed, each reported at runtime: Jamf-managed content, tenant defaults (built-in roles, Default group, Default Analytic Set), API clients (the server issues a new secret on create), data forwarding (its response is not its update shape and embeds a tenant-specific IAM ExternalId), and identity provider connections (no create API).

Bugs fixed

Bug	Cause
analytics export | analytics apply failed for every analytic	export emits the community schema (actions = objects), apply decoded the SDK input (Actions = strings). The pipe was documented in CLAUDE.md and had never worked
groups/users/API clients exported raw role IDs	Role IDs are small sequential integers, so a foreign ID didn't error — it bound to whatever role held that integer. Wrong grant, silently
Plans lost threatPreventionStrategy + customEngineConfig	hand-written planExport had drifted from PlanInput
Analytics lost startup, label, matchReason	same drift. Every Jamf analytic has a label, so every export lost one
Analytics lost longDescription, remediation	emitted by the export converter, never read by the import converter
Groups-only users couldn't be created	nil slice marshalled as null; API wants []
unified-logging-filters export | apply failed for every filter	export emits the community schema (predicate), apply decoded the SDK input (Filter). Same bug as the analytics row, in the sibling resource — found late, by pointing each export at its own apply

All seven shared one root cause — assuming the shape you read is the shape you can write — which is now docs/solutions/logic-errors/response-shape-is-not-input-shape-2026-08-18.md. The resources that export the SDK input type directly had no bugs; every hand-written or community-schema export shape did.

The seventh is the interesting one: it was found after the other six were fixed and documented, by running one line of shell — export | apply for each resource in turn. Unit tests over the converters passed the whole time, because both directions were wrong in the same way. That probe is now a test (protect_roundtrip_test.go) covering twelve resources in both YAML and JSON, driving each document through both the restore closure and the apply command's decoder, since those are different code for six resources and the gap between them is exactly where this bug lived.

Output shape changes

Four resources' export output moved from server IDs to names. This is the fix for the RBAC row above, and it is a breaking change for anything parsing that output:

Command	Was	Now
groups export	RoleIDs, ConnectionID	roles: (names), connection: (name)
users export	RoleIDs, GroupIDs, ConnectionID	roles:, groups:, connection: (names)
api-clients export	RoleIDs	roles: (names)
exception-sets export	analyticUuid only	adds analytic: (name); JSON top-level keys go Name/Exceptions → name/exceptions

Reading is unaffected in both directions: rbacDocumentUsesIDs routes an ID-shaped document down the old path, exception sets still accept a uuid-only exception, and JSON keys that were PascalCase still bind because encoding/json matches case-insensitively. Tests pin both legacy shapes.

Why it had to change: role IDs are small sequential integers, so an ID-shaped document applied to a second tenant did not error — it bound to whatever role held that integer. A silent wrong grant is worse than a broken export.

Verification

Cloned one live tenant into another and diffed the result — the check the postmortem calls definitive, re-run on the current head after review fixes changed the exception-set document shape, the restore order, filename allocation and retention.

26 documents applied, 0 failed, re-run idempotent. 24 of 32 files byte-identical, with all eight differences accounted for:

Difference	Why it's correct
3 plans: commsConfig.fqdn	region-assigned IoT endpoint (us-east-1 → eu-central-1); the target keeps its own
1 exception set: analyticuuid	rebound to the target's uuid. Equality here would mean the rebinding failed
analytic-overrides	target holds its own overrides too; restore never deletes
connections	tenant-specific, no create API, never replayed
data-forwarding, 4 × api-clients	never replayed by design, each with a stated reason

Name→ID rebinding proven explicitly, on objects whose IDs all differ between tenants:

analytic      zz-clone-analytic-alpha  4adea5b2…  →  87ad78a7…
analytic-set  zz-clone-analytic-set    908c75e9…  →  c2d5ab33…
role          zz-clone-role            2348       →  541
group         zz-clone-group           2313       →  570
plan          zz-clone-plan            1686       →  563

Every set, plan, user and group binding in the target points at the target's own IDs, including the case that motivated the exception-set change: an exception targeting a custom analytic, whose uuid is per-tenant. Fixtures were built to cover the four resources the earlier run left unexercised (analytics, telemetry, custom-prevent-lists, removable-storage-control-sets), plus two analytic input types, vendor USB rules, CUSTOM_ENGINES threat prevention with a full customEngineConfig, disabled+tagged logging filters, and a plan binding all of it.

Real users were held back rather than replayed — eight of the nine in the source tenant have email alerts enabled, and creating those elsewhere emails real people. Synthetic users covered the path instead, including one in a group with no direct role (the []-not-null case). Both tenants were returned to their prior state.

Wire facts recorded in CLAUDE.md

None of these are derivable from the schema:

• Jamf-published analytic UUIDs are stable across tenants (BlazingKeylogger is da360eb3-… in both); custom ones are not. So an exception set's analyticUuid is portable only when it points at a Jamf analytic.
• Identity provider connection names are tenant-specific, so a user or group naming one restores only where that name exists.
• Action config params is AWSJSON! — read back as an object, but the input wants a JSON-encoded string, and it can't be omitted either.
• USB vendor/product IDs need an 0x prefix.
• Telemetry events is [String]! but validated against an undeclared allowlist; network_connect is valid, network/network_listen/dns_request/process_exec are not.
• Data retention updates are rate-limited to once per 24 hours.
• Analytic.hash is a revision stamp, not a content hash — unusable as a change-detection or identity key.

Also

• jamf-backup and jamf-migrate skills learn about Protect. Both declared themselves "Jamf Pro assistant" and would never have found Protect's backup, or its restore. They now lead with the asymmetry: Pro has backup+diff but no restore; Protect has backup+restore but no diff.
• Credential-bearing documents are written 0600 and reported. An HTTP action config's request headers are captured verbatim — the SDK's query selects headers { header value } in full — so a backup can contain a bearer token, and the jamf-backup skill tells users to git-init the directory. The run now names which resources those are before you commit the tree. The legacy ForwardSentinel.sharedKey is redacted outright, since data forwarding is never replayed.
• Backup exits non-zero on partial failure, matching pro backup, with --allow-partial-failure to downgrade. A backup that exits 0 with a resource missing is indistinguishable from a good one to the job that scheduled it.
• --resources and --exclude both shell-complete, and --help lists the vocabulary, generated from the resource table so it cannot drift. Previously the only ways to discover a resource name were completion on one of the two flags or passing a wrong value to read the error.
• insights, config-freeze and connections added to the backup set. Both writable ones compare before writing — not an optimisation: the API refuses a write of the value already held ("Tenant '…' is not in a change freeze"), so comparing is what makes replay idempotent.

Test plan

• make test — 29 packages pass
• make lint — 0 issues
• go vet ./..., go fix ./... (nothing to apply), make fmt (no changes)
• make verify-generated, make verify-site
• End-to-end clone between two live tenants, verified by diff
• Both tenants returned to their pre-test state

🤖 Generated with Claude Code

[neilmartin83]

Scope of review

PR #330 — protect backup/restore, protect analytics overrides, six export/apply fidelity fixes. 3464/-91 across 24 files. High risk (>500 lines, new source files, changes RBAC reference handling, adds credential-bearing on-disk artefacts).

Reviewed against the worktree at 1cd63c9. Verified the load-bearing wire claims independently against jamfprotect-go-sdk@v0.8.0's schema/schema.graphql and query text rather than taking the PR body at its word. go build ./... and go test ./internal/commands/ ./internal/protect/ both pass locally. No Jira key in title, description, or branch name. No prior reviews.

Specialist agents were not dispatched (session policy); their lanes — silent-failure, security, test quality — were covered inline.

Prior review status

No prior reviews, review comments, or issue comments on this PR.

---

This is unusually good work. The wire-fact documentation, the Order-chain test that asserts the dependency relation rather than the numbers, and the docs/solutions postmortem are all things most PRs of this size skip. Every one of the six claimed bug fixes has a test that would catch a regression, and I confirmed the updateInternalAnalytic semantics the design rests on: buildInternalAnalyticVariables omits unset fields and the mutation passes them as GraphQL variables into the input object, so "an omitted flag leaves that half untouched" is genuinely true.

The findings below are about what happens at the edges of the new bulk paths — where a single-object export becomes a 15-resource sweep to disk.

Findings

🟧 (1) protect backup writes third-party bearer tokens to plaintext files, in a directory the skill tells users to git-init — (security, confidence: high)

internal/commands/protect_action_configs.go:211 · internal/commands/protect_backup.go:437

actionConfigToInput re-encodes each client's params blob verbatim into the exported document. For an HTTP report client that blob contains headers, and I confirmed the SDK's query selects them in full:

params {
    ... on HttpClientParams {
        headers { header value }
        method
        url
    }

pruneEmptyValues does not recurse into []any, so Authorization: Bearer … survives byte-for-byte into action-configs/<name>.yaml at mode 0o644. The same applies to data-forwarding.yaml: ForwardSentinel.sharedKey is a plain String in the schema (only ForwardSentinelV2 uses the secretExists: Boolean pattern) and data_forwarding.go:30 selects it.

Two things make this a new exposure rather than a pre-existing one. protect backup sweeps every action config to disk in one unattended command, where action-configs export was a deliberate per-object act. And skills/skills/jamf-backup/SKILL.md — modified in this same PR — carries "Offer git initialization for backup directories to enable version tracking", so the default guided workflow commits those files to a repo.

Suggested fix: redact secret-shaped values on export — HTTP header values and sharedKey — writing a sentinel ("<redacted>") and reporting it, the way apiClientExport already deliberately drops the client secret. If redaction breaks the restore path for those resources, prefer 0o600 on any file that can carry one plus an explicit warning line, and add a sentence to the skill telling users to --exclude action-configs,data-forwarding before git-initialising.

Acceptance criteria: a backup of a tenant with an HTTP action config carrying an Authorization header contains no bearer token in cleartext, or the command says on stderr that it just wrote one and where.

🟧 (2) protect backup exits 0 on partial failure, unlike pro backup — (correctness, confidence: high)

internal/commands/protect_backup.go:497-504

if len(failures) > 0 {
    ...
    fmt.Fprintf(os.Stderr, "\nCompleted with %d failure(s) — see %s%s\n", ...)
    return nil
}

pro backup does the opposite for the identical condition (pro_backup.go:356-362): it returns exitcode.PartialOrPropagate(...) and requires an explicit --allow-partial-failure to downgrade to success. protect backup has no such flag and no way to get a non-zero code.

The consequence is concrete rather than stylistic. A scheduled protect backup && git commit sees exit 0 and commits a backup that is silently missing a resource; the failure is only visible to whoever reads stderr, which in CI nobody does. The command's own Long text says it mirrors the Pro layout — _meta/_failures filenames were chosen for exactly that — so the divergence is in the one place a script can actually observe.

Suggested fix: mirror pro_backup.go: return exitcode.PartialOrPropagate(total, len(failures), nil, msg) and add --allow-partial-failure with the same semantics and help string.

Acceptance criteria: a test where one resource's Export returns an error asserts a non-zero exit, and asserts --allow-partial-failure downgrades it to nil.

🟧 (3) File-name collisions silently drop objects while the count still reports them — (correctness, confidence: high)

internal/commands/protect_backup.go:400-411, protectFileNameSafe at :738

protectFileNameSafe collapses every run of non-[\w.@-] to a single _. Nothing tracks which safe names have been used:

path := filepath.Join(dir, protectFileNameSafe(e.Name)+ext)
if err := os.WriteFile(path, data, 0o644); err != nil { ... }
written++

os.WriteFile truncates, so two objects whose names normalise identically leave one file — while written increments twice, so _meta.yaml reports 2 and the summary line reports a count the directory does not contain. Protect names are free text, and the function's own comment says they "routinely contain slashes and colons", so Alert: High and Alert - High, or Test/1 and Test 1, both collide. On macOS the default APFS volume is case-insensitive, which extends this to any pair differing only in case — including two user documents keyed on emails like A@example.com and a@example.com.

A silent object loss in a backup tool is the failure mode that matters most, because it is only discovered at restore.

Suggested fix: keep a map[string]bool of safe names per resource directory; on collision, append a short deterministic discriminator (e.g. the first 8 chars of a SHA-256 of the original name) so both objects survive and the mapping stays stable across runs. Lowercase the key when checking so case-insensitive filesystems are covered.

Acceptance criteria: TestProtectFileNameSafe (or a new sibling) covers {"Alert: High", "Alert - High"} and {"MyPlan", "myplan"} producing two distinct paths, and counts equals the number of files actually on disk.

🟧 (4) Exception sets export a raw analyticUuid, so a cross-tenant restore fails unactionably — (correctness, confidence: high — wire-verified)

internal/commands/protect_backup.go (exception-sets, Order 10) → protect_exception_sets.go:183-185

backup's help promises "cross-resource references are names, not IDs, so a backup can be restored into a different tenant". One reference escapes that: exceptionToInput copies e.Analytic.UUID through unchanged, and rebuildExceptionSetInput is what the backup writes.

The PR knows this — CLAUDE.md now states an exception set's analyticUuid "is portable when it points at a Jamf analytic and broken when it points at a custom one" — but nothing in the code acts on it.

Corrected after wire-probing (iacsandbox): the server does not silently accept a foreign UUID. Applying two documents differing only in that one field, the real UUID created the set and the foreign one was refused with:

createExceptionSet: jamfprotect: graphql error: Action blocked due to dependencies on this resource.

So the failure mode is a loud but unactionable one, not a dangling reference: a cross-tenant restore of an exception set targeting a custom analytic fails, naming neither the analytic, nor the UUID, nor the reason. Still worth fixing — an operator has nothing to act on — but it does not silently misconfigure suppression, and severity rests on the failed restore rather than on a security hole.

Everything needed to fix it is already in hand: analytics is exported at the same Order and custom analytics are exported by name, so the resolver can rebind.

Suggested fix: carry the analytic name in the exception document alongside (or instead of) the UUID and resolve it against the target at restore, matching what analyticSetExportToInput and planExportToInput already do. If resolution by name is out of scope for this PR, at minimum have the exception-sets export warn per-object when an exception names an analytic that is not Jamf-managed, so the operator learns at backup time rather than never.

Acceptance criteria: a restore of an exception set whose exception targets a custom analytic either binds to the target tenant's UUID for that analytic, or reports the unresolvable reference on stderr.

🟩 (5) data-retention restore skips the compare-before-write the other two settings do — (correctness, confidence: high)

internal/commands/protect_backup.go (data-retention, Order 70)

config-freeze and insights both read current state and write only real differences, and CLAUDE.md explains why that is correctness rather than optimisation. data-retention writes unconditionally, and the PR itself documents the outcome: "re-running a restore reports that resource as failed even though the desired state is already applied." GetDataRetention is already in the interface and dataRetentionToInput already normalises the response — comparing the two structs is a three-line change that makes replay idempotent and removes a spurious failure from every re-run inside 24 hours.

🟩 (6) upsertByName treats every resolve error as "absent" — (reliability, confidence: high)

internal/commands/protect_backup.go:88-105

id, err := resolve(ctx, name)
if err != nil {
    if _, err := create(ctx, input); err != nil { ... }

The resolvers return an indistinguishable error for "not found" and for "the list call failed" (resolve.go:363: fmt.Errorf("listing connections: %w", err)). A transient network blip or an expired token during a restore therefore converts an update into a create, and the operator sees whatever confusing duplicate-name error the server returns instead of the real cause.

This generalises a pattern the single-object apply commands already use, so it is consistent — but the blast radius is different: this is now the shared path for all fifteen resources in an unattended 42-document run. Worth a typed sentinel (protect.ErrNotFound) checked with errors.Is, so a genuine API failure aborts the object rather than mutating the tenant a different way.

🟩 (7) overrides apply aborts mid-document with no partial-progress report — (reliability, confidence: high)

internal/commands/protect_analytic_overrides.go (apply RunE)

A failing UpdateInternalAnalytic returns immediately, so an operator applying 40 overrides who fails at #20 gets an error naming only that one analytic and no indication that 19 already landed. restore gets this right — count, continue, report, non-zero at the end. Applying the same shape here keeps the two commands' behaviour predictable and makes a retry safe to reason about.

🟩 (8) Hand-rolled contains/indexOf in the test file — (code quality, confidence: high)

internal/commands/protect_backup_test.go:341-354

These reimplement strings.Contains with a redundant guard clause, and they take the unqualified names contains and indexOf at package scope in package commands — a package with a lot of test files, so the names are worth not claiming. Use strings.Contains.

🟩 (9) startup is now always sent for community-schema imports — (correctness, confidence: medium)

internal/commands/protect_analytics.go:464 — Startup: &ay.Startup

AnalyticInput.Startup is *bool and buildAnalyticVariables omits the variable when nil, so before this change a community YAML file with no startup: key left the field to the server's default. It now always resolves to a non-nil false and is always sent. That is correct for export | apply round-trips (which is the bug being fixed) but changes behaviour for analytics import of upstream jamf/jamfprotect files, which don't declare the field. If the server default is anything other than false, those imports now differ from before. Worth confirming on the wire, or making the field *bool in analyticYAML so absent stays absent.

🟩 (10) The RBAC export shape change is user-facing and isn't flagged as one — (documentation, confidence: high)

protect groups export, protect users export, and protect api-clients export previously emitted RoleIDs/ConnectionID; they now emit roles:/connection:/groups: as names. Input compatibility is preserved by rbacDocumentUsesIDs, which is the right call — but anything that parses the export output (a script reading .RoleIDs, a Terraform-adjacent wrapper) breaks silently. The PR body frames this under "Bugs fixed", which is accurate about the cause and quiet about the consequence. Worth an explicit "output shape changed" line in the PR body and in the wiki pages for those three commands, so the release notes carry it.

🟥 (11) unified-logging-filters apply cannot consume unified-logging-filters export — (correctness, confidence: high — wire-verified)

internal/commands/protect_ulf.go:128

Found by pointing each export at its own apply against a live tenant:

$ jamf-cli protect unified-logging-filters export "Some filter" -o yaml | \
    jamf-cli protect unified-logging-filters apply --yes
CreateUnifiedLoggingFilter: input → filter: '' should be non-empty

ulfToYAML writes the community schema, whose predicate key is predicate; jamfprotect.UnifiedLoggingFilterInput calls the same field Filter. apply decoded the SDK shape directly, so the predicate never bound and the server refused every filter in every tenant.

This is a seventh instance of the bug class this PR documents in docs/solutions/logic-errors/response-shape-is-not-input-shape-2026-08-18.md — in the resource sitting next to the one the PR fixed. backup/restore were unaffected because they route through the YAML converters, which is precisely why unit tests over the converters didn't catch it.

Suggested fix: ulfInputFromDocument, mirroring the analyticInputFromDocument the PR already added; predicate vs filter is an unambiguous discriminator.

Acceptance criteria: export | apply round-trips for a filter, --scaffold | apply still works, and both shapes are covered by tests.

🟧 (12) Set exports churn because membership isn't sorted — (correctness, confidence: high — wire-verified)

internal/commands/protect_analytic_sets.go:339 · internal/commands/protect_ulf_sets.go:352

planToExport gained sort.Strings in this PR with the right reasoning attached — "the server returns it in its own order, so an unsorted export makes two identical plans diff". analyticSetToExport and ulfSetToExport have the same problem and didn't get the same fix.

Observed: backup, self-apply the Default Analytic Set, backup again → the two backups differ by the set's entire 33-line membership, rotated, with nothing actually changed. That lands directly on the workflow the jamf-backup skill recommends (git-tracked backup directories), where it reads as a real configuration change every time.

Suggested fix: sort.Strings(names) in both, matching planToExport.

Acceptance criteria: the same membership in two different server orders produces the same document.

What's genuinely good

• ✅ TestProtectResourceOrderEncodesDependencies asserts the dependency relation rather than the order numbers. That is the test that keeps working after someone renumbers the table, and it is the one most people don't write.
• ✅ TestProtectResourcesAreRestorableOrExplained makes "not replayable" a declared property with a reason string instead of a nil function nobody notices. Both directions are asserted, including the both-set contradiction.
• ✅ The compare-before-write on config-freeze and insights, with the comment explaining that it is idempotency and not an optimisation. That reasoning is exactly why a reader won't "simplify" it away later.
• ✅ Verifying the SDK's own semantics before relying on them: I checked buildInternalAnalyticVariables and the updateInternalAnalytic mutation text myself, and the "omitted flag leaves that half untouched" contract holds precisely as documented.
• ✅ docs/solutions/logic-errors/response-shape-is-not-input-shape-2026-08-18.md generalises six specific bugs into one reusable rule, with frontmatter that the repo's own convention will surface to the next person working in internal/commands.

Review coverage

• Design and architecture — table-driven resource set, Order as the correctness argument, generic upsertByName
• Correctness — findings 2, 3, 4, 5, 6, 9; verified SDK variable-omission semantics and the GraphQL mutation text
• Security — finding 1 (secret-bearing export content); confirmed apiClientExport correctly omits the client secret
• Cross-repo / wire contracts — verified InternalAnalyticInput, HttpClientParams, ForwardSentinel, DataRetentionSettings against jamfprotect-go-sdk@v0.8.0's schema and query text
• Reliability — findings 6, 7; restore's per-object tolerate-and-count is otherwise right
• Test coverage — each of the six bug fixes has a targeted regression test; gaps noted under 2 and 3
• Code quality — finding 8
• Documentation — CLAUDE.md, solutions doc, both skills; gap noted under 10
• [na] Performance — bounded per-object fetches for the two summary-only list queries, correctly explained in the struct comment
• [na] Database migrations, infrastructure, dependencies — none touched
• [na] Frontend — none touched
• Whether the server accepts a foreign-tenant analyticUuid (finding 4) — resolved by wire-probing: it rejects it with an opaque error, so the finding is a failed restore rather than a silent misconfiguration

Rating: 4/5

Excellent engineering with genuinely exemplary documentation and test design — the Order-chain test, the restorable-or-explained test, and the postmortem are all better than what most PRs this size ship. What holds it off 5 is that the PR's own thesis wasn't applied exhaustively: the seventh instance of "response shape is not input shape" was one resource away from the six it fixed, and the plans-membership sort it added wasn't carried to the two sibling set types.

All twelve findings are now fixed on this branch (bf51f93, 0cf1041) and wire-verified against iacsandbox: every export | apply pipe round-trips across thirteen resources, a full self-restore applies 14 documents with 0 failures and leaves the tenant byte-identical, self-applying both set types produces no diff, and back-to-back retention restores both report "already matches". 29 packages pass, lint 0 issues, verify-generated and verify-site clean. Probe fixtures removed and the tenant returned to its prior state.

Note

Findings 11 and 12 were found by a one-line probe worth adopting for any export/apply pair: pipe each export straight into its own apply. Unit tests over the converters passed the whole time, because both directions were wrong in the same way.

---

_{Reviewed with Claude Code. Findings verified against jamfprotect-go-sdk@v0.8.0's schema and query text, and wire-probed against a live tenant.}

[ktn-jamf]

Warning

⚠️ Needs changes — Adds protect backup/restore, protect analytics overrides, and seven export/apply fidelity fixes.
Blocking: (1), (2), (3), (4), (5). See collapsed sections for 9 nice-to-have suggestions and a decomposition recommendation.

Rating: 3/5

• Would be a 5 with the 0600 guarantee in (1), the tenant guard on pruning in (2), plan-membership convergence in (3), and the two consistency defects in (4) and (5).
• Coverage: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate and test-quality-reviewer have never been dispatched on this PR — their lanes were covered inline by the orchestrator, so the rating reflects one reader's pass over those dimensions rather than seven independent ones.

Findings

🟧 (1) (security, c=100) — internal/commands/protect_backup.go:1271: the 0600 the run promises is not applied to a file that already exists

mode is chosen at :1244-1248 and handed to os.WriteFile, which passes perm to OpenFile(…, O_CREATE|O_TRUNC, perm) — so the mode applies only when the file is created. Every re-run into an existing tree keeps whatever mode the file already had, while :1329 still prints WARNING: … were written 0600. Git does not record non-exec permissions, so the workflow skills/skills/jamf-backup/SKILL.md recommends — git-init the backup directory — reproduces the file at 0644 on every clone. TestRunProtectBackupTightensPermissionsOnSensitiveResources uses a fresh t.TempDir(), so it only ever exercises the creation path.

Failure scenario: a backup repo containing action-configs/siem.yaml is cloned (git creates it 0644), then protect backup --output <that dir> re-writes it with the bearer token → the file stays world-readable and stderr reports it as 0600, reachable from the PR as shipped.

Suggested fix:

 			if err := os.WriteFile(path, data, mode); err != nil {
 				return fmt.Errorf("writing %s: %w", path, err)
 			}
+			// os.WriteFile only applies perm at creation, so an existing file
+			// keeps whatever mode it had (a git checkout hands back 0644).
+			if mode != 0o644 {
+				if err := os.Chmod(path, mode); err != nil {
+					return fmt.Errorf("tightening permissions on %s: %w", path, err)
+				}
+			}

Fixed when: a second protect backup into a directory whose sensitive document was pre-created 0644 leaves it at 0600, and TestRunProtectBackupTightensPermissionsOnSensitiveResources gains a sub-test that pre-creates the file at 0644 before the run.

---

🟧 (2) (correctness, c=75) — internal/commands/protect_backup.go:1285-1291: pruning is not scoped to the tenant the directory belongs to

protectPruneStale deletes every document in a resource directory that this run did not write. The run records provenance — TenantURL at :1303 — but nothing ever reads an existing _meta back, so the identity of the tree being pruned is never checked. Pointing protect backup at a directory that holds a different tenant's backup therefore deletes that tenant's documents for every object the current tenant does not have. The command carries no jamf:destructive annotation and no --yes, unlike restore, and the loss is reported only as a Pruned N document(s) count. TestBackupPrunesDocumentsThatNoLongerMatchTheTenant covers the deleted-object, format-switch, failed-export and bystander cases, but never a second tenant.

Failure scenario: jamf-cli -p protect-prod protect backup --output ./protect-backup run against a directory previously filled by -p protect-staging → every staging document for an object production lacks is removed in place, with the only signal a prune count, reachable from the PR as shipped.

Suggested fix: read the existing _meta before pruning and refuse when its tenant_url disagrees with cliCtx.ProtectURL:

+	// Pruning is destructive and keyed on "this tenant's object set", so it must
+	// not run against a directory that belongs to a different tenant.
+	if !noPrune {
+		if prior, ok := readProtectBackupMeta(outputDir); ok &&
+			prior.TenantURL != "" && prior.TenantURL != cliCtx.ProtectURL {
+			return fmt.Errorf("%s holds a backup of %s, not %s — pruning would delete that tenant's "+
+				"documents; use a different --output or pass --no-prune",
+				outputDir, prior.TenantURL, cliCtx.ProtectURL)
+		}
+	}

Fixed when: a backup run whose _meta names a different tenant_url errors out (or prunes nothing) instead of deleting, and a test asserts it with two mock tenants writing to one directory.

---

🟧 (3) (correctness, c=100) — internal/commands/protect_backup.go:522-532: restore cannot converge a plan's membership, so a rollback silently leaves extra bindings attached

The plans closure calls planExportToInput (internal/commands/protect_plans.go:452), which sets ExceptionSets, AnalyticSets, ULFSets only under if len(…) > 0 (:470, :481, :492) and USBControlSet/Telemetry only under != "" (:503, :510). The SDK's buildPlanVariables (jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go:387) omits each of those GraphQL variables when nil, so an absent list means "leave unchanged". That is deliberate and documented for plans apply (CLAUDE.md), but restore's own help says "Existing objects of the same name are updated", and the SDK already exposes TelemetryV2Null for the explicit-null case. The three sibling set types converge correctly (analyticSetExportToInput, ulfSetExportToInput, groupExportToInput all send a non-nil empty slice), so plans are the one asymmetric resource — and omitempty on planExport means the on-disk document cannot even express "no members".

Failure scenario: a plan gains an analytic set on Monday; Tuesday's protect restore of Sunday's backup reports applied <plan> → the Monday set is still bound in the target, reachable from the PR as shipped.

Suggested fix: keep plans apply additive and make restore convergent, by parameterising the conversion rather than changing the shared one:

-func planExportToInput(ctx context.Context, e planExport, r *protect.Resolver) (jamfprotect.PlanInput, error) {
+// clearAbsent sends an empty list for a membership field the document omits, so
+// restore converges the target. `plans apply` passes false: CLAUDE.md documents
+// that an omitted list there leaves membership alone.
+func planExportToInput(ctx context.Context, e planExport, r *protect.Resolver, clearAbsent bool) (jamfprotect.PlanInput, error) {
 	...
+	if clearAbsent {
+		if e.ExceptionSets == nil { input.ExceptionSets = []string{} }
+		if e.AnalyticSets == nil  { input.AnalyticSets = []jamfprotect.PlanAnalyticSetInput{} }
+		if e.ULFSets == nil       { input.UnifiedLoggingFilterSets = []string{} }
+		if e.Telemetry == ""      { input.TelemetryV2Null = true }
+	}

Fixed when: restoring a plan document with no analyticSets/unifiedLoggingFilterSets/exceptionSets/telemetry clears those bindings in the target, plans apply keeps its documented additive behaviour, and protect_roundtrip_test.go gains an empty-membership plan fixture asserting the cleared input.

---

🟧 (4) (reliability, c=100) — internal/commands/protect_backup.go:404: the analytic-overrides restore closure aborts at the first refused override, and the sibling command's comment claims it does not

overrides apply was changed to report-and-continue, with the reasoning spelled out at internal/commands/protect_analytic_overrides.go:452-456 — "'protect restore' takes the same approach". It does not: the restore closure returns on the first UpdateInternalAnalytic error, so the entries after it are never attempted and the applied count computed at :407 is discarded. The whole singleton then reports as one FAILED line. That is the exact defect the author fixed one file over, with a comment asserting parity that the code does not hold.

Failure scenario: a 40-entry analytic-overrides.yaml whose 20th entry names an action the target tenant has not provisioned → restore prints one FAILED analytic-overrides.yaml line, 19 overrides are already written and unreported, and entries 21–40 are never attempted, reachable from the PR as shipped.

Suggested fix:

 					if _, err := c.UpdateInternalAnalytic(ctx, a.UUID, input); err != nil {
-						return "", fmt.Errorf("applying override for %q: %w", o.Analytic, err)
+						fmt.Fprintf(os.Stderr, "  FAILED override for %q: %v\n", o.Analytic, err)
+						failed++
+						continue
 					}
 					applied++
 				}
-				return fmt.Sprintf("%d override(s)", applied), nil
+				if failed > 0 {
+					return "", fmt.Errorf("%d of %d override(s) failed (%d applied)", failed, len(doc.Overrides), applied)
+				}
+				return fmt.Sprintf("%d override(s)", applied), nil

Fixed when: a refused override is reported and the remaining entries still apply, the summary names both counts, and the resource still exits non-zero — matching what overrides apply now does and what its comment claims.

---

🟧 (5) (usability, c=100) — internal/commands/protect_backup.go:1163 and :1403: the two new commands declare local flags that shadow root persistent flags, removing -o and -n

--output (:1163) and --dry-run (:1403) collide by name with the root persistent -o, --output (output format) and -n, --dry-run (internal/commands/root.go:794, :800). Cobra's AddFlagSet skips an inherited flag whose name is already taken, so the shorthand goes with it. Probed on a binary built at head:

$ jamf-cli protect backup -o json --output /tmp/x     → unknown shorthand flag: 'o' in -o   (exit 2)
$ jamf-cli protect restore -n --input /tmp/x          → unknown shorthand flag: 'n' in -n   (exit 2)

Neither -o, --output nor -n, --dry-run appears in those commands' --help Global Flags list. This is the same mechanism the head commit (7e90e09) fixed for --allow-partial-failure, whose replacement comment at :1189 states the rule. pro backup shares the --output collision, so that half is precedent; --dry-run has no precedent, and the root flag already means exactly "preview changes without executing".

Failure scenario: the documented CI mechanism JAMF_CLI_ARGS='-o json' jamf-cli protect backup --output ./d exits 2 with unknown shorthand flag: 'o', reachable from the PR as shipped (verified).

Suggested fix: drop the local --dry-run and read the package var the way allowPartialFailure is now read; if --output is kept for parity with pro backup, say in the Long text that -o is unavailable on this command.

-	cmd.Flags().BoolVar(&dryRun, "dry-run", false, "report what would be applied without calling the API")
+	// --dry-run is the root persistent flag (-n), read directly the way
+	// allowPartialFailure is. A local flag of the same name drops the shorthand.

(and take dryRun from the package var in runProtectRestore's caller)

Fixed when: protect restore -n --input <dir> performs a dry run, and either protect backup -o json parses or the help text says why it cannot.

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

Nice-to-have suggestions (9 items)

🟩 (6) (correctness, c=75) — internal/commands/protect_groups.go:149: groups apply does not share restoreGroup's access-group workaround, so groups export | groups apply still fails for a connection-less local access group with the raw server message the PR set out to replace. Route groups apply's update branch through restoreGroup.

🟩 (7) (code-quality, c=100) — internal/commands/protect_groups.go:112, protect_users.go:124, protect_api_clients.go:108: apply --scaffold still prints the SDK input shape (roleids/connectionid) while export now emits names. exception-sets got its scaffold updated in this PR; these three teach the shape the PR deprecates.

🟩 (8) (reliability, c=100) — internal/commands/protect_backup.go:1338-1341: the _failures manifest is written under if err == nil { _ = os.WriteFile(…) }, so a marshal or write failure is silent while the very next line tells the operator to "see _failures.yaml". Report the write error rather than swallowing it.

🟩 (9) (correctness, c=100) — internal/commands/protect_backup.go:1430 vs :1046: singleton restore accepts only .yaml/.json, but protectRestoreExts (used for pruning) and the collection walk at :1454 both accept .yml. So insights.yml is deleted by a backup run and ignored by a restore. Use protectRestoreExts in all three places.

🟩 (10) (code-quality, c=100) — internal/commands/protect_backup.go:1109: pruned singletons are reported as <resource>/<file> (e.g. insights/insights.json) though the file lives at the directory root. Join only when !res.Singleton.

🟩 (11) (usability, c=100) — internal/commands/protect_backup.go:1546: restore returns a plain error, so it exits General (1) rather than PartialFailure (7) and ignores --allow-partial-failure, unlike the backup path at :1352. Use exitcode.PartialOrPropagate(applied, failed, nil, msg) for symmetry.

🟩 (12) (correctness, c=100) — internal/commands/protect_backup.go:668: the insights restore iterates the want map, so the order of live mutations and of the log lines is nondeterministic — against the "Deterministic order so a restore is reproducible and its log diffable" rule stated at :1458. Sort the labels before the loop.

🟩 (13) (reliability, c=75) — internal/commands/protect_action_configs.go:248-252: a json.Marshal failure substitutes "{}", so an action client whose params cannot be encoded is backed up with empty params and restored as a broken client. Return the error instead — params is AWSJSON! and a silent {} is not a valid fallback.

🟩 (14) (performance, c=75) — internal/commands/protect_backup.go:198 and :355: analytics and analytic-overrides each call ListAnalytics (~156 rows in a stock tenant) on every full backup. One fetch shared between the two closures would halve it; worth it only if the resource table grows a way to share state.

PR decomposition

Scope Review

Recommendation: split into 3, merge in order. At +5970/−117 across 30 files this is past the "very large" threshold, and it carries three concerns that are independently valuable, independently testable, and independently revertable. The strongest evidence that they are separable is the PR's own narrative: the seven export/apply fixes were discovered by building backup/restore, but none of them depends on backup/restore existing, and one of them (the RBAC shape change) is a breaking output change that deserves its own release note rather than a row in a feature PR's table.

• PR 1 — export/apply fidelity (~1400 lines). protect_analytics.go, protect_ulf.go, protect_plans.go, protect_exception_sets.go, protect_rbac_refs.go, protect_groups.go, protect_users.go, protect_api_clients.go, protect_action_configs.go, protect_org.go, protect_conversions_test.go, protect_exception_sets_test.go, protect_rbac_refs_test.go, plus docs/solutions/logic-errors/response-shape-is-not-input-shape-2026-08-18.md. Ships the seven bug fixes and the breaking four-resource output-shape change on their own, so the release note is about exactly that. Findings (6) and (7) land here.
• PR 2 — protect analytics overrides (~700 lines). protect_analytic_overrides.go + its test, the registry.go UpdateInternalAnalytic addition, the CLAUDE.md overrides paragraphs. A self-contained subcommand tree with no dependency on backup.
• PR 3 — protect backup/restore (~3500 lines). protect_backup.go, protect_backup_test.go, protect_roundtrip_test.go, resolve.go's ErrNotFound, the wiring in protect.go/groups.go/root.go, and the two skills. Depends on both predecessors — which is the argument for the order, not for the merge. Findings (1)–(5) all land here, and reviewing 3500 lines of new backup/restore semantics without 2500 lines of unrelated converter churn in the same diff is the concrete benefit.

Nothing here argues the work is wrong or that it should be redone — it argues that the five 🟧 findings above all sit in one of the three parts, and a reviewer looking only at that part would have had a better chance of catching them. If the author prefers to land it as one PR, that is a defensible call for a single-author repo; the recommendation carries no severity and does not hold the merge.

Review coverage

• Design and architecture: the table-driven protectResources() with Order as the correctness argument is the right shape; upsertByName generic and the Export/Restore closure pair keep the set declarative. Read protectResources() end to end and traced every Order band against its references. The one design gap is the plan-membership asymmetry in (3).
• Correctness: findings (2), (3), (4), (6), (9), (10), (12), (14). Traced restore ordering (analytics/unified-logging-filters 10 → sets 20 → plans 30 → roles 40 → groups 50 → users/api-clients 60), the per-resource resolver reset at protect_backup.go:1519-1526, protectSelectResources include/exclude composition, protectNameAllocator collision handling, and protectPruneStale's candidate derivation for both the singleton and collection cases.
• Security: finding (1). Confirmed pruneEmptyValues does not recurse into []any, so HttpClientParams.headers values survive verbatim into action-configs/*.yaml — which is what SensitiveReason exists for; confirmed apiClientExport omits the client secret and redactDataForwarding replaces ForwardSentinel.SharedKey without mutating its input. Verified os.WriteFile's perm-on-create-only semantics with a standalone probe.
• Cross-repo / wire contracts: resolved the load-bearing SDK couplings against jamfprotect-go-sdk@v0.8.0 source rather than the PR body — buildPlanVariables (plan.go:387) confirms the nil-omission that finding (3) rests on, and confirms TelemetryV2Null exists as the explicit-null mechanism the fix can use. No cross-repo assumption left unresolved.
• Reliability: findings (4), (8), (13). Verified protect.ErrNotFound/notFoundError.Is and that upsertByName only creates on a genuine not-found, including through the %w wrappers in groupExportToInput/planExportToInput.
• Performance: finding (14). The two N+1 fetches (action-configs, exception-sets) are forced by summary-only list queries and are documented at the Export field comment; restore's per-resource resolver reset is a deliberate correctness trade.
• Test coverage: 12-resource × 2-format round-trip harness plus TestProtectRoundTripTableCoversEveryRestorableResource's exempt-with-a-reason gate is the strongest part of the PR. Gaps named under (1) (creation path only), (2) (no second tenant), (3) (no empty-membership plan fixture), (4) (no override-failure fixture).
• Code quality: findings (7), (10). Comment density is high but every long comment carries a wire fact, not narration. Prior review's hand-rolled contains/indexOf are gone.
• Simplification: no redundant abstraction introduced; protectResourceListHelp generating the vocabulary from the table is the right call over a hand-maintained list.
• [na] Frontend concerns: no UI, site, or template files changed.
• Documentation currency: CLAUDE.md gains the routing-table rows, the backup/restore section, and the wire-fact list; both skills learn the Pro/Protect asymmetry; the postmortem's frontmatter (module: internal/commands, category: logic-errors) matches the existing docs/solutions/logic-errors/ convention. No consolidated documentation finding warranted — but note finding (4)'s comment is the one place a doc claim and the code disagree.
• Scope and decomposition: covered in the PR decomposition section — +5970 across three separable concerns.
• Project rules compliance: no .claude/rules/ directory. Checked against CLAUDE.md: credential-input policy (no new credential flags — --token-file/env only, unaffected), generated-code boundary (no files under */generated/ touched), Protect conventions (positional <name> args, apply upsert, --yes/interactive confirm on destructive paths — restore and overrides clear both annotated jamf:destructive and gated; backup's prune is the exception, see (2)), filename prefixes (protect_*), help groups and aliases wired in groups.go.

Important files changed

File	Score	Notes
internal/commands/protect_backup.go	3/5	1549 new lines carrying every 🟧 in this review
internal/commands/protect_roundtrip_test.go	5/5	Certified clean: the two-axis (restore closure + apply decoder) × two-format harness, plus the coverage gate that fails when a restorable resource is added without a fixture, is the right test for this bug class and has no gap I could find
internal/protect/resolve.go	5/5	Certified clean: notFoundError.Is gives every one of the 17 resolvers an errors.Is-matchable sentinel without changing a single user-visible message
internal/commands/protect_action_configs.go	4/5	The AWSJSON! re-encoding and the batchConfig Int!-vs-nullable split are correct and well-argued; the marshal fallback is (13)
internal/commands/protect_plans.go	4/5	threatPreventionStrategy/customEngineConfig recovery and the three membership sorts are right; the conversion's clearing semantics are (3)
internal/commands/protect_rbac_refs.go	5/5	Certified clean: name-based references with []string{} initialisation in all three converters, and rbacDocumentUsesIDs keeps the legacy shape readable in both encodings

Scope of review

Full diff (30 files, +5970/−117) at head 7e90e09, base origin/main (027396a), in the worktree at /tmp/pr-review-Jamf-Concepts-jamf-cli-330.

Read in full: internal/commands/protect_backup.go (1549 lines, whole file at head), internal/commands/protect_analytic_overrides.go, internal/commands/protect_rbac_refs.go, and the complete diffs of protect_plans.go, protect_exception_sets.go, protect_ulf.go, protect_ulf_sets.go, protect_analytic_sets.go, protect_analytics.go, protect_action_configs.go, protect_org.go, protect_groups.go, protect_users.go, protect_api_clients.go, internal/protect/resolve.go, internal/registry/registry.go, internal/commands/root.go, groups.go, protect.go, and CLAUDE.md.

Sampled rather than read line by line (per the very-large-diff guidance): the 2900 lines of test additions — read protect_roundtrip_test.go's driver and coverage gate in full, and in protect_backup_test.go the permission, partial-failure, prune, upsertByName and restoreGroup tests in full; the remaining ~60 conversion/flatten unit tests were read at the level of their names and assertions. Both skills/skills/*/SKILL.md and the postmortem were read for factual agreement with the code, not line by line.

Beyond the diff: internal/commands/pro_backup.go (partial-failure and --output parity), internal/exitcode/exitcode.go (PartialOrPropagate semantics), internal/commands/root.go:793-813 (persistent flag set), and jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go (buildPlanVariables).

Executed: go build ./..., go vet ./internal/... (clean), go test ./internal/commands/ ./internal/protect/ (both pass), plus three behavioural probes on a binary built at head — the -o/-n shadowing in (5), and a standalone Go program confirming os.WriteFile's perm-on-create-only semantics for (1). No live tenant was available, so every wire claim in the PR body about server behaviour (the updateInternalAnalytic semantics, the foreign-analyticUuid rejection, the retention rate limit, the accessGroup create-only asymmetry, the telemetry event allowlist) is taken as reported and not independently verified; the SDK-side half of each was checked against v0.8.0's source.

Never-run specialist lanes. No agents were dispatched this round (session policy: the orchestrator covered the lanes inline). Eligible but never dispatched on this PR: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate, test-quality-reviewer — all seven eligible on a high-risk diff of this shape, all seven covered inline instead, which is one reader rather than seven. fidelity-reviewer is inapplicable: no Jira key in the title, branch or description, and no linked issue, so there is no spec to check against — the PR body was used as the statement of intent instead.

Unresolved: none. No cross-repo contract was left unchecked.

Prior review status

One prior review by @neilmartin83 at 0cf1041 (round 1, marker ran= ever=), commit VALID (reachable from head), carrying 12 findings and claiming all 12 fixed. Verified each against head 7e90e09: 11 fixed — (1) secrets now declared via SensitiveReason, written 0600 and reported (but see (1) above, which is the residual); (2) exitcode.PartialOrPropagate plus the root --allow-partial-failure, and the duplicate local flag dropped in 7e90e09; (3) protectNameAllocator with a name-derived discriminator and a case-folded key; (4) exception sets carry analytic: and rebind by name; (5) data-retention compares before writing; (6) protect.ErrNotFound + errors.Is; (8) hand-rolled contains/indexOf replaced by strings.Contains; (9) analyticYAML.Startup is now *bool; (10) the shape change is called out in the PR body and CLAUDE.md; (11) ulfInputFromDocument; (12) sort.Strings in both set exports. 1 partially fixed: (7) — overrides apply now reports-and-continues, but the analytic-overrides restore closure still aborts at the first failure, which is promoted to (4) above.

What's done well

• ✅ TestProtectRoundTripTableCoversEveryRestorableResource makes coverage a declared property: adding a restorable resource without a round-trip fixture fails the build, and each exemption carries a reason string rather than being skipped by a type check. Paired with the two-axis driver — the restore closure and the apply decoder, which are different code for six resources — this is the test that would actually have caught the seventh bug, and it is the one most authors do not write.
• ✅ The prune rails are argued from the failure mode rather than the feature: only files this command could have written, only extensions restore would read, and never after a failed export because "the true object set is unknown". TestBackupPrunesDocumentsThatNoLongerMatchTheTenant's bystander sub-test pins the boundary. Finding (2) is a missing dimension on that reasoning, not a contradiction of it.
• ✅ Recording the wire facts that are not derivable from the schema — Analytic.hash being a revision stamp, the accessGroup create-only asymmetry, params being AWSJSON! but non-nullable, the undeclared telemetry event allowlist — directly in CLAUDE.md, each with the exact server message. That is the half of this work that will still be paying off in a year.

_{🤖 Generated by the pr-review:review skill · reviewed head 7e90e09}

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

[neilmartin83]

Addressed in 433b72e. All five blocking findings, eight of the nine nice-to-haves, one declined with reasoning.

Blocking

(1) 0600 not applied to an existing file — fixed. Explicit os.Chmod after the write, gated on mode != 0o644. TestRunProtectBackupTightensPermissionsOnAnExistingFile pre-creates the document at 0644 and asserts the run tightens it; mutation-checked (the test fails with the chmod removed).

Worth adding to the record: the mode protects the working copy, not the repository. Git stores no non-exec permissions, so a clone reproduces 0644 regardless of what the run wrote — a private repo is not a substitute for --exclude. Both the skill and CLAUDE.md now say so, since the previous wording implied 0600 travelled with the commit.

(2) Pruning not scoped to the tenant — fixed as suggested. readProtectBackupMeta reads the existing _meta in either format, and the run refuses when its tenant_url disagrees with cliCtx.ProtectURL. --no-prune writes alongside. TestBackupRefusesToPruneAnotherTenantsDirectory covers all three: refused before anything is deleted, --no-prune allowed, and the owning tenant still prunes (the guard must not turn pruning off wholesale). Both sides of the comparison must be non-empty, so the existing tests that build a CLIContext with no ProtectURL are unaffected.

(3) Plan membership not convergent — fixed with the parameterisation you proposed. planExportToInput takes clearAbsent: restore passes true, plans apply passes false. exceptionSets/analyticSets/unifiedLoggingFilterSets become non-nil empty slices, telemetry becomes TelemetryV2Null.

One gap I could not close: usbControlSet has no explicit-null mechanism in the SDK — a plain *string with no Null sibling, so buildPlanVariables has no way to emit usbControlSet: null. Sending "" is untested on the wire and would more likely be refused as an unresolvable ID than read as a clear. It is commented at the site and documented in CLAUDE.md as the one binding restore cannot converge. Fixing it needs an SDK change.

TestPlanExportToInputClearsAbsentMembershipOnlyForRestore asserts both directions plus that a populated list is unaffected by the flag.

(4) analytic-overrides restore aborts at the first refusal — fixed. Report-and-continue, then fail at the end with both counts, so the resource still counts as failed and the run still exits non-zero. TestRestoreAnalyticOverridesReportsAndContinues seeds three entries, fails the middle one, and asserts the first and third were both attempted (internalAnalyticWrites == [a1, a3]) and that the error names 1 of 3 and 2 applied.

(5) Flags shadowing root persistent flags — --dry-run dropped, runProtectRestore's caller reads the package var the way allowPartialFailure is read. TestProtectRestoreUsesTheRootDryRunFlag asserts no local declaration and that -n resolves to --dry-run after ParseFlags. Verified on a binary: protect restore -n --input … now parses and reaches the directory check.

--output stays for parity with pro backup, and backup's Long now says the global -o is unavailable and that --format picks the encoding, asserted by TestProtectBackupHelpExplainsTheLostOutputShorthand. Also added the general rule to CLAUDE.md's Conventions, since this is the second instance in this PR alone.

Nice-to-haves

• (6) groups apply shares the guard. Extracted as protectGroupUpdateSatisfied in protect_groups.go, which now holds the wire facts; restoreGroup's comment points at it. Rather than routing apply through restoreGroup (which returns a name string and would lose apply's printResult), apply fetches the existing group and runs the same guard. TestGroupsApplySharesTheAccessGroupGuard drives the command end to end for both outcomes.
• (7) The three RBAC scaffolds print the export shape. This needed the reference fields to lose omitempty — an empty scaffold otherwise omits the very fields it is meant to teach. That is a defensible change on its own: an absent list and an empty one mean the same thing to the converters (they always send []), so writing them out keeps a document self-describing and keeps two tenants' copies of one group diffable when either holds no roles. Output is now name/connection/accessGroup/roles, email/connection/roles/groups/receiveEmailAlert, name/roles.
• (8) _failures write errors are returned. Both the marshal and the write, since the next line tells the operator to read that file.
• (9) Singleton restore uses protectRestoreExts, and the collection walk now uses slices.Contains over the same list, so all three sites share one definition. TestCollectProtectRestoreFilesAcceptsYmlForSingletons covers insights.yml; mutation-checked.
• (10) Pruned singletons report the bare file name; only collections join the resource directory.
• (11) Restore returns exitcode.PartialOrPropagate and honours --allow-partial-failure. TestRunProtectRestoreExitsPartialOnMixedResults asserts PartialFailure on a mixed run and nil under the flag.
• (12) Insight labels sorted before the loop.
• (13) A params marshal failure returns an error. actionConfigToInput now returns (input, error); both call sites propagate. Agreed on the reasoning — AWSJSON! carrying the client's whole configuration means "{}" is not a degraded version of it, it is a client backed up broken and restored broken.

(14) declined. Sharing one ListAnalytics between the analytics and analytic-overrides exports saves one GraphQL call per backup, and the only way to do it is a memo captured in protectResources() — mutable state shared across a table that tests, completion and help all instantiate freely. Bad trade for the size of the win, and your own note conditioned it on the table growing a way to share state, which it has not.

Decomposition

Taking the "single-author repo, defensible call" branch and landing it as one PR. The three parts are separable as you describe, but PR 3 depends on both predecessors, so splitting now means three sequential review rounds on code that is already reviewed twice — and the five 🟧 findings were all found, which is the outcome the split was meant to buy. Noted for the next feature of this size: the fidelity fixes should have gone out first, before backup/restore existed to discover them.

go build ./..., go vet ./internal/..., golangci-lint run ./internal/commands/... (0 issues) and go test ./... all pass.

[ktn-jamf]

Warning

⚠️ Needs changes — round 2 on 433b72e. All five round-1 blockers are genuinely fixed and mutation-checked; two of the fixes are incomplete in a way that reopens the risk they were meant to close.
Blocking: (1) --no-prune rewrites _meta, disarming the new tenant guard for the next run. (2) The shared access-group guard now reports success while dropping a requested change.

Rating: 3/5

• Would be a 5 with the _meta provenance made durable in (1) and the access-group guard distinguishing "nothing to do" from "cannot do this" in (2). Everything from round 1 is closed.
• Coverage: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate and test-quality-reviewer have never run on this PR — their lanes were covered inline by the orchestrator across both rounds, so the rating reflects one reader's pass over those dimensions rather than seven independent ones.

Prior findings status (14 items)

#	Location	State	Notes
(1) c=100	protect_backup.go:1342	✅ Fixed	Explicit os.Chmod after os.WriteFile, gated on mode != 0o644 (the only other value is 0o600, :1307-1309). Mutation-checked: deleting the chmod block fails TestRunProtectBackupTightensPermissionsOnAnExistingFile with mode = 0644, want 0600. The added CLAUDE.md and SKILL.md wording about 0600 not surviving a clone is correct and was not asked for.
(2) c=75	protect_backup.go:1258-1264	⚠️ Partial	The guard is exactly as suggested, refuses before anything is written or deleted, and is pinned by TestBackupRefusesToPruneAnotherTenantsDirectory (mutation-checked — stubbing the condition to false fails it). But _meta is rewritten unconditionally at :1371-1377, including on the --no-prune run the guard's own error recommends, so the directory's recorded tenant flips and the next pruning run deletes the first tenant's documents. Carried forward as (1) below, verified by execution.
(3) c=100	protect_plans.go:534-555, protect_backup.go:533	✅ Fixed	clearAbsent parameterisation exactly as proposed; restore passes true, plans apply passes false, and TestPlanExportToInputClearsAbsentMembershipOnlyForRestore asserts both directions plus the populated-list case. The usbControlSet gap is real and the author's reason checks out: verified in jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go:160,417 that it is a bare *string with no Null sibling, while TelemetryV2Null (:154-155, :412) does emit telemetryV2: null. Two residuals are 🟩 (3) and 🟩 (4) below.
(4) c=100	protect_backup.go:396-414	✅ Fixed	Report-and-continue, then fail with both counts so the resource still counts as failed. TestRestoreAnalyticOverridesReportsAndContinues asserts internalAnalyticWrites == [a1, a3]; mutation-checked (restoring the early return fails it). The protect_analytic_overrides.go comment that claimed parity is now true.
(5) c=100	protect_backup.go:1443-1485	✅ Fixed	Local --dry-run dropped; runProtectRestore's caller reads the package var. Verified on a binary built at 433b72e: protect restore -n --input … now parses through to the auth check, and -n, --dry-run appears in protect backup --help's Global Flags. --output is kept for pro backup parity with the Long text saying -o is unavailable — protect backup -o json still exits 2 with unknown shorthand flag: 'o' in -o, which is the "say why it cannot" branch of the round-1 acceptance criteria. The new CLAUDE.md Conventions rule is the right place for it.
(6) c=75	protect_groups.go:114-125, :186-196	⚠️ Partial	The guard is genuinely shared now and TestGroupsApplySharesTheAccessGroupGuard drives both outcomes end to end. But extracting it also gave groups apply a success path that skips the update for reasons unrelated to what the document asks for. Carried forward as (2) below, verified by execution.
(7) c=100	protect_groups.go:143, protect_users.go:124, protect_api_clients.go:108	✅ Fixed	All three scaffolds now print the export shape, and all three apply commands decode either shape (groupInputFromDocument/userInputFromDocument/apiClientInputFromDocument), so the scaffold is accepted by the command that prints it — checked on the binary. The omitempty removal that made this possible is argued and documented; one format-asymmetry consequence is 🟩 (5).
(8) c=100	protect_backup.go:1411-1419	✅ Fixed	Both the marshal and the write are returned.
(9) c=100	protect_backup.go:1511, :1537	✅ Fixed	All three sites read protectRestoreExts; the collection walk uses slices.Contains over the same list. Mutation-checked — narrowing the singleton set back to .yaml/.json fails TestCollectProtectRestoreFilesAcceptsYmlForSingletons.
(10) c=100	protect_backup.go:1144-1150	✅ Fixed	Singletons report the bare file name.
(11) c=100	protect_backup.go:1630-1638	✅ Fixed	Byte-for-byte the shape pro_backup.go:357-362 and protect_backup.go:1423-1431 use, so all three report a mixed result identically. Mutation-checked against TestRunProtectRestoreExitsPartialOnMixedResults.
(12) c=100	protect_backup.go:673-682	✅ Fixed	Labels sorted before the loop.
(13) c=75	protect_action_configs.go:252-256	✅ Fixed	actionConfigToInput returns (input, error); both call sites propagate.
(14) c=75	protect_backup.go:198, :355	💬 Dismissed	Author: sharing one ListAnalytics "is a memo captured in protectResources() — mutable state shared across a table that tests, completion and help all instantiate freely. Bad trade for the size of the win". Reasonable, and it matches the condition the round-1 finding itself attached.

Findings

🟧 (1) (correctness, c=100) — internal/commands/protect_backup.go:1371-1377: _meta is rewritten on every run including --no-prune, so following the tenant guard's own advice disarms it

The guard at :1258-1264 is correct in isolation. What it trusts is not: TenantURL at :1376 is overwritten unconditionally at the end of every successful run, --no-prune included. So the sequence the refusal message itself recommends — "use a different --output, or pass --no-prune to write alongside them" — relabels the directory as belonging to the second tenant, and the next run by that tenant passes the guard and prunes the first tenant's documents. The --no-prune sub-test in TestBackupRefusesToPruneAnotherTenantsDirectory stops one run short of this.

Two smaller cracks in the same trust: _meta is written last (:1371), so a run that failed or was interrupted leaves documents with no provenance at all; and readProtectBackupMeta treats an unparseable manifest as "unknown" (:1068-1081) rather than as a reason not to prune. Both leave the guard blind on a directory that is demonstrably not empty.

Failure scenario (reproduced in the worktree, three runProtectBackup calls against one directory):

staging  → dir            : writes zz-staging-only.yaml, zz-shared.yaml, _meta(staging)
prod     → dir --no-prune : refusal avoided as documented; _meta now says prod
prod     → dir            : "Pruned 1 document(s)" — zz-staging-only.yaml is gone

Reachable from the PR as shipped, and reached by doing exactly what the error text says.

Suggested fix: make the provenance record cumulative rather than last-writer-wins, and prune only when the current tenant is its sole owner.

-		TenantURL: cliCtx.ProtectURL,
+		TenantURL: cliCtx.ProtectURL,
+		// Append-only: a --no-prune run adds itself rather than relabelling the
+		// directory, so a later pruning run still sees the other tenant.
+		Tenants:   appendTenant(prior.Tenants, prior.TenantURL, cliCtx.ProtectURL),

and at :1258, refuse when prior names any tenant other than this one — including via Tenants — and treat an unparseable _meta beside a non-empty resource directory the same way.

Fixed when: the three-call sequence above leaves zz-staging-only.yaml on disk, a test pins it, and a corrupt _meta in a directory that already holds documents does not silently enable pruning.

---

🟧 (2) (reliability, c=100) — internal/commands/protect_groups.go:114-125 and :186-196: the shared access-group guard reports "nothing to update" for any change, so groups apply silently discards one

protectGroupUpdateSatisfied returns satisfied = true on input.AccessGroup && isLocal && existing.AccessGroup alone — it never compares roles, connection, or anything else the document carries. groups apply (:186-196) then prints Group %q is already an access group; nothing to update, echoes the existing group through printResult, and exits 0. Before this round the same input reached UpdateGroup and failed loudly with the server's message; the round-1 (6) fix replaced a bad error with a lost update. restoreGroup (protect_backup.go:157-162) has the same behaviour, so a restore of an edited backup reports applied for a group it did not change.

The server genuinely cannot perform that update — that is the wire fact CLAUDE.md records — so the right answer is not to attempt it, but the command has to say the change could not be applied rather than that there was nothing to apply.

Failure scenario (reproduced in the worktree): target holds local access group zz-group with no roles; document is its own export with roles: [zz-role] added. groups apply --from-file … --yes → updateGroup calls 0, exit 0, message nothing to update. The role grant is silently dropped, and the same document restores as applied.

Suggested fix: split the two cases.

-	if existing.AccessGroup {
-		// Desired state already holds; updateGroup could only refuse it.
-		return true, nil
-	}
+	if existing.AccessGroup {
+		// updateGroup would refuse this document outright, so nothing can be sent.
+		// Only "no other change was asked for" makes that a no-op rather than a
+		// dropped change.
+		if diff := groupUpdateWouldChange(existing, input); diff != "" {
+			return false, fmt.Errorf("group %q is a connection-less local access group, so updateGroup "+
+				"refuses any update to it, and this document changes %s — clear accessGroup in the "+
+				"document, or delete and recreate the group in the target", input.Name, diff)
+		}
+		return true, nil
+	}

Fixed when: applying a document that changes a local access group's roles exits non-zero naming the field it could not change, re-applying an unchanged one still makes no updateGroup call, and TestGroupsApplySharesTheAccessGroupGuard gains a changed-roles sub-test.

Those are the only two blocking items — with them addressed this is merge-ready.

Nice-to-have suggestions (4 items)

🟩 (3) (test coverage, c=100) — internal/commands/protect_backup.go:533: the blocking fix for round-1 (3) is not pinned at its call site. Flipping planExportToInput(ctx, e, r, true) to false — i.e. reverting the whole convergence fix — leaves go test ./internal/commands/ green (verified). The new conversion test covers the function with both flag values, and the round-trip harness's plan case passes false because it exercises the apply decoder, so nothing asserts that the restore closure asks for clearing. The round-1 acceptance criterion was an empty-membership plan fixture in protect_roundtrip_test.go asserting the cleared input; a cheaper equivalent is a direct assertion on the resource's Restore closure with a mock recording the PlanInput it receives.

🟩 (4) (correctness, c=100) — internal/commands/protect_plans.go:534-555: restore's convergence has a second unconverged field beyond the documented one. usbControlSet is called out in the code and in CLAUDE.md, and the SDK limitation is real (verified at plan.go:160,417). The legacy telemetry reference is not: buildPlanVariables omits telemetry when nil (plan.go:409-411) and clearAbsent only sets TelemetryV2Null, while planToExport (protect_plans.go:416-419) deliberately reads a plan bound through the legacy field. So a plan whose telemetry lives in telemetry rather than telemetryV2 keeps it through a rollback, with no comment saying so. Either null it too or give it the same one-line note usbControlSet got.

🟩 (5) (code-quality, c=100) — internal/commands/protect_rbac_refs.go:36, :43, :54 vs :85, :141, :203: dropping omitempty does not achieve the stated goal in JSON. The three converters leave the slices nil when the object holds no members (e.Roles = append(...) on a zero value), and a nil slice marshals as roles: null in JSON but roles: [] in YAML — confirmed with a standalone probe. Nothing breaks (the converters always send [] on the way in, and the round-trip harness passes in both formats), but a JSON backup now carries an explicit null where the comment promises a self-describing empty list, and the two formats' documents for the same object disagree. Initialise Roles/Groups to []string{} in groupToExport, userToExport and apiClientToExport.

🟩 (6) (usability, c=75) — internal/commands/protect_backup.go:1259-1264: the tenant guard compares ProtectURL as a raw string. resolveProtectClient (root.go:1155-1231) assigns the URL verbatim from --url, JAMFPROTECT_URL, the profile or JAMF_URL with no normalisation, so https://x.protect.jamfcloud.com/ and https://x.protect.jamfcloud.com are two tenants to this check — a legitimate re-run is refused with holds a backup of A, not B naming two strings that differ by a slash. Compare a normalised host (trim the trailing slash, fold case) instead.

Review coverage

• Design and architecture: the two blocking items are both design gaps in the round-1 fixes rather than coding slips — (1) is a provenance record that is not authoritative, (2) is a predicate answering a narrower question than its call sites ask. The clearAbsent parameterisation and the extracted protectGroupUpdateSatisfied are otherwise the right shapes, and the CLAUDE.md Conventions rule about shadowing flags is exactly the kind of thing that stops a third instance.
• Correctness: findings (1), (2), (4), (5), (6). Traced the whole new tenant-guard path including readProtectBackupMeta's failure modes, the unconditional _meta write, the clearAbsent branch against the SDK's buildPlanVariables, and the extension-set unification across all three sites.
• Security: finding (1) from round 1 is closed. Confirmed the chmod gate covers both modes the code can produce (0o644/0o600 only, :1307-1309) and that the new mode != 0o644 guard cannot leave a sensitive document loose. The doc additions about 0600 not surviving git are accurate.
• Cross-repo / wire contracts: re-resolved every SDK claim the fixes rest on against jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go rather than the PR comment — TelemetryV2Null → vars["telemetryV2"] = nil (:412-413), USBControlSet a bare *string with no null sibling (:160, :417-419), and the nil-omission for the three list fields (:400-408). The author's stated SDK limitation is real.
• Reliability: finding (2). The overrides report-and-continue and the _failures write-error propagation are both correct; the restore partial-failure block matches pro_backup.go line for line.
• Performance: (14) declined with a reason that holds. The one new API call is GetGroup per groups apply update, which is one round trip on an interactive single-object command.
• Test coverage: +318 lines in protect_backup_test.go and +94 in protect_conversions_test.go, and they are real tests, not shape assertions — six of the seven fixes I mutation-checked were caught by a named test with a useful failure message. The one that survives is finding (3): the clearAbsent call site. TestProtectRestoreUsesTheRootDryRunFlag's ParseFlags-then-ShorthandLookup pattern is the right way to pin a Cobra merge behaviour.
• Code quality: finding (5). The comments carry wire facts and failure modes rather than narration, and each new test opens with why the bug was possible.
• Simplification: no new abstraction; protectGroupUpdateSatisfied is the correct extraction even though its contract needs widening.
• [na] Frontend concerns: no UI, site, or template files in the incremental diff.
• Documentation currency: CLAUDE.md and skills/skills/jamf-backup/SKILL.md were read against the code. Every claim checks out — the clearAbsent paragraph, the prune-refusal sentences, the os.Chmod wire fact, the 0600-is-lost-on-clone warning in both places, the protectGroupUpdateSatisfied sharing note, and the new Conventions rule. Two wordings to tighten rather than findings: the Long text at :1196-1199 now runs a sentence into the pre-existing paragraph (--no-prune writes alongside them. Without that a backup directory…), and the SKILL.md line "keep one directory per tenant (or pass --no-prune)" recommends the action that finding (1) shows is unsafe to repeat.
• Scope and decomposition: author took the single-PR branch with a stated rationale and a note for next time. No severity; the recommendation never held the merge.
• Project rules compliance: no .claude/rules/. Re-checked against CLAUDE.md — no new credential flags, nothing under */generated/, protect_* prefixes, Protect conventions intact, and the PR's own new Conventions rule is satisfied by the code it ships (protect restore no longer declares --dry-run). Note for a follow-up ticket, not a finding: that rule already has pre-existing violations elsewhere — pro_jcds.go:463 re-declares --dry-run/-n, version.go:47 re-declares -v, and pro_setup.go/protect_setup.go/school_setup.go/config.go re-declare --url.

Scope of review

Incremental diff 7e90e09..433b72e (12 files, +659/−64) in the worktree at /tmp/pr-review-Jamf-Concepts-jamf-cli-330, read in full — no sampling was needed at this size. Baseline is my round-1 review at 7e90e09 (#330 (review)), whose commit is reachable from the new head, so the diff bisects cleanly.

Beyond the incremental diff: internal/commands/protect_groups.go and protect_rbac_refs.go in full at head, protect_backup.go:1240-1440 and :1500-1640 at head, internal/commands/root.go:1150-1235 (Protect URL resolution) and :790-815 (persistent flag set), internal/exitcode/exitcode.go (PartialOrPropagate), internal/commands/pro_backup.go:350-365, and jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go.

Executed: go build ./..., go vet ./internal/..., go test ./internal/commands/ ./internal/protect/ (all pass); golangci-lint run ./internal/commands/... reports 6 staticcheck issues, all pre-existing in pro_device_platform_test.go, which this PR does not touch (confirmed present on origin/main). Six mutation spot-checks on the fixes, each applied to a staged copy swapped in by rename and restored the same way, with git status --porcelain verified empty afterwards: removing the os.Chmod, stubbing the tenant guard to false, restoring the overrides early return, narrowing the singleton extension set, replacing restore's PartialOrPropagate with a plain error — all five were caught by the named new test. Flipping planExportToInput(…, true) to false at protect_backup.go:533 was not caught, which is finding (3). Two behavioural probes reproduced findings (1) and (2) as throwaway tests in the same fashion. Flag behaviour was checked on a binary built at 433b72e.

Prior feedback since the baseline: one issue comment from @neilmartin83 (2026-08-20T14:35Z) walking through all 14 findings. No new review comments, no other reviewers, nothing unaddressed from anyone else. The comment's claims were treated as claims: each was checked against the code, and the two ⚠️ Partial classifications above are where the code does less than the comment reports.

Not verified: no live tenant, so every server-behaviour claim remains as reported — in particular that updateGroup refuses any update to a connection-less local access group (finding (2) assumes the author's wire fact is right and argues about the CLI's response to it), and that a GraphQL exceptionSets: [] actually clears rather than being rejected. The SDK-side half of each was checked against v0.8.0.

Never-run specialist lanes. No agents were dispatched this round either (session policy: the orchestrator ran every lane inline). Eligible but never dispatched on this PR across both rounds: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate, test-quality-reviewer. fidelity-reviewer remains inapplicable — no Jira key in the title, branch or description and no linked issue, so there is no spec to check against.

What's done well

• ✅ Every new test is written from the failure it prevents, and six of the seven survive a mutation check. TestBackupRefusesToPruneAnotherTenantsDirectory's third sub-test — "the owning tenant still prunes" — is the one most authors skip: it pins that the new guard did not quietly turn the feature off, which is the usual cost of adding a safety check.
• ✅ Reporting the usbControlSet gap instead of papering over it, with the reason (*string with no Null sibling, "" untested on the wire), a comment at the site, and a line in CLAUDE.md. I verified the claim against the SDK and it is exactly right. A fix that names what it could not fix is worth more than one that quietly sends "".
• ✅ Turning the -o/-n collision into a written repo rule in CLAUDE.md's Conventions rather than a local fix, and pinning the Cobra behaviour with a test that parses the flag set instead of asserting on help text. That is the half of the round-1 finding that stops the third occurrence.

_{🤖 Generated by the pr-review:review skill · re-reviewed head 433b72e (baseline 7e90e09)}

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

[neilmartin83]

Round 2 addressed in 236f75d. Both blockers plus all four nice-to-haves.

🟧 (1) _meta rewritten on every run, disarming the guard

Confirmed and fixed. You were right that the guard was fine and the record was not — and the sharper point is that the guard's own error text was the reproduction path.

Provenance is now authoritative in four ways rather than one:

• Cumulative. protectBackupMeta.Tenants is append-only; every run adds its own normalised URL to whatever the prior manifest held. TenantURL stays for readability and older manifests, and tenantURLs() reads both, so a manifest written before this change still contributes its tenant. --no-prune records itself instead of relabelling the directory.
• Written first. _meta is claimed before the export loop, then replaced with counts at the end. A run that dies mid-export now leaves provenance for the next run's guard.
• Single-file. writeProtectBackupMeta removes any manifest an earlier --format left in the other extension. Otherwise a yaml-then-json directory holds two, and readProtectBackupMeta answers with whichever extension it checks first rather than the current one — which would have quietly reintroduced the same blindness.
• Unreadable is a refusal, not a blank slate. readProtectBackupMeta now returns three states (absent / parsed / present-but-broken), and protectBackupProvenanceUsable refuses when the broken case sits beside documents. That check runs regardless of --no-prune, which is a slightly stronger stance than you suggested and I think the right one: the run is about to rewrite the manifest, and rewriting one it could not read drops another tenant's claim and hands the next run a guard that passes. Same data loss, one run later. An unreadable manifest beside an empty tree is still just noise.

Your three-call sequence now ends in a refusal with staging's document still on disk — TestBackupProvenanceSurvivesANoPruneRunByAnotherTenant runs exactly those three calls and then asserts tenantURLs() holds both tenants.

🟩 (6) folded in

Normalisation is part of the same fix rather than a separate one, since the guard is only trustworthy if it is also not wrong in the other direction. normaliseTenantURL trims and lowercases; TestBackupTenantGuardNormalisesTheURL re-runs a backup as https://Staging.Protect.jamfcloud.com/ and asserts it is allowed and still prunes — a match that silently disables pruning would be the wrong kind of pass.

🟧 (2) the access-group guard reported success while dropping a change

Confirmed and fixed, and your framing is the one I took: the server genuinely cannot do the update, so the fix is not to attempt it — it is to stop claiming there was nothing to apply.

groupUpdateWouldChange compares every field updateGroup's mutation can carry (name, accessGroup, roleIds — roles sorted, since the server returns them in its own order and an unsorted compare would report a change on every re-apply of an unedited export) plus the connection it cannot: a document that moves a group off a connection is also asking for something update cannot express, so it belongs in the diff rather than in the no-op branch. A non-empty diff is an error naming the fields; an empty one keeps the existing skip, still with zero updateGroup calls.

restoreGroup propagates it unchanged, so an edited backup now fails loudly instead of reporting applied. TestGroupsApplySharesTheAccessGroupGuard gained your changed-roles sub-test plus a restoreGroup one, and both fail when the diff check is stubbed to "".

🟩 (3) the clearAbsent call site was unpinned

Reproduced — flipping true to false at protect_backup.go:533 left the suite green. TestPlansRestoreAsksForAbsentMembershipToBeCleared drives the plans resource's Restore closure through a mock that records the PlanInput it receives, and asserts the three lists are non-nil and TelemetryV2Null is set. It fails on that flip.

🟩 (4) the legacy telemetry reference

Correct — I'd carried the reference from the wrong field. PlanInput.Telemetry is a bare *string with no Null sibling and buildPlanVariables omits the key when nil, so it is unconverged for exactly the reason usbControlSet is, not for the reason my comment implied. Took the one-line-note option: nulling it is not expressible, and planToExport reads that field deliberately, so the gap is reachable rather than theoretical. The comment now says so.

🟩 (5) omitempty removal and JSON null

Confirmed with a probe of my own — nil marshals as null in JSON and [] in YAML, so the two formats' documents for one group disagreed and JSON carried an explicit null where the type's comment promises a self-describing empty list. Roles/Groups are initialised in all three *ToExport functions. TestRBACExportsCarryEmptyListsInBothFormats marshals a memberless group, user and API client in both formats and rejects any null.

Wording notes

Both taken. The Long paragraph is split so the prune rationale and the tenant guard are separate paragraphs instead of one sentence running into the other, and the SKILL.md line no longer recommends --no-prune as a bare workaround — it says what --no-prune now records, since that is what makes repeating it safe.

Verification

go build ./..., go vet ./internal/..., go test ./... all pass; golangci-lint run ./internal/commands/... reports 0 issues (the six pre-existing staticcheck findings you saw in pro_device_platform_test.go are gone from main). Eight mutation checks on the new tests, each applied and reverted with a clean git status afterwards: non-cumulative Tenants, no early _meta claim, no unreadable-manifest guard, no URL normalisation, stale manifests kept, the group diff stubbed to "", clearAbsent flipped to false, and the group export's Roles left nil — all eight caught by a named test.

Still not verified on a live tenant: that updateGroup refuses any update to a connection-less local access group (finding (2) rests on that wire fact, as you noted), and that exceptionSets: [] clears rather than being rejected.

[ktn-jamf]

Warning

⚠️ Needs changes — round 3 on 236f75d. Both round-2 blockers are genuinely closed, and this is the strongest fix round of the three: eight mutation checks, eight caught by a named test. The one blocking item is not a regression in those fixes — it is a sibling of round-1 (5) that neither of the first two rounds looked for.
Blocking: (1) protect backup advertises the root -n, --dry-run and ignores it, so a "preview" run deletes files.

Rating: 4/5

• Would be a 5 with (1). Every prior finding — both 🟧 and all four 🟩 — is fixed, and each fix fails a named test when reverted.
• Coverage: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate and test-quality-reviewer have never run on this PR across all three rounds — their lanes were covered inline by the orchestrator, so the rating reflects one reader's pass over those dimensions rather than seven independent ones.

Prior findings status (6 items)

#	Location	State	Notes
(1) c=100	protect_backup.go:1371-1377 → now :1181-1206, :1407-1438	✅ Fixed	Provenance is authoritative in four ways, and I verified each independently. Cumulative: Tenants is append-only via tenantURLs(), which reads the legacy TenantURL too. Written first: the claim write at :1427 precedes the export loop; the counts write at :1542 replaces it. Single-file: writeProtectBackupMeta removes the other extension's copy. Unreadable refuses: readProtectBackupMeta now returns three states and protectBackupProvenanceUsable refuses on broken-beside-documents, --no-prune included. Executed the round-2 three-call sequence: run 3 is refused naming staging.protect.jamfcloud.com, and zz-staging-only.yaml is still on disk. Mutation-checked five ways (see Scope) — every one caught. The four edge cases I was asked to chase: a directory with no _meta, and one whose _meta parses to no tenant, still prune → 🟩 (2); empty TenantURL is skipped by tenantURLs() and harmless; a first-ever --no-prune run does record provenance (probed: tenants=[https://staging.x], and the next tenant's pruning run is refused); a --format switch leaves exactly one manifest (probed).
(2) c=100	protect_groups.go:114-125, :186-196	✅ Fixed	groupUpdateWouldChange (:138-171) compares Name, AccessGroup, RoleIDs (both sides sorted, so an unedited export re-applies as a no-op) and the connection an update cannot carry — the last is reachable only when the document drops a connection, since a document that keeps one fails isLocal and never enters the branch. A non-empty diff is an error naming the fields; updateGroups stays at 0. Mutation-checked: stubbing the diff to "" fails both new sub-tests with expected an error: the role grant cannot be applied, so success would drop it and the restoreGroup equivalent. Roles-only change on a local access group now errors — never silently succeeds.
(3) c=100	protect_backup.go:533 (now :566)	✅ Fixed	TestPlansRestoreAsksForAbsentMembershipToBeCleared drives the plans resource's own Restore closure through a mock that records the PlanInput, and asserts all three lists non-nil plus TelemetryV2Null. Mutation-checked: flipping the call site to false — the exact revert that left round 2 green — now fails it. This is the right test: it pins the decision at the call site rather than the function.
(4) c=100	protect_plans.go:534-555	✅ Fixed	Took the one-line-note option, and the note is correct: PlanInput.Telemetry is a bare *string and buildPlanVariables omits a nil key, so it is unconverged for the same reason as usbControlSet, not the reason the old comment implied. The comment now also says the gap is reachable because planToExport reads the legacy field deliberately.
(5) c=100	protect_rbac_refs.go:36, :43, :54	✅ Fixed	Roles/Groups initialised to []string{} in all three *ToExport functions. Mutation-checked: reverting all three fails TestRBACExportsCarryEmptyListsInBothFormats for group, user and api-client in JSON. The failure message's wire fact (roleIds: None is not of type 'array') is the one already recorded at CLAUDE.md:268, not a new claim.
(6) c=75	protect_backup.go:1259-1264	✅ Fixed	normaliseTenantURL trims whitespace, strips the trailing slash and folds case; tenantURLs() dedupes and sorts. Mutation-checked: making it identity fails TestBackupTenantGuardNormalisesTheURL. That test asserts the right pair of things — the differently-spelled re-run is allowed and still prunes, so a normalisation that silently disabled pruning would not pass.

Findings

🟧 (1) (reliability, c=100) — internal/commands/protect_backup.go:1392: protect backup advertises the root -n, --dry-run and ignores it, so a "preview" run deletes files

This is the last member of round-1 finding (5)'s family, on the command that round never probed. --dry-run is a root persistent flag (root.go:800, "preview changes without executing"), protect backup declares no local flag of that name, so it is inherited cleanly and appears in the command's own help:

$ jamf-cli protect backup --help
Global Flags:
  -n, --dry-run                 preview changes without executing

runProtectBackup never reads it. grep -n dryRun internal/commands/protect_backup.go finds it only in the restore path (:1640, :1644, :1723, :1748) — the half round 2 fixed. So the flag parses, is documented on the command, and does nothing, on the one command in this PR that removes files without a confirmation prompt and without a jamf:destructive annotation (which is exactly the asymmetry round-1 (2) called out and this round's guard was built around).

That is also out of step with the rest of Protect: confirmDelete, confirmAction and confirmReplace (protect_helpers.go:83, :106, :129) all branch on dryRun and print [dry-run] Would …. pro backup sets a partial precedent for -n still writing files — the dryRunClient wrapper (root.go:716) only intercepts mutating HTTP — but pro backup never deletes anything, so nothing in the repo makes deletion-under--n precedented.

Failure scenario (executed in a scratch copy — probe TestProbeBackupIgnoresDryRun, dryRun set to true around a second runProtectBackup): a directory holds unified-logging-filters/zz-gone.yaml from an earlier run; the object has since been deleted in the tenant; the operator runs protect backup -n --output <dir> to see what a re-run would do. Result: zz-gone.yaml is still present = false. The file is gone, and the operator never got the preview the help promised. Reachable from the PR as shipped, by using the flag as documented.

Suggested fix: the smallest correct shape is to treat -n as implying no-prune and say so, rather than teaching the whole export loop about it:

 func runProtectBackup(ctx context.Context, cliCtx *registry.CLIContext, outputDir, format, filter, exclude string, noPrune bool) error {
 	if format != "yaml" && format != "json" {
 		return fmt.Errorf("invalid --format %q: must be yaml or json", format)
 	}
+	// -n is a root persistent flag and shows up in this command's help, so it has
+	// to mean something here. Backup's only destructive act is the prune, so a
+	// preview run reports what it would delete and deletes nothing. Writing the
+	// documents themselves is what --output asked for.
+	if dryRun {
+		noPrune = true
+	}

and where pruned is reported, prefix the lines [dry-run] would prune …. If the intent is instead that backup writes nothing at all under -n, say that in the Long text; either contract is fine, silence is not.

Fixed when: protect backup -n --output <dir> leaves every file on disk that a run without -n would have pruned, reports what it would have removed, and a test asserts it by setting the package dryRun var the way TestProtectRestoreUsesTheRootDryRunFlag does for restore.

That is the only blocking item — with it addressed this is merge-ready.

Nice-to-have suggestions (4 items)

🟩 (2) (correctness, c=100) — internal/commands/protect_backup.go:1182-1187: the guard refuses an unreadable _meta beside documents but waves through an absent or contentless one

protectBackupPruneAllowed returns nil on !found, and tenantURLs() returns empty for a manifest that parsed to its zero value — both read as "no tenant to protect" and permit the prune. Two probes, both executed:

P1  documents on disk, _meta.yaml deleted        → run err=<nil>, staging doc present=false
P2  documents on disk, _meta.yaml truncated to 0 → run err=<nil>, staging doc present=false

P2 is the sharper one, because a zero-length manifest is precisely what an interrupted or ENOSPC write leaves — and interruption safety is this round's stated reason for writing the manifest first. So the fix's own worst case lands in the branch the fix does not cover, and it lands there looking clean rather than broken. Both need a second tenant afterwards to cause loss, which is why this is 🟩 and not a reopened blocker.

The helper to close it already exists and is already called on the adjacent path: extend protectBackupProvenanceUsable (or protectBackupPruneAllowed) to refuse when protectBackupHoldsDocuments(dir) is true and the manifest names no tenant, absent or empty. The message can be the same one, with "cannot be read" replaced by "records no tenant".

🟩 (3) (usability, c=100) — internal/commands/protect_backup.go:1200-1203: once a second tenant has run --no-prune, the owning tenant permanently loses pruning, and the error text names no way back

Cumulative provenance is the right call, and SKILL.md states the consequence honestly ("a later run by either tenant is still refused"). Executed (P4): staging backs up → prod runs --no-prune as the refusal recommends → staging's own pruning run into its own directory is now refused with holds a backup of https://prod.x, not https://staging.x. Both remedies the message offers — a different --output, or --no-prune — abandon pruning; the only way to reclaim the directory is to hand-edit _meta, which the message does not mention and the docs do not either. Add a third clause naming it (or remove the other tenant from <dir>/_meta.yaml if you know those documents are gone), since the operator who reads this error is usually the one who caused it one run earlier.

🟩 (4) (code-quality, c=100) — internal/commands/protect_backup.go:41-43, :1434: the claim write overwrites the manifest's Resources/Counts with empties, and the struct comment promises a reader that does not exist

The claim manifest carries Resources: []string{} and Counts: map[string]int{}. Executed (P5): a good run records resources=[unified-logging-filters] counts=map[unified-logging-filters:2]; a run that then fails mid-export leaves resources=[] counts=map[] beside the previous run's intact documents — the manifest now describes a backup that never happened. Nothing breaks, because grep finds no reader of either field: readProtectBackupMeta's three call sites (:1163, :1182, :1407) are all inside backup, and restore never opens _meta at all. Which is the other half of this — the type comment says the manifest exists "so a restore can report the provenance of what it is about to write", and no restore path does. Either drop the promise from the comment, or carry prior.Resources/prior.Counts into the claim write so an interrupted run degrades to stale rather than to blank.

🟩 (5) (test quality, c=100) — internal/commands/protect_backup_test.go:1470-1478: TestRBACExportsCarryEmptyListsInBothFormats asserts on the substring null anywhere in the document

The assertion is !strings.Contains(string(data), "null") over the whole marshalled doc. It catches the defect it was written for — mutation-checked, it fails all six format×resource combinations when the initialisers are reverted — but it also fails for an object legitimately named zz-nullable-test, or the day any string field on these exports can hold null. Assert on the parsed shape instead (unmarshal to map[string]any and require roles/groups to be a non-nil []any), or scope the substring to ": null".

Review coverage

• Design and architecture: both fixes changed shape rather than adding a special case, which is the right answer to the round-2 diagnosis. readProtectBackupMeta's three-state return (absent / parsed / broken) is the structural fix — round 2's defect was that "unknown" and "fresh" were the same value, and the new signature makes them different types of answer rather than the same answer with a comment. groupUpdateWouldChange widens a predicate's question to match what its call sites ask, and putting the connection field in the diff (the field an update cannot carry) rather than in the no-op branch is the subtle half, done right. Finding (2) is where the new shape still has one branch that answers "no tenant" and "no manifest" identically.
• Correctness: findings (2), (4). Traced the whole new provenance path at head — tenantURLs()'s dedupe/sort and its legacy-TenantURL fold, protectBackupHoldsDocuments's singleton naming (checked against protectPruneStale:1252-1255, same res.Name+ext convention, so the two agree), writeProtectBackupMeta's write-then-remove ordering, and the guard/claim/prune call order in runProtectBackup:1403-1438 (the guard runs before anything is written — confirmed by reading, and by the unreadable-manifest test asserting the document survives). Re-derived groupUpdateWouldChange's reachability for each of its four branches: name and accessGroup are dead in this branch (lookup is by name; both flags are true to get here), roles and connection are the live ones.
• Security: nothing new. The os.Chmod gate from round 1 is untouched and still covers both producible modes. _meta is written 0o644 and carries a tenant URL, not a credential, unchanged from round 2. writeProtectBackupMeta's os.Remove loop is bounded to protectBackupMetaFile+ext inside dir — no path the caller controls reaches it.
• Cross-repo / wire contracts: re-resolved the two claims the new comments rest on against jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go rather than the PR comment — PlanInput.Telemetry is a bare *string (plan.go:153) with no Null sibling next to TelemetryV2Null (:155), and buildPlanVariables omits a nil key (:409-410), so the new legacy-telemetry note is accurate. jamfprotect.GroupInput (group.go:91-96) carries exactly Name, ConnectionID, AccessGroup, RoleIDs, which is what groupUpdateWouldChange compares — the field set is complete, not a sample.
• Reliability: finding (1). Also re-checked that the claim write cannot itself brick a fresh run (a failure there returns before any document is written) and that protectBackupHoldsDocuments swallowing os.ReadDir errors is safe — it only ever downgrades to "no documents", which is the conservative direction for the positive check but is the permissive direction for the guard, i.e. the same gap as finding (2).
• Performance: no change. protectBackupHoldsDocuments walks the resource table with one Stat/ReadDir per resource, and only on the error path.
• Test coverage: +303 lines, and this is the round where the tests carry their weight. Eight mutation checks, eight caught (details in Scope). Two of the new tests are better than the fix they pin: TestBackupClaimsTheDirectoryBeforeExporting reproduces "interrupted run" with a file placed where a resource directory belongs — a real, deterministic mid-export failure rather than a mock — and TestBackupTenantGuardNormalisesTheURL asserts the normalised match still prunes, closing the "fixed it by turning the feature off" hole. TestPlansRestoreAsksForAbsentMembershipToBeCleared fixes exactly the gap round 2 identified, at the call site rather than the function. Residual: finding (5)'s brittle substring, and no test covers the absent/empty-manifest branch of finding (2).
• Code quality: findings (4), (5). Read every new comment against the code — this was the round-2 lesson and it landed. Tenants's field comment, normaliseTenantURL's four-sources rationale, readProtectBackupMeta's three-states paragraph, protectBackupProvenanceUsable's "same data loss, one run later", the Long text's new paragraph and groupUpdateWouldChange's "sorted because the server returns its own order" all check out against behaviour I executed. The one comment that overstates is the pre-existing protectBackupMeta doc line in finding (4). Nit not worth a finding: sort.Strings in protect_groups.go:161-162 alongside a fresh slices import — slices.Sort would drop the second import.
• Simplification: no new abstraction beyond the two small helpers, and both earn their names. tenantURLs() as a method on the manifest is the right place for the legacy-field fold.
• [na] Frontend concerns: no UI, site, or template files in the incremental diff.
• Documentation currency: CLAUDE.md:255 and :271 and skills/skills/jamf-backup/SKILL.md:47 read against the code, sentence by sentence, since round 2 found a comment asserting the opposite of its code. Every claim holds: "cumulative and written first", "the comparison folds case and a trailing slash", "removing any copy an earlier --format left in the other extension so one manifest is authoritative", "present but unparseable … refuses the run outright, --no-prune included", and the group paragraph's field list (name, accessGroup, roleIds — sorted … plus the connection it cannot). SKILL.md's line no longer recommends --no-prune as a bare workaround and now states what it records — which is what round 2 asked for. Both round-2 wording notes are closed. The undocumented behaviour is finding (1): nothing in either document says -n does not apply to protect backup.
• Scope and decomposition: unchanged and settled. The author took the single-PR branch with a stated rationale and a note for next time; the recommendation never held the merge.
• Project rules compliance: no .claude/rules/. Re-checked against CLAUDE.md — no new credential flags, nothing under */generated/, protect_* prefixes intact. The PR's own new Conventions rule (don't declare a local flag that shadows a root persistent one) is still satisfied by the code it ships; finding (1) is the adjacent failure the rule does not cover — inheriting a root flag and then ignoring it — which is worth a second sentence in that rule.

Scope of review

Incremental diff d7d3d89..236f75d (8 files, +587/−41) in the worktree at /tmp/pr-review-Jamf-Concepts-jamf-cli-330, read in full. Baseline is my round-2 review at 433b72e (#330 (review)). d7d3d89 between them is a pure merge of main (parents 433b72e + d4fb969) bringing in merged PRs #332 and #335; it touches nothing on this PR's surface but one CLAUDE.md doc line, so the new work is exactly the range above and it bisects cleanly.

Beyond the incremental diff: internal/commands/protect_backup.go:1095-1230 and :1380-1560 at head, protect_groups.go and protect_rbac_refs.go in full at head, protect_helpers.go:75-135 (the dryRun branches finding (1) compares against), internal/commands/root.go:716, :800 (dryRunClient, the persistent flag), protect_conversions_test.go:20-75 (the mock's embedded-interface base, which is why the three new plan methods compile), and jamfprotect-go-sdk@v0.8.0/jamfprotect/plan.go.

Executed: go build ./..., go vet ./internal/..., go test ./internal/... — all pass. golangci-lint run ./internal/commands/... reports 0 issues; the six pre-existing staticcheck findings I reported in round 2 are gone from main, as the author states. Flag behaviour checked on a binary built at 236f75d.

Eight mutation checks, each applied to a full copy of the worktree in a scratch directory (the worktree itself was never modified — git status --porcelain empty before and after, verified):

Mutation	Caught by
tenants := prior.tenantURLs() → var tenants []string (last-writer-wins)	TestBackupProvenanceSurvivesANoPruneRunByAnotherTenant
claim write removed (_meta written last again)	TestBackupClaimsTheDirectoryBeforeExporting — a run that died mid-export must still leave provenance
protectBackupProvenanceUsable condition stubbed false	TestBackupRefusesAnUnreadableManifestBesideDocuments/refused,_--no-prune_included
normaliseTenantURL → identity	TestBackupTenantGuardNormalisesTheURL
stale-manifest removal loop deleted	TestBackupKeepsOneManifestAcrossAFormatChange
groupUpdateWouldChange diff stubbed to ""	both new TestGroupsApplySharesTheAccessGroupGuard sub-tests
planExportToInput(ctx, e, r, true) → false	TestPlansRestoreAsksForAbsentMembershipToBeCleared
Roles/Groups initialisers reverted in all three *ToExport	TestRBACExportsCarryEmptyListsInBothFormats (all 3 resources, JSON)

Six behavioural probes in the same scratch copy, throwaway tests, results quoted in the findings above: missing _meta beside documents (P1), zero-length _meta (P2), first-ever --no-prune run into an empty directory (P3), the owning tenant after a foreign --no-prune run (P4), manifest counts after an interrupted run (P5), and protect backup under the root --dry-run (P6, finding (1)).

Prior feedback since the baseline: one issue comment from @neilmartin83 (2026-08-20T15:32Z) walking through both blockers and all four nice-to-haves, including its own eight mutation checks. Every claim in it was treated as a claim and checked against the code — this round they all hold, and the eight mutations it names are the eight I reproduced independently. No review comments, no other reviewers, nothing unaddressed from anyone else.

Not verified: no live tenant, so the two server-behaviour claims the group fix rests on remain as reported — that updateGroup refuses any update to a connection-less local access group, and that exceptionSets: [] clears rather than being rejected. The SDK-side half of each was checked against v0.8.0. Nothing in this round's diff depends on a wire fact I could not resolve in the SDK source.

Never-run specialist lanes. No agents were dispatched this round (session constraint: the orchestrator cannot spawn subagents, and ran every lane inline). Eligible but never dispatched on this PR across all three rounds: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate, test-quality-reviewer. fidelity-reviewer remains inapplicable — no Jira key in the title, branch or description and no linked issue, so there is no spec to check against.

What's done well

• ✅ The fix for the provenance blocker is not the fix that was suggested — it is a better one, in a way that is easy to check. Round 2 asked for a cumulative record and a refusal on a corrupt manifest. This ships both, plus two the review did not ask for: writing the manifest first so an interrupted run still leaves a claim, and collapsing the two-manifest state a --format switch creates so readProtectBackupMeta cannot answer with the stale extension. Both of those are ways the suggested fix would still have been blind, found by the author rather than the reviewer.
• ✅ Extending protectBackupProvenanceUsable to run under --no-prune, and saying why in the comment ("the run is about to rewrite the manifest … same data loss, one run later"). That is a deliberately stronger stance than the finding asked for, with the reasoning written down at the site — and the test proves the stance by looping the sub-test over both noPrune values.
• ✅ groupUpdateWouldChange putting the connection in the diff. The finding only asked for roles. Recognising that a document dropping a connection is also asking for something update cannot express — and that this belongs in the "could not apply" bucket rather than the no-op one — is the generalisation that stops a fourth instance of this bug class.
• ✅ Eight mutation checks reported with the mutation and the test that caught it, reproduced here independently and matching exactly. Round 2's one surviving mutation is now caught. A fix round that arrives with its own falsification evidence is the reason this review took hours rather than days.

_{🤖 Generated by the pr-review:review skill · re-reviewed head 236f75d (baseline 433b72e)}

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

[neilmartin83]

Round 3 addressed in c13f5bc. All five findings fixed — the blocker and all four nice-to-haves. Six mutations applied to a scratch copy, six caught by a named test.

🟧 (1) protect backup ignored the root -n

Confirmed as reported, and the diagnosis is right: the flag is inherited cleanly, so it appears in the command's own help whether or not the code reads it.

I took the first of your two contracts — -n covers the prune, documents are still written — because pro backup -n writes its files too and pruning is this command's only destructive act, so that is the half a preview exists for. It is stated in the Long, at runProtectBackup, in CLAUDE.md and in SKILL.md, so it is a contract rather than a silence.

I did not take the suggested diff. if dryRun { noPrune = true } skips the protectPruneStale call, so nothing is decided and the pruned list is empty — the run deletes nothing and reports nothing, which reads as "there was nothing to prune". Instead the gate is on the os.Remove inside protectPruneStale (remove bool): the same decision by the same route, stopping at the deletion, so the list the operator asked for is what gets printed as [dry-run].

That distinction is what the second sub-test exists for — mutation M2 below applies your suggested shape and it fails, on the reporting assertion only.

🟩 (2) absent / contentless _meta beside documents

Fixed, but in protectBackupPruneAllowed rather than protectBackupProvenanceUsable, because I think it is a different question from the unreadable case. There is no claim here to overwrite and lose — the rationale for protectBackupProvenanceUsable running under --no-prune does not apply — so this is purely "may this run prune", and that makes --no-prune a genuine way forward instead of a way to disarm the guard: it records the tenant, and the run after it prunes normally. The refusal says so, and the test asserts the whole three-run sequence including that the prune then actually happens (a guard that never lets go is a disabled feature).

Covers all three states you named plus the one you did not: !found, zero-length, and a manifest that parses fine but names no tenant.

🟩 (3) no way back from the tenant refusal

Third clause added, naming protectBackupMetaPath(dir) — the file that is actually on disk, not the _meta stem, since the extension follows --format and a reader cannot guess which of the three it is.

🟩 (4) claim write blanked Resources/Counts, and the comment promised a reader

Both halves. The claim carries prior.Resources/prior.Counts forward (non-nil, since neither field has omitempty and a nil map or slice marshals as JSON null — the same defect as (5) one field over), so an interrupted run degrades to stale rather than blank. CreatedAt stays this run's, because the claim is a claim; only Tenants is this run's own news, and the comment says so. And protectBackupMeta's doc comment now says what is true: the tenant guard is its only programmatic reader, restore never opens it, and the inventory fields are for the human reading the tree.

🟩 (5) the null substring assertion

Now asserts the parsed shape — unmarshal, then require roles/groups present and []any. I probed your exact example: a group named zz-nullable-test marshals to a document containing null in both formats, so the old assertion would have failed on it. The new one passes.

Checking presence as well as type is deliberate: the type carries no omitempty precisely so a reader can tell an empty list from an absent one, and asserting only the type would let a future omitempty through.

Mutation checks

Each applied to a full copy of the worktree in a scratch directory; the worktree itself was never modified (git status --porcelain empty before and after, and the scratch copy's three Go files verified byte-identical to the shipped ones afterwards, so the test and lint runs cover exactly what is committed).

#	Mutation	Test that caught it
M1	protectPruneStale(…, !dryRun) → true (the shipped defect)	TestBackupUnderTheRootDryRunFlagPrunesNothing
M2	your suggested if dryRun { noPrune = true } shape	…/what_it_would_have_pruned_is_reported only — deletes nothing, reports nothing
M3	drop the no-tenant-on-record refusal	TestBackupRefusesToPruneWhenNoTenantIsOnRecord, all three states
M4	drop the manifest-path clause	TestBackupTenantRefusalNamesTheManifestToEdit
M5	claim write back to []string{} / map[string]int{}	TestBackupClaimCarriesThePreviousInventoryForward
M6	RBAC list initialisers back to nil	TestRBACExportsCarryEmptyListsInBothFormats (group, user, api-client in JSON)

One thing your round-3 findings did not name

Writing the dry-run test surfaced a leak in the existing TestProtectRestoreUsesTheRootDryRunFlag: dryRun is bound to the persistent flag, so its ParseFlags([]string{"-n"}) writes the package var and nothing put it back. It leaked a true into every test that ran after it. Harmless while nothing downstream read dryRun — and it stopped being harmless the moment backup did: my first run of the new suite failed with a prune that never happened, three sub-tests deep, for that reason. Both tests now go through setDryRun, which restores.

Coverage note

On your rating note about the never-run specialist lanes — agreed, and worth saying plainly that the same constraint applied here: no subagents, so security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate and test-quality-reviewer have still never run on this PR. Finding (1) being a sibling of round-1 (5) that two rounds missed is the concrete cost of that, and it is a fair reason to treat the 4/5 as one reader's pass rather than seven.

Verified: go build ./..., go vet ./internal/..., go test ./... all pass; golangci-lint run ./internal/commands/... reports 0 issues; make verify-generated clean; -n and the new help text checked on a binary built at c13f5bc.

[ktn-jamf]

Tip

✅ Merge-ready — round 4 on c13f5bc. The round-3 blocker is closed, and closed better than the fix that was suggested: the gate sits on the os.Remove inside protectPruneStale rather than on the call, so a preview still decides what would go and prints it. All four nice-to-haves are fixed too. Six mutations applied, six caught by a named test — including one that applies my own suggested diff and fails it.
No blocking items. Three 🟩 remain, all follow-up material.

Rating: 5/5

• Every prior finding is fixed and every fix fails a named test when reverted. The flag sweep this round was asked to do comes back clean on both backup and restore. What is left is one product-wide gap the PR inherits rather than creates, and two sentences of doc precision.
• Coverage: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate and test-quality-reviewer have never run on this PR across all four rounds — the session cannot spawn subagents, so I ran each lane inline myself. Read the rating as one reader's pass over those seven dimensions, not seven independent ones. Round 3's blocker was the concrete cost of that; a human skim of the whole-PR diff is still worth an hour.

Prior findings status (5 items)

#	Location	State	Notes
(1) c=100	protect_backup.go:1392 → now :1302, :1545, :1611, :1642	✅ Fixed	protectPruneStale gained a remove bool, passed !dryRun from both call sites, and the reporting block branches to [dry-run] Would prune …. Executed the round-3 scenario: zz-gone.yaml survives under -n, zz-keep.yaml is still written, and the removal is reported. Mutation-checked twice. M1 (!dryRun → true, the shipped defect) fails …/the_file_a_real_run_would_prune_survives with -n deleted unified-logging-filters/zz-gone.yaml. M2 applies my own suggested shape (if !noPrune && !marshalFailed && !dryRun) and fails …/what_it_would_have_pruned_is_reported — deletes nothing, reports nothing. The author is right that my diff was the weaker fix and the test exists to say so. Third sub-test pins the flag surface itself: no local dry-run, -n parses, ShorthandLookup("n").Name == "dry-run", and Long mentions --dry-run.
(2) c=100	protect_backup.go:1182-1187 → now :1198-1219	✅ Fixed	protectBackupPruneAllowed now drops the !found early return and refuses when tenantURLs() is empty and protectBackupHoldsDocuments(dir). All three states covered — absent, zero-length, and parses-cleanly-to-no-tenant — plus the empty-directory case still allowed. Executed: my round-3 P1 and P2 probes both now error and leave the document on disk. Mutation-checked (M3, guard short-circuited): all three sub-tests fail. The test also asserts the way forward works — --no-prune once, then the next run prunes for real — which is the assertion that stops "fixed it by disabling it". Fixing this in protectBackupPruneAllowed rather than protectBackupProvenanceUsable is the right call and the comment gives the reason: there is no claim here to overwrite, so --no-prune is a remedy rather than a bypass.
(3) c=100	protect_backup.go:1200-1203 → now :1223-1251	✅ Fixed	Third clause added, naming protectBackupMetaPath(dir) — the file on disk, resolved across .yaml/.yml/.json, not the _meta stem. Executed: the refusal now reads … or if that tenant's documents are gone, delete its entry from <dir>/_meta.yaml. Mutation-checked (M4, back to the bare constant): TestBackupTenantRefusalNamesTheManifestToEdit fails, and it asserts the full path rather than the stem, so a fix that named _meta would not pass.
(4) c=100	protect_backup.go:41-43, :1434 → now :42-47, :1495-1516	✅ Fixed	Both halves. The claim now carries prior.Resources/prior.Counts forward (nil-guarded, and the comment gives the JSON-null reason, which is finding (5) one field over). Mutation-checked (M5, back to empties): TestBackupClaimCarriesThePreviousInventoryForward fails with the claim wrote resources=[] counts=map[]. The type comment no longer promises a restore-side reader; I re-grepped every readProtectBackupMeta call site (:1167, :1186, :1475) and all three are inside backup, so "its only programmatic reader is backup itself … restore never opens it" is true as written.
(5) c=100	protect_backup_test.go:1470-1478 → now :1736-1779	✅ Fixed	Now unmarshals and requires each list field present and []any. Mutation-checked (M6, initialisers back to nil): fails for group/user/api-client on JSON with has "roles" = <nil>, want an empty list. YAML passes because a nil slice marshals as [] there — which is the format disagreement the finding was about, so the asymmetric failure is correct. Checking presence as well as type is a deliberate extra: it catches a future omitempty. I also verified the author's counter-claim about my example, and it holds — zz-nullable-test contains the substring null, so the old assertion would have failed on it in both formats.

Nice-to-have suggestions (3 items)

🟩 (1) (correctness, c=100) — internal/commands/protect_analytic_overrides.go:219-271: protect overrides set is new in this PR, inherits -n, and ignores it — while its two siblings in the same new command group honour it

This is the fourth generation of the flag-surface class, and the reason it is 🟩 rather than a fourth blocker is worth stating explicitly.

Inside protect_analytic_overrides.go, clear (:309) and apply (:409) both call confirmAction, which branches on dryRun and prints [dry-run] Would … (protect_helpers.go:106). set calls cliCtx.ProtectClient.UpdateInternalAnalytic directly with no dryRun check and no confirm* helper, so protect overrides set zz-analytic --severity Low -n performs a real tenant write. Its Long does not mention -n. That fails the rule this commit wrote: "Every command that inherits -n either honours it or says in its Long what it does not cover" (CLAUDE.md:324). The operator who learns from overrides clear -n that the group previews will be wrong about set.

Why not 🟧: the write is reversible (overrides clear, or set back), and it is consistent with the pre-existing Protect convention on main — grep -n dryRun internal/commands/protect_*.go finds it only in protect_helpers.go and protect_backup.go, so analytic-sets add-analytic (protect_analytic_sets.go:205, unchanged by this PR beyond six lines), exception-sets add-exception and ulf-sets add-filter all mutate under -n today. The three findings that held this PR up were an exit-2 crash on a documented CI mechanism and two file deletions; this one is neither.

The pattern is the finding. Four rounds have each produced one instance of "a root persistent flag and a command disagree", found one at a time by reading. A fifth will follow unless something enumerates it. The cheap version is a table-driven test that walks the command tree and asserts, for every command whose RunE reaches a mutating client call, either a dryRun read or a --dry-run mention in Long — the same shape as the existing TestApplyProGroups_AllCommandsGrouped guard. That belongs in a follow-up, not in round 4 of a 7,500-line PR.

Fixed when: either overrides set honours -n (report the intended override and return), or its Long says -n does not cover it — and preferably a test enumerates the rule instead of a fifth review round finding a fifth instance.

🟩 (2) (documentation, c=100) — CLAUDE.md:255: "refuses the prune — not the run" describes behaviour the command does not have

The new sentence reads: "A manifest that is absent, zero-length, or parses to no tenant beside documents refuses the prune — not the run — for the same reason one run later." Executed (probe P3): runProtectBackup returns an error and the command exits non-zero. Nothing is pruned because nothing runs.

The intended contrast is with the unreadable case, which is refused even under --no-prune — and the rest of the sentence makes that clear. But the literal reading tells an operator (and an agent reading CLAUDE.md as ground truth) that a scheduled protect backup against such a directory succeeds with the prune skipped, when it fails. The in-code comment at :1206-1211 gets this right ("a pruning question, not a provenance-rewrite one") and SKILL.md:52 gets it right too ("the prune is refused until one --no-prune run puts the tenant on record"). Only the CLAUDE.md phrasing overstates. Suggest: "refuses the run, but as a pruning question rather than a provenance one — so --no-prune gets through, unlike the unreadable case."

🟩 (3) (documentation, c=100) — internal/commands/protect_backup.go:1412-1417: the -n contract enumerates the document writes but not the _meta claim it also leaves behind

The Long says "documents are still written … but nothing is deleted". Executed (probe P1) — protect backup -n into a fresh directory leaves [_meta.yaml unified-logging-filters], with tenants=[https://staging.x] and counts=map[unified-logging-filters:2]. So a preview run permanently claims the directory for the running tenant.

This is not a defect, and I chased the ways it could be one. The guard runs before the claim, so -n cannot claim a directory it does not own: probe P2 (-n at another tenant's directory) is refused with the tenant list unchanged, and probe P3 (-n at a no-owner directory) is refused too. -n --no-prune records the claim (P4) — which is exactly what round 3's own suggested fix (if dryRun { noPrune = true }) would have done, so the behaviour is not a deviation from the agreed contract. And a claim on an otherwise-empty directory only ever moves the next run toward a refusal, which is the safe direction.

It is still a lasting on-disk side effect under a flag whose root help says "preview changes without executing", and the Long currently accounts for the documents but not the manifest. One clause — "and the run records this tenant in _meta the way any run does" — closes it.

Flag sweep — `protect backup` and `protect restore` against the root persistent set

Round 3 asked for this in one pass instead of one finding at a time. Done, against internal/commands/root.go:793-813 (19 persistent flags) and both commands' local sets, verified on a binary built at c13f5bc.

Local flags, shadow check.

Command	Local flags	Collides with a root persistent name?
protect backup	--output, --format, --resources, --exclude, --no-prune	--output only — deliberate, and the Long says the -o shorthand is gone
protect restore	--input, --resources, --exclude, --include-defaults, --yes	none

No new shadow. protect backup --help confirms it: Global Flags lists -n, --dry-run and omits -o, --output entirely, which is the documented consequence.

Advertised-and-read, on backup. -n/--dry-run (now — the fix), --allow-partial-failure (:1450), -q/--quiet (:1641). On restore: -n/--dry-run (:1733, :1837), --allow-partial-failure, -q/--quiet.

JAMF_CLI_ARGS (executed). injectEnvArgs prepends before the subcommand, so the shorthands resolve on the target command's merged flagset:

JAMF_CLI_ARGS='-n'      protect backup  → parses; fails later on missing creds (exit 2, auth)
JAMF_CLI_ARGS='-n'      protect restore → parses; fails later on missing creds (exit 2, auth)
JAMF_CLI_ARGS='-o json' protect backup  → "unknown shorthand flag: 'o' in -o" (exit 2)

The third is the documented, deliberate consequence of the --output collision, unchanged by this round and already stated in both the Long and the CLAUDE.md convention. The -n half now works on both commands.

Advertised-but-unread on backup, and why none is a finding. --field, --select, --compact, -w/--wide, --out-file, -v/--verbose all appear in protect backup --help and do nothing there. Every one is a root persistent flag that shapes structured record output or Pro HTTP tracing, and protect backup prints neither — so each is inert on pro backup, config add-profile and every other non-record command on main too. That is a repo-wide property of a flat persistent-flag set, not something this PR introduced or can reasonably fix inside its scope. The class this round was asked to sweep — a shadow, or a flag whose absence of effect is surprising and destructive — is clean on both commands.

One residual instance outside these two commands is finding (1): protect overrides set, new in this PR, inherits -n and neither honours nor documents it.

Review coverage

• Design and architecture: the two structural choices this round are both right, and both are choices rather than the obvious edit. Gating remove inside protectPruneStale rather than gating the call keeps the decision path identical between a preview and a real run, so the reported list is produced by the same code that would do the deleting — the alternative produces an empty list and a silence that reads as "nothing to prune". And putting the no-owner refusal in protectBackupPruneAllowed rather than protectBackupProvenanceUsable correctly separates "may this run prune" from "may this run rewrite provenance", which is what makes --no-prune a remedy in one case and a bypass in the other. Both are argued at the site.
• Correctness: findings (1), (2). Traced the full new path at head. protectBackupPruneAllowed:1186-1252 — the _, _, err discard of found is correct now that empty tenantURLs() subsumes the absent case, and protectBackupProvenanceUsable still runs first for the unreadable one. protectBackupMetaPath:1245-1252 resolves across all three protectRestoreExts and falls back to the extensionless stem; the fallback is unreachable from the only caller (a recorded tenant implies a readable manifest), and harmless if it ever were. protectPruneStale:1302-1355 — the os.Stat continue still precedes the removal, so a remove=false run reports only files that exist, which is what makes the preview list truthful. Call order at :1475-1516 unchanged: read prior → guard → claim → export.
• Security: nothing new. No credential path touched; _meta still 0o644 carrying a tenant URL. protectBackupMetaPath composes dir with a package constant and a fixed extension list — no caller-controlled segment. Worth naming, since it is a -n question: a preview run does write credential-bearing documents 0600 and prints the sensitive-file warning. That follows from the documented "documents are still written" contract and is the same on a real run, so it is not a regression — but an operator reaching for -n to avoid touching disk will be surprised, and the Long should arguably say so alongside finding (3)'s clause.
• Cross-repo / wire contracts: none changed this round. The -n semantics are local; protectPruneStale's new parameter is package-private with two call sites, both updated (grep confirms no third).
• Reliability: the interrupted-run story is now complete in both directions — the claim lands before any document (round 3) and no longer blanks the inventory when it does (finding (4)). A dry-run reports a removal that a real run might then fail on (permission denied at the os.Remove); that is inherent to any preview and not worth a finding.
• Performance: negligible. protectBackupMetaPath adds at most three os.Stat calls, and only on the refusal path.
• Test coverage: +318 lines and they carry their weight. Six mutations, six caught (table in Scope). The standout is M2 — the second sub-test exists specifically to fail the weaker fix the review suggested, which is a test written against a class of wrong answer rather than against the one defect. The setDryRun helper is a real catch the review did not ask for: dryRun is bound to the persistent flag, so TestProtectRestoreUsesTheRootDryRunFlag's ParseFlags("-n") was leaking true into every later test in the package; harmless until backup started reading it, and now restored by both tests. TestBackupRefusesToPruneWhenNoTenantIsOnRecord covers all three manifest states plus the empty-directory control plus the full three-run recovery sequence. Residual: nothing pins finding (1)'s command, and no test asserts the -n _meta side effect either way.
• Code quality: read every new comment against executed behaviour, which is the standing lesson from round 2. protectBackupMeta's rewritten doc comment, protectPruneStale's remove paragraph, the protectBackupPruneAllowed zero-tenant block, protectBackupMetaPath's rationale, the claim-carry-forward comment and the runProtectBackup dryRun note all check out. The Long paragraph's pro backup precedent claim was the one worth verifying independently, and it holds: dryRunClient.Do (root.go:704-712) passes GET/HEAD through, and pro_backup.go reads only, so pro backup -n does write its files.
• Simplification: one new eight-line helper and one new bool parameter. Both earn their place; nothing to collapse.
• [na] Frontend concerns: no UI, site, or template files in the incremental diff.
• Documentation currency: findings (2), (3). Read all four changed doc surfaces sentence by sentence against executed behaviour. CLAUDE.md:324's new convention clause is accurate and generalises correctly ("a root persistent flag appears in a command's own --help whether the code reads it or not"). SKILL.md:52's two new sentences are both accurate. The Long addition is accurate but incomplete (finding (3)). CLAUDE.md:255's "refuses the prune — not the run" is the one claim that does not survive execution (finding (2)) — the third round in a row where a doc sentence needed a probe rather than a read, which is itself the argument for keeping these paragraphs shorter.
• Scope and decomposition: unchanged and settled three rounds ago.
• Project rules compliance: no .claude/rules/. Re-checked against CLAUDE.md — no credential flags added, nothing under */generated/, protect_* prefix intact, make verify-generated not needed (no spec or template touched). The PR's own new -n rule is satisfied by protect backup and protect restore and violated by protect overrides set — finding (1).

Scope of review

Incremental diff 236f75d..c13f5bc (4 files, +420/−33) in the worktree at /tmp/pr-review-Jamf-Concepts-jamf-cli-330, read in full. Baseline is my round-3 review at 236f75d. The new work is exactly one commit, so it bisects cleanly.

Beyond the incremental diff: internal/commands/protect_backup.go:1095-1260 and :1400-1660 at head; internal/commands/root.go:700-740 and :793-813 (dryRunClient, the full persistent-flag set); internal/commands/protect_helpers.go:80-135 (the three confirm* dryRun branches); internal/commands/protect_analytic_overrides.go in full at head (finding (1)); internal/commands/protect_analytic_sets.go:205-255 (the pre-existing add-analytic comparison); internal/commands/pro_backup.go (the -n precedent claim); cmd/jamf-cli/main.go's injectEnvArgs.

Executed: go build ./..., go vet ./internal/..., go test ./internal/... -count=1 — all pass. golangci-lint run ./internal/commands/... reports 0 issues. Flag behaviour and help output checked on a binary built at c13f5bc.

Six mutation checks, each applied to a full copy of the worktree in a scratch directory. The worktree itself was never modified (git status --porcelain empty before and after, verified), and the scratch copy's two mutated files were diffed byte-identical to the shipped ones afterwards.

Mutation	Caught by
M1 protectPruneStale(…, !dryRun) → true (the shipped defect)	TestBackupUnderTheRootDryRunFlagPrunesNothing/the_file_a_real_run_would_prune_survives
M2 my round-3 suggested shape — gate the call, not the removal	…/what_it_would_have_pruned_is_reported, on the reporting assertion only
M3 no-tenant-on-record refusal short-circuited	TestBackupRefusesToPruneWhenNoTenantIsOnRecord, all three states
M4 protectBackupMetaPath(dir) → bare protectBackupMetaFile	TestBackupTenantRefusalNamesTheManifestToEdit
M5 claim write back to []string{} / map[string]int{}	TestBackupClaimCarriesThePreviousInventoryForward
M6 RBAC list initialisers back to nil	TestRBACExportsCarryEmptyListsInBothFormats (group, user, api-client, JSON)

These are the same six the author reports, reproduced independently and matching exactly, including M2's single-sub-test failure.

Five behavioural probes in the same scratch copy, throwaway tests, results quoted in the findings: -n into a fresh directory (P1, finding (3)), -n at another tenant's directory (P2, refused, tenant list unchanged), -n at a no-owner directory (P3, refused — also the evidence for finding (2)), -n --no-prune (P4, claim recorded), and the zz-nullable-test false positive (P5, the author's counter-claim confirmed).

Prior feedback since the baseline: one issue comment from @neilmartin83 (2026-08-20T16:04Z) walking through all five findings with its own six mutation checks. Every claim in it was treated as a claim and checked; all hold, including the two that correct my round-3 review — that my suggested -n diff was the weaker shape, and that zz-nullable-test would have tripped the old substring assertion in both formats. No review comments, no other reviewers, nothing unaddressed from anyone else.

Not verified: no live tenant, so the two server-behaviour claims underneath the round-3 group fix remain as reported (that updateGroup refuses any update to a connection-less local access group, and that exceptionSets: [] clears rather than being rejected). Nothing in this round's diff depends on a wire fact I could not resolve in source.

Never-run specialist lanes. No agents dispatched, this round or any prior one — the session cannot spawn subagents, so every lane ran inline. Eligible but never dispatched across all four rounds: security-reviewer, usability-reviewer, performance-reviewer, scope-reviewer, silent-failure-hunter, devil-advocate, test-quality-reviewer. fidelity-reviewer remains inapplicable — no Jira key in the title, branch or description, and no linked issue.

What's done well

• ✅ The fix rejects the reviewer's diff, gives the reason, and ships a test that fails the reviewer's diff. if dryRun { noPrune = true } skips the protectPruneStale call, so nothing is decided and the report is empty — a preview that prints nothing is indistinguishable from "there was nothing to prune". Gating os.Remove instead keeps the decision and stops at the deletion. M2 applies my shape and fails exactly one sub-test, which is the sub-test that exists for it. That is a fix round arguing with the review on the evidence and being right.
• ✅ The no-owner guard was fixed in the other function, deliberately. Putting it in protectBackupPruneAllowed rather than protectBackupProvenanceUsable draws the line at "may this run prune" versus "may this run rewrite provenance" — which is what makes --no-prune a genuine remedy here and a bypass there. The test then asserts the remedy works, including that the run after it prunes for real, closing the "fixed it by disabling the feature" hole for the second round running.
• ✅ The test-hygiene catch the review missed. dryRun is bound to the persistent flag, so the existing restore test's ParseFlags("-n") was leaking true into every later test in the package — inert until backup started reading it, and then a three-sub-test-deep failure. setDryRun fixes both sites. Round 3 introduced the condition for that bug and did not see it.
• ✅ Four rounds, and each one arrives with its own falsification evidence — the mutation, the test that catches it, and (this round) two corrections to the reviewer. Twenty-two mutation checks across four rounds, every one reproduced independently and every one matching. That is why this closed in four rounds rather than ten.
• ✅ The CLAUDE.md convention got the general rule, not the instance. "A root persistent flag appears in a command's own --help whether the code reads it or not … a documented flag that does nothing is worse than an absent one" is the statement that would have prevented all three prior instances of this class. Finding (1) is what is left to enumerate, not to understand.

_{🤖 Generated by the pr-review:review skill · re-reviewed head c13f5bc (baseline 236f75d)}

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