Skip to content

🧹 Remove legacy uid branch for wipe job - #12490

Open
undivisible wants to merge 6 commits into
mainfrom
code-health-legacy-uid-wipe-job-62989738989302225
Open

🧹 Remove legacy uid branch for wipe job#12490
undivisible wants to merge 6 commits into
mainfrom
code-health-legacy-uid-wipe-job-62989738989302225

Conversation

@undivisible

@undivisible undivisible commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🎯 What: Removed the legacy uid branch and associated audience constraints for the account deletion wipe job.
💡 Why: It was explicitly marked as a legacy branch (TODO(#9760): Remove this legacy branch after the Cloud Tasks max-retry window has elapsed.) to be removed once the job_id migration was complete. Removing it cleans up dead code, redundant audience checks (legacy_sync), and simplifies the payload contract.
Verification: Verified by checking modified files and successfully running local backend tests (pytest backend/tests/routers/test_users.py backend/tests/unit/test_sync_cloud_tasks.py).
Result: Improved code maintainability and clarity within the users' router and database helper functions by enforcing a single job_id flow.

Failure-Class: none


PR created automatically by Jules for task 62989738989302225 started by @undivisible

Review in cubic


Note

Medium Risk
Changes authentication and payload handling on the destructive account-deletion worker path; any in-flight legacy uid tasks would be dropped after deploy, which is intended post-migration.

Overview
Account deletion wipe now only accepts Cloud Tasks payloads with a non-empty job_id; the legacy uid branch, resolve_legacy_deletion_wipe_uid, and related Firestore helper constants are removed.

OIDC verification for deletion tasks no longer tries the sync legacy_sync audience or returns an audience on AccountDeletionTaskAuthentication—only the dedicated account-deletion handler URL is accepted. Cross-checks that paired payload shape with audience are gone from run_account_deletion_wipe.

Tests and the Firestore query coverage baseline are updated accordingly; startup validation tests patch queue existence when exercising prod config.

Reviewed by Cursor Bugbot for commit 2a265dd. Configure here.

- Removes legacy `uid` payload parsing in `backend/routers/users.py`
- Removes `legacy_sync` audience verification in `backend/utils/cloud_tasks.py`
- Removes unused `resolve_legacy_deletion_wipe_uid` helper in `backend/database/users.py`
- Updates and removes corresponding unit tests in `test_sync_cloud_tasks.py` and `test_users.py`

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-08-31T16:59:36.986027Z d4815bc PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

- Removes legacy `uid` payload parsing in `backend/routers/users.py`
- Removes `legacy_sync` audience verification in `backend/utils/cloud_tasks.py`
- Removes unused `resolve_legacy_deletion_wipe_uid` helper in `backend/database/users.py`
- Updates and removes corresponding unit tests in `test_sync_cloud_tasks.py` and `test_users.py`
- Mocks out network calls in tests to prevent CI hermetic network blocks.
- Adds missing type annotations in `task_integrations_ops.py` and `proactive_engine.py`

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @undivisible — well-scoped cleanup, and the direction is right: the legacy uid lane has passed any reasonable drain window (the compatibility path landed 2026-07-14, and Cloud Tasks queue retention caps at 31 days), and at this head there are no residual references in the deletion path (legacy_sync, resolve_legacy_deletion_wipe_uid, payload_kind are all gone).

What I verified:

  • backend/routers/users.py — the handler now accepts only job_id payloads and resolves exclusively through resolve_deletion_wipe_job_id; the task_authentication.retry_count terminal-attempt accounting is untouched. Net effect is strictly tighter: a legacy-payload task can no longer be accepted under the old sync audience.
  • backend/utils/cloud_tasks.pyverify_account_deletion_cloud_tasks_oidc no longer falls back to the sync OIDC audience, and AccountDeletionTaskAuthentication is reduced to retry_count. Good simplification of the auth surface.
  • backend/database/users.pyresolve_legacy_deletion_wipe_uid and _DELETION_WIPE_LEGACY_ACTIONABLE_STATUSES are removed with no remaining callers.
  • backend/tests/routers/test_users.py / backend/tests/unit/test_sync_cloud_tasks.py — the three legacy-lane tests are removed together with the code they covered, and mocking assert_account_deletion_queue_exists in the startup-guard test is the correct hermeticity fix now that the guard probes the real queue (#12431).

One blocking nit:

  • backend/utils/cloud_tasks.py:19Literal is still imported but no longer used; the audience: Literal[...] field was its only consumer. This is exactly what is failing the Backend unit suite job (pyright reportUnusedImport); the pytest preflight itself passed before the typecheck step errored. Dropping Literal from the typing import should turn the run green.

Minor notes (non-blocking):

  • backend/utils/task_integrations_ops.py — the _google_tasks_post annotation change is unrelated to this cleanup; fine to keep, just flagging that it adds a small extra surface to the diff.
  • After this change, a hypothetical straggler legacy-uid task is rejected at verification or dropped as invalid_payload rather than executed — the safe outcome — but the drain-window go/no-go itself deserves an explicit maintainer sign-off since this is the account-deletion path.

Requesting changes just for the unused import; everything else looks ready once CI is green.

Automated review on behalf of the maintainers. Human sign-off specifically wanted on the drain-window timing for removing this OIDC audience lane from the account-deletion wipe endpoint before merge.

@Git-on-my-level Git-on-my-level added backend Backend Task (python) needs-maintainer-review Needs a human maintainer to sign off before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces labels Aug 31, 2026

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve only: dead-code removal/refactor, and Backend unit suite CI check is failing — hard floor blocks merge.

@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_874f7b01-8d07-4d96-9d71-7cfbaab199a4)

@undivisible

Copy link
Copy Markdown
Collaborator Author

Addressed Git-on-my-level pyright CR: dropped unused Literal from backend/utils/cloud_tasks.py. Pushed in b66e453.

- Removes legacy `uid` payload parsing in `backend/routers/users.py`
- Removes `legacy_sync` audience verification in `backend/utils/cloud_tasks.py`
- Removes unused `resolve_legacy_deletion_wipe_uid` helper in `backend/database/users.py`
- Updates and removes corresponding unit tests in `test_sync_cloud_tasks.py` and `test_users.py`
- Mocks out network calls in tests to prevent CI hermetic network blocks.
- Adds missing type annotations in `task_integrations_ops.py` and `proactive_engine.py`
- Regenerates `firestore_query_coverage_baseline.json`.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @undivisible — the b66e453 fix was exactly right, and the Backend unit suite went green on that commit (the only delta versus the previously failing head was dropping the unused Literal). Appreciate the quick turnaround.

Unfortunately the current head (97d2f2a) re-introduces that import, so I'm re-flagging it:

Blocking (same as before, regressed at head)

  • backend/utils/cloud_tasks.py:19Literal is back in from typing import Any, Dict, Literal, NamedTuple, Optional, but the removed audience: Literal['account_deletion', 'legacy_sync'] field was its only consumer, so it's unused again at this head. This is the exact condition that failed the Backend unit suite pyright step before (A/B inside this PR: failed on 8042130, passed on b66e453). No CI runs have triggered on 97d2f2a yet; dropping Literal from the import should turn it green again. Looks like the baseline-regen commit accidentally reverted that one line.

Per-file verification at this head:

  • backend/utils/cloud_tasks.pyverify_account_deletion_cloud_tasks_oidc is single-audience only (no sync-audience fallback), and AccountDeletionTaskAuthentication is just retry_count. Correct and strictly tighter. Only the import issue above.
  • backend/routers/users.py — handler accepts only job_id; both legacy-audience guard branches are gone together with the lane they protected; the drop log keeps the resolution outcome. No residual legacy_sync / payload_kind / resolve_legacy* references at this head.
  • backend/database/users.pyresolve_legacy_deletion_wipe_uid and _DELETION_WIPE_LEGACY_ACTIONABLE_STATUSES removed with no remaining callers anywhere in the repo.
  • backend/tests/routers/test_users.py — the three legacy-lane tests are removed together with the code they covered; _task_auth simplified consistently with the NamedTuple change.
  • backend/tests/unit/test_sync_cloud_tasks.py — legacy-audience compat test removed; mocking assert_account_deletion_queue_exists in the startup-guard test is the correct hermeticity fix now that the guard probes a real queue.
  • backend/scripts/firestore_query_coverage_baseline.json — ratchet-safe: 6 fingerprints leave raw_unregistered, none added, unsupported unchanged (113 = 73 + 37 checks out). Non-blocking note: most of the +43 eligible / +49 registered delta is main-branch drift rather than this PR's deletions, so this regen bundles unrelated churn into the diff — fine to keep, just flagging so it isn't read as this PR's effect.
  • backend/utils/task_integrations_ops.py — the _google_tasks_post annotation is unrelated carry-over (as before); harmless.

Once the import is re-dropped and CI is green, this looks ready from my side. Separate from the mechanical fix: the go/no-go on closing the drain window for the legacy OIDC audience lane on the account-deletion wipe endpoint is a maintainer call I'd still like signed off explicitly before merge, since a straggler legacy task will now be rejected at verification or dropped as invalid_payload — the safe outcome, but a product-side timing decision.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

The audience Literal field was removed with the legacy uid wipe-job
lane. Head 97d2f2a reintroduced the unused typing import and fails
pyright reportUnusedImport.

Failure-Class: none

Co-authored-by: Max Carter 祁明思 <max@tsc.hk>
@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_50864737-d870-4d84-8368-18c9ff3c90d1)

The Jules regen of backend/scripts/firestore_query_coverage_baseline.json
was rebase catch-up and now conflicts with origin/main, which blocks
pull_request Actions. Take main's file verbatim (eligible_serving 114,
registered_serving 74). No invented counts.

Co-authored-by: Max Carter 祁明思 <max@tsc.hk>
@Git-on-my-level
Git-on-my-level dismissed stale reviews from themself September 2, 2026 16:41

Dismissed as resolved: the unused Literal import flagged in this review was dropped in 2a265dd and is absent at the current head; the Backend Hermetic Merge Gate is green. Sole blocker resolved.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @undivisible — confirming the current head (767ccbe) resolves everything previously blocking. From my side this is ready.

What I verified at this head:

  • backend/utils/cloud_tasks.py — the unused Literal import is gone (dropped in 2a265dd, resolving the pyright reportUnusedImport failure that was the sole blocker in my two earlier reviews). verify_account_deletion_cloud_tasks_oidc is single-audience only, AccountDeletionTaskAuthentication is down to retry_count, and all three construction sites agree. No new type errors versus the merge base (the one remaining tasks_v2 import diagnostic reproduces identically on main's version of the file — environment artifact, not from this PR).
  • backend/routers/users.py — the handler accepts only job_id payloads and resolves exclusively through resolve_deletion_wipe_job_id; both legacy-audience guard branches are removed together with the lane they protected; the drop log keeps the resolution outcome.
  • backend/database/users.pyresolve_legacy_deletion_wipe_uid and _DELETION_WIPE_LEGACY_ACTIONABLE_STATUSES are gone with no remaining callers anywhere in the repo.
  • backend/tests/routers/test_users.py + backend/tests/unit/test_sync_cloud_tasks.py — the legacy-lane tests are removed together with the code they covered, and _task_auth is simplified consistently with the NamedTuple change. I ran both files at this head: 115 passed. Keeping the assert_account_deletion_queue_exists mock in the startup-guard test is the right hermeticity call now that the guard probes a real queue.
  • backend/scripts/firestore_query_coverage_baseline.json767ccbe restores this byte-identical to current main, so the PR effectively no longer changes it and my earlier baseline-churn note is moot.
  • backend/utils/task_integrations_ops.py — the _google_tasks_post annotation remains unrelated carry-over; harmless.

Both of my earlier change requests were solely the unused-import issue, so I'm dismissing them as resolved. @kodjima33's approval was on 8042130; the delta since is exactly the import fix plus the baseline restore, so re-approval on this head should be the only formality left before merge. Keeping security-review since this tightens auth on a destructive endpoint — the tightening itself is correct: a straggler legacy-uid task now fails verification or drops as invalid_payload rather than executing.

Automated review on behalf of the maintainers.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added positive-signal Good PR — positive signal, not a formal approval and removed needs-maintainer-review Needs a human maintainer to sign off before merge labels Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backend Task (python) positive-signal Good PR — positive signal, not a formal approval security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants