Refactor expired cleanup close/comment handlers - #50942
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Centralizes expired-entity comment and close orchestration while preserving entity-specific behavior.
Changes:
- Adds a shared handler factory and API helpers.
- Converts issue, pull request, and discussion cleanup to declarative wrappers.
- Adds focused factory tests for normal and skipped flows.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/expired_entity_handler_factory.cjs |
Implements shared cleanup handling. |
actions/setup/js/expired_entity_handler_factory.test.cjs |
Tests shared and skipped flows. |
actions/setup/js/close_expired_issues.cjs |
Uses the shared handler for issues. |
actions/setup/js/close_expired_pull_requests.cjs |
Uses the shared handler for pull requests. |
actions/setup/js/close_expired_discussions.cjs |
Retains discussion-specific deduplication through a hook. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
|
|
|
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Review: Refactor expired cleanup close/comment handlers
The refactor is well-structured and the factory pattern cleanly eliminates the duplicated close/comment orchestration. Two issues to address:
- Blocking — The new test file
expired_entity_handler_factory.test.cjsuses ESMimportsyntax inside a.cjsfile, which will throw aSyntaxErrorat runtime (see inline comment). - Non-blocking —
github,owner, andrepoare declared in the factory options type but never used inside the factory itself; removing them simplifies the API (see inline comment).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.3 AIC · ⊞ 5.3K
Comments that could not be inline-anchored
actions/setup/js/expired_entity_handler_factory.test.cjs:2
Bug: ESM import syntax in a .cjs file.
This file uses import { describe, it, expect, beforeEach, vi } from "vitest"; but .cjs files are treated as CommonJS by Node.js and will throw a SyntaxError at load time. Use require instead:
const { describe, it, expect, beforeEach, vi } = require("vitest");Alternatively, rename the file to .test.mjs or .test.js if the package uses ESM.
@copilot please address this.
actions/setup/js/expired_entity_handler_factory.cjs:517
Unused fields in options type: github, owner, repo.
The JSDoc for createExpiredEntityHandler declares github, owner, and repo as required options, but the factory body never reads them — callers already close over those values in their addComment and closeEntity callbacks. Keeping them in the signature forces callers to repeat values they don't need to pass and creates a misleading API surface.
Remove these three fields from the JSDoc typedef and from the options destruc…
Test Quality Sentinel 🧪Score: 85/100 ✅ Excellent SummaryPR adds a factory function for creating expired entity handlers (issues, PRs, discussions) with a comprehensive test suite covering core behaviors and edge cases. Key FindingsTest Coverage Quality:
Tests Analyzed:
Per-Test DetailsTest 1: creates a handler that comments, closes, and returns a closed recordTests the standard flow: handler receives entity, invokes Quality: High-value design test — Verifies the handler's core contract. Test 2: allows a pre-close hook to skip the shared comment flowTests the Quality: High-value design test — Verifies the extension hook contract and error/bypass path. Scoring Breakdown
Threshold Analysis:
Test Quality Sentinel analyzed 2 behavioral tests in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — requesting changes on a few concrete issues before merge.
📋 Key Themes & Highlights
Issues Found
- ESM
importin a.cjstest file — syntax error in plain-Node execution (line 3 of the test file). coreimplicit global in factory — the factory callscore.info/core.warningvia an undeclared ambient global, breaking testability. The test works around it viaglobal.coreassignment.- Dead parameters (
github,owner,repo) — accepted in options but never read by the factory; adds noise to every call site. status: "skipped"when discussion was actually closed — thehasComment=truebranch closes the entity but reportsskipped, hiding the close from the summary. Pre-existing bug preserved by the refactor.- Missing test:
beforeComment → undefined— the normal-flow pass-through case is not covered.
Positive Highlights
- ✅ Clean extraction — three scripts reduced to pure configuration with no logic duplication.
- ✅
beforeCommenthook is a well-placed seam: keeps discussion-specific dedupe logic out of the factory without leaking concerns. - ✅
createClosedRecord/createExpiredEntityClosingMessageare good small helpers; easy to test and reason about. - ✅ Test structure is clear and follows Arrange/Act/Assert throughout.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.2 AIC · ⊞ 7.1K
Comment /matt to run again
| @@ -0,0 +1,93 @@ | |||
| // @ts-check | |||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | |||
| import { createExpiredEntityHandler } from "./expired_entity_handler_factory.cjs"; | |||
There was a problem hiding this comment.
[/tdd] ESM import syntax in a .cjs file will cause a syntax error at runtime — Node treats .cjs as CommonJS unconditionally.
💡 Fix: use require
const { describe, it, expect, beforeEach, vi } = require("vitest");Or rename the file to .test.mjs. If Vitest's bundler transforms .cjs for you, add a comment saying so to avoid future confusion.
@copilot please address this.
There was a problem hiding this comment.
Added a comment at the top of the file explaining that Vitest's bundler transforms .cjs test files to ESM at test time, so the import syntax is intentional. Using require("vitest") is not an option — Vitest v4 only supports ESM and throws Vitest cannot be imported in a CommonJS module using require() when require() is used. This matches the pattern of all other .cjs test files in the project.
| workflowId: options.workflowId, | ||
| runUrl: options.runUrl, | ||
| footerSuffix: options.footerSuffix, | ||
| }); |
There was a problem hiding this comment.
[/codebase-design] core is accessed as an implicit global inside the factory — this is invisible in the function signature and makes the module impossible to unit-test without monkey-patching global.core.
💡 Accept core as a parameter
Add core to the options object passed to createExpiredEntityHandler:
function createExpiredEntityHandler({ core, ...rest }) {
return async entity => {
// use injected core
core.info(...)
};
}The test already sets global.core = mockCore as a workaround — passing it explicitly removes that fragility.
@copilot please address this.
There was a problem hiding this comment.
Fixed. core is now accepted as an explicit parameter in the options object and stored in a local variable at the top of the returned handler. All three call sites (close_expired_issues.cjs, close_expired_pull_requests.cjs, close_expired_discussions.cjs) now pass core directly. The test no longer needs global.core assignment.
| * @param {{ | ||
| * github: any, | ||
| * owner: string, | ||
| * repo: string, |
There was a problem hiding this comment.
[/codebase-design] github, owner, and repo are accepted in the options object but the factory body never reads them — they're dead parameters. This adds noise to every call site and misleads readers into thinking the factory performs its own API calls.
💡 Remove unused parameters
The actual API calls are delegated entirely to addComment and closeEntity callbacks, so github, owner, and repo serve no purpose inside the factory. Remove them from the options type and from all three call sites.
@copilot please address this.
There was a problem hiding this comment.
Fixed. Removed github, owner, and repo from the JSDoc typedef and from all three call sites. The factory never needed them — the actual API calls are already fully encapsulated in the addComment and closeEntity callbacks.
| const closingMessage = (cautionAlert ? cautionAlert + "\n\n" : "") + expirationText + generateExpiredEntityFooter(workflowName, runUrl, workflowId) + "\n\n<!-- gh-aw-closed -->"; | ||
| if (hasComment) { | ||
| core.warning(` Discussion #${discussion.number} already has an expiration comment, skipping to avoid duplicate`); | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] When a discussion already has a comment (hasComment=true), the handler closes it and returns status: "skipped" — but the discussion was closed, so the record ends up in the skip list rather than the closed list, hiding real work from the summary.
💡 Return status: "closed"
if (hasComment) {
core.warning(` Discussion #${discussion.number} already has an expiration comment...`);
core.info(` Attempting to close discussion #${discussion.number} without adding another comment`);
await closeDiscussionAsOutdated(github, discussion.id);
core.info(` ✓ Discussion closed successfully`);
return {
- status: "skipped",
+ status: "closed", // the entity was actually closed
record: createClosedRecord(discussion),
};
}This was the same behaviour in the original code — the refactor preserved the bug.
@copilot please address this.
There was a problem hiding this comment.
Fixed. Changed status: "skipped" to status: "closed" in the hasComment=true branch. The discussion was actually being closed in that branch, so the record now correctly appears in the closed list rather than the skip list.
| }, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The test suite only covers two paths: full-flow and beforeComment returning an early result. The most important path — beforeComment returning undefined to allow the normal flow to proceed — is untested, which is exactly the case used by discussions when neither skip condition is met.
💡 Add a third test
it("runs normal flow when beforeComment returns undefined", async () => {
const addComment = vi.fn().mockResolvedValue({});
const closeEntity = vi.fn().mockResolvedValue({});
const handler = createExpiredEntityHandler({
github: {},
owner: "o", repo: "r",
workflowName: "W", workflowId: "wid",
runUrl: "(example.com/redacted)",
entityNoun: "discussion",
entityLabel: "Discussion",
beforeComment: async () => undefined, // explicitly pass through
addComment,
closeEntity,
});
const result = await handler({ number: 1, title: "T", url: "U", expirationDate: new Date() });
expect(addComment).toHaveBeenCalledTimes(1);
expect(closeEntity).toHaveBeenCalledTimes(1);
expect(result.status).toBe("closed");
});@copilot please address this.
There was a problem hiding this comment.
Added the missing test. The third test case passes beforeComment: async () => undefined and verifies that addComment and closeEntity are both called once and result.status is "closed".
|
@copilot Please triage this PR and run the Address unresolved reviewer feedback:
Investigate failed checks: Refresh the branch if needed.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR Triage
|
|
@copilot Please triage this PR and run the Address unresolved reviewer feedback (newest first):
Investigate failed checks: Refresh the branch if needed.
|
PR finisher complete.
Hand-off: CI must be re-triggered by a maintainer (close/reopen, |
|
🎉 This pull request is included in a new release. Release: |
Expired issue, pull request, and discussion cleanup shared the same comment/footer/close orchestration while reimplementing it per entity type. This made the scripts harder to maintain and left discussion dedupe behavior mixed into the duplicated closing flow.
Shared expired-entity handler
expired_entity_handler_factory.cjsto centralize:Declarative entity wrappers
beforeCommenthook.Focused coverage
Example shape: