fix(dotc1z): drop malformed grants (nil principal/entitlement) at write time

[arreyder]

Summary

Drop malformed grants (nil principal or nil entitlement) at the c1z write path instead of silently persisting them with empty identity columns.

Fixes #916.

Problem

baseGrantRecord extracts a grant's principal/entitlement identity columns straight off the live proto using nil-safe opaque getters. A grant with a nil principal or entitlement therefore does not panic and is not rejected — it's written with principal_resource_id = "" / entitlement_id = "" (empty satisfies the not null constraint). The result is an unqueryable junk row that nothing surfaces.

Every other layer already treats these fields as required:

• grant.proto marks principal and entitlement (validate.rules).message = {required: true}
• NewGrant() panics on a nil principal
• the grant task handler (pkg/tasks/c1api/grant.go:35) rejects nil principal/entitlement as a non-retryable "malformed grant task"

The local write path was the only place that let them through. Surfaced during review of #863.

Fix

upsertGrantsInternal (which backs both PutGrants and StoreExpandedGrants) now runs dropMalformedGrants first:

• drops any grant with a nil principal or entitlement
• logs the dropped grant id at error level
• increments a new c1z_grant_malformed_dropped_total metric
• fast-paths (no reallocation) when every grant is well-formed

Policy choice — drop-and-continue vs. hard-fail. This PR drops-and-continues so one buggy connector grant can't abort an entire sync's grant write, matching the graceful-degradation already used in grant expansion. The strict alternative (fail the batch, matching the task handler) is a one-line change. @ggreer — flagging for your call; happy to flip it.

Test plan

• TestPutGrants_DropsMalformedGrants — interleaved nilPrincipal, good, nilEntitlement batch → only the well-formed grant persists
• TestPutGrants_AllMalformedIsNoop — all-malformed batch writes nothing, no error
• TestDropMalformedGrants_AllValidReturnsSameSlice — fast path returns input untouched
• Pre-fix verification: with the drop disabled, all three grants persist (two with empty identity columns)
• go test -short ./pkg/dotc1z/... — pass
• golangci-lint run pkg/dotc1z/... — 0 issues

🤖 Generated with Claude Code

[github-actions]

General PR Review: fix(dotc1z): drop malformed grants (nil principal/entitlement) at write time

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Review mode: incremental since b9ae984
View review run

Review Summary

The new commits broaden the malformed-grant check from nil-only to nil-or-empty, extract it into the grantHasValidIdentity predicate, and cap per-batch drop logging at 10. Tests now cover empty-principal-identity, empty-grant-id, the slim writer path, the StoreExpandedGrants path, and the predicate directly. No security or correctness issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/dotc1z/grants_malformed_test.go: missing test fixture for a non-nil entitlement with an empty ID — every other "empty" identity variant is covered (emptyPrincipal, emptyGrantID) but there is no emptyEntitlement case to exercise the g.GetEntitlement().GetId() != "" branch of grantHasValidIdentity.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/dotc1z/grants_malformed_test.go`:
- Around line 20-42 (grantTestFixtures): Add an `emptyEntitlement` fixture — a grant whose entitlement is non-nil but has an empty ID (e.g. `v2.Entitlement_builder{Id: "", Resource: g1}.Build()`). Add it to the malformed batch in `TestPutGrants_DropsMalformedGrants` and add a `require.False(t, grantHasValidIdentity(f.emptyEntitlement), "empty entitlement id")` assertion in `TestGrantHasValidIdentity`.

[github-actions]

No blocking issues found.

[arreyder]

Update — review-driven additions (626ff10)

Ran a multi-agent review (bugs / perf / test) over the diff. Core logic came back clean (fast-path correctness, replace-mode has no stale-row risk, nil-element safety, no caller depends on written count). Two substantive additions came out of it:

1. Broadened the malformed check. The original nil-only test let through grants with a non-nil but empty identity — e.g. a principal carrying an empty ResourceId{}, or an empty entitlement/grant id — which baseGrantRecord then persisted as the exact empty-column junk the drop exists to prevent (and an empty grant id collides on the (external_id, sync_id) unique index). New grantHasValidIdentity rejects nil or empty principal resource type/id, entitlement id, and grant id. Verified the old check let an empty-ResourceId principal through.

2. Capped per-batch drop logging to the first 10 grants + one aggregate line, so a broken connector emitting a run of malformed grants can't flood the logs. The exact count stays on c1z_grant_malformed_dropped_total.

Test additions: parameterized the drop test over full-blob and slim writers, added a StoreExpandedGrants (PreserveExpansion) drop test, added empty-identity fixtures + a direct grantHasValidIdentity boundary test, and fixed the fast-path test to assert slice identity (require.Same) rather than just contents.

Consciously deferred: asserting the grantMalformedCounter increment requires wiring an OTel manual-reader MeterProvider — moderate harness cost for modest value given the drop is well-tested. Happy to add if you'd like it.

The drop-and-continue vs. hard-fail policy question for @ggreer is unchanged.

[github-actions]

No blocking issues found.

[kans]

I think we should reject these and error out when the connector surfaces them to the SDK? @ggreer thoughts?