Skip to content

Enhance validation and reminder logic for pending user response automation - #23020

Open
nil-is-all wants to merge 19 commits into
mainfrom
nil-is-all-patch-9
Open

nil-is-all wants to merge 19 commits into
mainfrom
nil-is-all-patch-9

Conversation

@nil-is-all

Copy link
Copy Markdown
Contributor

Refactor reminder logic to improve user response handling and comment retrieval. Earlier version just printed logs, not closing or removing labels. It also did not fetch comments and detect latest automated reminder well.

…response automation

Refactor reminder logic to improve user response handling and comment retrieval.
Earlier version just printed logs, not closing or removing labels. It also did not fetch comments and detect latest automated reminder well.
@nil-is-all nil-is-all self-assigned this Sep 22, 2026
@nil-is-all nil-is-all added topic: not user facing release notes: none Do not include this in the release notes labels Sep 22, 2026
@pytorch-bot

pytorch-bot Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23020

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 59e5cda with merge base 0a4a05e (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2026
@nil-is-all
nil-is-all requested review from psiddh and removed request for digantdesai September 22, 2026 19:52
indentation and fixed logic for days since last reminder
@nil-is-all

Copy link
Copy Markdown
Contributor Author

@claude check if above comments are resolved by latest commit

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @nil-is-all's task in 1m 17s —— View job


Most of the earlier comments are resolved in ef0fa1b/819e0da, but one blocking problem remains: the follow-up reminder branch runs before the close branch, so on the weekly cron issues get a reminder every week and are effectively never closed. It is also likely that the job lacks write permissions.

Detailed status and remaining issues

PR Review: #23020 (at 68c509f)

  • Read the current pending_user_response.py and .yml
  • Check each earlier item (1–6, A, B, should-fix, nits)
  • Post results

Status of earlier comments

# Item Status
1 Naive vs. aware datetime ✅ Resolved
2 Issues never closed ❌ Regressed, see R1 below
3 @0 in reminder ✅ Resolved
4 Issues with no comments never reminded ✅ Resolved: labeled_at falls back to issue.created_at (line 52)
5 Reopened issues closed again ✅ Resolved: the label is removed on close (line 85)
6 Only the author's reply counts ⚠️ Unchanged, and still no comment saying it is intended
A Re-labeled issues lose the label or get closed at once ✅ Resolved: the logic is now limited to comments after the latest labeled event (lines 44–55)
B Reminders after the author replied / reminder and close in the same run ✅ Half resolved: the reply check now runs first (line 58). The "up to 6 bot comments" half is replaced by R1
— DRY_RUN not set in .yml ✅ Resolved: false for schedule, true for workflow_dispatch (yml line 26)
— Reminder text uses DAYS_BEFORE_REMINDER for the close deadline ❌ Not fixed (line 18)
— f"\nHi @{{0}}..." written as an f-string only to escape braces ❌ Not fixed (line 17). Line 81 print(f"Close issue/PR due to inactivity") is a new f-string with no placeholders (F541)
— Trailing whitespace ❌ New occurrences on lines 46, 48, 59
— Pin PyGithub>=2,<3 Not done (optional)

Code Quality

R1. The close branch can't be reached, so issues are reminded weekly forever (.github/workflows/pending_user_response.py:74-85)
The elif order is: reply check, then first reminder, then follow-up if the last reminder is ≥ 7 days old, then close. The cron runs every 7 days, so at each run reminders[-1] is about 7 days old and the follow-up branch matches. The close branch only runs when the last reminder is < 7 days old and the first one is ≥ 30 days old. That only happens through scheduling jitter (for example, a run that starts a few seconds earlier than last week's gives .days == 6). In practice the issue gets "Please respond… this will be closed in 30 days" every Monday and stays open. This is the same symptom as the original item #2.

Fix: drop the follow-up branch and REMINDER_COOLDOWN_DAYS, as suggested last time. If weekly follow-ups are wanted, move the close check above the follow-up check:

elif (now - reminders[0].created_at).days >= DAYS_BEFORE_CLOSE:
    # close
elif (now - reminders[-1].created_at).days >= REMINDER_COOLDOWN_DAYS:
    # follow-up

Fix this →

Infrastructure

R2. The workflow may not have permission to comment, label or close (.github/workflows/pending_user_response.yml:9)
Every other workflow here that writes to issues or PRs declares permissions: explicitly (stale.yml, docathon-sync-label.yml, pin-bump-ci-handler.yml, claude-code.yml). That suggests the default GITHUB_TOKEN in this repo is read-only. If so, the first scheduled run with DRY_RUN=false will fail with 403 on create_comment, remove_from_labels and edit. Confidence is low because I can't see the repo's token settings. Adding this to the job removes the doubt:

    permissions:
      issues: write
      pull-requests: write

Recommendation

Request Changes. R1 must be fixed before this runs for real: DRY_RUN is now false on the schedule, so next Monday's run will start posting weekly reminders that never lead to a close. R2 should be confirmed or fixed in the same change.

I reviewed by reading the code only. I didn't run linters or change any code, and the GitHub App can't edit files under .github/workflows/ anyway.

Put close block before reminder block to ensure issue is not in a limbo
Added permissions for issues and pull requests.
@nil-is-all

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @nil-is-all's task in 1m 4s —— View job


The latest commits fix R1 (close ordering) and R2 (permissions), but they add a blocking crash: reminders[0] is read before the code checks whether reminders is empty, so the scheduled job will fail on the first labeled issue that hasn't had a reminder yet.

Detailed review (at df8c5ad)

PR Review: #23020

  • Read the diff at HEAD
  • Check the items still open from the last review
  • Post the review

Summary

This PR turns the need-user-input automation from a log-only dry run into a job that reminds, removes the label and closes, scoped to the latest labeled event. Commit 52669aa moved the close branch above the follow-up branch, but it put it above the not reminders branch too, which adds a crash. The same commit also dropped the @author mention from the reminder.

Code Quality

1. IndexError on every issue that hasn't had a reminder yet (.github/workflows/pending_user_response.py:67)

elif (now - reminders[0].created_at).days >= DAYS_BEFORE_CLOSE:   # line 67
    ...
elif not reminders:                                               # line 73

Any issue whose author hasn't replied since the label was added, and that has had no reminder since then, reaches line 67 with reminders == []. Every newly labeled issue is in this state. Nothing catches the exception, so the whole run aborts on the first such issue and no later issue is processed. Because the first reminder is never posted, every Monday's run will fail the same way. Fix it by checking not reminders before the close branch:

if author_replied:
    ...
elif not reminders:
    if (now - labeled_at).days >= DAYS_BEFORE_REMINDER: ...
elif (now - reminders[0].created_at).days >= DAYS_BEFORE_CLOSE:
    ...  # close
elif (now - reminders[-1].created_at).days >= REMINDER_COOLDOWN_DAYS:
    ...  # follow-up

Fix this →

2. The reminder no longer mentions the author (lines 15–19, 77, 82)
52669aa removed Hi @{0}, from REMINDER_COMMENT, so the string has no {0} and .format(user) does nothing. Without an @ mention, GitHub doesn't notify the author, and the author is the person who has to respond. The marker is also now joined straight to the text with no \n. Fix it with a plain (non-f) string such as "\nHi @{0}, this issue/PR has been marked as 'need-user-input'. ". That also removes the F541 warning on line 17.

3. The reminder still states the close deadline with DAYS_BEFORE_REMINDER (line 18)
The close deadline is DAYS_BEFORE_CLOSE. Both are 30 today, so the text is correct for now, but it will be wrong if either value changes. Follow-ups also repeat "in 30 days" each week while the real deadline counts from the first reminder. The weekly text is wrong: the week-4 reminder says 30 days when about 7 are left. Either drop the follow-up branch or leave the day count out of follow-ups.

4. Still open from earlier reviews (minor):

  • Line 58: only the issue author's reply clears the label. If that is intended, add a one-line comment saying so.
  • Line 68: f"Close issue/PR due to inactivity" has no placeholders (F541).
  • Line 86: trailing whitespace.

Recommendation

Request Changes. Item 1 is blocking: DRY_RUN is false on schedule, so next Monday's run will crash before posting anything. Item 2 should be fixed in the same change so the reminders notify the author.

I checked this by reading the code only. I didn't run the script or linters, and the GitHub App can't edit files under .github/workflows/.

added checks for review comments and commits
ensured to check for issues and PRs

This branch was previously deployed

1 inactive deployment
cadence — 59e5cdad Deployed Oct 14, 2026 by nil-is-all
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes topic: not user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants