Add stale unassign GitHub Action #31
Conversation
- Automatically unassigns stale issues after grace period - Checks for open PRs authored by assignee - Supports minutes (testing) and days (production) - Tested with 5-minute grace period in talawa-admin - Includes retry logic and dry-run capability Resolves PalisadoesFoundation#24
WalkthroughAdds a composite GitHub Action and a scheduled workflow to unassign issues labeled as stale after a configurable grace period, with checks for archived repos, exclusion labels, open linked PRs, dry-run support, retry/backoff, and templated comments. Files added: Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as GitHub Scheduler
participant Workflow as .github/workflows/stale-unassign.yml
participant Action as .github/actions/stale-unassign
participant API as GitHub API
participant Repo as Repository
Scheduler->>Workflow: Trigger (daily / manual)
Workflow->>Action: Start with inputs (grace period, dry-run, etc.)
Action->>API: Get repository info (archived?)
API-->>Action: Repo status
Action->>API: Ensure stale & exclusion labels exist
API-->>Action: Label presence
Action->>API: List open issues with stale label (max cap)
API-->>Action: Issues list
loop For each issue
Action->>API: Get issue details & timeline events
API-->>Action: Details & events
Action->>Action: Detect open linked PRs and authors
alt Blocking open linked PR by assignee exists
Action->>Action: Skip issue
else No blocking PR
Action->>Action: Compute time since stale label added
alt Grace period exceeded
Action->>API: Remove assignees (or log dry-run)
API-->>Action: Confirmation
Action->>API: Post templated comment (or log dry-run)
API-->>Action: Comment created
else Grace period not exceeded
Action->>Action: Skip issue
end
end
end
Action-->>Workflow: Emit summary (processed/skipped/errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/actions/stale-unassign/action.yml:
- Around line 69-75: The Retry-After handling block currently assumes a numeric
value and uses parseInt which yields NaN for HTTP-date strings; update the logic
inside the same conditional (the block that reads retryAfter from
error.response.headers['retry-after']) to detect whether retryAfter is numeric
and, if not, parse it as an HTTP-date (using Date.parse) to compute
milliseconds; ensure you only call Math.max with valid numbers (fallback to
existing waitTime if parsing fails) so setTimeout never receives NaN.
In @.github/workflows/stale-unassign.yml:
- Around line 49-52: Update the example comment-template to remove the extra '@'
in the placeholder so it doesn't produce a duplicate at-sign; replace the
current placeholder syntax @{assignee} with {assignee} in the comment-message
example (referencing the comment-message key in the workflow) so the action,
which adds '@' itself, will render a single `@username`.
🧹 Nitpick comments (2)
.github/actions/stale-unassign/action.yml (2)
121-125: Consider throwing an error for invalid stale-label instead of silent return.When
stale-labelis empty, the script returns silently and the workflow succeeds. This could mask configuration issues. Throwing an error would make misconfiguration more visible.Suggested fix
// Validate stale-label is non-empty if (!staleLabel || staleLabel.trim() === '') { - console.error('Error: stale-label input is empty. Cannot proceed.'); - return; + throw new Error('stale-label input is empty. Cannot proceed.'); }
323-326: Consider tracking comment failures in error metrics.Comment errors are logged but not reflected in
errorCount, while assignee removal errors (lines 301-306) do increment it. If comment failures become frequent, they'd be invisible in the summary report.Optional: Track comment errors
+ let commentErrorCount = 0; + // ... (at top of script) + } catch (error) { console.error(`Issue #${issue.number}: Error posting comment: ${error.message}`); - // Don't fail the whole workflow for a comment error + commentErrorCount++; }Then include in the summary table.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/stale-unassign/action.yml.github/workflows/stale-unassign.yml
🔇 Additional comments (6)
.github/actions/stale-unassign/action.yml (4)
1-34: LGTM - Well-designed action inputs.Good security practice reading inputs via environment variables to prevent script injection. The
grace-period-minutesfor testing with override behavior is a thoughtful addition for development workflows.
222-249: LGTM - Solid linked PR detection logic.Correctly checks all required criteria: same repository, PR is open (ignoring closed/merged), and authored by one of the assignees. The logging for non-matching PRs aids debugging.
260-283: LGTM - Grace period calculation handles re-labeling correctly.Using the most recent label event handles edge cases where the stale label was removed and re-added, resetting the grace period appropriately.
338-349: LGTM - Good use of job summary for visibility.The summary table provides clear metrics for each run, making it easy to monitor the action's effectiveness over time.
.github/workflows/stale-unassign.yml (2)
30-33: LGTM - Appropriately scoped permissions.Minimal permissions following principle of least privilege:
issues: writefor unassignment and commenting,contents: readfor checkout,pull-requests: readfor linked PR detection.
40-48: LGTM - Clean action invocation.The checkout step correctly precedes the local action reference, and input fallbacks (
|| '0',|| '7') properly handle scheduled runs wheregithub.event.inputsis null.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Enhanced Retry-After header parsing to support both numeric (seconds) and HTTP-date formats
- Added validation to prevent NaN from being passed to setTimeout
- Fixed duplicate '@' in comment-message example by removing '@' from {assignee} placeholder
|
@palisadoes pls review i have tested it fully |
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
Please explain how this works.
- There are lots of inputs
- It's not clear whether the grace period is after the label is found, though I assume it is.
|
@palisadoes |
|
@palisadoes pls review and tell me if any more change is required |
|
Please update the script so that the Action isn't activated for any one of a list of labels not just the |
- Renamed 'stale-label' input to 'exclusion-labels' to accept comma-separated list - Updated logic to process issues with ANY of the specified exclusion labels - Added validation to check each label exists in repository - Maintained backward compatibility with default 'no-issue-activity' label - Added example in workflow file showing multi-label configuration
|
@palisadoes done |
|
@coderabbitai full review based on your configuration. Provide your output in a readable format. This GitHub action aims to unassign people from issues if a specific label is assigned to the issue and there are no PRs referencing the issue after a timeout period. There is the option of the action exiting without implementing the feature if one or more specified labels are applied. |
🧩 Analysis chain🏁 Script executed: Length of output: 17005 🏁 Script executed: Length of output: 1873
📋 Overall AssessmentThe action is well-structured with robust error handling and retry logic. However, there's a critical naming and logic inconsistency that makes the configuration confusing and potentially contradicts the stated objectives. 🔴 Critical Issues1. Naming Confusion: "exclusion-labels" vs Actual BehaviorProblem: The input is named Current behavior: // This filters issues that HAVE the exclusion labels
const issues = allIssues.filter(issue => {
const issueLabels = issue.labels.map(l => l.name);
return validExclusionLabels.some(label => issueLabels.includes(label));
});What's happening: Issues WITH Expected interpretation based on naming:
Your comment mentions: "There is the option of the action exiting without implementing the feature if one or more specified labels are applied." This suggests there should be BOTH:
But the current implementation only has one set of labels doing both jobs, which is confusing. Recommendation: inputs:
stale-label:
description: 'Label that triggers unassignment countdown (e.g., "no-issue-activity")'
required: false
default: 'no-issue-activity'
exclusion-labels:
description: 'Comma-separated labels that prevent unassignment (e.g., "waiting-for-maintainer,blocked")'
required: false
default: ''🟡 Design Questions2. PR Link Logic ClarificationThe code checks for open PRs authored by the assignee: const isAuthoredByAssignee = assigneeLogins.includes(prAuthor);
return isSameRepo && isOpen && isAuthoredByAssignee;Current behavior: Skips unassignment if assignee has an open linked PR (they're working). Question: Should this also check for ANY open PR linked to the issue (not just by assignee)? If someone else has an active PR for the issue, should we still unassign the original assignee? From PR objectives: "Authored by the issue assignee" suggests current logic is correct, but please confirm this is the desired behavior. 3. Multiple Assignees HandlingWhen an issue has multiple assignees: // Removes ALL assignees at once
await github.rest.issues.removeAssignees({
assignees: assigneeLogins
});Behavior: All assignees are removed together, even if only one is inactive. Question: Is this intended? Should the action evaluate each assignee individually (checking if each has their own PR)? 🟢 Strengths4. Excellent Error Handling
5. Security Best Practices
6. Testing & Safety Features
7. Robust Validation
🔵 Minor Improvements8. Workflow Configuration
Suggestion: Add a README or documentation showing common configurations. 9. Grace Period Calculationconst daysSinceLabel = (now - labelAddedDate) / (1000 * 60 * 60 * 24);
10. Timeline Event Lookupconst labelEvents = timelineEvents.filter(event =>
event.event === 'labeled' &&
validExclusionLabels.includes(event.label?.name)
);
📝 Documentation Gaps11. Missing DocumentationThe action would benefit from:
🎯 Summary & Recommendations
Key Action Items:
The action is functionally solid with excellent error handling, but the naming confusion around "exclusion-labels" could lead to misconfiguration. Once this is clarified or fixed, it will be production-ready. Would you like me to help create a refactored version that separates trigger and exclusion labels, or is the current single-label behavior actually what you intended? ✅ Actions performedFull review triggered. |
|
In reference to this analysis: These are the responses:
|
|
Tip For best results, initiate chat on the files or code changes.
That leaves the critical naming inconsistency as the remaining issue to resolve. The current implementation has a logical contradiction: 🔴 Remaining Critical Issue: "exclusion-labels" LogicCurrent code behavior: // Line ~155 - Filters issues that HAVE the exclusion labels
const issues = allIssues.filter(issue => {
const issueLabels = issue.labels.map(l => l.name);
return validExclusionLabels.some(label => issueLabels.includes(label));
});What this does: Issues WITH Problem: The input is named ❓ Clarification NeededIn your earlier comment you mentioned: "There is the option of the action exiting without implementing the feature if one or more specified labels are applied." Question: Should there be TWO separate label concepts? Option A - Current Implementation (Single Label System):
Option B - Dual Label System:
Which behavior do you want?
Please clarify which approach you'd like, and I can help implement or document it accordingly. |
- stale-label: triggers unassignment countdown (default: no-issue-activity) - exclusion-labels: PREVENTS unassignment when present (e.g., waiting-for-maintainer,blocked) - Fixed naming confusion: exclusion labels now correctly exclude/protect issues - Updated workflow comments to clarify the dual-label system
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Post your conclusion as an easily readable comment. |
🧩 Analysis chain🏁 Script executed: Length of output: 18508 🏁 Script executed: Length of output: 1983 🏁 Script executed: Length of output: 1089
✅ CRITICAL ISSUE RESOLVEDThe naming inconsistency has been completely fixed in commit
The code logic now perfectly matches the input names and descriptions. Issues are fetched by the stale-label, then filtered out if they have any exclusion labels. This is exactly what was needed. ✨ 🎯 DESIGN DECISIONS CONFIRMEDBoth design questions from the previous review are resolved as intended: ✅ PR-Link Logic: Correctly skips unassignment only if the assignee has authored an open PR 🏆 STRENGTHS - PRODUCTION READY1. Robust Error Handling
2. Security Best Practices
3. Testing & Safety Features
4. Flexible Configuration
5. Efficient Implementation
6. Proper Workflow Permissions
📋 LOGIC VERIFICATIONThe Action correctly implements this flow: graph TD
A[Start] --> B{Repo archived?}
B -->|Yes| END[Skip]
B -->|No| C{Stale label exists?}
C -->|No| END
C -->|Yes| D[Fetch open issues with stale label]
D --> E{For each issue}
E --> F{Is PR?}
F -->|Yes| SKIP[Skip to next]
F -->|No| G{Has assignees?}
G -->|No| SKIP
G -->|Yes| H{Has exclusion label?}
H -->|Yes| SKIP
H -->|No| I{Has open PR by assignee?}
I -->|Yes| SKIP
I -->|No| J{Grace period exceeded?}
J -->|No| SKIP
J -->|Yes| K[Remove all assignees]
K --> L[Post comment]
L --> SKIP
SKIP --> E
E --> M[Generate summary report]
M --> END
Key behaviors verified:
💡 MINOR SUGGESTIONS (Optional Enhancements)1. DocumentationConsider adding a README at
2. Workflow CommentsThe workflow file has helpful commented examples, but could add: # Example use case: Unassign after 14 days, but protect issues waiting for maintainers
# exclusion-labels: 'waiting-for-maintainer,blocked,dependencies'3. Job Summary EnhancementConsider adding to the summary report:
Example: await core.summary
.addHeading('Stale Issue Unassignment Report')
.addRaw(`**Configuration:** Grace period: ${gracePeriodDisplay}, Stale label: "${staleLabel}", Exclusion labels: ${exclusionDisplay}, Dry-run: ${dryRun}`)
.addTable([...])
.write();4. Edge Case: Label Removed and Re-addedCurrent behavior: Uses the most recent label event (correct). if (labelEvents.length > 1) {
console.log(`Issue #${issue.number}: Stale label was re-applied ${labelEvents.length} times, using most recent`);
}🎓 BEST PRACTICES DEMONSTRATEDThis Action exemplifies excellent GitHub Actions development:
🚀 FINAL VERDICT: PRODUCTION READY
📝 SUMMARYThe stale-unassign GitHub Action is well-architected, secure, and production-ready. The critical naming/logic issue has been completely resolved. The implementation demonstrates best practices in error handling, security, and testing. The only remaining suggestions are optional documentation enhancements. Recommendation: ✅ Approve and merge. This Action is ready for use across PalisadoesFoundation repositories. Excellent work, ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/actions/stale-unassign/action.yml:
- Around line 56-117: The fallback DEFAULT_GRACE_PERIOD (used when
GRACE_PERIOD_DAYS is invalid) is set to 1 but the input default is 7; update the
constant or validation so they match. Change DEFAULT_GRACE_PERIOD to 7 (the
input default) or make the gracePeriodDays validation use the configured input
default value, ensuring the variables DEFAULT_GRACE_PERIOD, gracePeriodMinutes,
and gracePeriodDays in the retry/validation block are consistent so an invalid
GRACE_PERIOD_DAYS falls back to 7 days rather than 1.
- Around line 283-325: The same-repo PR detection in the hasOpenLinkedPR block
uses the undocumented event.source.issue.repository?.full_name which can be
missing; change the logic in the hasOpenLinkedPR predicate (the
timelineEvents.some callback) to parse and compare
event.source.issue.repository_url instead (extract owner/repo from the URL and
compare to `${context.repo.owner}/${context.repo.repo}`) and remove the fallback
that treats missing repository as same-repo; keep the existing checks for open
state (event.source.issue.state) and assignee author check
(assigneeLogins.includes(prAuthor)) and update the console.log messages to
reflect when repository_url mismatches or is missing.
|
@Ritesh-251 Please address the findings |
- Change DEFAULT_GRACE_PERIOD from 1 to 7 to match input default - Update same-repo PR detection to use repository_url instead of undocumented full_name - Add proper URL parsing and error handling for missing repository_url - Remove fallback that treated missing repository as same-repo
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/stale-unassign/action.yml:
- Around line 365-376: The grace-period calculation doesn’t guard against an
invalid timeline event date; validate latestLabelEvent.created_at by checking
labelAddedDate is a valid Date (e.g., isNaN(labelAddedDate.getTime())) before
computing daysSinceLabel, and if invalid log a clear message referencing Issue
#${issue.number} and latestLabelEvent, increment skippedCount and continue;
update the block that computes labelAddedDate, daysSinceLabel and the if
(daysSinceLabel < gracePeriodInDays) branch to include this defensive check so
malformed or missing created_at won’t produce NaN and cause incorrect
unassignment.
🧹 Nitpick comments (2)
.github/actions/stale-unassign/action.yml (2)
144-148: Early exits bypass job summary output.When
stale-labelis empty, the action returns early without writing tocore.summary. The same applies to lines 174-176 (archived repo) and 186-189 (missing label). This leaves the GitHub Actions job summary unpopulated, making it harder to diagnose why the action did nothing.Consider writing a summary before early exits for better observability.
Example for consistent summary on early exit
if (!staleLabel || staleLabel.trim() === '') { console.error('Error: stale-label input is empty. Cannot proceed.'); + await core.summary + .addHeading('Stale Issue Unassignment Report') + .addRaw('⚠️ Action skipped: stale-label input is empty.') + .write(); return; }
394-428: Error counts are double-counted in skipped metric.When an error occurs (lines 396-398 and 426-427), both
errorCountandskippedCountare incremented. This makes the summary misleading—if you have 10 skipped and 3 errors, 3 of those skipped are actually the errors.Consider either excluding errors from
skippedCountor clarifying the metric label.Option 1: Don't increment skippedCount on errors
} catch (error) { console.error(`Issue #${issue.number}: Error removing assignees: ${error.message}`); errorCount++; - skippedCount++; continue; }} catch (error) { console.error(`Issue #${issue.number}: Unexpected error: ${error.message}`); errorCount++; - skippedCount++; }Option 2: Rename to clarify metrics include overlap
.addTable([ ['Metric', 'Count'], ['Processed', processedCount.toString()], - ['Skipped', skippedCount.toString()], + ['Skipped (incl. errors)', skippedCount.toString()], ['Errors', errorCount.toString()] ])
- Validate labelAddedDate using isNaN(labelAddedDate.getTime()) - Skip issues with malformed or missing created_at timestamps - Prevent NaN in daysSinceLabel calculation that could cause incorrect unassignment
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/stale-unassign/action.yml:
- Around line 218-240: The code currently calls github.paginate(...) causing all
pages to be fetched before applying the maxIssuesPerRun limit; update the fetch
loop to use github.paginate.iterator (or github.paginate with a done callback)
so you can break early: iterate over
github.paginate.iterator(github.rest.issues.listForRepo, { owner:
context.repo.owner, repo: context.repo.repo, state: 'open', labels: staleLabel,
per_page: 100 }) and push data into the issues array until issues.length >=
limit (where limit = maxIssuesPerRun > 0 ? maxIssuesPerRun : Infinity), then
break and slice to limit; keep the existing error handling and console.log that
references issues and staleLabel.
- Use github.paginate.iterator instead of github.paginate - Break early when maxIssuesPerRun limit is reached - Avoid fetching unnecessary pages to reduce API calls - Update console.log message to reflect when limit is applied
|
@palisadoes pls review i have done all the suggested changes |
2 participants
Resolves #24
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.