fix(pro): diff mis-keyed every compliance benchmark

[ktn-jamf]

Problem

backup writes a compliance benchmark's name in the title field, and it names the file SlugifyName(title). Live mode keys the benchmark on bm.Title.

The root scan of the backup directory passed no name field, so backupObjectName found neither name, displayName nor general.name. It fell back to the filename stem. A disk key of cis-level-1 then faced a live key of CIS Level 1.

Result: jamf-cli pro diff --source ./backup --target production reported every compliance benchmark as both removed and added, on every run.

This is the same class of defect as the groupName mis-keying in #332. That fix made the curated resources read their declared BackupEndpoint.NameField. Compliance benchmarks are an SDK-backed Platform resource that backup writes straight to the backup root, so they are not in the BackupResources table and the root scan had no name field to consult.

Fix

platformNameFields declares the name field for the resources that backup writes to the backup root:

var platformNameFields = map[string]string{
    "compliance-benchmarks": "title",
}

The root scan passes platformNameFields[name] where it passed "". A directory with no entry gets "", which is the current behaviour.

Blueprints need no entry. blueprintToExport emits a name field, which backupObjectName already finds. Blueprints were never affected.

Scope

A diff between two directories is not affected. Both sides read through the same loader, so both used the stem and the keys agreed.

Only a directory-against-instance diff was wrong.

Tests

Two tests, both confirmed to fail before the fix:

• TestLoadSnapshotFromDirectory_ComplianceBenchmarkKeyedOnTitle — asserts the benchmark is keyed CIS Level 1, and asserts the stem cis-level-1 is not a key.
• TestLoadSnapshotFromDirectory_EveryPlatformNameFieldIsHonoured — the platformNameFields analogue of TestLoadSnapshotFromDirectory_EveryCuratedNameFieldIsHonoured. A Platform resource added later with a new name field fails here, not in a diff against a live tenant. The test fails if the table is emptied, so it cannot pass by asserting nothing.

Verified by mutation:

Mutation	Result
Revert the root-scan wiring to ""	Both tests fail
Delete the compliance-benchmarks entry	Both tests fail

go test ./... passes. go vet, gofmt, gofumpt clean. golangci-lint run ./internal/commands/ reports 0 issues.

Found by

Round-2 review of #332 (review), where it was held as non-blocking because it pre-dates that PR and sits outside the NameField mechanism.

🤖 Generated with Claude Code

[ktn-jamf]

Thanks — finding (1) was correct, and it was the more important half. Fixed in 2ace905.

(1) field-set disagreement — fixed. benchmarkToExport is now the one projection, called from backupBenchmarks and from the diff live loader, as you suggested. rules stays on both sides rather than being dropped from both: it is part of what a benchmark is, so a backup without it could not restore one.

(2) guard test — fixed structurally rather than with a slices.Contains check. platformNameFields is gone. nonStandardBackupFilters now carries a NameField per entry and the root scan reads it through nonStandardBackupNameField, so there is one table instead of two lists that must agree. TestLoadSnapshotFromDirectory_EveryNonStandardNameFieldIsHonoured iterates that table on both axes.

(3) postmortem — updated. New section on the non-standard name field and on why keying and field set are two halves of one requirement, plus the new guards and a new applies_when entry.

One thing I could not pin, stated plainly. I mutation-tested the three fixes. Two are caught:

Mutation	Result
Drop NameField: "title" from the table	2 tests fail
Rename a table entry to a directory no backup writes	2 tests fail
Revert the live loader to its old 5-field projection	suite stays green

The parity test calls benchmarkToExport for both sides, so it proves the projection is single and correct but cannot reach the live loader's call site — that path needs a Platform SDK client. Re-inlining a literal projection there would not fail a test. I recorded this limit in the postmortem rather than leaving it implied. If you want it genuinely pinned, the seam would be extracting the list-and-project loop behind an interface, which is a larger change than this PR should carry.

go test ./..., go vet, gofmt, gofumpt clean; golangci-lint run ./internal/commands/ 0 issues.

🤖 Addressed by Claude Code

[neilmartin83]

Tip

✅ Merge-ready — Shares one benchmark projection between backup and diff, and folds the name field into nonStandardBackupFilters so there is one table rather than two lists that must agree.
The blocking finding (1) is genuinely fixed: both sides now run the same List → Get → benchmarkToExport path, and a parity test asserts an unchanged benchmark produces no field diffs. Two nice-to-haves in the collapsed section, neither holding the merge.

Rating: 5/5

• Clean — the 2 nice-to-have suggestions below are optional. Both are about what the guard can prove, not about whether the fix is right.
• Coverage: test-quality-reviewer, devil-advocate, silent-failure-hunter and usability-reviewer have never run on this PR — the rating reflects the dimensions that were searched, not the whole diff.

Prior findings status

#	Location	State	Notes
(1) c=75	pro_diff.go:466-476	✅ Fixed	benchmarkToExport (pro_compliance_benchmarks.go:301) is now the single projection, called from backupBenchmarks (pro_backup.go:828) and the diff live loader (pro_diff.go:459). Both reach it via the same ListBenchmarks → GetBenchmark path, so rules cannot be populated on one side only. TestBenchmarkToExport_DiskAndLiveAgreeFieldForField pins it through diffObjects
(2) c=100	pro_diff_test.go:1157	⚠️ Partial	The cross-list drift is structurally gone — better than the slices.Contains check suggested — but the guard still derives the name field from the entry it validates, and its comment still claims a coverage it doesn't have. Carried forward as (1) below
(3) c=75	docs/solutions/logic-errors/diff-missed-nested-backup-subdirs-2026-08-19.md	✅ Fixed	New "non-standard resources have their own name field" section, the keying-and-field-set framing, a new applies_when entry, and the new guards — including an honest note of what the parity test cannot reach

Findings

Nice-to-have suggestions (2 items)

🟩 (1) (test-coverage, c=100) — internal/commands/pro_diff_test.go:1171: the guard still can't fail for either reason it now claims

Moving the name field into nonStandardBackupFilters removed the two-lists-must-agree failure mode, which was the right call. But the test still derives its fixture from the entry under test — writeBackupFileForTest(filepath.Join(dir, entry.FilterName, ...)) with field := entry.NameField defaulting to "name" — so:

• An entry naming a directory no backup writes still passes. The test creates that directory itself, and the root scan reads any directory it finds. The comment (:1155) and the postmortem (:154) both claim this case "fails here"; it doesn't. What actually catches a renamed entry is TestLoadSnapshotFromDirectory_ComplianceBenchmarkKeyedOnTitle, which hardcodes compliance-benchmarks — so the author's mutation table is right about the count and wrong about which test earns it.
• A future entry added without the NameField it needs still passes. The fixture writes whatever field the entry declares, so NameField: "" gets a name: file and keys correctly. This is the original (2) unchanged, just relocated.

Closing it needs one assertion the table cannot make about itself — that the declared field is where the writer puts the name:

+	// The declared field is only right if it is where the writer puts the name.
+	// Nothing else in the suite couples the table to the projection.
+	nameField := nonStandardBackupNameField("compliance-benchmarks")
+	if got := benchmarkToExport(&compliancebenchmarks.BenchmarkResponseV2{Title: "x"}); got[nameField] != "x" {
+		t.Errorf("compliance-benchmarks declares NameField %q, but benchmarkToExport keys the name elsewhere: %v", nameField, got)
+	}

Fixed when: either the assertion above exists, or the two comments are corrected to claim only what the test proves. Correcting the comments is the cheaper half and worth doing regardless — the postmortem is the file CLAUDE.md sends the next agent to grep, so an overclaim there is load-bearing.

🟩 (2) (code-quality, c=100) — internal/commands/pro_compliance_benchmarks.go:288: benchmarkToPortable's doc comment now documents benchmarkToExport

The new function landed between benchmarkToPortable's comment block and its func line, with no blank line separating them. Lines 288-300 are now one comment run attached to benchmarkToExport, so go doc / IDE hover opens its documentation with "benchmarkToPortable converts a benchmark API response to the portable export format" — directly contradicting the next paragraph of the same block, which exists to say this is the snapshot shape and not the portable one. benchmarkToPortable (:319) is left with no doc comment at all.

-// benchmarkToPortable converts a benchmark API response to the portable export format.
-// Device group IDs are replaced with name+type for cross-instance portability.
-// Rules are converted from the detailed info format to the request format.
 // benchmarkToExport is the one projection of a benchmark that both `pro backup`
@@
 func benchmarkToExport(bm *compliancebenchmarks.BenchmarkResponseV2) map[string]any {
@@
 }
 
+// benchmarkToPortable converts a benchmark API response to the portable export format.
+// Device group IDs are replaced with name+type for cross-instance portability.
+// Rules are converted from the detailed info format to the request format.
 func benchmarkToPortable(bm *compliancebenchmarks.BenchmarkResponseV2, groupByID map[string]devicegroups.DeviceGroupListReadRepresentationV1) *benchmarkPortableInput {

Fixed when: each function's doc comment sits directly above it. gofmt and golangci-lint both pass on the current arrangement, so nothing in CI will catch this.

Review coverage

• Design and architecture: one table (nonStandardBackupFilter struct) replaces the parallel-lists design (2) objected to; one projection replaces two, matching the blueprintToExport precedent. All four call sites of nonStandardBackupFilters converted to the struct field (isKnownBackupFilter:151, BackupFilterNames:208, nonStandardBackupNameField:189, the test) — none left iterating strings
• Correctness: traced both benchmark paths to confirm the field sets can no longer diverge — backupBenchmarks and the diff live loader both do ListBenchmarks → GetBenchmark(b.ID) → benchmarkToExport(bm), so rules/sources/selectedOsVersions are populated identically. Confirmed normalizeViaJSON (pro_backup.go:640) and normaliseViaJSON (pro_diff.go:325) are functionally identical marshal/unmarshal round-trips, so the disk write and the live read do not diverge on types. Confirmed _meta is stripped on read (pro_diff.go:281), so the field backup adds is not diffed. nil Rules round-trips to nil through both YAML and JSON, so an empty benchmark is also clean. Root-scan lookup returns "" for every curated directory, so the new argument is a no-op outside the three non-standard resources
• [na] Security: no auth, credential, transport or privilege surface in the diff
• [na] Performance: one linear scan of a 3-entry slice per root directory entry
• Test coverage: go test ./internal/commands/ -run 'TestLoadSnapshotFromDirectory|TestBenchmarkToExport|TestBackupFilterNames|TestApplyProGroups' passes on the branch. The new parity test asserts both the field-set agreement and that tenantId/benchmarkId/lastUpdatedAt stay out of the snapshot, which is more than (1) asked for. Residual guard weakness is finding (1); the author's disclosed gap (a re-inlined literal in the live loader is not caught) is accepted — see Scope
• Reliability: unchanged — root read failures still fatal, per-file parse failures still warn
• Code quality: the comments carry the why on both the struct field and the table; finding (2) is a mechanical placement slip, not a style disagreement. gofmt clean, go vet clean, CI ci pass
• [na] Simplification: the change is a net reduction in duplicated projections
• [na] Frontend concerns: no UI
• Documentation currency: postmortem extended with the new mechanism, the keying-vs-field-set framing, and the new guards. One overclaimed sentence is folded into finding (1). CLAUDE.md needs no edit — pro_resources.go is already its listed entry point for backup resources
• [na] Cross-repo contracts: Title/Rules/Sources/SelectedOsVersions are read from jamfplatform-go-sdk's BenchmarkResponseV2 and consumed only inside this repo; no wire key, path or enum introduced or renamed
• Project rules compliance: CLAUDE.md — no generated-code boundary crossed (all four files hand-written), no credential flags, docs/solutions/ convention honoured. No .claude/rules/ directory in the repo

Scope of review

Incremental diff since baseline 0f2b08e is one fix commit, 2ace905, plus a merge of main bringing in #330. The merge contributes nothing to this PR's own diff — origin/main...HEAD is still the same 6 files, +243/−37 — so review scope is 2ace905 and the whole-PR production surface it lands in: benchmarkToExport, backupBenchmarks, the diff live loader, loadSnapshotFromDirectory / readObjectsFromSubdir / backupObjectName, nonStandardBackupFilters and its four call sites, the three new tests, and the postmortem. CI green.

Accepted gap, disclosed by the author. TestBenchmarkToExport_DiskAndLiveAgreeFieldForField calls benchmarkToExport for both sides, so re-inlining a literal projection at pro_diff.go:459 would not fail a test. Verified: nothing else in the suite reaches that call site, because it needs a Platform SDK client. Accepted rather than raised — pinning it means extracting the list-and-project loop behind an interface, which is a larger change than this PR should carry, and the limit is recorded in the postmortem rather than left implied.

Not re-verified by mutation. The author's mutation table was checked by reading the test bodies against the table, not by running mutations — the review worktree is read-only. That reading is what produced finding (1): the row "rename a table entry to a directory no backup writes → 2 tests fail" is correct on the count, but the two tests that fail are the benchmark-specific ones, not the table guard.

No specialists dispatched. Two properties of the input: the whole production surface is one shared function plus one struct-field lookup, and correctness turns on three projections read in full here, so a scoped-diff lane would re-read the same three. This session also carries a standing instruction not to spawn subagents unless asked — which likewise means the §12 confidence pass was self-scored under the orchestrator carve-out, with every cited line quoted above.

Never-run lanes: test-quality-reviewer, devil-advocate, silent-failure-hunter, usability-reviewer — eligible on this diff (production logic plus three new tests; a new lookup table; warn-and-continue paths in the touched function; CLI-visible diff output) but skipped for the reasons above. security-reviewer, performance-reviewer, scope-reviewer, fidelity-reviewer — inapplicable: no security or hot-path file, 243 lines in one concern, and no linked spec or ticket (the PR cites a review thread on #332).

What's done well

✅ Fixing (2) structurally instead of taking the suggested slices.Contains check is the better call, and worth saying so explicitly: a slices.Contains guard would have made two lists agree, where giving nonStandardBackupFilters a NameField per entry removes the second list entirely. The suggestion was the cheap fix; this is the right one.

✅ Recording the un-pinnable mutation in the postmortem, with the reason it cannot be reached and what the seam would cost, rather than omitting it from the table or over-claiming the guard. A known gap written down is worth more than a green suite that implies there isn't one — and it is what let this round confirm the gap in one read instead of re-deriving it.

✅ Keeping rules on both sides rather than dropping it from both, with the reason given ("a backup without it could not restore one"). That is the correct half of the either/or (1) offered — the diff being bulky is a display concern, an unrestorable backup is a data concern.

_{🤖 Generated by the pr-review:review skill v1.30.0 · reviewed head 9e06e2d}