[LFXV2-2645] feat(users): scrub deleted user's username from project settings

[andrest50]

Summary

• Subscribe to lfx.v1-sync-helper.user.deleted and scrub the deleted user's LFID from project settings (writers, auditors, meeting coordinators, executive director, program manager, opportunity owner).
• Full-scan project settings (same pattern as HandleInviteAccepted), with revision-conflict retries and OpenSearch reindex — no project_settings.updated notification emails.
• Adds NATS subject constant, queue subscription wiring, docs, and unit tests.

Context

Mirrors the committee-service settings scrub in lfx-v2-committee-service#161, but project settings have no separate member records to update.

Depends on v1-sync-helper publishing lfx.v1-sync-helper.user.deleted (lfx-v1-sync-helper PR or #133).

Test plan

• go test ./internal/service/ -run 'TestHandleUserDeleted|TestProjectSettingsHasUsername'
• Deploy with v1-sync-helper event publisher; soft-delete a user who is a project writer/auditor/coordinator and verify username is cleared in KV + search index

Made with Cursor

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds handling for v1-sync-helper user-deletion events. The service scans project settings, clears deleted usernames with retry logic, refreshes indexes and OpenFGA tuples, updates reconciliation timeouts, and documents and tests the new NATS flow.

Changes

User deletion scrubbing

Layer / File(s)	Summary
Event contract and subscription .claude/skills/project-service-dev/references/nats-messaging.md, cmd/project-api/main.go, pkg/constants/nats.go, pkg/events/project.go	Defines the deleted-user payload and subject, subscribes HandleUserDeleted, and documents core NATS delivery behavior.
Scrub handler and side effects internal/service/project_subscriber.go	Scans project settings, clears matching usernames across role fields, retries revision conflicts, publishes reindex and OpenFGA updates with bounded retries, and uses the longer settings-scan timeout for invite reconciliation.
Scrub behavior validation internal/service/project_subscriber_test.go	Tests service authorization context, malformed and empty payloads, matching role fields, update conflicts, side-effect retries, no-match handling, and listing errors.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant V1SyncHelper
  participant NATS
  participant ProjectsService
  participant ProjectSettingsRepository
  participant SideEffectPublisher
  V1SyncHelper->>NATS: Publish user.deleted event
  NATS->>ProjectsService: Deliver queued event
  ProjectsService->>ProjectSettingsRepository: List and scrub matching settings
  ProjectsService->>SideEffectPublisher: Publish index and OpenFGA updates

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly names the main change: scrubbing deleted users from project settings.
Description check	✅ Passed	The description matches the changeset and accurately summarizes the new event subscription, scrubbing logic, docs, and tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/LFXV2-2645-scrub-username-on-user-delete

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Adds deletion-event handling to scrub stale usernames from project settings.

Changes:

• Subscribes to the v1-sync-helper user-deletion event.
• Scrubs role fields with conflict retries and reindexing.
• Adds tests and NATS documentation.

Reviewed changes

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

Show a summary per file

File	Description
pkg/constants/nats.go	Defines the inbound event subject.
internal/service/project_subscriber.go	Implements username scrubbing.
internal/service/project_subscriber_test.go	Tests scrub scenarios and retries.
cmd/project-api/main.go	Registers the event handler.
.claude/skills/project-service-dev/references/nats-messaging.md	Documents the inbound subject.

---

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

internal/service/project_subscriber.go:528

• This wire payload is declared under internal/, contrary to the repository's explicit NATS contract convention (pkg/events/project.go:4-10). Keeping a second local definition also lets the consumer silently drift from the producer. Move the event type to pkg/events and unmarshal/use that canonical type here and in the tests.

// HandleUserDeleted scrubs the deleted user's username from project settings.
// Best-effort: partial failures are logged but do not block the overall scrub.
// Unlike committee data, project settings have no separate member records — only
// the settings writers/auditors/meeting coordinators and named role fields are updated.
func (s *ProjectsService) HandleUserDeleted(ctx context.Context, msg domain.Message) error {

internal/service/project_subscriber.go:676

• Making the FGA refresh best-effort leaves a security-relevant partial state: the KV scrub can succeed, then GetProjectBase or SendAccessMessage can fail, and this core-NATS handler still completes with no retry. The deleted LFID's existing OpenFGA tuple then remains indefinitely, so a reused LFID can inherit project access. Persist or retry a durable FGA reconciliation (returning an error alone is insufficient with the current QueueSubscribe) before considering the scrub complete.

		Data:           *settings,
		IndexingConfig: settings.IndexingConfig(projectUID),
	}

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/project_subscriber.go:708

• A transient GetProjectBase failure bypasses all FGA retries after the settings write has already committed. Replaying the deletion cannot repair access because the username is now blank and the initial scan skips this project, leaving the deleted LFID's tuples stale. Retry this prerequisite just like the FGA publish (or make the side effect durably reconcilable).

	projectBase, err := s.ProjectRepository.GetProjectBase(ctx, projectUID)
	if err != nil {
		slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub",
			constants.ErrKey, err, "project_uid", projectUID)
		return

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/project_subscriber.go:706

• baseErr is logged but never assigned to accessErr. If the index publish succeeds while GetProjectBase fails, the success check immediately returns and OpenFGA is never refreshed or retried, leaving the deleted username's access tuples active. Propagate the base-read error into the retry condition.

		projectBase, baseErr := s.ProjectRepository.GetProjectBase(ctx, projectUID)
		if baseErr != nil {
			slog.WarnContext(ctx, "project_subscriber: failed to load project for FGA refresh after username scrub",
				constants.ErrKey, baseErr, "project_uid", projectUID)
		} else {

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

internal/service/project_subscriber.go (1)

683-729: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Retries re-publish the side effect that already succeeded, with no backoff.

If only the FGA publish fails, the next attempt re-sends the indexer message too (and vice versa), so a single transient failure can produce up to 4 duplicate publishes on the healthy path. Tracking each side effect separately also lets you skip the redundant GetProjectBase read. Adding a short backoff between attempts would avoid hammering NATS while it's degraded.

♻️ Sketch: per-side-effect flags + backoff

 func (s *ProjectsService) publishProjectSettingsScrubSideEffects(ctx context.Context, projectUID string) {
+	indexDone, accessDone := false, false
 	for attempt := 0; attempt < scrubSideEffectMaxRetries; attempt++ {
+		if attempt > 0 {
+			select {
+			case <-ctx.Done():
+				return
+			case <-time.After(time.Duration(attempt) * 250 * time.Millisecond):
+			}
+		}
 		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)
 		...
-		indexErr := s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false)
+		var indexErr error
+		if !indexDone {
+			indexErr = s.MessageBuilder.SendIndexerMessage(ctx, constants.IndexProjectSettingsSubject, indexMsg, false)
+			indexDone = indexErr == nil
+		}
 
 		var accessErr error
-		projectBase, baseErr := s.ProjectRepository.GetProjectBase(ctx, projectUID)
-		...
-		if indexErr == nil && accessErr == nil {
+		if !accessDone {
+			// ... publish, then accessDone = accessErr == nil
+		}
+		if indexDone && accessDone {
 			return
 		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/project_subscriber.go` around lines 683 - 729, Update the
retry loop in the scrub side-effects flow to track indexer and FGA publication
success independently, retrying only the failed side effect and avoiding
repeated GetProjectBase calls when the FGA publish has already succeeded. Add a
short backoff before each retry, while preserving the existing retry limit and
final warning behavior for failures.

internal/service/project_subscriber_test.go (1)

1268-1288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a GetProjectBase failure case.

The suite covers a transient SendAccessMessage failure but not a GetProjectBase error, which is the path where the FGA refresh is currently swallowed (see internal/service/project_subscriber.go Line 702). A case returning an error from GetProjectBase with a successful indexer publish would pin down that behavior once the handler propagates baseErr.

Note that SendIndexerMessage .Times(2) here encodes the current duplicate-publish behavior; it will need updating if the side-effect retry loop is changed to skip already-succeeded publishes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/project_subscriber_test.go` around lines 1268 - 1288, Add a
test case alongside the existing FGA publish retry case that makes
GetProjectBase return an error while SendIndexerMessage succeeds, and assert the
handler’s expected propagation of baseErr from the project subscriber path. Keep
the existing SendAccessMessage retry case intact, and update publish call counts
only if the retry implementation changes to avoid repeating successful indexer
publishes.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/service/project_subscriber.go`:
- Around line 701-713: Propagate the project-base load failure in the
username-scrub FGA refresh flow: when GetProjectBase returns baseErr, assign
that error to accessErr in addition to logging it. Keep the existing
message-send assignment for successful loads so the indexErr/accessErr check
triggers the reload and retry path for either failure.
- Around line 534-571: Update HandleUserDeleted and its project-settings scrub
flow to remove the entire matching settings row rather than only blanking
Username, ensuring the associated Email entry is deleted as well. Reuse the
existing project settings deletion mechanism and preserve best-effort processing
and timeout behavior for each affected project.

---

Nitpick comments:
In `@internal/service/project_subscriber_test.go`:
- Around line 1268-1288: Add a test case alongside the existing FGA publish
retry case that makes GetProjectBase return an error while SendIndexerMessage
succeeds, and assert the handler’s expected propagation of baseErr from the
project subscriber path. Keep the existing SendAccessMessage retry case intact,
and update publish call counts only if the retry implementation changes to avoid
repeating successful indexer publishes.

In `@internal/service/project_subscriber.go`:
- Around line 683-729: Update the retry loop in the scrub side-effects flow to
track indexer and FGA publication success independently, retrying only the
failed side effect and avoiding repeated GetProjectBase calls when the FGA
publish has already succeeded. Add a short backoff before each retry, while
preserving the existing retry limit and final warning behavior for failures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 84306425-d9f1-49ed-94f0-5a0cf6f6bd8e

📥 Commits

Reviewing files that changed from the base of the PR and between 96631a3 and b5444b0.

📒 Files selected for processing (6)

• .claude/skills/project-service-dev/references/nats-messaging.md
• cmd/project-api/main.go
• internal/service/project_subscriber.go
• internal/service/project_subscriber_test.go
• pkg/constants/nats.go
• pkg/events/project.go

[dealako]

Hi @andrest50 👋 — thorough, defensive work here, and it shows: this PR has already absorbed ~15 findings across multiple Copilot/CodeRabbit rounds (missing FGA refresh, wire-type placement, timeouts, snapshot staleness, GetProjectBase propagation, named-role test coverage) and all of them look correctly addressed. Build is green and the four new test functions pass locally. The handler faithfully mirrors the HandleInviteAccepted pattern.

I ran a security pass and a code-quality pass (in parallel) on top of my own review. No blocking correctness or security defects — everything below is refinement-level. The single highest-value item is that the confirm-revision double-read added in response to the earlier "stale snapshot" feedback doesn't actually deliver the guarantee its comment claims, and can be simplified away (which also erases an untested branch).

Reconciliation with existing bot comments

• ✅ Agree the prior Copilot/CodeRabbit findings are resolved (FGA refresh via publishProjectSettingsScrubSideEffects, V1UserDeletedEvent moved to pkg/events, settingsScanTimeout, reload-before-publish, baseErr→accessErr propagation, PM/Opportunity-Owner test rows).
• I did not re-litigate CodeRabbit's line-576 blank-vs-delete thread — keeping the committee-service scrub contract (blank the username, let FGA full-sync drop the tuple) is the right call.

Issue count

• 🔴 Blocking: 0
• 🟡 Minor: 3 — ineffective confirm-revision double-read (+ misleading comment + untested branch); email elevated to WARN-level logs; duplicated retry constant 4.
• ⚪ Nit: 2 — near-identical function names scrubUsername*In* vs *From*ProjectSettings; negative-path test gaps.
• ❔ Question: 1 — reuse-guard is bypassed for entries with no email (forged/reused-username event can strip an active user's access).

Decision: 🔴 Needs changes before approval

Nothing here is a defect, so this is a soft request-changes — purely to land the confirm-read simplification and the reuse-guard clarification before merge. Happy to re-review promptly. Everything else is take-it-or-leave-it.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/project_subscriber.go:716

• The revision is discarded here, so the previously discussed stale-snapshot race remains. A concurrent settings update can commit and publish revision N+1 after this read but before lines 748-751; these later full-state messages from revision N can then restore removed FGA tuples or stale search data. Keep and confirm the revision after loading the base before publishing (and retry on mismatch), or add revision-aware ordering at the consumers.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

internal/service/project_subscriber.go:664

• The reuse check is inverted for the normal deletion path. In production UserReader is always wired, while the linked sync-helper delete handler removes only its username index and publishes this event; it does not remove the Auth user. Thus a stored entry whose email matches the deleted account can still resolve to the same username at line 685 and is preserved instead of scrubbed. Treat a matching event/stored email as the deleted identity and reserve the auth lookup for differing emails (the actual reuse case); also define the fallback when the event has no email.

	if deletedEmailNorm != "" && entryEmail != "" && entryEmail != deletedEmailNorm {
		return false

internal/service/project_subscriber.go:730

• The KV revision is discarded here, so this can publish revision N after a concurrent settings update has committed and published N+1. Because both indexer and FGA updates are unversioned full-state projections, the late N message can restore removed roles/access. Preserve per-project ordering or carry the revision through both contracts so consumers reject stale updates; merely reloading once per retry does not close the read-to-publish race.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/service/project_subscriber.go:736

• The current implementation discards the settings revision and then performs two full-state publishes after another writer may have committed newer settings (or deleted the project). A delayed scrub publish can therefore overwrite the newer search projection and restore removed OpenFGA tuples. Preserve per-project ordering or carry the KV revision through these contracts so consumers reject stale messages; a read/recheck alone still leaves a TOCTOU window before publish.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)

[dealako]

Hi @andrest50 👋 — thanks for the fast, thorough turnaround. Since my last pass you've landed the confirm-revision simplification, consolidated the retry constant, renamed the near-duplicate helpers, and reworked the reuse guard to honor the new event Email. Build is green and all internal/service tests pass locally. I ran a fresh security pass and a code-quality pass in parallel on top of my own review.

👏 Nice work

• Confirm-revision double-read removed (dfe9d6a) — exactly the simplification I asked for; the misleading "revision guarantee" comment is gone and the untested branch went with it, replaced by an accurate "full-state and idempotent" note.
• Retry constant deduped — the local maxRetries = 4 and scrubSideEffectMaxRetries are now the single scrubMaxRetries.
• Helper names disambiguated — clearUsernameInSettings vs scrubProjectSettingsUsername read cleanly now.
• WARN-level email logging removed from the auth-lookup failure path; email is now only at Debug.
• Case-insensitive/trimmed matching (usernameMatches) and the email-aware reuse guard are a genuine robustness upgrade, with solid new negative-path tests (mismatch, username-only, empty, malformed, list error, retry paths).

Revision tracking (prior review items)

• ✅ Ineffective confirm-revision double-read + misleading comment + untested branch — resolved (dfe9d6a).
• ✅ Duplicated retry constant 4 — resolved.
• ✅ Email at WARN-level logs — resolved.
• ✅ Near-identical helper names — resolved.
• ⚠️ Reuse-guard behavior for the deletion identity — evolved via the Email field, but the new logic introduced a fresh correctness gap (see 🔴 below). Prior nit on negative-path test gaps is mostly closed but is missing the one case that would catch this gap.

Reconciliation with bot comments

• Agree with Copilot's suppressed line-664 comment ("reuse check inverted for the normal deletion path") — same root cause as my blocking item below.
• Disagree / documented trade-off: Copilot's repeated stale-snapshot / TOCTOU flag on the side-effect publish. Removing the confirm-read was intentional (my prior guidance); the indexer + FGA projections are full-state and idempotent, and true per-project ordering is a cross-service concern already tracked as follow-up. Not blocking here.
• CodeRabbit nitpicks (table-driven TestCtxWithServiceAuth, .Maybe() on async indexer expectations) are already addressed or take-it-or-leave-it.

Issue count (open items)

• 🔴 Blocking: 1 — positive event-email/entry-email match still runs the auth lookup and can permanently skip a genuine deletion (fire-and-forget, no replay). Fix: short-circuit return true on a definitive email match. Inline comment + suggested test added.
• 🟡 Minor: 2 — (1) missing test case for the email-match-with-UserReader scenario (the one that would fail today); (2) UserReader == nil degrades to an unverified scrub for email-bearing entries — consider fail-safe (skip + WARN) as defense-in-depth.
• ⚪ Nit: 2 — (1) the 3-slice-plus-3-pointer role iteration is duplicated across projectSettingsHasUsername/clearUsernameInSettings; a shared forEachSettingsUser helper would remove drift risk; (2) the HandleInviteAccepted timeout bump to settingsScanTimeout is a reasonable but out-of-scope change — worth a one-line note in the PR description.

Decision: 🔴 Needs changes before approval

Only the single blocking item (plus its test) needs to land before merge — it's a small, well-contained short-circuit but it materially affects whether the feature scrubs the primary deletion case in production. Everything else is optional. Happy to re-review promptly once that's in.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

internal/service/project_subscriber.go:737

• The settings revision is discarded, so this full-state snapshot can be published after a concurrent settings update or project deletion has already emitted newer state. The delayed index/FGA messages can then restore stale roles or recreate access for a deleted project. The earlier revision-confirmation fix is absent from the current diff; robustly preserve per-project ordering or carry the KV revision through both contracts so consumers reject stale updates.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)

internal/service/project_subscriber.go:562

• This warning writes the deleted user's raw LFID to application logs. Repository PII guidance prohibits new raw username/email fields at info or warn level (docs/reviews/knowledge-base/logging-and-pii.md:23-25); the error already identifies the failed scan without this value.

		slog.WarnContext(ctx, "project_subscriber: failed to list project settings for username scrub",
			constants.ErrKey, listErr, "username", event.Username)

internal/service/project_subscriber.go:716

• The success path adds the deleted user's raw LFID to an INFO log. This conflicts with the repository's established PII logging rule for new info/warn statements (docs/reviews/knowledge-base/logging-and-pii.md:23-25); project_uid is sufficient to correlate the operation.

			slog.InfoContext(ctx, "project_subscriber: cleared username from project settings",
				"project_uid", projectUID, "username", username)

internal/service/project_subscriber.go:555

• This new INFO log persists the deleted user's raw LFID. The repository's PII guidance explicitly says new info/warn logs must omit raw username/email and use non-identifying correlates instead (docs/reviews/knowledge-base/logging-and-pii.md:23-25).

This issue also appears in the following locations of the same file:

• line 561
• line 715

	slog.InfoContext(ctx, "project_subscriber: scrubbing deleted user's username from project settings",
		"username", event.Username)

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/service/project_subscriber.go (1)

560-572: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not swallow failed deletion scrubs.

HandleUserDeleted returns nil when the full scan fails, and scrubProjectSettingsUsername/publishProjectSettingsScrubSideEffects only log exhausted failures. With fire-and-forget core NATS delivery, a transient repository or publish failure can permanently leave the deleted username in settings, OpenSearch, or OpenFGA. Persist failed project IDs for retry, or otherwise introduce a durable retry mechanism instead of acknowledging the event unconditionally.

Also applies to: 697-728, 734-792

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/project_subscriber.go` around lines 560 - 572, The
HandleUserDeleted deletion-scrub flow currently acknowledges events after scan,
repository, or side-effect failures, allowing deleted usernames to remain.
Update HandleUserDeleted, scrubProjectSettingsUsername, and
publishProjectSettingsScrubSideEffects to propagate failures or persist affected
project IDs through a durable retry mechanism, and only return success after all
scrubs complete reliably; do not merely log exhausted failures or return nil on
failed scans.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/service/project_subscriber.go`:
- Around line 560-572: The HandleUserDeleted deletion-scrub flow currently
acknowledges events after scan, repository, or side-effect failures, allowing
deleted usernames to remain. Update HandleUserDeleted,
scrubProjectSettingsUsername, and publishProjectSettingsScrubSideEffects to
propagate failures or persist affected project IDs through a durable retry
mechanism, and only return success after all scrubs complete reliably; do not
merely log exhausted failures or return nil on failed scans.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3b82edc7-ad17-4027-ab1f-17618442685e

📥 Commits

Reviewing files that changed from the base of the PR and between 8041489 and eff20e5.

📒 Files selected for processing (2)

• internal/service/project_subscriber.go
• internal/service/project_subscriber_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/service/project_subscriber_test.go

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

internal/service/project_subscriber.go:737

• The revision is discarded again, so this reintroduces the race from the earlier review thread: another settings update or project deletion can commit after this read, then these later full-state index/FGA publishes restore the older roles (or recreate deleted access). The developer reply said this was fixed by re-reading and comparing confirmRevision, but that check is absent from the current diff. Preserve per-project ordering or carry/validate the KV revision before publishing stale full-state projections.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)

internal/service/project_subscriber.go:703

• A transient KV read ends this project after the first attempt even though scrubMaxRetries is documented to cover settings work. Because the inbound core-NATS event is not replayed, that one failure permanently leaves the deleted username and its access tuples in place. Retry this read through the configured attempt budget, as the post-write reload path already does.

		settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)
		if err != nil {
			slog.DebugContext(ctx, "project_subscriber: failed to get settings for username scrub — skipping",
				constants.ErrKey, err, "project_uid", projectUID)
			return

internal/service/project_subscriber.go:723

• Non-conflict write failures are also terminal on the first attempt. UpdateProjectSettings maps transient NATS/KV failures to ErrInternal, so a brief outage bypasses the remaining retry budget and the non-durable deletion event cannot repair the stale username/access later. Retry these failures too, while retaining the final-attempt warning.

		if !errors.Is(updateErr, domain.ErrRevisionMismatch) || attempt == scrubMaxRetries-1 {
			slog.WarnContext(ctx, "project_subscriber: failed to clear username from project settings",
				constants.ErrKey, updateErr, "project_uid", projectUID)
			return

internal/service/project_subscriber.go:690

• A transient auth-service failure is treated identically to a verified reuse and permanently skips this entry. clearUsernameInSettings then reports no change, the outer retry loop returns, and the core-NATS deletion event is never replayed, leaving stale access. Propagate an indeterminate result so the project attempt can retry instead of converting lookup availability into a keep decision.

		slog.WarnContext(ctx, "project_subscriber: auth lookup failed during username scrub — skipping entry",
			constants.ErrKey, err)
		return false

[dealako]

Hi @andrest50 👋 — great turnaround on the blocking item from my last pass. You nailed the fix and added exactly the test I hoped for. I re-pulled (eff20e5), ran build + internal/service and internal/infrastructure/nats tests (all green), and ran fresh security and code-quality passes in parallel over the delta since 9ed39e7.

👏 Nice work

• Blocking item resolved (8041489) — shouldScrubSettingsUsername now return true immediately on a definitive event-email/entry-email match, short-circuiting the auth lookup that could false-skip a genuine deletion. The docstring was rewritten to match the new control flow accurately.
• The exact test I asked for — "event email matches entry email — scrub without auth lookup" stubs UsernameByEmail and asserts it's called zero times via AssertNumberOfCalls, so a regression that reintroduces the lookup will fail loudly. Nicely defensive.
• Clean adoption of the async-only FGA API (eff20e5) — swapping SendAccessMessage(..., false) for PublishAccessMessage(...) is behavior-preserving (the old call already took the async false path), accessErr still carries real publish errors, and the scrubMaxRetries loop stays meaningful. All five FGA call sites migrated consistently — no leftovers.
• Tightened mock matchers to mock.AnythingOfType("types.GenericFGAMessage") — a good precision upgrade over mock.Anything.

Revision tracking

• ✅ Positive email match ran the auth lookup and could permanently skip a genuine deletion — resolved in 8041489 + test.
• ✅ Prior round's resolved items (confirm-revision removal, retry-constant dedupe, WARN-email removal, helper renames) remain good.

Reconciliation with new bot comments

• Agree (and incorporating as blocking) — Copilot's PII-logging finding. It cites docs/reviews/knowledge-base/logging-and-pii.md:19-25, a Critical repo rule. Three new INFO/WARN lines log the raw LFID username. I under-weighted this in earlier rounds; Copilot is right and it's consistent with the maintainer escalation on PR #70. See inline. (Full disclosure: this is a self-correction on my part — I should have flagged it two rounds ago.)
• Disagree — Copilot :737 "revision race / re-add confirm-revision". Removing the confirm-read was my prior guidance; the indexer + FGA projections are documented full-state and idempotent, and true per-project ordering is a cross-service concern tracked as follow-up. Not re-adding.
• Trade-off, not blocking — CodeRabbit "don't swallow failed scrubs" (Major) + Copilot :703/:723 (retry transient read/write). These flag the fire-and-forget core-NATS pattern with no durable replay. The ListAllProjectsSettings fail-fast is pre-existing repository behavior (not introduced here), and durable JetStream/outbox delivery is an acknowledged cross-service follow-up matching the committee-service scrub contract. You already acknowledged both on-thread; reasonable to defer.

Issue count (open)

• 🔴 Blocking: 1 — raw LFID username logged at INFO/WARN on 3 new lines (project_subscriber.go:555, 562, 716), violating the Critical PII rule. Trivial fix: drop the "username" field, keep project_uid.
• 🟡 Minor: 1 — auth-lookup failure at :688-690 is treated as a permanent skip; because the event isn't replayed, a transient auth outage can strand a genuine scrub. Conservative (won't wrongly strip access), so acceptable if intentional, but worth a comment noting the trade-off.
• ⚪ Nit: 1 — in the new TestShouldScrubSettingsUsername case, the got := ... + assert.Equal pair is duplicated across the two userReader branches; could hoist above the if. Cosmetic.

Decision: 🔴 Needs changes before approval

Only the single PII-logging blocker needs to land before merge — it's a 3-line change. Everything else is optional. Apologies for not catching the logging issue sooner; the feature logic itself is in excellent shape. Happy to re-review the moment the log fields are dropped.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

internal/service/project_subscriber.go:702

• A transient settings read exits on the first attempt even though this event is non-durable and the surrounding loop has four attempts. Since the initial scan already found this project, returning here permanently leaves its username and OpenFGA tuples intact; retry the read (logging only after the final failure) instead of abandoning the project immediately.

		settings, revision, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)
		if err != nil {
			slog.DebugContext(ctx, "project_subscriber: failed to get settings for username scrub — skipping",
				constants.ErrKey, err, "project_uid", projectUID)
			return

internal/service/project_subscriber.go:736

• The KV revision is discarded, so another settings update can commit and publish newer state after this read but before these full-state index/FGA publishes. This later stale snapshot can undo the newer search projection or restore removed access tuples. The side-effect contracts need per-project ordering or revision propagation so consumers can reject stale updates; reloading once per attempt does not close this race.

		settings, _, err := s.ProjectRepository.GetProjectSettingsWithRevision(ctx, projectUID)