Skip to content

Deduplicate closeOlder issue/PR wrapper wiring with a shared adapter #45045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copilot wants to merge 4 commits into from
+199 −42
Closed

Deduplicate closeOlder issue/PR wrapper wiring with a shared adapter #45045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eebb323
Initial plan
Copilot Jul 12, 2026
6957de7
refactor closeOlder issue/pr wrapper wiring into shared adapter
Copilot Jul 12, 2026
41e9757
test: clarify close_older adapter test intent names
Copilot Jul 12, 2026
a253e57
test: split close_older adapter behavior assertions
Copilot Jul 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions actions/setup/js/close_older_adapter.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// @ts-check
/// <reference types="@actions/github-script" />

const { closeOlderEntities } = require("./close_older_entities.cjs");

/**
* Close older issues/PRs using shared adapter wiring.
* @param {object} params
* @param {any} params.github
* @param {string} params.owner
* @param {string} params.repo

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaky internal abstraction: messageParams forces callers to know closeOlderEntities's private {newEntityUrl, newEntityNumber} field names, coupling adapter consumers to an implementation detail they cannot see.

💡 Details

The adapter's getCloseMessage closure calls params.getCloseMessage(params.messageParams(closeParams)) where closeParams is the shape produced internally by closeOlderEntities:

// Inside close_older_entities.cjs (line 112):
config.getCloseMessage({
  newEntityUrl: newEntity.url || newEntity.html_url,
  newEntityNumber: newEntity.number,
  workflowName,
  runUrl,
});

That opaque shape leaks out as the required input to every messageParams callback in callers (close_older_issues.cjs, close_older_pull_requests.cjs). If closeOlderEntities ever renames those fields, all callers silently produce undefined in their close messages — no type error, no runtime guard, no test failure at the boundary.

Fix options:

  1. Add a @typedef CloseMessageParams JSDoc visible to callers documenting the expected shape.
  2. Pass newEntity + context directly to getCloseMessage and remove the messageParams indirection entirely.

* @param {string} params.workflowId
* @param {{number: number, html_url: string}} params.newEntity
* @param {string} params.workflowName
* @param {string} params.runUrl
* @param {string | undefined} params.callerWorkflowId
* @param {string | undefined} params.closeOlderKey
* @param {string} params.entityType
* @param {string} params.entityTypePlural
* @param {(github: any, owner: string, repo: string, workflowId: string, excludeNumber: number, callerWorkflowId?: string, closeOlderKey?: string) => Promise<Array<{number: number, html_url?: string}>>} params.searchOlderEntities
* @param {(params: any) => string} params.getCloseMessage
* @param {(github: any, owner: string, repo: string, entityId: number, message: string) => Promise<any>} params.addComment
* @param {(github: any, owner: string, repo: string, entityId: number) => Promise<any>} params.closeEntity
* @param {number} params.delayMs
* @param {(params: {newEntityUrl: string, newEntityNumber: number, workflowName: string, runUrl: string}) => any} params.messageParams
* @returns {Promise<Array<{number: number, html_url: string}>>}

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{"pull_request_number":45045,"path":"actions/setup/js/close_older_adapter.cjs","line":26,"body":"[/codebase-design] The messageParams indirection adds an extra moving part without adding safety — callers already own the mapping, so the adapter interface is unnecessarily wide.

������ Simpler alternative

Instead of accepting both getCloseMessage and messageParams, accept a single already-adapted getCloseMessage from the caller:

// In close_older_issues.cjs
getCloseMessage: params => getCloseOlderIssueMessage({
  newIssueUrl: params.newEntityUrl,
  newIssueNumber: params.newEntityNumber,
  workflowName: params.workflowName,
  runUrl: params.runUrl,
}),

This removes messageParams from closeOlderWithAdapter entirely and eliminates the double-indirection on line 40.

@copilot please address this."}

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] messageParams adds a double-indirection layer that widens the adapter interface without providing safety — callers already own the message-shape translation.

💡 Simpler alternative: merge into `getCloseMessage`

Drop messageParams from the adapter params and let callers adapt directly in getCloseMessage:

// close_older_issues.cjs
getCloseMessage: params => getCloseOlderIssueMessage({
  newIssueUrl: params.newEntityUrl,
  newIssueNumber: params.newEntityNumber,
  workflowName: params.workflowName,
  runUrl: params.runUrl,
}),

This removes one adapter param, eliminates the chained call on line 40 (params.getCloseMessage(params.messageParams(closeParams))), and makes the data flow easier to trace.

@copilot please address this.

*/
async function closeOlderWithAdapter(params) {
const result = await closeOlderEntities(params.github, params.owner, params.repo, params.workflowId, params.newEntity, params.workflowName, params.runUrl, {
entityType: params.entityType,
entityTypePlural: params.entityTypePlural,
// Use a closure so callerWorkflowId and closeOlderKey are forwarded to searchOlderEntities
// without going through the closeOlderEntities extraArgs mechanism (which appends
// excludeNumber last)
searchOlderEntities: (gh, o, r, wid, excludeNumber) => params.searchOlderEntities(gh, o, r, wid, excludeNumber, params.callerWorkflowId, params.closeOlderKey),
getCloseMessage: closeParams => params.getCloseMessage(params.messageParams(closeParams)),
addComment: params.addComment,
closeEntity: params.closeEntity,
delayMs: params.delayMs,
getEntityId: entity => entity.number,
getEntityUrl: entity => entity.html_url,
});

return result.map(item => ({
number: item.number,
html_url: item.html_url || "",

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] html_url: item.html_url || "" silently coerces a missing URL to an empty string. Callers that concatenate or render html_url will silently produce broken links.

💡 Make the sentinel explicit

Consider returning html_url: item.html_url ?? "" (same runtime behaviour) and adding a core.warning when html_url is absent:

if (!item.html_url) {
  core.warning(`closeOlderWithAdapter: entity #${item.number} has no html_url`);
}
return { number: item.number, html_url: item.html_url ?? "" };

This surfaces data quality issues in CI logs without changing the return type.

@copilot please address this.

}));
}

module.exports = {
closeOlderWithAdapter,
};
107 changes: 107 additions & 0 deletions actions/setup/js/close_older_adapter.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// @ts-check

import { describe, it, expect, beforeEach, vi } from "vitest";
import { closeOlderWithAdapter } from "./close_older_adapter.cjs";

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CJS module imported via ESM import — use createRequire instead.

All other .test.cjs files in this directory (e.g. action_conclusion_otlp.test.cjs) import CJS modules-under-test with the pattern:

import { createRequire } from "module";
const req = createRequire(import.meta.url);
const { closeOlderWithAdapter } = req("./close_older_adapter.cjs");

Using a bare ESM import for a .cjs file bypasses the shared CJS module cache. This can cause test isolation issues and inconsistent mock behaviour. It also diverges from the established project convention.

@copilot please address this.

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The test file uses ES module import syntax but the implementation uses CommonJS require. This mismatch will cause the test suite to fail unless the vitest config handles CJS interop explicitly.

💡 Fix: use `require` to match the CJS module system 
// `@ts-check`
const { describe, it, expect, beforeEach, vi } = require("vitest");
const { closeOlderWithAdapter } = require("./close_older_adapter.cjs");

Look at close_older_entities.test.cjs — it uses import statements too, so this may be intentional via a vitest ESM transform. If so, add a brief comment explaining the interop setup so future contributors don't trip over it.

@copilot please address this.


describe("close_older_adapter", () => {
beforeEach(() => {
vi.clearAllMocks();
global.core = {
info: vi.fn(),
warning: vi.fn(),
error: vi.fn(),
};
});

it("forwards callerWorkflowId and closeOlderKey to searchOlderEntities", async () => {
const github = {};
const searchOlderEntities = vi.fn().mockResolvedValue([
{
number: 12,
title: "Older issue",
html_url: "https://example/12",
},
]);
const getCloseMessage = vi.fn().mockReturnValue("close message");
const addComment = vi.fn().mockResolvedValue({ id: 1 });
const closeEntity = vi.fn().mockResolvedValue({ number: 12, html_url: "https://example/12" });

const result = await closeOlderWithAdapter({
github,
owner: "owner",
repo: "repo",
workflowId: "workflow",
newEntity: { number: 55, html_url: "https://example/new" },
workflowName: "Workflow",
runUrl: "https://example/run",
callerWorkflowId: "caller-id",
closeOlderKey: "close-key",
entityType: "issue",
entityTypePlural: "issues",
searchOlderEntities,
getCloseMessage,
addComment,
closeEntity,
delayMs: 1,
messageParams: params => ({ transformedEntityUrl: params.newEntityUrl, transformedEntityNumber: params.newEntityNumber }),
});

expect(searchOlderEntities).toHaveBeenCalledWith(github, "owner", "repo", "workflow", 55, "caller-id", "close-key");
expect(result).toEqual([{ number: 12, html_url: "https://example/12" }]);
});

it("applies messageParams transformation before getCloseMessage", async () => {
const github = {};
const getCloseMessage = vi.fn().mockReturnValue("close message");
const addComment = vi.fn().mockResolvedValue({ id: 1 });
const closeEntity = vi.fn().mockResolvedValue({ number: 12, html_url: "https://example/12" });

await closeOlderWithAdapter({
github,
owner: "owner",
repo: "repo",
workflowId: "workflow",
newEntity: { number: 55, html_url: "https://example/new" },
workflowName: "Workflow",
runUrl: "https://example/run",
callerWorkflowId: "caller-id",
closeOlderKey: "close-key",
entityType: "issue",
entityTypePlural: "issues",
searchOlderEntities: vi.fn().mockResolvedValue([{ number: 12, title: "Older issue", html_url: "https://example/12" }]),
getCloseMessage,
addComment,
closeEntity,
delayMs: 1,
messageParams: params => ({ transformedEntityUrl: params.newEntityUrl, transformedEntityNumber: params.newEntityNumber }),
});

expect(getCloseMessage).toHaveBeenCalledWith({ transformedEntityUrl: "https://example/new", transformedEntityNumber: 55 });
expect(addComment).toHaveBeenCalledWith(github, "owner", "repo", 12, "close message");
expect(closeEntity).toHaveBeenCalledWith(github, "owner", "repo", 12);
});

it("normalizes missing html_url in mapped result", async () => {
const result = await closeOlderWithAdapter({
github: {},
owner: "owner",
repo: "repo",
workflowId: "workflow",
newEntity: { number: 10, html_url: "https://example/new" },
workflowName: "Workflow",
runUrl: "https://example/run",
callerWorkflowId: undefined,
closeOlderKey: undefined,
entityType: "issue",
entityTypePlural: "issues",
searchOlderEntities: vi.fn().mockResolvedValue([{ number: 22, title: "Older issue" }]),
getCloseMessage: vi.fn().mockReturnValue("close message"),
addComment: vi.fn().mockResolvedValue({ id: 1 }),
closeEntity: vi.fn().mockResolvedValue({ number: 22 }),
delayMs: 1,
messageParams: params => params,
});

expect(result).toEqual([{ number: 22, html_url: "" }]);
});
});
41 changes: 20 additions & 21 deletions actions/setup/js/close_older_issues.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
/// <reference types="@actions/github-script" />

const { sanitizeContent } = require("./sanitize_content.cjs");
const { closeOlderEntities, MAX_CLOSE_COUNT: SHARED_MAX_CLOSE_COUNT } = require("./close_older_entities.cjs");
const { MAX_CLOSE_COUNT: SHARED_MAX_CLOSE_COUNT } = require("./close_older_entities.cjs");
const { closeOlderWithAdapter } = require("./close_older_adapter.cjs");
const { buildMarkerSearchQuery, filterByMarker, logFilterSummary } = require("./close_older_search_helpers.cjs");

/**
Expand Down Expand Up @@ -186,32 +187,30 @@ function getCloseOlderIssueMessage({ newIssueUrl, newIssueNumber, workflowName,
* @returns {Promise<Array<{number: number, html_url: string}>>} List of closed issues
*/
async function closeOlderIssues(github, owner, repo, workflowId, newIssue, workflowName, runUrl, callerWorkflowId, closeOlderKey) {
const result = await closeOlderEntities(github, owner, repo, workflowId, newIssue, workflowName, runUrl, {
return closeOlderWithAdapter({
github,
owner,
repo,
workflowId,
newEntity: newIssue,
workflowName,
runUrl,
callerWorkflowId,
closeOlderKey,
entityType: "issue",
entityTypePlural: "issues",
// Use a closure so callerWorkflowId and closeOlderKey are forwarded to searchOlderIssues
// without going through the closeOlderEntities extraArgs mechanism (which appends
// excludeNumber last)
searchOlderEntities: (gh, o, r, wid, excludeNumber) => searchOlderIssues(gh, o, r, wid, excludeNumber, callerWorkflowId, closeOlderKey),
getCloseMessage: params =>
getCloseOlderIssueMessage({
newIssueUrl: params.newEntityUrl,
newIssueNumber: params.newEntityNumber,
workflowName: params.workflowName,
runUrl: params.runUrl,
}),
searchOlderEntities: searchOlderIssues,
getCloseMessage: getCloseOlderIssueMessage,
messageParams: params => ({
newIssueUrl: params.newEntityUrl,
newIssueNumber: params.newEntityNumber,
workflowName: params.workflowName,
runUrl: params.runUrl,
}),
addComment: addIssueComment,
closeEntity: closeIssueAsNotPlanned,
delayMs: API_DELAY_MS,
getEntityId: entity => entity.number,
getEntityUrl: entity => entity.html_url,
});

// Map to issue-specific return type
return result.map(item => ({
number: item.number,
html_url: item.html_url || "",
}));
}

module.exports = {
Expand Down
41 changes: 20 additions & 21 deletions actions/setup/js/close_older_pull_requests.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
/// <reference types="@actions/github-script" />

const { sanitizeContent } = require("./sanitize_content.cjs");
const { closeOlderEntities, MAX_CLOSE_COUNT: SHARED_MAX_CLOSE_COUNT } = require("./close_older_entities.cjs");
const { MAX_CLOSE_COUNT: SHARED_MAX_CLOSE_COUNT } = require("./close_older_entities.cjs");
const { closeOlderWithAdapter } = require("./close_older_adapter.cjs");
const { buildMarkerSearchQuery, filterByMarker, logFilterSummary } = require("./close_older_search_helpers.cjs");

/**
Expand Down Expand Up @@ -185,32 +186,30 @@ function getCloseOlderPullRequestMessage({ newPullRequestUrl, newPullRequestNumb
* @returns {Promise<Array<{number: number, html_url: string}>>} List of closed pull requests
*/
async function closeOlderPullRequests(github, owner, repo, workflowId, newPullRequest, workflowName, runUrl, callerWorkflowId, closeOlderKey) {
const result = await closeOlderEntities(github, owner, repo, workflowId, newPullRequest, workflowName, runUrl, {
return closeOlderWithAdapter({
github,
owner,
repo,
workflowId,
newEntity: newPullRequest,
workflowName,
runUrl,
callerWorkflowId,
closeOlderKey,
entityType: "pull request",
entityTypePlural: "pull requests",
// Use a closure so callerWorkflowId and closeOlderKey are forwarded to searchOlderPullRequests
// without going through the closeOlderEntities extraArgs mechanism (which appends
// excludeNumber last)
searchOlderEntities: (gh, o, r, wid, excludeNumber) => searchOlderPullRequests(gh, o, r, wid, excludeNumber, callerWorkflowId, closeOlderKey),
getCloseMessage: params =>
getCloseOlderPullRequestMessage({
newPullRequestUrl: params.newEntityUrl,
newPullRequestNumber: params.newEntityNumber,
workflowName: params.workflowName,
runUrl: params.runUrl,
}),
searchOlderEntities: searchOlderPullRequests,
getCloseMessage: getCloseOlderPullRequestMessage,
messageParams: params => ({
newPullRequestUrl: params.newEntityUrl,
newPullRequestNumber: params.newEntityNumber,
workflowName: params.workflowName,
runUrl: params.runUrl,
}),
addComment: addPullRequestComment,
closeEntity: closePullRequest,
delayMs: API_DELAY_MS,
getEntityId: entity => entity.number,
getEntityUrl: entity => entity.html_url,
});

// Map to pull-request-specific return type
return result.map(item => ({
number: item.number,
html_url: item.html_url || "",
}));
}

module.exports = {
Expand Down
Loading