mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 02:06:43 +00:00
fix(scripts): bound duplicate PR closure GitHub lookups (#110750)
* fix(scripts): bound duplicate PR closure GitHub lookups The duplicate PR closure helper performs multiple sequential gh API reads and writes after a merge. Each call used execFileSync without a timeout, so one stalled GitHub request could block the surrounding workflow job until it expires. Add a 60-second timeout and SIGKILL to the default gh runner, applied to both read-only lookups and pr edit writes that forward stdin. * test(scripts): add real behavior proof for duplicate PR closure timeout Add two live subprocess tests using the real spawnSync (not mocked) to prove the timeout + SIGKILL surface works against actual child processes: - A 1 ms timeout against a long-running node subprocess proves SIGKILL is delivered and the timeout triggers correctly. - A fast node -e 0 subprocess with the production 60-second bound proves the timeout does not false-positive on healthy completions. Switch the vi.mock to a partial mock using importOriginal so spawnSync remains the real implementation for live tests while execFileSync stays mocked for the DI-based option assertions. * refactor(scripts): inject duplicate closure GitHub runner * fix(scripts): declare trusted tooling pin validator --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
9d0836ee25
commit
e33257de11
@@ -25,6 +25,26 @@ export function parseArgs(
|
||||
landedPr: number;
|
||||
repo: string;
|
||||
};
|
||||
/**
|
||||
* Runs a GitHub CLI command with the workflow timeout policy.
|
||||
*/
|
||||
export function defaultRunGh(
|
||||
args: string[],
|
||||
options?: { input?: string },
|
||||
params?: {
|
||||
execFileSyncImpl?: (
|
||||
command: string,
|
||||
args: string[],
|
||||
options: {
|
||||
encoding: "utf8";
|
||||
input?: string;
|
||||
killSignal: "SIGKILL";
|
||||
stdio: ["ignore", "pipe", "inherit"] | ["pipe", "pipe", "inherit"];
|
||||
timeout: number;
|
||||
},
|
||||
) => string;
|
||||
},
|
||||
): string;
|
||||
/**
|
||||
* Parses changed hunk ranges from unified diff text.
|
||||
*/
|
||||
|
||||
@@ -3,6 +3,10 @@ import { execFileSync } from "node:child_process";
|
||||
import { pathToFileURL } from "node:url";
|
||||
|
||||
const DEFAULT_LABELS = ["duplicate", "close:duplicate", "dedupe:child"];
|
||||
// Duplicate PR closure performs multiple sequential gh API reads and writes.
|
||||
// Keep enough headroom for GitHub latency while preventing one stalled request
|
||||
// from blocking the surrounding workflow job.
|
||||
const GH_COMMAND_TIMEOUT_MS = 60_000;
|
||||
|
||||
function usage() {
|
||||
return `Usage: node scripts/close-duplicate-prs-after-merge.mjs --landed-pr <number> --duplicates <numbers> [--repo owner/repo] [--apply]
|
||||
@@ -89,10 +93,13 @@ function ghJson(args, runGh) {
|
||||
return JSON.parse(runGh(args));
|
||||
}
|
||||
|
||||
function defaultRunGh(args, options = {}) {
|
||||
return execFileSync("gh", args, {
|
||||
export function defaultRunGh(args, options = {}, params = {}) {
|
||||
const execFileSyncImpl = params.execFileSyncImpl ?? execFileSync;
|
||||
return execFileSyncImpl("gh", args, {
|
||||
encoding: "utf8",
|
||||
killSignal: "SIGKILL",
|
||||
stdio: options.input ? ["pipe", "pipe", "inherit"] : ["ignore", "pipe", "inherit"],
|
||||
timeout: GH_COMMAND_TIMEOUT_MS,
|
||||
...(options.input ? { input: options.input } : {}),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -120,6 +120,17 @@ export function validateCandidateCheckout({
|
||||
toolingSha: unknown;
|
||||
workflowRef: unknown;
|
||||
};
|
||||
export function validateTrustedToolingPin({
|
||||
toolingSha,
|
||||
pinnedToolingSha,
|
||||
latestTrustedToolingSha,
|
||||
isAncestor,
|
||||
}: {
|
||||
toolingSha: string;
|
||||
pinnedToolingSha: string;
|
||||
latestTrustedToolingSha: string;
|
||||
isAncestor?: (ancestor: string, target: string) => boolean;
|
||||
}): string;
|
||||
export function candidateCumulativeShippedPullRequests(
|
||||
changelog: string,
|
||||
label: string,
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
// Close Duplicate Prs After Merge tests cover close duplicate prs after merge script behavior.
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
applyClosePlan,
|
||||
buildDuplicateClosePlan,
|
||||
defaultRunGh,
|
||||
parseArgs,
|
||||
parsePrNumberList,
|
||||
parseUnifiedDiffRanges,
|
||||
@@ -253,3 +254,53 @@ Closing #70592 as a duplicate.`,
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("defaultRunGh", () => {
|
||||
it("bounds each GitHub lookup with a timeout and SIGKILL", () => {
|
||||
const execFileSyncImpl = vi.fn(() => "result");
|
||||
|
||||
const result = defaultRunGh(["pr", "view", "123"], {}, { execFileSyncImpl });
|
||||
|
||||
expect(result).toBe("result");
|
||||
expect(execFileSyncImpl).toHaveBeenCalledWith(
|
||||
"gh",
|
||||
["pr", "view", "123"],
|
||||
expect.objectContaining({
|
||||
encoding: "utf8",
|
||||
killSignal: "SIGKILL",
|
||||
stdio: ["ignore", "pipe", "inherit"],
|
||||
timeout: 60_000,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("forwards stdin input while keeping the timeout bound", () => {
|
||||
const execFileSyncImpl = vi.fn(() => "ok");
|
||||
|
||||
defaultRunGh(
|
||||
["pr", "edit", "123", "--add-label", "duplicate"],
|
||||
{ input: "body" },
|
||||
{ execFileSyncImpl },
|
||||
);
|
||||
|
||||
expect(execFileSyncImpl).toHaveBeenCalledWith(
|
||||
"gh",
|
||||
["pr", "edit", "123", "--add-label", "duplicate"],
|
||||
expect.objectContaining({
|
||||
input: "body",
|
||||
killSignal: "SIGKILL",
|
||||
stdio: ["pipe", "pipe", "inherit"],
|
||||
timeout: 60_000,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("propagates timeout failures from the GitHub CLI process", () => {
|
||||
const timeout = Object.assign(new Error("spawnSync gh ETIMEDOUT"), { code: "ETIMEDOUT" });
|
||||
const execFileSyncImpl = vi.fn(() => {
|
||||
throw timeout;
|
||||
});
|
||||
|
||||
expect(() => defaultRunGh(["pr", "view", "123"], {}, { execFileSyncImpl })).toThrow(timeout);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user