feat(admin): finish Cloud GDPR deletion catalog v2 - #5397
Conversation
47e46ee to
8e479a5
Compare
Move remaining CSA-owned deletion work onto the Cloud queue: ticket-only intake, Pylon finalize, and CSA support-db scrub. Freeze catalog v1 and add v2 steps so new requests run the full pipeline. Ticket email now resolves at preflight. PostHog uses environment APIs with a 3-attempt verify cap. Local deletion-mock covers PostHog, Pylon, Substack, Customer.io, and CSA.
8e479a5 to
ef2baa4
Compare
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summaries (2 snapshots, latest commit 93eebb4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 93eebb4)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit ef2baa4)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (56 files)
Reviewed by grok-4.6 · Input: 137.5K · Output: 7.6K · Cached: 350.7K Review guidance: REVIEW.md from base branch |
Ticket preflight used endsWith('@service.usepylon.com'), so a requester
like user@mail.service.usepylon.com was treated as the customer and the
thread was never walked. Match the existing includes() check used by
pylon-reply and drop the duplicate refused-resolved helper.
Keep preflight request-level, not a catalog step, and lock cleanup until promotion.
intentionally-left-nil
left a comment
intentionally-left-nil
left a comment
There was a problem hiding this comment.
I only got as far along as pylon-client
| const [result] = await enqueueUserDeletionTargets({ | ||
| actor: { kiloUserId: null, email: actorEmail }, | ||
| targets: [{ email: user.google_user_email, trustedUserId: user.id }], | ||
| catalogVersion: 1, |
There was a problem hiding this comment.
What do catalogVersions mean in this context? Should this be 1 or 2?
There was a problem hiding this comment.
v1 is the frozen original catalog. v2 is current (USER_DELETION_CATALOG_VERSION) and adds Posthog, Substack, pylon_finalize, and csa_support_db. This CSA support route stays on 1 so CSA-initiated deletes do not run Cloud-owned v2 steps, including calling CSA back. Admin enqueue uses 2.
So currently this flow is split into two parts with one being on CSA (customer support website) and the other in Cloud admin. And I'm moving it all to Cloud admin. In the end after everything is done there will be no catalog version needed.
| } | ||
|
|
||
| const headers: Record<string, string> = { | ||
| Authorization: `Bearer ${secret}`, |
There was a problem hiding this comment.
I checked, and if deletionFetch fails, there are no errors thrown and the bearer secret ought not to be in logfiles
There was a problem hiding this comment.
Yes. deletionFetch classifies the failure and does not log request headers, so the bearer should not land in logs.
| if (stop) return stop; | ||
|
|
||
| const emailOrOutcome = requireTargetEmail(request); | ||
| if (typeof emailOrOutcome !== 'string') return emailOrOutcome; |
There was a problem hiding this comment.
Without trying to get in and suggest a whole new way of determining codeflows and error handlings - this type of checking is the result of overloading the return type, as opposed to e.g. chaining promises, or other exception types.
My first instinct would have been to check for a generic Outcome type, but since these have no hierarchy, you can't do e.g. an instanceof check.
Anyways, it's not worth redoing this whole code around how requireTargetEmail (and all of these outcome patterns work), but it's one of the first things I noticed
There was a problem hiding this comment.
Same union as the other handlers (string | DeletionHandlerOutcome). Not changing that pattern in this PR.
| type DeletionHandler, | ||
| } from '@/lib/user/deletion-queue/handlers/common'; | ||
|
|
||
| export const handleCsaSupportDb: DeletionHandler = async ({ request, context }) => { |
There was a problem hiding this comment.
Given that this function returns bare objects, can we specify a return type to catch any future tying errors?
There was a problem hiding this comment.
Already typed as DeletionHandler, which is Promise<DeletionHandlerOutcome>. The bare objects have to match that.
| const propertyEmail = properties ? properties.email : undefined; | ||
| if (typeof propertyEmail === 'string' && propertyEmail) emails.add(propertyEmail); | ||
| if (typeof person.name === 'string' && person.name.includes('@')) emails.add(person.name); | ||
| for (const value of [properties?.email, properties?.$email]) { |
There was a problem hiding this comment.
Oof why is there a .email and a .$email
There was a problem hiding this comment.
PostHog reserved person property is $email; some payloads also set email. We collect both.
| if (!isRecord(payload) || !Array.isArray(payload.results)) return null; | ||
| const persons: PosthogPerson[] = []; | ||
| function parsePersons(payload: unknown): ParsePersonsResult { | ||
| if (!isRecord(payload) || !Array.isArray(payload.results)) return { kind: 'incomplete' }; |
There was a problem hiding this comment.
Is this expected? Would we want any kind of error logging to determine what happened?
There was a problem hiding this comment.
parsePersons only classifies. lookupPersons maps incomplete to needs_attention / posthog_lookup_incomplete, so it does surface.
| if (!isRecord(entry)) continue; | ||
| const uuid = typeof entry.uuid === 'string' && entry.uuid.length > 0 ? entry.uuid : null; | ||
| if (!uuid) { | ||
| missingUuid = true; |
There was a problem hiding this comment.
This is another case we could not delete emails if the uuid is a different type. Would it be better to error out here instead?
There was a problem hiding this comment.
Non-string or missing uuid already fails the whole parse as missing_uuid → manual_action_required. The continue is so we still scan the rest of the page before returning that.
| await db | ||
| .select({ id: kilocode_users.id, blocked_reason: kilocode_users.blocked_reason }) | ||
| .from(kilocode_users) | ||
| .where(eq(sql`lower(${kilocode_users.google_user_email})`, normalizeDeletionEmail(email))) |
There was a problem hiding this comment.
Do we normalize the email in any other way besides lowercasing it?
There was a problem hiding this comment.
Trim and lowercase only, same as normalizeDeletionEmail everywhere else. No plus-tag or Gmail-dot folding.
| if (!statusPoll.response.ok) return { outcome: classifyPosthogResponse(statusPoll.response) }; | ||
| const body = await readJsonUnknown(statusPoll.response); | ||
| if (!isRecord(body) || !Array.isArray(body.results)) { | ||
| return { pending: true }; |
There was a problem hiding this comment.
Why is this case pending, but not an error?
There was a problem hiding this comment.
Malformed or empty deletion_status while PostHog is still processing is treated as not-yet-verified. We retry, then after USER_DELETION_POSTHOG_MAX_VERIFY_ATTEMPTS succeed unconfirmed instead of blocking the queue.
The most important part is that we make sure that deletion request was accepted. Posthog deletion is an async process and we don't want to wait forever (it really takes a while) till they actually delete it, so we try a few times and then mark it as success.
3 participants
Summary
Move remaining CSA-owned GDPR deletion work onto the Cloud admin queue.
pylon_finalize,csa_support_db).delete-complete, close the ticket, then delete the contact.deletion-mockcovers PostHog, Pylon, Substack, Customer.io, and CSA.Verification
pnpm format:changedandgit diff --checkpnpm --filter web exec tsgo --noEmitandpnpm --filter @kilocode/db exec tsgo --noEmit#1001completed 12/12; 429/5xx/expired-Substack/missing-user/delete_ready_missingparked as designedVisual Changes
Admin deletion queue intake is visible. New v2 steps: Pylon finalize and CSA support DB.