Dedupe close-handler REST wrappers across entity flows#51649
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage
Draft, no CI runs yet, 0 reviews. Net -184/+33 with unit test coverage added. Low risk internal refactor of close-handler wrappers. Group with #51648, #51647 for batch review once ready for review.
|
|
Thanks for this refactoring! 👍 The consolidation of duplicate REST wrappers across the five close-entity flows is well-focused and clearly addresses #51646. Here's what stands out: ✅ Focused refactor — eliminates 184 lines of duplicate code by centralizing the shared helpers into close_rest_helpers.cjs. ✅ Tests included — new close_rest_helpers.test.cjs provides unit coverage for the shared helpers. ✅ Preserved semantics — call-site logging, error handling, and response shaping remain unchanged; no exported names shift. ✅ Low risk — net −184/+33 across handlers, with integration delegated cleanly to thin wrappers. This PR looks ready for review. The test coverage and detailed changelog make it easy for reviewers to verify correctness.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
There was a problem hiding this comment.
Pull request overview
Centralizes repeated Octokit REST operations used by close-entity workflows while preserving existing sanitization, logging, and result shapes.
Changes:
- Adds shared issue/PR fetch, comment, and close helpers with unit tests.
- Refactors standard, older, and expired entity flows to use the shared helpers.
- Preserves issue-intent handling and call-site sanitization.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/close_rest_helpers.cjs |
Defines shared REST wrappers. |
actions/setup/js/close_rest_helpers.test.cjs |
Tests shared wrapper behavior. |
actions/setup/js/close_issue.cjs |
Delegates issue REST operations. |
actions/setup/js/close_pull_request.cjs |
Delegates pull-request REST operations. |
actions/setup/js/close_older_issues.cjs |
Reuses helpers while preserving logging and shaping. |
actions/setup/js/close_older_pull_requests.cjs |
Reuses helpers while preserving logging and shaping. |
actions/setup/js/expired_entity_handler_factory.cjs |
Delegates expired-entity operations while retaining sanitization. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
🧪 Test Quality Sentinel ReportOverviewPR: #51649 — Dedupe close-handler REST wrappers Quality Score: 76/100
|
| Module | Test Count | Coverage | Notes |
|---|---|---|---|
getIssueDetails |
2 | Happy path + error (null data) | Both success & failure |
getPullRequestDetails |
2 | Happy path + error (null data) | Both success & failure |
addIssueThreadComment |
1 | Happy path only | Basic success case |
closeIssue |
2 | Happy path + default parameter | Parameter behavior verified |
closePullRequest |
1 | Happy path only | Basic success case |
| Total | 8 | — | — |
Quality Analysis
✅ Strengths
- Error path coverage: Fetch operations (
getIssueDetails,getPullRequestDetails) explicitly test error handling with.rejects.toThrow()forERR_NOT_FOUND. - Parameter defaults:
closeIssuedefaultstateReason("not_planned") is explicitly verified. - Mock API verification: All tests verify correct Octokit parameters (
owner,repo,issue_number, etc.) viatoHaveBeenCalledWith(). - Return value shape: Each test validates the returned object fields match what callers expect.
- Clean test isolation: Mocks are appropriate for external I/O (GitHub REST API), not business logic.
- No inflation: Test:source ratio is 106:123 = 0.86:1 (well below 2:1 threshold).
- Framework best practices: Uses
vitestwithbeforeEach,vi.fn(), async/await patterns correctly.
⚠️ Gaps & Observations
-
Happy-path heavy: 5 of 8 tests are happy-path. While appropriate for a newly extracted module, error paths could be deeper:
- ✓ Null data checked (
getIssueDetails,getPullRequestDetails) - ✗ What if Octokit promise itself rejects? (Not currently tested)
- ✓ Null data checked (
-
Limited edge cases:
addIssueThreadComment— no error path (what if createComment fails?)closePullRequest— no error path (what if update fails?)- These are wrapped by higher-level handlers (e.g.,
close_older_pull_requests.cjs), which test error logging, but the core wrapper has no failure test.
-
Test description specificity: "missing" could clarify "when API returns null data".
-
No schema validation: Mock calls verify parameter names but not Octokit API contract (e.g., if GitHub API changes parameter names).
Score Breakdown
Formula:
design_tests = 7 of 8 tests verify API contract (design-level)
edge_cases = 3 tests cover error/edge: 2 ERR_NOT_FOUND + 1 default param
duplicates = 0 (no pattern repetition)
inflation = 0.86:1 (< 2:1 threshold) → no penalty
score = (7/8 * 40) + (3/8 * 30) + (20 - 0) + 10
= 35 + 11.25 + 20 + 10
= 76
| Metric | Result | Status |
|---|---|---|
| Design-level tests | 87.5% | ✅ Excellent |
| Edge-case coverage | 37.5% | |
| Duplication | 0 clusters | ✅ Clean |
| Test inflation | 0.86:1 | ✅ Healthy |
| Implementation detail tests | 12.5% | ✅ Low (threshold: 30%) |
| Build tag violations | None | ✅ Pass |
| Mock library violations | None | ✅ Pass |
Assessment
✅ APPROVE — This is a high-confidence new test suite for a newly extracted helper module. The PR refactors duplicated REST wrappers from 5 files into 1 shared module, and these 8 tests validate:
- The public API contract (inputs/outputs) is correct
- Error handling for the two fetch operations is present
- Default parameters are honored
- Mocks don't over-specify business logic
The test:source ratio (0.86:1) is healthy, implementation detail coverage is low (12.5%, threshold 30%), and there are no build-tag or mock-library violations.
Risk: Future changes to addIssueThreadComment and closePullRequest error handling won't be caught by these tests, but they're wrapped by higher-level handlers that have their own error test coverage.
🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 15.9 AIC · ⌖ 2.92 AIC · ⊞ 7.6K · ◷
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on test correctness and a dead-code guard.
📋 Key Themes & Highlights
Key Issues
- ESM
importin a.cjsfile (lines 5–6 ofclose_rest_helpers.test.cjs) — Node.js will reject these at load time, so the tests will never run. All sibling test files userequire. This is the highest-priority fix. - Mock isolation risk — if ESM/CJS interop is unresolved,
vi.fn()stubs may not intercept the actual calls, producing false positives. - Unreachable
ERR_NOT_FOUNDguard — theif (!issue)check after a successful Octokit call is dead code; Octokit throws on 404 before returningdata. The existing behaviour is carried forward unchanged, but it's worth fixing now that it's in a shared module. - Hardcoded error-string literals in tests — should reference the
ERR_NOT_FOUNDconstant so test and code stay in sync. - Missing error-path tests for
addIssueThreadComment,closeIssue, andclosePullRequest.
Positive Highlights
- ✅ Clean extraction: shared module is narrow and single-purpose.
- ✅ Sanitization deliberately kept at call sites — good explicit boundary.
- ✅ No exported-name regressions across the five handler files.
- ✅ PR description is thorough and the before/after examples are clear.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.5 AIC · ⌖ 6.9 AIC · ⊞ 7.1K
Comment /matt to run again
|
|
||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { getIssueDetails, getPullRequestDetails, addIssueThreadComment, closeIssue, closePullRequest } from "./close_rest_helpers.cjs"; | ||
|
|
There was a problem hiding this comment.
[/tdd] ESM import syntax in a .cjs file will cause the test runner to fail — Node.js treats .cjs as CommonJS and rejects bare import statements.
💡 Fix: switch to require()
// Replace:
import { describe, it, expect, beforeEach, vi } from "vitest";
import { getIssueDetails, ... } from "./close_rest_helpers.cjs";
// With:
const { describe, it, expect, beforeEach, vi } = require("vitest");
const { getIssueDetails, ... } = require("./close_rest_helpers.cjs");All sibling test files use require — this file should match that convention.
@copilot please address this.
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { getIssueDetails, getPullRequestDetails, addIssueThreadComment, closeIssue, closePullRequest } from "./close_rest_helpers.cjs"; | ||
|
|
||
| describe("close_rest_helpers", () => { |
There was a problem hiding this comment.
[/tdd] Importing from ./close_rest_helpers.cjs via ESM import also means the vitest mock isolation (vi.fn()) inside a CommonJS module may not intercept calls correctly. If you keep ESM syntax, move this file to .test.mjs and verify the vitest config covers that extension — silently passing tests that don't actually call the mocks is a hard-to-catch regression.
@copilot please address this.
|
|
||
| if (!issue) { | ||
| throw new Error(`${ERR_NOT_FOUND}: Issue #${issueNumber} not found in ${owner}/${repo}`); | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The if (!issue) guard after a successful Octokit call is unreachable in practice — the REST API throws on 4xx/5xx, so data will never be null on a 200 response. This means the ERR_NOT_FOUND path is dead code and won't fire when a caller expects a not-found sentinel.
💡 Better pattern
Either rely on Octokit throwing a RequestError (status 404) and catch it upstream, or check issue.state / other meaningful fields instead of truthiness of data:
try {
const { data: issue } = await github.rest.issues.get({ ... });
return issue;
} catch (err) {
if (err.status === 404) throw new Error(`${ERR_NOT_FOUND}: ...`);
throw err;
}@copilot please address this.
| const result = await getPullRequestDetails(mockGithub, "owner", "repo", 42); | ||
|
|
||
| expect(mockGithub.rest.pulls.get).toHaveBeenCalledWith({ owner: "owner", repo: "repo", pull_number: 42 }); | ||
| expect(result).toEqual({ number: 42, title: "Test PR" }); |
There was a problem hiding this comment.
[/tdd] The getIssueDetails not-found test asserts on the hardcoded string "ERR_NOT_FOUND: Issue #123 not found in owner/repo" rather than using the ERR_NOT_FOUND constant from error_codes.cjs. If the constant value changes, this test passes silently while production behaviour diverges.
💡 Fix
const { ERR_NOT_FOUND } = require("./error_codes.cjs");
// ...
await expect(...).rejects.toThrow(`${ERR_NOT_FOUND}: Issue #123 not found in owner/repo`);@copilot please address this.
| owner: "owner", | ||
| repo: "repo", | ||
| issue_number: 123, | ||
| state: "closed", |
There was a problem hiding this comment.
[/tdd] addIssueThreadComment only has a happy-path test. The helper is the sole gate for comment creation across all five flows — add an error-path test to confirm network/API failures propagate rather than being silently swallowed.
💡 Suggested test
it('propagates API errors', async () => {
mockGithub.rest.issues.createComment.mockRejectedValue(new Error('rate limit'));
await expect(
addIssueThreadComment(mockGithub, 'owner', 'repo', 1, 'body')
).rejects.toThrow('rate limit');
});@copilot please address this.
There was a problem hiding this comment.
Review: Dedupe close-handler REST wrappers
The refactoring is clean and well-structured — the new close_rest_helpers.cjs correctly centralizes shared REST wrappers and the callers are updated consistently. One blocking issue was found in the test file.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.6 AIC · ⌖ 6.52 AIC · ⊞ 5.4K
| // @ts-check | ||
|
|
||
| import { describe, it, expect, beforeEach, vi } from "vitest"; | ||
| import { getIssueDetails, getPullRequestDetails, addIssueThreadComment, closeIssue, closePullRequest } from "./close_rest_helpers.cjs"; |
There was a problem hiding this comment.
Bug: ESM import syntax in a .cjs file
This test file uses ESM import declarations:
import { describe, it, expect, beforeEach, vi } from "vitest";
import { getIssueDetails, ... } from "./close_rest_helpers.cjs";Files with the .cjs extension are treated as CommonJS by Node.js. Using import statements here will cause a syntax error at runtime. Use require() instead, or rename the file to .test.mjs/.test.js if the project is configured as ESM.
@copilot please address this.
|
@copilot Quick triage nudge for this PR. Please review the blocking maintainer-facing feedback, refresh the branch if GitHub can update it cleanly, and run the Open review context (newest first):
Branch refresh was requested.
|
PR TriageCategory: refactor · Risk: low · Priority: low · Score: 40/100 Score breakdown
Recommended action: Batch: Automated triage via PR Triage Agent.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing blockers below, and run the Open review context (newest first):
Branch refresh was requested.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing blockers below, and run the Open review context (newest first):
Branch refresh was requested.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing blockers below, and run the Open review context (newest first):
Branch refresh was requested.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing blockers below, and run the Open review context (newest first):
Branch refresh was requested.
|
Five close-entity flows each defined their own near-identical Octokit wrappers for the same three operations: fetch issue/PR details, create an issue-thread comment, close an issue/PR.
Changes
actions/setup/js/close_rest_helpers.cjs— owns the raw REST calls:getIssueDetails,getPullRequestDetails(both withERR_NOT_FOUNDguards),addIssueThreadComment,closeIssue(…, stateReason = "not_planned"),closePullRequest.close_issue.cjs— dropped localgetIssueDetails/addIssueComment; legacy close path delegates to sharedcloseIssue. Issue-intent close path and native duplicate marking are unchanged.close_pull_request.cjs— dropped all three local wrappers.close_older_issues.cjs/close_older_pull_requests.cjs— keep exported names, per-call logging and{id, html_url}/{number, html_url}shaping; only the API call is delegated.expired_entity_handler_factory.cjs— keeps its exported wrappers (includingsanitizeContenton comment bodies) as thin delegates.close_rest_helpers.test.cjs— unit coverage for the shared wrappers.Sanitization stays at the call sites so semantics don't shift; the shared module never mutates bodies.
Net −184/+33 across the handlers, with no change to exported names or return shapes.
Run: https://github.com/github/gh-aw/actions/runs/31375792022> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.8 AIC · ⌖ 5.46 AIC · ⊞ 8.5K · ◷