Pull request overview

Adds committee username cleanup when deleting merged users.

Changes:

• Adds query-service configuration and client.
• Scrubs committee members and settings with reuse guards.
• Adds unit tests for cleanup behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

	if username != "" {
		email, emailErr := getPrimaryEmailForUser(ctx, userSfid)
		if emailErr != nil {
			logger.With(errKey, emailErr, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to look up primary email for deleted user; committee username scrub skipped")

	if err := natsConn.Publish(v1SyncHelperUserDeletedSubject, payload); err != nil {
		logger.With(errKey, err, "key", key).
			ErrorContext(ctx, "failed to publish user-deleted event; committee username scrub skipped")
		return

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

	if username != "" {
		email, emailErr := getPrimaryEmailForUser(ctx, userSfid)
		if emailErr != nil {
			logger.With(errKey, emailErr, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to look up primary email for deleted user; committee username scrub skipped")

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

		email, _ := getPrimaryEmailForUserFn(ctx, userSfid)
		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

	publishUserDeletedEventFn = publishUserDeletedEvent
	getPrimaryEmailForUserFn  = getPrimaryEmailForUser

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

		email, _ := getPrimaryEmailForUserFn(ctx, userSfid)
		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

	golang.org/x/text v0.39.0

Hey @andrest50 👋 — thanks for the quick turnaround on my last pass. This is a follow-up review after the three new commits (e92d7d1, 688db4a, 02e5766) that landed and dismissed my prior "Approved with comments."

👏 Nice work

• Question #1 (raw-vs-normalized username contract) — resolved. e92d7d1 now normalizes the published username via normalizeKVSegment (TrimSpace → ToLower → NFC), so decomposed-Unicode / case / whitespace variants can't silently miss the downstream scrub. Good call routing the shared pipeline through one helper.
• Prior Nit (untested publish wrapper) — resolved. TestPublishUserDeletedEvent + the injectable natsPublishBytesFn seam now cover the publish path, including the error-swallowed case. Clean use of the existing Fn-var pattern.
• Question #2 (username reuse) — partially mitigated by carrying the primary email in the payload so downstream scrubbers can distinguish LFID reuse. Good instinct — see the caveat below on whether it actually populates.
• All CI green (MegaLinter, CodeQL, License, DCO), targeted tests pass locally.

Revision tracking

New findings this round

🔴 Blocking

1. Primary-email enrichment is usually empty in the common delete path — undermining the reuse-detection it was added for (handlers_users.go:161-163)
getPrimaryEmailForUserFn(ctx, userSfid) reconstructs the primary email by reading the v1-merged-user.alternate-emails.<sfid> mapping and the per-email rows. But your own TODO at lines 155-159 states those alternate-email rows are "deleted before or alongside the user row." So at user-delete time this lookup will frequently hit ErrKeyNotFound → email == "". Since 688db4a added email specifically so downstream can distinguish LFID reuse (the exact risk from my Q2), an empty email in the common case means a delayed event after LFID reassignment could still scrub a new owner. Please either capture the primary email earlier (e.g. from the user payload / a value written on update, which is what 0caa31c attempted before it was reverted), or drop the email field if it can't be reliably populated — shipping a field that's empty in the common path is worse than not having it. A design confirmation would close this out.

🟡 Minor

2. Doc comment detached from toKVKey, now misdescribes normalizeKVSegment (handlers_users.go:64-70)
e92d7d1 inserted normalizeKVSegment directly beneath the existing toKVKey godoc block, so that comment ("...encodes it as a URL-safe base64 key segment ... RawURLEncoding keeps keys opaque and short") is now the godoc for normalizeKVSegment, which does none of the base64 encoding. Meanwhile toKVKey is left undocumented. Give normalizeKVSegment its own one-liner (TrimSpace→ToLower→NFC) and move the base64/RawURLEncoding sentence back down onto toKVKey.

3. getPrimaryEmailForUser error silently discarded (handlers_users.go:162)
email, _ := getPrimaryEmailForUserFn(...) drops the error, so an empty email is indistinguishable from a lookup failure in production logs. Combined with #1, a one-line DebugContext on the error would make the (expected) empty-email case observable.

4. Test gap: empty/failed-email path through handleMergedUserDelete not covered (handlers_users_test.go)
TestHandleMergedUserDeleteScrub always stubs getPrimaryEmailForUserFn → "deleted@example.com", nil. Given #1 makes empty-email the expected production path, add a case where the lookup returns "", err and assert wantPublished: true, wantEmail: "". (TestPublishUserDeletedEvent exercises "" but bypasses the handler.)

5. Primary email logged at DEBUG, now reachable from the delete path (lfx_v1_client.go:295)
getPrimaryEmailForUser logs the full fallbackEmail. Pre-existing and DEBUG-gated, but the new delete flow widens its exposure — with DEBUG=true every deleted user's primary email now hits logs. Consider masking. (Good that publishUserDeletedEvent itself keeps email out of its INFO line.)

⚪ Nit

6. Redundant double-normalization (handlers_users.go:145 & 161) — usernameToKVKey(username) and normalizeKVSegment(username) both run the NFC/lower/trim pipeline on the same input. Compute norm := normalizeKVSegment(username) once and derive the index key from it. Not worth churn on its own.

AI reconciliation

Copilot has 0 active inline comments — its earlier notes were against removed code (primary-email cache / query-service client) and are stale. No new bot findings to reconcile.

Issue count (carried + new)

• 🔴 Blocking: 1 — email enrichment empty in common delete path
• 🟡 Minor: 4 — detached godoc; swallowed error; empty-email test gap; DEBUG email log
• ⚪ Nit: 1 — redundant double-normalization

🔴 Final decision: Needs changes before approval

No mechanical bugs — tests are green and the username scrub itself is sound. The blocker is a design/correctness question: the email field added this round is likely empty in the common case, so it doesn't yet deliver the reuse-detection it was meant to. Confirm the ordering assumption and either populate email reliably or drop it, then knock out the doc/test/logging minors. Happy to re-approve quickly once #1 is settled.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

	// Primary emails are not linked as Auth0 identities (they are the Auth0 user's own email).
	// Cache the address in mappings so handleMergedUserDelete can supply it to downstream
	// scrubbers even after the alternate-email rows have been cleaned up ahead of the
	// merged-user row.
	if isPrimary, _ := v1Data["primary_email__c"].(bool); isPrimary {

			if _, err := mappingsKV.Put(ctx, cacheKey, []byte(emailAddr)); err != nil {
				logger.With(errKey, err, "key", key, "user_sfid", leadorcontactid).
					WarnContext(ctx, "failed to cache primary email for user deletion scrub")
			}

	if normalizedUsername := normalizeKVSegment(username); normalizedUsername != "" {

	golang.org/x/text v0.39.0

		// Best-effort cleanup of the cached primary-email key.
		if err := deleteIndexKeyFn(ctx, kvKeyPrimaryEmailByUserSfid+userSfid); err != nil {
			logger.With(errKey, err, "key", key, "user_sfid", userSfid).
				WarnContext(ctx, "failed to delete cached primary email after user-deleted event")
		}

Hey @andrest50 👋 — thanks for the thorough turnaround on this. This follow-up review covers the single new commit 3047696 ("restore primary-email cache for user.deleted event"), and it cleanly closes out my blocker from the last round.

👏 Nice work

• 🔴 Blocker resolved — email enrichment now actually populates. getCachedPrimaryEmailForUser reads a v1-user.primary-email.<sfid> cache written at handleAlternateEmailUpdate time (which survives the alternate-email-row cleanup that races ahead of the user delete), with a live-lookup fallback. This is exactly the ordering fix that was needed — and you wired in best-effort cache cleanup on delete plus a doc comment explaining the rationale.
• Godoc fixed — normalizeKVSegment (TrimSpace→ToLower→NFC) and toKVKey (RawURLEncoding) now each carry their own accurate comment.
• Error handling fixed — the lookup error and the empty-email case are now logged at Debug before publishing without email.
• Strong test additions — TestGetCachedPrimaryEmailForUser (hit + miss→live) and the extended scrub test asserting the email-error path and cache-key cleanup. Fixtures are synthetic (cached@/live@/deleted@example.com). Tests green, go vet clean.

Revision tracking (prior round)

New findings this round

Both my parallel security + code-quality passes agree there are no blocking issues — the mechanics are correct and well-tested. Remaining items cluster around the new cache's lifecycle:

🟡 Minor

1. Stale cached email on demote / primary-row delete (handlers_users.go:325, :444) — the cache is written only when primary_email__c=true and cleaned only on full user delete. If a user's primary email is demoted (true→false) or the primary alt-email row is deleted (which handleAlternateEmailDelete explicitly skips at line 444), the cache is neither updated nor removed. Since getCachedPrimaryEmailForUser is cache-first, a later delete would emit a stale address, and the PII copy is retained indefinitely for a demoted-but-never-deleted user. For a PR whose whole purpose is scrubbing PII, that retention gap is worth closing — add a cache delete before the line-444 early return (and/or on demote). If you'd rather defer, a one-line comment at the write site noting the demote/delete path is intentionally uncovered + a follow-up ticket would satisfy me.
2. Cache key orphaned when a deleted user has an empty username (handlers_users.go:176-188) — the deleteIndexKeyFn(...primary-email...) cleanup sits inside the if normalizedUsername != "" guard, so a user with a cached primary email but blank username never gets the cache key removed. Consider hoisting the cache cleanup outside the username guard.
3. Cache write path untested (handlers_users.go:327) — reads go through the swappable readMappingsKVValueFn (tested), but the mappingsKV.Put write in handleAlternateEmailUpdate has no coverage ("primary email arrives → cache key written"). Asymmetric testability.
4. Missing empty/whitespace cache-value test branch — getCachedPrimaryEmailForUser correctly TrimSpaces and falls through to live lookup on a blank cached value, but no test exercises that branch.

⚪ Nit

5. Const naming (handlers_users.go:26) — kvKeyPrimaryEmailByUserSfid drops the Prefix suffix its three siblings use (kvKeyUsernamePrefix, etc.); rename to kvKeyPrimaryEmailPrefix for consistency.
6. Repeated kvKeyPrimaryEmailByUserSfid + userSfid across write/read/delete (3×) — a tiny primaryEmailCacheKey(sfid) helper would centralize it. Optional.

AI reconciliation

Copilot has no new active inline comments this round. No external bot findings to reconcile.

Issue count

• 🔴 Blocking: 0
• 🟡 Minor: 4 — stale/retained cache on demote/primary-delete; orphaned cache on empty-username delete; untested cache write; missing empty-value test branch (+1 carried: DEBUG email log)
• ⚪ Nit: 3 — const naming; repeated key expression; carried double-normalization

🔴 Final decision: Needs changes before approval

This is very close — the blocker is gone and the feature is correct and well-tested. I'm holding on a formal approval only for the cache-lifecycle cluster (item #1 in particular, since stale/retained PII is thematically at odds with a scrub feature). Address #1 (fix or consciously defer with a follow-up ticket) and, ideally, the quick #2/#3, and I'll re-approve promptly. #4–#6 I'm happy to take as follow-ups.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

		clearPrimaryEmailCacheIfMatched(ctx, key, userSfid, emailAddr)

		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

			cacheKey := kvKeyPrimaryEmailPrefix + leadorcontactid
			if _, err := mappingsKV.Put(ctx, cacheKey, []byte(emailAddr)); err != nil {
				logger.With(errKey, err, "key", key, "user_sfid", leadorcontactid).
					WarnContext(ctx, "failed to cache primary email for user deletion scrub")
			}

	if normalizedUsername := normalizeKVSegment(username); normalizedUsername != "" {

	golang.org/x/text v0.39.0

Hey @andrest50 👋 — thanks for hardening the cache lifecycle so quickly. Both of my prior minors are cleanly resolved and the design is right. We're down to the last mile: one formatting fix and one test that actually asserts the new behavior.

👏 Nice work

• Stale/retained cache on demote & primary-row delete — resolved. The new clearPrimaryEmailCacheIfMatched helper is a nice design: it only deletes the cache when the stored value still matches the affected address (EqualFold + TrimSpace), so a legitimate newer primary (A→B) is never wiped. Wired into both the demotion branch (handleAlternateEmailUpdate, non-primary) and the primary-row-delete branch (handleAlternateEmailDelete). Both my parallel passes confirmed the ordering is correct and the value-match protects against false clears.
• Blank-username orphan — resolved. Cache cleanup hoisted outside the if normalizedUsername != "" guard, and you added whitespace-only / no username test cases that assert the cache key is still deleted.
• Const rename — done (kvKeyPrimaryEmailPrefix, matches siblings).

Revision tracking (prior round)

New findings this round

🟡 Minor

1. gofmt not clean — const block misaligned by the rename (handlers_users.go:26). The rename shortened the longest identifier, so the whole const block needs to collapse one column, but the padding wasn't updated — gofmt -l flags the file and make check will reformat it. (Interestingly CI MegaLinter is green, so it won't hard-block the gate, but it violates the repo's gofmt standard.) One-liner fix:

   gofmt -w cmd/lfx-v1-sync-helper/handlers_users.go
   

2. The new clear-on-match logic ships with no test that asserts a deletion (handlers_users.go:214, :357, :468). This is the substantive one:
   • There's no TestHandleAlternateEmailUpdate at all, so the demotion clear path (:357) — the behavior this round adds — has zero coverage.
   • In TestHandleAlternateEmailDelete, the new readMappingsKVValueFn stub always returns errors.New("cache miss"), so the primary-delete clear (:468) hits the read-error early-return and no-ops — nothing asserts a cache delete.
   • clearPrimaryEmailCacheIfMatched isn't unit-tested for its four documented branches (match→delete, no-match→skip, empty-email→skip, read-error→skip).
     A small direct table test on clearPrimaryEmailCacheIfMatched (asserting deleteIndexKeyFn is/ isn't called per branch) would lock in the logic and is the cleanest way to close this.

⚪ Nit

3. Read-error silently leaves stale cache (handlers_users.go:217) — reasonable (you can't value-match without the value, and blind-deleting risks dropping a different valid primary), but a one-line comment noting the trade-off, or a DebugContext, would help. A transient read error during a genuine demotion leaves the cache stale until the user-delete path cleans it.
4. Hard-delete of a primary row doesn't clear the cache (handlers_users.go — handleAlternateEmailDelete returns early on v1Data == nil before the clear). Edge case only (soft deletes always carry a payload); the unconditional user-delete cleanup still catches it.
5. Redundant userSfid == "" guard in clearPrimaryEmailCacheIfMatched — both call sites already guarantee non-empty. Harmless defense-in-depth.

AI reconciliation

No new external bot inline comments this round. Author replies confirm the fixes map to 09bf1c2. Nothing to reconcile.

Issue count

• 🔴 Blocking: 0
• 🟡 Minor: 2 — gofmt not clean; clear-on-match logic has no asserting test
• ⚪ Nit: 3 — silent read-error; hard-delete edge case; redundant guard (+ carried nits)

🔴 Final decision: Needs changes before approval

Genuinely the last mile — the feature is correct and the cache lifecycle is now sound. Holding formal approval only for the two 🟡 items: run gofmt -w, and add a test that actually asserts clearPrimaryEmailCacheIfMatched deletes on a value match (ideally covering the demotion path too). Do those and I'll approve on the next pass; the nits are take-it-or-leave-it.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated no new comments.

		publishUserDeletedEventFn(ctx, key, normalizedUsername, email)

		clearPrimaryEmailCacheIfMatched(ctx, key, userSfid, emailAddr)

	golang.org/x/text v0.39.0

Hey @andrest50 👋 — that's the last mile done. Both items from my previous review are resolved, and you proactively went a step further on PII. Approving. 🎉

👏 Nice work

• gofmt — fixed. Const block realigned; gofmt -l and go vet are clean, full package test suite green.
• Clear-on-match now has real coverage. TestClearPrimaryEmailCacheIfMatched is a proper table test: match→delete, case-insensitive match→delete, no-match→skip, empty-email→skip, empty-sfid→skip, read-error→skip — each asserting exactly one delete (of the right key) or zero. This pins down the exact behavior I was worried about last round. With the function directly unit-tested, the thin demotion/delete call-sites no longer need handler-level coverage.
• Proactive PII hardening (above and beyond):
  • Removed username from the publish-success Info log — the LFID is the very thing being scrubbed, so keeping it out of logs is the right call.
  • Added primary-email cache cleanup to the hard-KV-delete (v1Data == nil) branch (my prior nit), and asserted it in TestHandleMergedUserDeleteScrub — closes the last cache-orphan path.

Revision tracking — all resolved

Remaining items (all optional — none blocking)

• ⚪ Nit — the clearPrimaryEmailCacheIfMatched Warn reads "failed to delete stale primary email cache," but at the hard-delete/demote call-sites the entry is a matched live cache being cleaned, not necessarily stale. Cosmetic wording only.
• ⚪ Nit — TestClearPrimaryEmailCacheIfMatched could add an empty/whitespace cached value case, but the TrimSpace+EqualFold behavior is already correct.
• 📌 Follow-up (out of scope for this PR) — the pre-existing Auth0 link/unlink paths (handlers_users.go ~499/503/529/533) still log real email values at Warn/Error. Not touched by this PR, but worth a follow-up ticket to align them with the PII-hardening direction this change establishes.

AI reconciliation

Copilot's latest review carried no actionable inline comments. Nothing to reconcile.

Issue count

• 🔴 Blocking: 0
• 🟡 Minor: 0
• ⚪ Nit: 2 (both optional) + 1 out-of-scope follow-up

✅ Final decision: Approved with minor comments

Clean, correct, well-tested, and MERGEABLE. Thanks for the thorough iteration through the cache-lifecycle and PII edge cases — nicely done. Ship it once you're happy with the optional nits (or file the Auth0-logging follow-up separately).