Add stale unassign GitHub Action#31
Add stale unassign GitHub Action#31Ritesh-251 wants to merge 7 commits intoPalisadoesFoundation:mainfrom
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 that unassigns issues labeled 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 |
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where mock cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
🧩 Analysis chain
🏁 Script executed: Length of output: 20082 🏁 Script executed: Length of output: 1983 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1784 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 9
✅ ISSUE
|
| Requirement | Status | Implementation |
|---|---|---|
| Run only on non-archived repos | ✅ | Lines 157-167: Checks repo.archived and exits early |
| Identify issues with stale trigger label | ✅ | Lines 201-230: Fetches open issues with stale-label |
| Verify issue has assignee | ✅ | Lines 251-255: Skips if no assignees |
| Check for open linked PR by assignee | ✅ | Lines 268-318: Timeline check for same-repo, open, assignee-authored PRs |
| Grace period before unassignment | ✅ | Lines 324-356: Calculates days since label, supports minutes/days |
| Unassign and post comment | ✅ | Lines 363-399: Removes all assignees, posts customizable comment |
| Exclusion labels configuration | ✅ | Lines 11-14, 169-188, 258-265: Validates and prevents unassignment |
| Reusable from central repo | ✅ | Composite action in .github/actions/ called by workflow |
🎯 PREVIOUS CODERABBIT RECOMMENDATIONS: STATUS
✅ Implemented (Critical)
- Dual-label system (commit
38a5297): Correctly separatedstale-label(trigger) fromexclusion-labels(prevention) - PR-link logic: Confirmed to check only assignee-authored PRs (lines 308-309)
- Multiple assignees: Confirmed to remove all at once (line 369)
- Optimization (commit
929b6fb): Usespaginate.iteratorwith early break to avoid unnecessary API calls - Date validation (commit
78e44c7): Validates timeline event dates (lines 345-350)
⚠️ Not Implemented (Optional Enhancements)
- Documentation README: No README.md in
.github/actions/stale-unassign/ - Enhanced job summary: Basic summary exists (lines 408-416), but missing configuration details (grace period, labels, dry-run status)
- Re-labeling edge case logging: No log message when
labelEvents.length > 1
🔬 TEST COVERAGE & FLAKINESS ANALYSIS
❌ No Automated Tests Found
Finding: The action has zero automated test files (no .test.js, .spec.js, or test workflows).
Implications:
- ✅ Manual testing capabilities exist: Dry-run mode (
dry-run: true) and minutes override (grace-period-minutes: 5) allow safe manual validation - ✅ Tested in production: PR description states "tested with a 5-minute grace period in talawa-admin"
- ❌ No CI/CD test coverage: Changes can't be validated automatically
- ❌ No regression protection: Future modifications could break existing behavior
Recommendation: While the action is functional, adding tests would improve maintainability. However, since this is a composite action with inline GitHub Script, traditional unit testing is challenging. Acceptable alternatives:
- Integration tests: Create test issues/PRs in a sandbox repo and run the action
- Mock tests: Use
@actions/githubmock in a separate.test.jsfile - Documented manual test procedure: Checklist for manual validation before releases
🟡 Potential Flakiness Concerns
1. GitHub API Rate Limiting (Medium Risk)
Location: Lines 201-230 (issue fetching), 268 (timeline fetching)
Risk: In repositories with many stale issues, the action could hit rate limits despite retry logic.
Mitigations present:
- ✅
max-issues-per-runlimit (default: 50) - ✅
paginate.iteratorwith early break (commit929b6fb) - ✅ Exponential backoff retry with
Retry-Afterheader parsing (lines 40-75)
Flakiness verdict: Low risk in production. The max-issues-per-run cap and retry logic make this robust.
2. Timeline Event Pagination (Low Risk)
Location: Lines 268-276
Risk: For issues with thousands of events, pagination could be slow or fail mid-stream.
Mitigations present:
- ✅ Uses
github.paginatewhich handles pagination automatically - ✅ Retry wrapper catches network/server errors
- ✅ Per-issue error handling (lines 242-406) prevents one failure from breaking the entire run
Flakiness verdict: Very low risk. Error handling is comprehensive.
3. Date Parsing Edge Cases (Very Low Risk)
Location: Lines 336-350
Risk: Invalid created_at timestamps from GitHub API could cause crashes.
Mitigations present:
- ✅ Validates date is not NaN (lines 345-350, added in commit
78e44c7) - ✅ Skips issue with clear log message if invalid
Flakiness verdict: Negligible risk. Validation added.
4. Cross-Repository PR Detection (Low Risk)
Location: Lines 286-301
Risk: Regex parsing of repository_url could fail if GitHub API changes URL format.
Mitigations present:
- ✅ Null checks for missing
repository_url(lines 291-294) - ✅ Logs warnings for unparseable URLs (line 298)
⚠️ Treats parsing failures as "different repo" (safe default, but might miss same-repo PRs)
Flakiness verdict: Low risk, but could benefit from defensive logging.
Suggestion:
if (!urlMatch) {
console.warn(`Issue #${issue.number}: Unexpected repository_url format: ${prRepoUrl}, treating as different repo`);
}This would help diagnose API format changes.
5. Sharded Testing Environment Concerns (N/A)
Finding: The action does not use mocks and has no test suite, so sharded testing concerns don't apply.
If tests were added in the future:
- Use
nockor similar to mock GitHub API responses - Ensure mock cleanup in
afterEach()blocks - Use unique issue numbers per test to avoid collisions in parallel runs
- Mock system time (
Date.now()) to avoid time-dependent flakiness
📊 CODE QUALITY ASSESSMENT
✅ Strengths
- Robust error handling: Retry logic with exponential backoff,
Retry-Afterheader support (both numeric and HTTP-date formats) - Security: Environment variables prevent script injection
- Input validation: All inputs validated with defaults and warnings
- Graceful degradation: Per-issue errors don't fail entire workflow
- Safety features: Dry-run mode, max limits, archived repo check
- Performance optimization: Early pagination break, batch assignee removal
⚠️ Areas for Improvement
1. Documentation Gap (Medium Priority)
Missing: README in .github/actions/stale-unassign/
Should include:
- Use case examples
- Input parameter explanations
- Label workflow diagram
- Troubleshooting guide
- Testing instructions
Impact: Users might misconfigure the action without clear documentation.
2. Job Summary Limited (Low Priority)
Current output:
| Metric | Count |
|------------|-------|
| Processed | 5 |
| Skipped | 12 |
| Errors | 0 |
Could include:
- Configuration used (grace period, stale label, exclusion labels)
- Dry-run status
- List of processed issue numbers
- Reasons for skips (breakdown: "no assignee", "has exclusion label", "grace period not met", etc.)
Impact: Minimal. Current summary is functional but could be more informative.
3. Re-labeling Not Logged (Very Low Priority)
Location: Lines 324-342
Current behavior: Uses most recent label event (correct), but doesn't log when multiple events exist.
Suggestion:
if (labelEvents.length > 1) {
console.log(`Issue #${issue.number}: Stale label was re-applied ${labelEvents.length} times, using most recent (${labelAddedDate.toISOString()})`);
}Impact: Helps diagnose issues where labels are removed and re-added.
🔒 SECURITY REVIEW
✅ No security concerns identified:
- Environment variables prevent script injection
- No use of
eval()or dynamic code execution - Token permissions properly scoped (issues: write, contents: read, pull-requests: read)
- No exposure of secrets in logs
- Archived repo check prevents unintended runs
🎓 PRODUCTION READINESS
| Category | Rating | Notes |
|---|---|---|
| Functionality | ✅ Excellent | All requirements met |
| Error Handling | ✅ Excellent | Comprehensive retry logic, rate limiting |
| Security | ✅ Strong | No vulnerabilities identified |
| Documentation | Comments in code are good, but no README | |
| Test Coverage | ❌ None | No automated tests (manual testing exists) |
| Flakiness Risk | ✅ Low | Good mitigations for API rate limits, errors |
| Maintainability | Well-structured, but would benefit from tests |
🚀 FINAL VERDICT & RECOMMENDATIONS
✅ APPROVED FOR PRODUCTION
The action is functionally complete, secure, and robust. All critical issues from previous reviews are resolved, and all issue #24 requirements are met.
📋 Recommendations Before Merge (Optional)
-
Add README.md (15 minutes):
# Stale Issue Unassigner Automatically unassigns stale issues after a grace period. ## Inputs - `stale-label`: Triggers countdown (default: `no-issue-activity`) - `exclusion-labels`: Prevents unassignment (comma-separated) - `grace-period-days`: Days before unassign (default: 7) - `grace-period-minutes`: Minutes override for testing (default: 0) ... ## Example ```yaml - uses: ./.github/actions/stale-unassign with: exclusion-labels: 'waiting-for-maintainer,blocked' grace-period-days: '14'
-
Enhance job summary (10 minutes):
Add configuration details to the summary table. -
Add re-labeling log (2 minutes):
Log when multiple label events detected.
📋 Future Enhancements (Post-Merge)
- Add integration tests: Create test workflow in sandbox repo
- Metrics collection: Track unassignment trends over time
- Configurable comment per repo: Allow repos to override default message
📝 SUMMARY
✅ All issue #24 goals achieved
✅ All critical CodeRabbit findings resolved
✅ Low flakiness risk due to comprehensive error handling
The action is production-ready and can be merged. The missing tests and documentation are nice-to-have improvements, not blockers.
Excellent work, @Ritesh-251! The implementation is solid and well-thought-out. 🎉
✅ Actions performed
Full review triggered.
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
|
@palisadoes this req has been pending since weeks !! pls look into it why it took so much time |
2 participants
Resolves #24
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.