ci(issues)!: close unlinked stale issues instead of report-only#1417
ci(issues)!: close unlinked stale issues instead of report-only#1417chaodu-agent wants to merge 6 commits into
Conversation
Maintainer decision: an open issue with no linked PR after 14 days signals insufficient priority. Replace the report-only triage scan with comment-and-close (state_reason: not_planned). - Comment tells the reporter to reopen or file a new issue if needed - workflow_dispatch gains a dry_run input for safe verification - Job summary reports per-issue outcome; run fails if any close errors - Permissions raised to issues: write accordingly BREAKING CHANGE: the daily scan now closes matching issues instead of only reporting them.
|
Note LGTM ✅ — Correctly implements the maintainer's closure policy with a static notification comment, What This PR DoesReplaces the report-only stale scan (#1414) with comment-and-close, per explicit maintainer policy: an open issue with no linked PR after 14 days signals insufficient priority. Each candidate receives a comment inviting a reopen or a fresh issue, then is closed as How It WorksSame search query and oldest-first ordering as #1414. For each candidate the script posts a static comment, closes with Findings
Baseline Check
VerifiedOn PR head
5️⃣ Three Reasons We Might Not Need This PR
|
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1417 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| // Pace mutative requests: GitHub's secondary rate limit caps | ||
| // content-generating requests (~80/min); 2 writes per issue | ||
| // at full speed would exceed it at the 50-issue cap. | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
🟡 F1 — Pace each mutation and honor throttling responses
The one-second delay follows two back-to-back content-generating writes. At the 50-item cap, that permits roughly 100 writes in 50 seconds (~120/minute), above GitHub’s documented general limit of 80 content-generating requests per minute. This can trigger the partial-failure path.
Requested change: Pace each write (or wait at least 1.5 seconds per two-write candidate with safety margin) and honor Retry-After/secondary-limit backoff before continuing.
| let outcome = 'dry run — skipped'; | ||
| if (!DRY_RUN) { | ||
| try { | ||
| await github.rest.issues.createComment({ |
There was a problem hiding this comment.
🟡 F2 — Make partial progress and concurrent runs idempotent
If createComment succeeds but issues.update fails or the run is interrupted, the issue remains open and the next run posts the same notification again. A scheduled and manual run can also overlap.
Requested change: Add a stable automation marker, detect an existing notification and retry only the missing close phase, and serialize mutating runs with a repository-scoped workflow concurrency group.
| const CLOSE_COMMENT = [ | ||
| `This issue has been open for more than ${STALE_DAYS} days with no linked pull request, so it is being closed as not planned.`, | ||
| '', | ||
| 'If this is still needed, feel free to reopen it or file a new issue with updated context — a linked PR is the strongest signal that the work matters.', |
There was a problem hiding this comment.
🟡 F3 — Make the reopen promise true
Reopening does not change the issue’s creation date or linkage, so an old reopened issue still matches the next daily state:open -linked:pr created:<... query and is closed again. That contradicts this user-facing recovery instruction.
Requested change: Persist and exclude a handled marker/label that survives reopening, or tell users that a linked PR is required before reopening will persist.
| let closed = 0; | ||
| let failed = 0; | ||
|
|
||
| for (const issue of candidates) { |
There was a problem hiding this comment.
🟡 F4 — Revalidate eligibility immediately before closing
The loop mutates a point-in-time search result. A later candidate can gain a closing-reference PR after the initial search but before its turn, causing the workflow to close an issue whose stated closure reason is no longer true.
Requested change: Immediately before commenting/closing each issue, verify that it remains open and has no closing-reference PR; skip and report candidates whose eligibility changed.
Addresses review round 1 on #1417: - F1: pace every content-generating write (1s) and honor Retry-After backoff on 403/429 throttling responses - F2: label-before-mutate with an auto-closed-stale marker label, a reconciliation phase that finishes interrupted closes without duplicate comments, and a workflow concurrency group serializing scheduled and manual runs - F3: the marker label survives reopening and is excluded from the candidate query, so user-reopened issues are permanently exempt — the notification text now states this - F4: revalidate each candidate via GraphQL immediately before mutating; skip issues that closed or gained a closing-reference PR since the initial search
|
Review round 1 findings addressed in
✅ Addressed — every content-generating request now goes through
✅ Addressed — three mechanisms: (1) a repo-scoped
✅ Addressed — the
✅ Addressed — each candidate is revalidated via GraphQL ( Verified on |
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED â� ï¸� â�� The recovery state machine can still close newly linked issues and cannot durably distinguish interrupted work from reopen exemptions.
Consolidated review: #1417 (comment)
GitHub event: COMMENT � self-review delivery only; this is not an approval.
| ...context.repo, issue_number: issue.number, body: CLOSE_COMMENT, | ||
| })); | ||
| } | ||
| await closeIssue(issue.number); |
There was a problem hiding this comment.
� F1 � Enforce current eligibility at the destructive boundary
Phase 1 reads closedByPullRequestsReferences but checks only state and reopen history before closing. The normal path also performs two paced writes after its linked-PR check. Either path can close an issue that gained a linked PR.
Requested change: Immediately before every closeIssue(), re-read state and linked-PR references and skip unless the issue is still open and unlinked.
| try { | ||
| const live = await revalidate(issue.number); | ||
| if (live.state !== 'OPEN') { continue; } | ||
| if (live.timelineItems.totalCount > 0) { |
There was a problem hiding this comment.
� F2 � Compare reopen ordering, not lifetime event count
timelineItems.totalCount > 0 means the issue was ever reopened, not that it was reopened after this automation closed it. A previously reopened issue interrupted after label creation is therefore misclassified as permanently exempt.
Requested change: Compare ordered event/marker timestamps or persist distinct transaction and exemption states; do not infer ordering from a lifetime count.
| // label present + open means either (a) a human reopened it → | ||
| // permanently exempt, or (b) a previous run was interrupted | ||
| // between labeling and closing → finish the close phase. | ||
| const { data: pending } = await github.rest.search.issuesAndPullRequests({ |
There was a problem hiding this comment.
� F3 � Keep durable exemptions out of the recovery queue
Legitimately reopened issues remain open with the handled label and re-enter this first-page query forever. Fifty exemptions or repeat failures can occupy the page and starve later interrupted transactions.
Requested change: Separate recovery-pending from permanent handled/exempt state, or paginate deterministically under one shared work budget.
| q: query, | ||
| sort: 'created', | ||
| order: 'asc', | ||
| per_page: MAX_ITEMS_PER_RUN, |
There was a problem hiding this comment.
� F4 � Apply one global per-run bound
MAX_ITEMS_PER_RUN is used independently for the pending and new-candidate searches, so one run can process 50 + 50 issues despite the advertised 50-issue safety bound.
Requested change: Use one remaining-item budget shared by both phases and report actual processed and deferred counts.
| state | ||
| closedByPullRequestsReferences(first: 1, includeClosedPrs: true) { totalCount } | ||
| timelineItems(itemTypes: [REOPENED_EVENT], first: 1) { totalCount } | ||
| comments(last: 30) { nodes { body } } |
There was a problem hiding this comment.
� F5 � Make marker lookup authoritative
Looking only at the latest 30 comments is not durable idempotency. If more than 30 comments arrive after an interrupted marker comment, recovery cannot see it and posts a duplicate notice.
Requested change: Paginate/search until the marker is found, or persist transaction state independently of a bounded comment window.
Addresses review round 2 on #1417: - F1 (critical): every close goes through verifyAndClose(), which re-reads state and closedByPullRequestsReferences at the destructive boundary in both recovery and normal paths - F2: reopen exemption decided by event ordering — a REOPENED_EVENT must postdate the transaction anchor (marker comment, else the pending-label event) — not by lifetime reopen count - F3: split states: stale-close-pending marks in-flight transactions (recovery queue contains only interrupted work); auto-closed-stale is applied at successful close and acts as a passive query-excluded exemption when a human reopens — exemptions never occupy the queue - F4: one global MAX_ITEMS_PER_RUN budget shared across all phases; summary distinguishes matched / processed / closed / recovered / exempt / skipped / failed / deferred - F5: marker lookup paginates all comments instead of a bounded window Also adds phase 0 label hygiene for issues closed mid-transaction.
|
Review round 2 findings addressed in
✅ Addressed — all closes now go through a single
✅ Addressed — exemption now requires a
✅ Addressed — states are now split:
✅ Addressed — a single
✅ Addressed — Verified on |
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1417 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| issue(number: $number) { | ||
| state | ||
| closedByPullRequestsReferences(first: 1, includeClosedPrs: true) { totalCount } | ||
| timelineItems(itemTypes: [REOPENED_EVENT, LABELED_EVENT], last: 50) { |
There was a problem hiding this comment.
🔴 F1 — Make the reopen exemption independent of a bounded timeline window
Recovery treats the last 50 reopen/label events as complete history. A valid state is OPEN + stale-close-pending + auto-closed-stale after close and handled-label writes succeed, pending-label removal is interrupted, and a user reopens. If 50 later matching events evict that reopen, reopenedAfterAnchor becomes false; because isEligible() also ignores the handled label, recovery can close the permanently exempt issue again.
Requested change: Read live labels and fail closed whenever HANDLED_LABEL is present (only remove stray pending state), and paginate timeline events back to the transaction anchor or persist an exemption signal that cannot be lost to a bounded window.
| inputs: | ||
| dry_run: | ||
| description: 'Report only, do not comment or close' | ||
| type: boolean |
There was a problem hiding this comment.
🟡 F2 — Require an affirmative choice for manual destruction
The workflow being replaced was report-only even when manually dispatched. With this default, clicking “Run workflow” without changing inputs labels, comments on, and closes up to 50 issues. That is an unsafe default for a destructive maintenance action.
Requested change: Default manual dispatch to dry-run, or use an affirmative execute_closure: false input and mutate only when the operator explicitly enables it. Scheduled runs can remain mutating.
| } | ||
|
|
||
| const processed = MAX_ITEMS_PER_RUN - budget; | ||
| const unqueried = Math.max(result.total - result.items.length, 0); |
There was a problem hiding this comment.
🟡 F3 — Report every recovery outcome and deferred record
unqueried covers only the new-candidate result. Overflow omitted by the Phase 0/1 per_page limits is absent from Deferred, and successful Phase 0 settlements consume budget without incrementing any named outcome. A simulated 60 stuck + 30 pending + 10 new matches reports 11 deferred although 50 remain.
Requested change: Add a settled counter, compute unqueried/deferred totals for stuck, pending, and result, and make the named outcomes reconcile with MAX_ITEMS_PER_RUN - budget.
…ccounting Addresses review round 3 on #1417: - F1 (critical): inspect() now returns live labels and isEligible() fails closed on auto-closed-stale in every destructive gate — an OPEN+PENDING+HANDLED issue only has its stray pending label removed, never re-closed. Reopen-ordering detection now paginates the FULL REST event history instead of a bounded 50-node GraphQL window. - F2: workflow_dispatch dry_run now defaults to true — a bare manual 'Run workflow' is report-only; mutation requires explicitly setting dry_run=false. Scheduled runs remain mutating per policy. - F3: added settled outcome counter; per-phase processed counters and deferred backlog computed from each phase's total matches; run logs warn if named outcomes fail to reconcile with the consumed budget.
|
Review round 3 findings addressed in
✅ Addressed, both halves:
✅ Addressed —
✅ Addressed — added a Verified on |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1417 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| const comments = await github.paginate(github.rest.issues.listComments, { | ||
| ...context.repo, issue_number: number, per_page: 100, | ||
| }); | ||
| return comments.find((c) => c.body?.includes(MARKER)) ?? null; |
There was a problem hiding this comment.
🟡 F1 — Authenticate and transaction-bind the recovery marker
This returns the first public comment containing MARKER without checking its author/application or whether it belongs to the current pending-label transaction. A stale or user-supplied marker can suppress the official notice and can move the reopen anchor before an unrelated historical reopen.
Requested change: Select only a marker created by the expected GitHub Actions identity/application at or after the latest stale-close-pending label event, with tests for stale/spoofed, trusted-current, and missing markers.
| // label (reopened by a human, or settlement interrupted) is | ||
| // permanently exempt — only tidy up the stray pending label. | ||
| if (live.labelNames.includes(HANDLED_LABEL)) { | ||
| counts.exempt += 1; |
There was a problem hiding this comment.
🟡 F2 — Keep terminal outcomes mutually exclusive
exempt is incremented before pending-label cleanup. If that write throws, the outer catch also increments failed, so one processed item contributes two outcomes; the reopened and linked-PR branches have the same pattern. In addition, a successful close followed by settlement failure is reported only as failed.
Requested change: Assign one terminal primary outcome after required operations settle, track cleanup debt separately, and preserve successful-close state when label settlement must be recovered.
… accounting Addresses review round 4 on #1417: - F1: findTrustedMarker() accepts only comments authored by github-actions[bot] and, when the transaction's pending-label event exists, only markers at or after it — a stale or user-pasted marker string can neither suppress the official notice nor shift the reopen-exemption anchor. The pending-label event is now the primary anchor; the trusted marker is a fallback only. - F2: every processed item lands in exactly one terminal outcome. Cleanup writes after a terminal decision go through tryCleanup(), which never throws and accumulates a separately reported cleanupDebt (recovered by phases 0/1 next run). verifyAndClose() returns closed/closed-unsettled/ineligible so a successful close with failed label settlement is still reported as closed. The accounting invariant (outcomes == processed) now fails the run instead of warning.
|
Review round 4 findings addressed in
✅ Addressed —
✅ Addressed — restructured along the "structured results + separate cleanup debt" option:
Verified on |
|
Important CHANGES REQUESTED What This PR DoesThis PR replaces the report-only daily scan for issues older than 14 days without a closing-reference pull request with a workflow that labels, notifies, and closes matching issues as How It WorksThe workflow settles closed interrupted transactions, recovers open issues carrying Findings
Finding Details🟡 F1: Roll back side effects when the final close gate rejects the issuePhase 2 first verifies eligibility, applies Requested change: Capture the created comment ID and, when the final gate returns 🟡 F2: Preserve the existing
|
| Reviewer | Angle | Outcome |
|---|---|---|
| Reviewer A | Correctness/state machine | Retained the false-notice/pending-label rollback gap (F1); confirmed round-4 outcome accounting. |
| Reviewer B | GitHub API/security | Confirmed marker provenance/binding; rejected the claimed -linked:pr semantic mismatch because GitHub documents it as closing-reference linkage. |
| Reviewer C | Operability/integration | Identified the uncoordinated closing-soon lifecycle (F2) and stale PR contract (F3). |
| Reviewer D | Independent audit | Rejected speculative timeout and incomplete-search concerns; retained only reproducible current-head issues. |
Baseline Check
- PR opened: 2026-07-16.
- Main already has: a 72-line report-only daily scan using the same 14-day/no-closing-reference policy, plus the label-driven
stale-issue.yml→close-stale-issues.ymlwarning/closure lifecycle. - Net-new value: automatic enforcement through notification and
not_plannedclosure, with paced writes, crash recovery, durable state labels, live eligibility checks, safe manual preview, and operator reporting.
Verified
- Reviewed exact head:
2d6ef8e5d8fc321a5ec13556e842a0d4921c25aeagainstorigin/mainat30bc145233a202754d205b21e8529491d97b1a7c. git diff --check: pass.- Exact-SHA checks:
check✅ andpoll-and-review✅. - Official GitHub search documentation confirms
linked:prmeans linkage by a closing reference, matchingclosedByPullRequestsReferences; no semantic-gap finding was retained. - Local limitation: this runner has no Node, Python, Ruby,
actionlint, oryq, so syntax parsing was not independently rerun; successful exact-SHA CI provides the available syntax validation.
What's Good (🟢)
- Mutation permission is scoped to
issues: write, and runs of this workflow are serialized. - Manual dispatch now defaults to report-only behavior.
- The destructive boundary re-reads issue state, closing-reference PRs, and handled labels.
- Comments and issue events paginate fully; ordinary writes are paced and honor
Retry-Afteronce. - Trusted marker binding, the two-label transaction, cleanup-debt reporting, and the enforced accounting invariant are substantial improvements over the initial implementation.
5️⃣ Three Reasons We Might Not Need This PR
- The report-only workflow was just introduced — maintainers could use its output for manual triage before automating destructive state changes.
- Creation age is not inactivity or lack of value — an old issue can remain active, strategic, or blocked without a closing-reference pull request.
- The repository already has a label-driven stale lifecycle — extending one explicit lifecycle may be easier to reason about than maintaining two independent closure authorities.
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED closing-soon grace period, and the PR description no longer matches the safe manual default.
Consolidated review: #1417 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| ...context.repo, issue_number: issue.number, body: CLOSE_COMMENT, | ||
| })); | ||
| const outcome = await verifyAndClose(issue.number); | ||
| if (outcome === 'ineligible') { |
There was a problem hiding this comment.
🟡 F1 — Roll back side effects when the final close gate rejects the issue
After the workflow has applied stale-close-pending and posted “it is being closed,” this branch records an eligibility-change skip but leaves both side effects behind. Phase 1 may remove the label on a later run, but the false closure notice remains permanently.
Requested change: Capture the transaction comment ID and, on ineligible, immediately best-effort remove the pending label and delete or correct that notice; track rollback failures as cleanup debt.
| } | ||
|
|
||
| // ── Phase 2: process new candidates ── | ||
| const query = `${repoQ} is:issue state:open -linked:pr -label:${HANDLED_LABEL} -label:${PENDING_LABEL} created:<${cutoff}`; |
There was a problem hiding this comment.
🟡 F2 — Preserve the existing closing-soon grace contract
This query can select an issue already carrying closing-soon. The existing lifecycle publicly promises a response window and closes only after its own label-age gate, but this workflow can close the issue solely from creation age before that grace expires. Its separate concurrency group also does not coordinate the two writers.
Requested change: Exclude closing-soon here and make its live presence fail closed at the destructive gate, or explicitly replace the old lifecycle in this PR so the repository has one non-contradictory close authority.
2 participants
Summary
state_reason: not_planned.workflow_dispatchgains adry_runboolean input (defaultfalse) to preview candidates without mutating anything; scheduled runs always close.report-unlinked-stale-issues.ymlis removed (renamed toclose-unlinked-stale-issues.yml).The daily 09:30 UTC scan now closes matching issues instead of only reporting them. Current candidate set: 17 issues (verified via live query). Closed issues remain searchable and can be reopened at any time.
Safety
issues: writescoped to this job only; comment body is static (no user-controlled interpolation).dry_rundispatch input for safe verification after merge.Validation
node --check.