chore: refactor spam list updating#1749
Conversation
…on step Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
…on step Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
|
Hi @MonaaEid, this is **LinkBot** 👋
Linking pull requests to issues helps us significantly with reviewing pull requests and keeping the repository healthy. 🚨 This pull request does not have an issue linked. Please link an issue using the following format: 📖 Guide: If no issue exists yet, please create one: Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1749 +/- ##
=======================================
Coverage 93.25% 93.25%
=======================================
Files 141 141
Lines 9061 9061
=======================================
Hits 8450 8450
Misses 611 611 🚀 New features to boost your workflow:
|
exploreriii
added
the
status: needs triage review
MonaaEid
changed the title
MonaaEid
requested review from
Copilot,
rbair23 and
rbarker-dev
and removed request for
Copilot
WalkthroughThe spam list update workflow is refactored to address a permission issue by replacing pull request creation with GitHub issue creation. The update script no longer writes directly to the spam-list.txt file, and the workflow configuration now requires Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/scripts/update-spam-list.js (3)
75-105:⚠️ Potential issue | 🟡 MinorStale reference: issue body still says "This PR" instead of "This issue".
Line 79 generates body text saying "This PR automatically updates the spam list", but the script now creates an issue, not a PR. The title format on Line 76 also uses a PR-style prefix.
Proposed fix
- const title = `chore: Update spam list (${additions.length} additions, ${removals.length} removals)`; + const title = `chore: update spam list (${additions.length} additions, ${removals.length} removals)`; let body = '## Automated Spam List Update\n\n'; - body += 'This PR automatically updates the spam list based on recent PR activity.\n\n'; + body += 'This issue reports spam list changes based on recent PR activity.\n\n';
67-71: 🛠️ Refactor suggestion | 🟠 Major
finalSpamListis computed but never consumed by the caller.
computeSpamListUpdatesstill builds and returnsfinalSpamList(and mutates it viaadd/delete), but the caller on Line 190 only destructures{ additions, removals }. Since the file-writing logic was removed, this is dead code.Proposed fix
return { additions, - removals, - finalSpamList: Array.from(finalSpamList).sort((a, b) => a.localeCompare(b)) + removals };
166-187:⚠️ Potential issue | 🟠 MajorUnbounded pagination on search results.
The pagination loop on Lines 170–186 has no hard upper bound on the number of pages or results processed. If the repository accumulates a very large number of matching PRs, this could lead to excessive API consumption and long run times. Additionally, each "rehabilitated PR" triggers a secondary
pulls.getcall inside the loop (Line 149), amplifying the cost.Consider adding a page counter or total-items cap to bail out after a reasonable threshold (e.g., 1000 results).
As per coding guidelines, "Pagination MUST: Be considered and justified, Enforce hard upper bounds".
.github/workflows/cron-update-spam-list.yml (1)
21-23:⚠️ Potential issue | 🟠 MajorMissing concurrency group.
This workflow mutates GitHub state (creates issues) but defines no
concurrencykey. If the cron and a manual dispatch overlap, or if the workflow is retried, duplicate issues could be created concurrently.Proposed fix
jobs: update-spam-list: runs-on: ubuntu-latest + concurrency: + group: cron-update-spam-list + cancel-in-progress: false steps:As per coding guidelines, "Workflows that mutate state MUST: Define a deterministic concurrency group".
| if (hasChanges) { | ||
| if (dryRun) { | ||
| console.log('[DRY RUN] Would create issue with:'); | ||
| console.log(`Title: ${title}`); | ||
| console.log(`Body: ${body}`); | ||
| } else { | ||
| await github.rest.issues.create({ | ||
| owner, | ||
| repo, | ||
| title, | ||
| body, | ||
| labels: ['spam-list-update', 'automated'] | ||
| }); | ||
| console.log('Issue created successfully'); | ||
| } | ||
| } else { | ||
| console.log('No changes needed, skipping issue creation'); | ||
| } |
There was a problem hiding this comment.
Missing deduplication: repeated runs will create duplicate issues.
There is no check-before-create logic to prevent duplicate issues. If the cron fires twice, gets retried, or is manually dispatched, a new issue is created each time. The coding guidelines require marker-based deduplication for workflows that mutate GitHub state.
Search for an existing open issue with the same label before creating a new one:
Proposed fix
if (hasChanges) {
if (dryRun) {
console.log('[DRY RUN] Would create issue with:');
console.log(`Title: ${title}`);
console.log(`Body: ${body}`);
} else {
+ // Check for existing open issue to avoid duplicates
+ const { data: existing } = await github.rest.issues.listForRepo({
+ owner,
+ repo,
+ labels: 'spam-list-update',
+ state: 'open',
+ per_page: 1
+ });
+ if (existing.length > 0) {
+ console.log(`Skipping issue creation: open issue #${existing[0].number} already exists`);
+ return;
+ }
await github.rest.issues.create({
owner,
repo,
title,
body,
labels: ['spam-list-update', 'automated']
});
console.log('Issue created successfully');
}As per coding guidelines, "Marker-based deduplication is required" and "Duplicate API calls MUST be avoided".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (hasChanges) { | |
| if (dryRun) { | |
| console.log('[DRY RUN] Would create issue with:'); | |
| console.log(`Title: ${title}`); | |
| console.log(`Body: ${body}`); | |
| } else { | |
| await github.rest.issues.create({ | |
| owner, | |
| repo, | |
| title, | |
| body, | |
| labels: ['spam-list-update', 'automated'] | |
| }); | |
| console.log('Issue created successfully'); | |
| } | |
| } else { | |
| console.log('No changes needed, skipping issue creation'); | |
| } | |
| if (hasChanges) { | |
| if (dryRun) { | |
| console.log('[DRY RUN] Would create issue with:'); | |
| console.log(`Title: ${title}`); | |
| console.log(`Body: ${body}`); | |
| } else { | |
| // Check for existing open issue to avoid duplicates | |
| const { data: existing } = await github.rest.issues.listForRepo({ | |
| owner, | |
| repo, | |
| labels: 'spam-list-update', | |
| state: 'open', | |
| per_page: 1 | |
| }); | |
| if (existing.length > 0) { | |
| console.log(`Skipping issue creation: open issue #${existing[0].number} already exists`); | |
| return; | |
| } | |
| await github.rest.issues.create({ | |
| owner, | |
| repo, | |
| title, | |
| body, | |
| labels: ['spam-list-update', 'automated'] | |
| }); | |
| console.log('Issue created successfully'); | |
| } | |
| } else { | |
| console.log('No changes needed, skipping issue creation'); | |
| } |
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| issues: write |
There was a problem hiding this comment.
Over-permissioned: contents: write and pull-requests: write are no longer needed.
The PR creation step was removed and the script no longer writes files. The only mutation is github.rest.issues.create, which requires issues: write. The contents: read permission is sufficient for the checkout step.
Proposed fix
permissions:
- contents: write
- pull-requests: write
+ contents: read
issues: writeAs per coding guidelines, "Requested permissions MUST exactly match behavior" and "Flag: Over-permissioned workflows".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| contents: write | |
| pull-requests: write | |
| issues: write | |
| permissions: | |
| contents: read | |
| issues: write |
| - Refactor spam list update logic and remove unused pull request creation step `.github/scripts/update-spam-list.js` `.github/workflows/cron-update-spam-list.yml`. | ||
| ### Removed |
There was a problem hiding this comment.
Missing blank line before ### Removed heading and missing issue reference.
The changelog entry on Line 292 runs directly into the ### Removed section header on Line 293 with no separating blank line, breaking the formatting pattern used throughout the rest of the file. Also, other entries in this changelog reference their issue/PR number (e.g., (#1664)), but this entry does not reference #1667 or #1749.
Proposed fix
-- Refactor spam list update logic and remove unused pull request creation step `.github/scripts/update-spam-list.js` `.github/workflows/cron-update-spam-list.yml`.
-### Removed
+- Refactor spam list update logic and remove unused pull request creation step `.github/scripts/update-spam-list.js` `.github/workflows/cron-update-spam-list.yml`. (`#1667`)
+
+### Removed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Refactor spam list update logic and remove unused pull request creation step `.github/scripts/update-spam-list.js` `.github/workflows/cron-update-spam-list.yml`. | |
| ### Removed | |
| - Refactor spam list update logic and remove unused pull request creation step `.github/scripts/update-spam-list.js` `.github/workflows/cron-update-spam-list.yml`. (`#1667`) | |
| ### Removed |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~292-~292: The official name of this software platform is spelled with a capital “H”.
Context: ...ep .github/scripts/update-spam-list.js .github/workflows/cron-update-spam-list.yml. #...
(GITHUB)
🪛 markdownlint-cli2 (0.20.0)
[warning] 293-293: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
I think some of the code rabbit suggestions are good 👍
exploreriii
added
status: needs developer revision
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Please check the permissions and the final comment needs to be updated to reflect its an issue now
2 participants
Description:
This pull request refactors the spam list update automation to create an issue instead of a pull request when changes are detected, and removes the now-unused logic for PR creation.
Related issue(s):
Fixes #1667
Checklist
.github/scripts/update-spam-list.jsand.github/workflows/cron-update-spam-list.yml