Skip to content

docs: correct issue/PR lifecycle table, fix reminder dedupe #1921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yuanchen8911 merged 4 commits into from
Jul 28, 2026
+31 −13
Merged

docs: correct issue/PR lifecycle table, fix reminder dedupe #1921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a51e79b
docs: correct issue/PR lifecycle table, fix reminder dedupe
yuanchen8911 Jul 27, 2026
b3cbc7f
Merge branch 'main' into docs/reconcile-stale-lifecycle
njhensley Jul 28, 2026
8594ed6
Merge branch 'main' into docs/reconcile-stale-lifecycle
njhensley Jul 28, 2026
343db27
Merge branch 'main' into docs/reconcile-stale-lifecycle
njhensley Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/inactive-pr-reminder.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,14 @@ jobs:
// Target 14-29 day window (before stale bot at 30 days)
if (daysInactive < 14 || daysInactive >= 30) continue;

// Check if we already left a reminder
const { data: comments } = await github.rest.issues.listComments({
// Check if we already left a reminder. Paginate: an unpaginated
// call returns only the 30 oldest comments, so on a busy PR the

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Inline comment asserts an unreachable failure as observed behavior

"so on a busy PR the reminder falls off page one and a duplicate goes out" reads as a described incident. Measured across all 1363 PRs in this repo, the maximum issue-comment count is 20 (#1915) — no PR has ever crossed 30. issues.listComments returns issue comments only; review comments live at pulls/N/comments, which is why GitHub's comments:>30 search qualifier (91 PRs) overcounts for this call. Unreachability is doubled: a duplicate needs >30 issue comments accumulated after the reminder — which itself resets updated_at — plus 14 more days of silence. Production history is 2 reminders ever posted (#485, #676), zero duplicates.

Blast radius: None at runtime. The fix is correct hardening and is cost-neutral (with max 20 comments, paginate issues exactly one request — identical to the pre-change call count). The concern is the durable comment describing a failure mode that has never been reachable here.

Fix: Reframe defensively rather than as an incident report, e.g. "an unpaginated call returns only the 30 oldest comments; paginating removes the bound so the guarantee holds regardless of comment volume."

// reminder falls off page one and a duplicate goes out.
const comments = await github.paginate(github.rest.issues.listComments, {
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pr.number,
per_page: 100,
});
const alreadyReminded = comments.some(c =>
c.user.type === 'Bot' && c.body.includes('inactive for')

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Dedupe marker is a bare literal, now load-bearing for a documented guarantee

The key 'inactive for' is coupled by nothing but convention to the message template on line 83. Reword that template ("idle for", "no activity in") and every previously-reminded PR reads as un-reminded; because the reminder bumps updated_at, each PR re-enters the 14–29 day window ~14 days later and is nudged again on a permanent 14-day cadence. A copy edit silently re-creates the exact defect this PR fixes — while CONTRIBUTING.md is simultaneously upgraded to assert "at most once per pull request" as a guarantee. Converse risk: any other Bot comment containing the substring suppresses the reminder forever (the stale and lock messages don't match, but a bot quoting PR discussion could).

Separately on this line: c.user.type is dereferenced without a guard. GitHub's schema marks issue-comment.user nullable, and there is no try/catch, so a null would kill the whole step. (Low likelihood — GitHub substitutes the ghost account in practice, and I found zero null-user comments in this repo.)

Blast radius: Latent. Requires a future edit to the message text to trigger, but the trigger is an innocuous-looking copy edit and the failure is silent and recurring.

Fix: Hoist a hidden marker — const MARKER = '<!-- aicr:inactive-pr-reminder -->' — append it to the line-83 body and match on it instead of the prose substring. Optionally narrow c.user.type === 'Bot' to c.user?.login === 'github-actions[bot]', which fixes the null deref and the false-positive at the same time.

Expand Down
37 changes: 26 additions & 11 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,32 @@ We welcome the use of AI tools (e.g., GitHub Copilot, ChatGPT, Claude) to help y

Automated bots manage the lifecycle of issues and pull requests:

| Day | Action |
|-----|--------|
| 0 | Issue/PR opened, `needs-triage` label added to issues |
| 14 | Inactive PRs receive a reminder comment |
| 30 | Inactive PRs marked `lifecycle/stale` |
| 44 | Stale PRs auto-closed |
| 60 | Inactive issues marked `lifecycle/stale` |
| 74 | Stale issues auto-closed |
| 90+ | Closed issues/PRs locked |

**To prevent auto-close:** Add the `lifecycle/frozen` label. PRs with `do-not-merge` are also exempt.
| When | Action |
|------|--------|
| On open | `needs-triage` label added to issues |

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — needs-triage row omits that the label is removed automatically

triage.yaml adds the label on issues: opened (:36-45), but the very next step infers an area/* label and strips needs-triage in the same run (:222-236); a separate step removes it on any non-needs-triage label event (:238-257). The inference is five-deep (component field → documentation label → title scope → title text → body text) and effectively always hits for template-created issues. Empirically 0 of 83 open issues currently carry the label.

Blast radius: Doc-only. The row is accurate as far as it goes; it just names an automation step whose immediate automated reversal is absent.

Fix: "| On open | needs-triage added to issues (removed automatically once an area/* label is applied) |"

| 14 days of PR inactivity | PR author receives a reminder comment (once per PR) |
| 30 days of PR inactivity | PR marked `lifecycle/stale` |
| 30 days after being marked stale | Stale PR auto-closed |

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 Nitpick — Two rows share identical "When" cell text

:205 and :207 both read "30 days after being marked stale", disambiguated only by row adjacency.

Blast radius: Cosmetic.

Fix: "30 days after the PR is marked stale" / "30 days after the issue is marked stale".

| 90 days of issue inactivity | Issue marked `lifecycle/stale` |
| 30 days after being marked stale | Stale issue auto-closed |
| 90 days of inactivity while closed | Issue/PR thread locked |

These are **inactivity windows, not days since opening**: any comment or update
restarts the applicable counter — including the day-14 reminder itself, which is
a comment and so pushes the stale mark out.

The day-14 reminder is the exception: it is posted **at most once per pull
request**. The workflow skips any PR that already carries a reminder, so

@ArangoGutierrez ArangoGutierrez Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"At most once per pull request" is very slightly stronger than the workflow guarantees. The listComments call in inactive-pr-reminder.yaml has no per_page and isn't paginated, so it only sees the 30 oldest comments — on a PR that piled up 30+ comments before its first reminder, the reminder falls off page 1, alreadyReminded comes back false, and a second one goes out. Easy to hit here given how chatty the review bots are. Either soften this to "normally once" or add per_page: 100 to that call, which would make the sentence exactly true.

@yuanchen8911 yuanchen8911 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in ce352ca42.

listComments at inactive-pr-reminder.yaml:66 passes no per_page and is not paginated, so it sees only the 30 oldest comments — while the PR-list call at :52 does pass per_page: 100. On a pull request with heavy traffic before day 14, the reminder falls off page one, alreadyReminded returns false, and another reminder goes out; each subsequent one compounds it.

"At most once per pull request" was too strong. It now reads "normally posted only once per pull request", with a clause naming the first-page limitation as the reason a second can occur.

I softened the prose rather than adding per_page: 100 to keep this PR doc-only — the workflow fix is worth doing, and belongs in its own PR for the same reason the stale.yaml change went to #1925 instead of here.

@yuanchen8911 yuanchen8911 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update — folded the workflow fix in rather than documenting the defect, so this reply supersedes my previous one.

inactive-pr-reminder.yaml now paginates:

const comments = await github.paginate(github.rest.issues.listComments, {
  owner: context.repo.owner, repo: context.repo.repo,
  issue_number: pr.number, per_page: 100,
});

One correction to the suggested fix: per_page: 100 on its own would not have made the sentence exactly true. It raises the ceiling from 30 to 100 without removing the failure mode, and this repo has PRs in that range — so the absolute claim would still have been false, just less often. github.paginate removes the bound, which is what lets the prose keep "at most once per pull request" rather than hedging to "normally once".

actionlint passes. The PR now also carries area/ci, and the title and body are updated to reflect that it is no longer doc-only.

Thanks for catching this — the original wording was mine, written this morning while fixing the opposite error (the table implied the reminder recurred), and I overcorrected into an absolute the code did not support.

replying and later going quiet again does not produce a second nudge — the next

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — "the day-30 stale mark" reintroduces the absolute-day framing line 210 just warned against

Substantively correct in its own scenario — after reply-then-quiet the reminder genuinely will not re-fire, so the stale mark is next. But "day-30" reads as a date three lines after the paragraph telling readers these are not days since opening. If the contributor does not reply, the day-14 reminder itself bumps updated_at, so the mark actually lands around day 44. Self-inflicted ambiguity in the one section whose purpose is removing exactly this confusion.

Blast radius: Doc-only; a reader gets two different implied dates from adjacent paragraphs.

Fix: "— the next automated action is the stale mark, 30 days after that reply."

automated action on that PR is the day-30 stale mark.

Each bot runs on a daily schedule, so an action lands on its next scheduled run
rather than the moment a threshold is crossed — expect up to ~24 hours of lag.

**To stay out of the stale process entirely:** Add the `lifecycle/frozen` label.

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — "stay out of the stale process entirely" overclaims — no label exempts locking

lock-threads.yaml has no label check anywhere — its only skips are issue.locked / pr.locked (:58, :87). Nothing exempts a closed thread from the 90-day lock, lifecycle/frozen included, yet the table at :208 lists locking as part of this same lifecycle. A reader who freezes a thread to keep the conversation open still finds it locked 90 days after close.

Also omitted here: both lifecycle/frozen and do-not-merge additionally suppress the day-14 reminder (skipLabels, :44, checked :57), so a contributor with do-not-merge gets no nudge either.

Blast radius: Doc-only, but it is the one place in the new section where a reader can be led to expect protection that does not exist.

Fix: Narrow the heading to "To stay out of stale-marking and auto-close:", note that exempt items are also skipped by the day-14 reminder, and add that no label exempts a closed thread from the 90-day lock.

Exempt items are never marked `lifecycle/stale` in the first place, not merely
spared from closing. `good first issue` also exempts issues, and `do-not-merge`
exempts pull requests.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

### Claiming an Issue

Expand Down
Loading