Deduplicate closeOlder issue/PR wrapper wiring with a shared adapter#45045
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>
🤖 PR Triage
Rationale: Draft. Deduplicates closeOlder issue/PR wrapper wiring into shared adapter. Low risk, 4 files. Batch with other refactor/dedup PRs. Triage run §29183606049
|
|
Hey This PR looks well-structured: it's focused on a single concern, includes a dedicated test file (
|
🤖 PR Triage
Draft. Deduplication refactor with moderate size (+199/-42). Review together with the refactor-dedup batch.
|
🤖 PR Triage
Rationale: DRAFT — deduplicates
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45045 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Centralizes duplicated issue/PR close-older orchestration in a shared adapter while preserving entity-specific behavior.
Changes:
- Adds a shared adapter for search context, message mapping, and result normalization.
- Simplifies issue and pull-request wrappers.
- Adds focused adapter tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/close_older_adapter.cjs |
Implements shared adapter wiring. |
actions/setup/js/close_older_adapter.test.cjs |
Tests forwarding, transformation, and normalization. |
actions/setup/js/close_older_issues.cjs |
Migrates issue handling to the adapter. |
actions/setup/js/close_older_pull_requests.cjs |
Migrates PR handling to the adapter. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Deduplicate closeOlder issue/PR wrapper wiring
The refactor is clean and well-structured. The adapter correctly centralizes callerWorkflowId/closeOlderKey forwarding via the search closure, and the messageParams indirection is a good design.
One issue to fix: The new test file imports the CJS module under test with a bare ESM import (line 4), which diverges from the project convention. All peer test files use createRequire(import.meta.url) to share the CJS module cache — this matters for correct mock behaviour with vi. See the inline comment for details.
No other issues found in the adapter implementation or the wrapper simplifications.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 29.2 AIC · ⌖ 4.4 AIC · ⊞ 4.8K
| // @ts-check | ||
|
|
||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { closeOlderWithAdapter } from "./close_older_adapter.cjs"; |
There was a problem hiding this comment.
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.
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 43/100 — Needs Improvement
📊 Metrics (3 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — clean refactor with good test coverage; a few suggestions to tighten the interface.
📋 Key Themes & Highlights
Key Themes
messageParamsdouble-indirection — the adapter accepts bothgetCloseMessageandmessageParamswhen one would suffice, widening the interface unnecessarily.- Silent empty-string coercion — missing
html_urlis silently normalised to"", which could produce broken links for callers that render URLs. - Import/require mismatch — test file uses ES module
importin a.cjsfile; works if vitest is configured for it, but the setup is undocumented.
Positive Highlights
- ✅ Clean extraction of shared orchestration — both wrappers are noticeably simpler.
- ✅ Closure-based forwarding of
callerWorkflowId/closeOlderKeyis well-reasoned and the comment explaining it is helpful. - ✅ Tests cover all three key adapter behaviours (forwarding context, message transformation, result normalisation).
- ✅ PR description includes a clear code example of the new call site — great for reviewers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.3 AIC · ⌖ 4.91 AIC · ⊞ 6.6K
Comment /matt to run again
| * @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}>>} |
There was a problem hiding this comment.
{"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."}
| * @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}>>} |
| * @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}>>} |
There was a problem hiding this comment.
[/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.
| // @ts-check | ||
|
|
||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { closeOlderWithAdapter } from "./close_older_adapter.cjs"; |
There was a problem hiding this comment.
[/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.
|
|
||
| return result.map(item => ({ | ||
| number: item.number, | ||
| html_url: item.html_url || "", |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Non-blocking review — one medium concern.
The refactor cleanly eliminates duplicated adapter wiring and the tests cover the key forwarding behavior. One issue worth addressing before further extension of this pattern:
** leaks internals.** Callers must know that the callback receives — a shape that lives entirely inside close_older_entities.cjs. If those field names change, all callers silently produce undefined-filled close messages with no compiler or test signal. Adding a @typedef CloseMessageParams in the adapter (or removing the indirection) would seal this gap.
🔎 Code quality review by PR Code Quality Reviewer · 50.3 AIC · ⌖ 5.13 AIC · ⊞ 5.4K
Comment /review to run again
| * @param {object} params | ||
| * @param {any} params.github | ||
| * @param {string} params.owner | ||
| * @param {string} params.repo |
There was a problem hiding this comment.
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:
- Add a
@typedef CloseMessageParamsJSDoc visible to callers documenting the expected shape. - Pass
newEntity+ context directly togetCloseMessageand remove themessageParamsindirection entirely.
closeOlderIssuesandcloseOlderPullRequestshad near-identical wrapper logic aroundcloseOlderEntities, creating duplicated adapter wiring and drift risk. This change centralizes the shared orchestration while preserving entity-specific behavior and output shape.Refactor: extract shared closeOlder adapter
actions/setup/js/close_older_adapter.cjswithcloseOlderWithAdapter(...)to handle:closeOlderEntities(...)config wiringcallerWorkflowId/closeOlderKeyinto search closures{ number, html_url }result mappingWrapper simplification: issues + pull requests
actions/setup/js/close_older_issues.cjsactions/setup/js/close_older_pull_requests.cjssearchOlder*, message mapping, comment/close handlers, labels), removing duplicated plumbing.Coverage for shared adapter behavior
actions/setup/js/close_older_adapter.test.cjsto validate:callerWorkflowId,closeOlderKey)getCloseMessagehtml_urlin mapped results