Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility script, scripts/cleanup_inactive_prs.py, designed to automate the lifecycle management of inactive pull requests within the firebase-tools repository. The script utilizes GitHub GraphQL to identify inactive PRs, apply warning labels, and close them after a grace period. The review feedback provides actionable improvements to enhance the script's robustness, including the implementation of a safe nested dictionary getter to handle potential null values in API responses, more rigorous error handling for subprocess execution, and validation of GraphQL response payloads to prevent runtime failures.
|
/joe-review |
- Remove unused isDraft field from GraphQL PRS_QUERY. - Increase timeline items window to last: 100 to prevent label event truncation. - Validate GraphQL response for errors and null payloads. - Filter bot users in get_last_activity and omit updatedAt from inactivity calculation. - Guard parse_iso against None before sorting dates. - Add get_nested() helper for safe dictionary traversal on GitHub API responses. - Check returncode on gh pr edit --remove-label and gh pr close before proceeding. - Read workflow inputs from environment variables as defaults in cleanup_inactive_prs.py.
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Have a whole lot of AI feedback that sounds plausible :)
| author { | ||
| login | ||
| } | ||
| timelineItems(itemTypes: [LABELED_EVENT, ISSUE_COMMENT, PULL_REQUEST_COMMIT], last: 100) { |
There was a problem hiding this comment.
🔴 [Architecture & Lifecycle Asymmetry] Unify Phase 1 and Phase 2 activity detection (or include PULL_REQUEST_REVIEW and isolate LabeledEvent)
Rationale:
- Missing
PULL_REQUEST_REVIEWin Phase 1: Phase 2 (get_last_activity) treats PR reviews as valid activity, butTIMELINE_QUERYonly fetches[LABELED_EVENT, ISSUE_COMMENT, PULL_REQUEST_COMMIT]. If an author or reviewer submits a PR review during the 5-day grace period without leaving a top-level issue comment or pushing a commit,process_existing_inactive_prsignores it and closes the PR. LabeledEventEviction Hazard: Sharing a singlelast: 100window acrossLABELED_EVENT,ISSUE_COMMENT, andPULL_REQUEST_COMMITmeans a rebase or merge with >=100 commits after labeling evicts theLabeledEvent, causing lines 345–347 (if not label_events: continue) to leave theinactivelabel stuck on an active PR.- Simpler Single-Query Architecture: Because any activity after
latest_label_dtguaranteesmax(activity_dt) > latest_label_dt, you can includelabeledEvents: timelineItems(itemTypes: [LABELED_EVENT], last: 10)directly inPRS_QUERY, compareget_last_activity(pr)[1] > latest_label_dtin Phase 1, and deleteTIMELINE_QUERY(and its N+1 API calls) altogether.
Suggested Fix (if keeping TIMELINE_QUERY, separate labeledEvents and add PULL_REQUEST_REVIEW):
| timelineItems(itemTypes: [LABELED_EVENT, ISSUE_COMMENT, PULL_REQUEST_COMMIT], last: 100) { | |
| labeledEvents: timelineItems(itemTypes: [LABELED_EVENT], last: 10) { | |
| nodes { | |
| ... on LabeledEvent { | |
| createdAt | |
| label { | |
| name | |
| } | |
| } | |
| } | |
| } | |
| timelineItems(itemTypes: [ISSUE_COMMENT, PULL_REQUEST_COMMIT, PULL_REQUEST_REVIEW], last: 100) { |
| if review_submitted_at: | ||
| dt = parse_iso(review_submitted_at) | ||
| if dt is not None: | ||
| dates.append(("review", dt)) |
There was a problem hiding this comment.
🔴 [Activity Detection Bug] Filter out bots and iterate reversed(reviews) in get_last_activity
Rationale:
- Above on lines 257–264,
commentsiterates in reverse and filters outBOT_USERS(which includes"gemini-code-assist"). However,reviewsonly checksreviews[-1]without checking the review author or skippingPENDINGreviews (submittedAt: null). Becausegemini-code-assistsubmitsPullRequestReviewobjects, an automated bot review atreviews[-1]will falsely reset the inactivity clock, or mask an earlier human review atreviews[-2]. Also consider selectingauthor { __typename login }on comments and reviews in GraphQL and excluding any actor with__typename == "Bot"so other bots in this repo (e.g.,codecov,dependabot[bot]) are automatically excluded. - Caveat on Commit Author Filtering: If you also filter
BOT_USERSon commits (lines 248–254 and 364–368), note two edge cases:- In GitHub GraphQL,
commit.author.userisnullwhenever a contributor's git commit email is not linked to a GitHub account (get_nested(..., "login")returnsNone). TreatNone/""as valid human activity (c_author not in BOT_USERS), neverif c_author and .... - For Dependabot PRs, when Dependabot rebases a PR during the grace period (
@dependabot rebase), the new commit's author isdependabot[bot]. Do not filter outdependabot[bot]on commits, or rebased Dependabot PRs will still be closed.
- In GitHub GraphQL,
Suggested Fix:
| dates.append(("review", dt)) | |
| reviews = get_nested(pr, "reviews", "nodes", default=[]) | |
| for r in reversed(reviews): | |
| r_author = get_nested(r, "author", "login") | |
| review_submitted_at = r.get("submittedAt") | |
| if r_author not in BOT_USERS and review_submitted_at: | |
| dt = parse_iso(review_submitted_at) | |
| if dt is not None: | |
| dates.append(("review", dt)) | |
| break |
There was a problem hiding this comment.
Fixed in 1c6a0e8: Added author { __typename login } to GraphQL queries. get_last_activity now iterates reversed(reviews) skipping bot accounts (BOT_USERS and actor __typename == "Bot") and pending reviews (submittedAt: null). Also treated unlinked commit author emails (author.user == null) as valid human activity and allowed commits by dependabot[bot] on Dependabot PRs.
|
|
||
| # 1. Autonomous AI improvement tasks (Buganizer component 2268168) | ||
| if head.startswith("ai-improve-") or "[AI Improvement]" in title or (author == "joehan" and "[Task]" in title): | ||
| return True, "Active AI Improvement task (Buganizer component 2268168)" |
There was a problem hiding this comment.
🔴 [Scrub AI/Agent Artifacts] Remove hardcoded internal Buganizer component ID and personal maintainer handle
Rationale: Public repository automation in firebase/firebase-tools should not hardcode internal Google Buganizer component numbers (2268168), internal task prefixes (ai-improve-*, [AI Improvement]), or personal GitHub usernames (author == "joehan"). EXEMPT_LABELS (DO NOT MERGE, ongoing, pinned) on lines 291–296 already provides a clean, repo-standard mechanism to exempt long-running PRs.
Suggested Fix: Have automated task workflows apply an exempt label (such as ongoing) when opening PRs, and simplify is_protected_pr to check EXEMPT_LABELS.
There was a problem hiding this comment.
Fixed in 1c6a0e8: Removed all internal Buganizer component ID (2268168), ai-improve-* branch patterns, [AI Improvement] title prefixes, and maintainer handle checks from the script and docstrings. PR protection is now strictly driven by repository EXEMPT_LABELS (DO NOT MERGE, ongoing, pinned).
|
|
||
| pr_num = pr["number"] | ||
| author = get_nested(pr, "author", "login", default="ghost") | ||
|
|
There was a problem hiding this comment.
🔴 [Lifecycle Safety] Check is_protected_pr(pr) in process_existing_inactive_prs
Rationale: tag_new_inactive_prs skips protected PRs via is_protected_pr(pr) (line 442), but process_existing_inactive_prs never checks it. If a maintainer applies DO NOT MERGE, ongoing, or pinned to an already-tagged inactive PR during the 5-day grace period without leaving a comment, Phase 1 will still close the protected PR once days_labeled >= grace_days.
Suggested Fix:
| pr_num = pr["number"] | |
| author = get_nested(pr, "author", "login", default="ghost") | |
| protected, reason = is_protected_pr(pr) | |
| if protected: | |
| print(f" #{pr_num} by @{author}: Protected ({reason}). Skipping closure.") | |
| continue |
There was a problem hiding this comment.
Fixed in 1c6a0e8: Added is_protected_pr(pr) check in process_existing_inactive_prs. If a PR carries an exempt label, it skips closure and removes the inactive label so it does not linger in an inactive state.
| check=False, | ||
| ) | ||
| if res_com.returncode != 0: | ||
| print(f" Warning: Failed to comment on #{pr_num}: {res_com.stderr.strip()}", file=sys.stderr) |
There was a problem hiding this comment.
🔴 [Partial Mutation Hazard] Post warning comment before applying inactive label
Rationale: tag_new_inactive_prs runs gh pr edit --add-label inactive (line 480) before gh pr comment (line 504). If posting the comment fails due to a transient API error or secondary rate limit, the script logs a warning and leaves the inactive label on the PR—causing Phase 1 to close the PR 5 days later without the contributor ever receiving the warning comment.
Suggested Fix: Post the warning comment first and only apply the inactive label after gh pr comment succeeds (or roll back the label if commenting fails).
There was a problem hiding this comment.
Fixed in 1c6a0e8: Reordered mutation operations in tag_new_inactive_prs to execute gh pr comment first. If posting the comment fails, the error is logged and label application is skipped, preventing untracked inactive labels.
| description: "Days before closing tagged inactive PRs (default: 5)" | ||
| required: false | ||
| type: string | ||
| default: "5" |
There was a problem hiding this comment.
🟡 Nit: Use type: number for numeric workflow_dispatch inputs and set timeout-minutes: 15
Rationale: GitHub Actions supports type: number for workflow_dispatch inputs (inactive_threshold_days, grace_period_days, tag_batch_limit), validating numeric inputs in the UI. Also consider adding timeout-minutes: 15 to the cleanup job (line 47) and dropping the redundant GITHUB_TOKEN env var on line 50 (gh only uses GH_TOKEN).
| default: "5" | |
| type: number | |
| default: 60 |
There was a problem hiding this comment.
Fixed in 1c6a0e8: Updated inactive_threshold_days, grace_period_days, and tag_batch_limit to type: number with numeric defaults in .github/workflows/cleanup-inactive-prs.yml, added timeout-minutes: 15 to the cleanup job, and removed the duplicate GITHUB_TOKEN environment variable mapping.
|
|
||
| CLOSURE_COMMENT = """Closing this pull request after {grace_days} days of inactivity following the `{label}` notification. | ||
|
|
||
| Thank you for contributing to `firebase-tools`! If you would like to revisit this change or continue work, please rebase against the latest `main` branch and feel free to reopen or open a new pull request. We appreciate your time!""" |
There was a problem hiding this comment.
🔴 [Reopen Loop & GitHub Permissions] Remove inactive on close, count REOPENED_EVENT / UNLABELED_EVENT as activity, and fix rebase/reopen instructions
Rationale:
- Reopen Loop:
gh pr close(line 405) leaves theinactivelabel attached when closing a PR. If a maintainer clicks Reopen without posting a comment, the next daily run evaluates the PR in Phase 1, ignoresREOPENED_EVENT, computesdays_labeled >= 5from the original pre-closureLabeledEvent, and immediately closes the newly reopened PR again with zero grace period. - Manual Unlabel Loop: If a maintainer manually removes the
inactivelabel in the GitHub UI (either during the 5-day grace period or when reopening) without leaving a comment,get_last_activityignoresUNLABELED_EVENTandREOPENED_EVENT, so Phase 2 seesdays_inactive >= 60and immediately re-appliesinactiveon the next daily run. - GitHub Permission & Rebase Lockout: When
GITHUB_TOKENcloses a PR, external contributors without write/triage access cannot click "Reopen" themselves—and if they rebase/force-push their branch while the PR is still closed as instructed here, GitHub permanently locks the PR so even maintainers cannot reopen it.
Suggested Fix:
- Remove the
inactivelabel when closing a PR (or includeREOPENED_EVENTandUNLABELED_EVENTforINACTIVE_LABELin activity detection). - Update the closure message:
| Thank you for contributing to `firebase-tools`! If you would like to revisit this change or continue work, please rebase against the latest `main` branch and feel free to reopen or open a new pull request. We appreciate your time!""" | |
| Thank you for contributing to `firebase-tools`! If you would like to continue working on this change, please leave a comment asking a maintainer to reopen this pull request (before rebasing or force-pushing), or feel free to open a new pull request. We appreciate your time!""" |
There was a problem hiding this comment.
Fixed in 1c6a0e8:
- Phase 1 now removes the
inactivelabel prior to closing a PR. - Added
REOPENED_EVENTandUNLABELED_EVENTtotimelineItemsinPRS_QUERYandTIMELINE_QUERY, recording them as fresh activity inget_last_activityandprocess_existing_inactive_prsto eliminate the reopen and manual unlabel loops. - Updated
CLOSURE_COMMENTto advise contributors to ask a maintainer to reopen before rebasing or force-pushing to prevent permanent lockouts.
| totalCount | ||
| nodes { | ||
| commit { | ||
| committedDate |
There was a problem hiding this comment.
🔴 [Git Timestamp Bug] Query pushedDate alongside committedDate on Commit nodes
Rationale: Both PRS_QUERY (line 84) and TIMELINE_QUERY (line 136) only inspect commit.committedDate (GIT_COMMITTER_DATE), which records when the commit was created locally in Git, not when it was pushed to GitHub. If a contributor responds to an inactive warning by pushing local commits that they committed on their machine prior to latest_label_dt (or rebasing with --committer-date-is-author-date, or force-pushing to drop a broken commit), c_dt > latest_label_dt (line 366) evaluates to False and Phase 1 closes the active PR anyway.
Suggested Fix: Query pushedDate alongside committedDate on commit and use max(pushedDate, committedDate) when pushedDate is non-null:
| committedDate | |
| committedDate | |
| pushedDate |
| for pr in labeled_prs: | ||
| if close_limit > 0 and closed_count >= close_limit: | ||
| print(f"Reached close limit of {close_limit} PRs. Stopping Phase 1.") | ||
| break |
There was a problem hiding this comment.
🔴 [Phase 1 Starvation] Move close_limit check inside if days_labeled >= grace_days: so active PRs are still unlabeled
Rationale: if close_limit > 0 and closed_count >= close_limit: break sits at the top of for pr in labeled_prs: (lines 325–327), before if new_activity: (line 370). Because all_prs is ordered by CREATED_AT ASC (oldest first), once close_limit (50) older expired PRs are closed, Phase 1 breaks out of the loop completely and skips removing the inactive label from newer PRs where contributors have already responded.
Suggested Fix: Move the close_limit check inside if days_labeled >= grace_days: (line 397) and skip only closures (continue), so if new_activity: always removes inactive from active PRs regardless of close_limit.
| } | ||
| } | ||
| } | ||
| reviews(last: 5) { |
There was a problem hiding this comment.
🟡 [GraphQL Slice Eviction] Increase labels(first: 20), comments(last: 10), and reviews(last: 5) slices
Rationale:
- In GitHub's GraphQL schema, every inline diff reply creates a
PullRequestReviewnode (state: COMMENTED). Fetching onlyreviews(last: 5)andcomments(last: 10)and filtering bots afterward in Python means >=5 bot review comments or >=10 bot issue comments (includingdependabot[bot], which should also be added toBOT_USERSfor comment filtering) evicts the latest human activity from the window. - Similarly,
labels(first: 20)(line 66) truncates on PRs with >20 labels, which can hideEXEMPT_LABELSorinactive.
Suggested Fix: Increase the slices in PRS_QUERY to labels(first: 100), comments(last: 50), and reviews(last: 50).
… workflow - Filter out bot accounts and iterate reversed reviews in get_last_activity to skip pending reviews. - Treat unlinked commit author emails as valid human activity and count Dependabot commits on Dependabot PRs. - Scrub internal Buganizer component ID and author handle; enforce protection strictly via EXEMPT_LABELS. - Check is_protected_pr in Phase 1 before closing; skip closure and remove inactive label if protected. - Post warning comment before applying inactive label in Phase 2 to prevent untracked labels. - Remove inactive label upon PR closure, query REOPENED_EVENT and UNLABELED_EVENT as activity, and update closure comment to instruct asking a maintainer to reopen before rebasing. - Validate CLI/env input bounds and exit non-zero (1) on mutation failures. - Pass GraphQL variables for owner and repo instead of manual string template replacement. - Extract DEPENDABOT_USERS constant and avoid pinging @ghost in greeting. - Use type: number for numeric workflow inputs, set 15m job timeout, and remove redundant token env var.
|
This is a lot of code that I primarily looked at with AI. I could do a much more thorough human review if you want. But I think we could also instead change a few things to test this more manually as a way to get confidence. What if we don't start this running by default on cron without a dry run (as is current) but check the workflow in with it defaulting to dry run and needing to be manually triggered. We could then go a few weeks of taking a few minutes to do a dry run and if things look good a manual real run. If we don't see any issues or get any complaints we care about then after gaining confidence (and fixing any issues) we turn it into a real cron that modifies all the open PRs on its own. |
Remove the daily cron trigger so the workflow only runs via workflow_dispatch while we validate it with manual runs. Leave a TODO to re-enable the schedule.
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Still probably a good idea to default to dry_run for now, but approving as is while it's all a manual workflow. We can play around and work out any kinks.
| __typename | ||
| login | ||
| } | ||
| timelineItems(itemTypes: [LABELED_EVENT, UNLABELED_EVENT, REOPENED_EVENT, ISSUE_COMMENT, PULL_REQUEST_COMMIT], last: 100) { |
There was a problem hiding this comment.
Nit (non-blocking): TIMELINE_QUERY (and the Phase 1 loop at lines 431–452) still omits PULL_REQUEST_REVIEW (... on PullRequestReview { submittedAt author { __typename login } }), so a maintainer or contributor submitting a PR review during the 5-day grace period won't remove the inactive label or prevent auto-closure. (Also, commits(last: 1) on line 88 could be bumped to commits(last: 10) and iterated in reverse in get_last_activity so a bot commit at the tip of a human PR doesn't hide prior human commits.) Fine to address in a follow-up if you touch this script again.
This branch has not been deployed
3 participants
Description
Sets up a tag-based lifecycle to manage inactive pull requests on
firebase-tools:inactivelabel, posting a comment letting contributors know they have 5 days to respond.DO NOT MERGE,ongoing,pinned).inactive.inactivelabel was added, the label is removed.Includes:
.github/workflows/cleanup-inactive-prs.yml: GitHub Actions workflow with a manualworkflow_dispatchtrigger (supports dry run and customizable thresholds). For now this is manual-only — there is a TODO to enable a daily cron schedule once we've validated the behavior through manual runs.scripts/cleanup_inactive_prs.py: Zero-dependency, standalone CLI and runner script using the GitHub GraphQL API.