🧹 Remove legacy uid branch for wipe job - #12490
Conversation
- 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>
|
👋 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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
left a comment
There was a problem hiding this comment.
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 onlyjob_idpayloads and resolves exclusively throughresolve_deletion_wipe_job_id; thetask_authentication.retry_countterminal-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.py—verify_account_deletion_cloud_tasks_oidcno longer falls back to the sync OIDC audience, andAccountDeletionTaskAuthenticationis reduced toretry_count. Good simplification of the auth surface.backend/database/users.py—resolve_legacy_deletion_wipe_uidand_DELETION_WIPE_LEGACY_ACTIONABLE_STATUSESare 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 mockingassert_account_deletion_queue_existsin 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:19—Literalis still imported but no longer used; theaudience: Literal[...]field was its only consumer. This is exactly what is failing the Backend unit suite job (pyrightreportUnusedImport); the pytest preflight itself passed before the typecheck step errored. DroppingLiteralfrom thetypingimport should turn the run green.
Minor notes (non-blocking):
backend/utils/task_integrations_ops.py— the_google_tasks_postannotation 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-
uidtask is rejected at verification or dropped asinvalid_payloadrather 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.
kodjima33
left a comment
There was a problem hiding this comment.
Approve only: dead-code removal/refactor, and Backend unit suite CI check is failing — hard floor blocks merge.
Bugbot couldn't run - usage limit reachedBugbot 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) |
|
Addressed Git-on-my-level pyright CR: dropped unused |
- 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
left a comment
There was a problem hiding this comment.
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:19—Literalis back infrom typing import Any, Dict, Literal, NamedTuple, Optional, but the removedaudience: 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; droppingLiteralfrom 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.py—verify_account_deletion_cloud_tasks_oidcis single-audience only (no sync-audience fallback), andAccountDeletionTaskAuthenticationis justretry_count. Correct and strictly tighter. Only the import issue above.backend/routers/users.py— handler accepts onlyjob_id; both legacy-audience guard branches are gone together with the lane they protected; the drop log keeps the resolution outcome. No residuallegacy_sync/payload_kind/resolve_legacy*references at this head.backend/database/users.py—resolve_legacy_deletion_wipe_uidand_DELETION_WIPE_LEGACY_ACTIONABLE_STATUSESremoved 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_authsimplified consistently with the NamedTuple change.backend/tests/unit/test_sync_cloud_tasks.py— legacy-audience compat test removed; mockingassert_account_deletion_queue_existsin 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 leaveraw_unregistered, none added,unsupportedunchanged (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_postannotation 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>
Bugbot couldn't run - usage limit reachedBugbot 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>
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.
|
Thanks @undivisible — confirming the current head (767ccbe) resolves everything previously blocking. From my side this is ready. What I verified at this head:
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 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 |
🎯 What: Removed the legacy
uidbranch 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 thejob_idmigration 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_idflow.Failure-Class: none
PR created automatically by Jules for task 62989738989302225 started by @undivisible
Note
Medium Risk
Changes authentication and payload handling on the destructive account-deletion worker path; any in-flight legacy
uidtasks 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 legacyuidbranch,resolve_legacy_deletion_wipe_uid, and related Firestore helper constants are removed.OIDC verification for deletion tasks no longer tries the sync
legacy_syncaudience or returns anaudienceonAccountDeletionTaskAuthentication—only the dedicated account-deletion handler URL is accepted. Cross-checks that paired payload shape with audience are gone fromrun_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.