diff --git a/docs/ci/pipeline.md b/docs/ci/pipeline.md index 30130de2bb8e..e8aaef9dd347 100644 --- a/docs/ci/pipeline.md +++ b/docs/ci/pipeline.md @@ -347,12 +347,20 @@ have a 30-second timeout. Secondary limits without timing guidance use at least one minute of exponential backoff. Small randomized delays spread retries after quota resets. Jobs have a 75-minute ceiling, and waiting occupies their runner. Recovery is automatic in the same run and does not require another PR event or -manual dispatch. Exhausted recovery fails the job without publishing success; -quota exhaustion can also prevent a new status from being published. Ordinary -permission errors, uncertain writes, and other evaluation errors are not retried. +manual dispatch. Exhausted recovery fails the job; GitHub errors can also prevent +a new status from being published. Ordinary permission errors and other +evaluation errors are not retried. Checkout, runtime setup, and separately minted autoscrub token expiry are outside this recovery mechanism. +Transient commit-status publication failures also restart the complete evaluation. +HTTP `500`, `502`, `503`, and `504` responses and recognized connection failures +use one-, two-, and four-second delays, sharing the three-restart limit and job +deadline with rate-limit recovery. GitHub may have accepted the failed write, so +the review rereads current PR, approval, role, and CI data instead of replaying an +old decision. This recovery applies only to commit-status publication; other +uncertain writes, cancellation, and request timeouts remain errors. + Separately, read-only `GET` and `HEAD` requests retry HTTP `500`, `502`, `503`, and `504` responses and recognized transient connection failures before a response arrives. They share one retry budget of one, two, and four seconds, diff --git a/scripts/github/guard-review.mjs b/scripts/github/guard-review.mjs index 3872ee4866d7..7ca484f26e87 100644 --- a/scripts/github/guard-review.mjs +++ b/scripts/github/guard-review.mjs @@ -157,7 +157,17 @@ export async function openGuard({ context, commentMarker, approvalCommand }, pre if (error instanceof GitHubRateLimitError) { throw error; } - await publishGuardStatus(guard, "failure", "Security review policy could not be evaluated"); + await publishGuardStatus( + guard, + "failure", + "Security review policy could not be evaluated", + ).catch( + /** @param {unknown} publicationError */ (publicationError) => { + console.error( + publicationError instanceof Error ? publicationError.message : String(publicationError), + ); + }, + ); throw error; } if (rollout.mode !== "enforced") { diff --git a/scripts/github/guard-shared.mjs b/scripts/github/guard-shared.mjs index d84f06831093..61307448408f 100644 --- a/scripts/github/guard-shared.mjs +++ b/scripts/github/guard-shared.mjs @@ -21,8 +21,8 @@ const githubApiRetryCodes = new Set([ const githubApiRetryDelaysMs = [1_000, 2_000, 4_000]; // One primary quota window plus room for a fresh evaluation. Persist the deadline // across detect/autoscrub/enforce so each step cannot start another hour of waits. -const githubRateLimitBudgetMs = 65 * 60_000; -const rateLimitDeadlineEnv = "OPENCLAW_SECURITY_REVIEW_DEADLINE_MS"; +const securityReviewBudgetMs = 65 * 60_000; +const recoveryDeadlineEnv = "OPENCLAW_SECURITY_REVIEW_DEADLINE_MS"; export class GitHubRateLimitError extends Error { constructor(message, response) { @@ -42,36 +42,44 @@ export class GitHubRateLimitError extends Error { } } -export async function withGitHubRateLimitRecovery(evaluate) { - const recorded = process.env[rateLimitDeadlineEnv]; - const deadline = recorded === undefined ? Date.now() + githubRateLimitBudgetMs : Number(recorded); +export class GitHubStatusPublicationError extends Error { + constructor(cause) { + super(cause.message, { cause }); + } +} + +export async function withSecurityReviewRecovery(evaluate) { + const recorded = process.env[recoveryDeadlineEnv]; + const deadline = recorded === undefined ? Date.now() + securityReviewBudgetMs : Number(recorded); if (!Number.isSafeInteger(deadline) || deadline <= 0) { throw new Error("Invalid Security Review recovery deadline."); } if (recorded === undefined && process.env.GITHUB_ENV) { - await appendFile(process.env.GITHUB_ENV, `${rateLimitDeadlineEnv}=${deadline}\n`); + await appendFile(process.env.GITHUB_ENV, `${recoveryDeadlineEnv}=${deadline}\n`); } for (let attempt = 0; ; attempt += 1) { try { return await evaluate(); } catch (error) { - if (!(error instanceof GitHubRateLimitError)) { + const rateLimited = error instanceof GitHubRateLimitError; + if (!rateLimited && !(error instanceof GitHubStatusPublicationError)) { throw error; } // Do not resume a status write with stale authority after waiting. The // caller restarts from live PR, file, comment, role, and CI observations. - const delay = - Math.max(error.retryAt - Date.now(), 60_000 * 2 ** attempt) + - 1_000 + - Math.floor(Math.random() * 15_000); + const delay = rateLimited + ? Math.max(error.retryAt - Date.now(), 60_000 * 2 ** attempt) + + 1_000 + + Math.floor(Math.random() * 15_000) + : githubApiRetryDelaysMs[attempt]; if (attempt >= 3 || Date.now() + delay + GITHUB_API_REQUEST_TIMEOUT_MS > deadline) { throw new Error( - "GitHub API rate-limit recovery budget exhausted; security review remains incomplete.", + "GitHub API recovery budget exhausted; security review remains incomplete.", { cause: error }, ); } console.warn( - `GitHub API rate limited (${error.status}); retrying the complete evaluation in ${Math.ceil(delay / 1_000)}s (attempt ${attempt + 1}/3).`, + `${rateLimited ? `GitHub API rate limited (${error.status})` : `GitHub status publication failed (${error.message})`}; retrying the complete evaluation in ${Math.ceil(delay / 1_000)}s (attempt ${attempt + 1}/3).`, ); await wait(delay); } @@ -163,18 +171,25 @@ export async function publishGuardStatus(guard, state, description) { } } } - await guard.api.request( - `/repos/${guard.owner}/${guard.repo}/statuses/${guard.pullRequest.head.sha}`, - { - method: "POST", - body: JSON.stringify({ - context: guard.context, - state, - description: `PR #${guard.pullRequest.number}: ${description}`, - target_url: guard.runUrl, - }), - }, - ); + try { + await guard.api.request( + `/repos/${guard.owner}/${guard.repo}/statuses/${guard.pullRequest.head.sha}`, + { + method: "POST", + body: JSON.stringify({ + context: guard.context, + state, + description: `PR #${guard.pullRequest.number}: ${description}`, + target_url: guard.runUrl, + }), + }, + ); + } catch (error) { + if (githubApiRetryStatuses.has(error?.status) || githubApiRetryCodes.has(error?.code)) { + throw new GitHubStatusPublicationError(error); + } + throw error; + } } export function sanitizeGuardDisplayValue(value) { @@ -364,10 +379,14 @@ export function createGitHubApi(token, options = {}) { continue; } const detail = error instanceof Error ? error.message : String(error); - throw new Error( + const requestError = new Error( `GitHub API ${method} ${path} failed: ${code ? `${code}: ` : ""}${detail}`, { cause: error }, ); + if (!requestSignal.aborted) { + requestError.code = code; + } + throw requestError; } if (response.status === 204) { return null; diff --git a/scripts/github/security-review-event.mjs b/scripts/github/security-review-event.mjs index a3d5fa68d944..85c936080ab8 100644 --- a/scripts/github/security-review-event.mjs +++ b/scripts/github/security-review-event.mjs @@ -6,7 +6,7 @@ import { parseApprovalCommands, publishGuardStatus, readSecurityReviewHistory, - withGitHubRateLimitRecovery, + withSecurityReviewRecovery, } from "./guard-shared.mjs"; const shaPattern = /^[a-f0-9]{40}$/u; @@ -201,7 +201,7 @@ async function main() { console.log(matrix); } -withGitHubRateLimitRecovery(main).catch( +withSecurityReviewRecovery(main).catch( /** @param {unknown} error */ (error) => { console.error(error instanceof Error ? error.message : String(error)); process.exitCode = 1; diff --git a/scripts/github/security-review.mjs b/scripts/github/security-review.mjs index 68c182ce2e13..bf12cf82240a 100644 --- a/scripts/github/security-review.mjs +++ b/scripts/github/security-review.mjs @@ -10,8 +10,9 @@ import { } from "./guard-review.mjs"; import { GitHubRateLimitError, + GitHubStatusPublicationError, publishGuardStatus, - withGitHubRateLimitRecovery, + withSecurityReviewRecovery, } from "./guard-shared.mjs"; import { securityReviewRollout } from "./security-review-rollout.mjs"; import { reviewSecuritySensitiveChanges } from "./security-sensitive-guard.mjs"; @@ -152,7 +153,9 @@ async function main() { } catch (error) { if ( error instanceof GitHubRateLimitError || - (error instanceof SupersededReviewError && errors.length === 0) + ((error instanceof GitHubStatusPublicationError || + error instanceof SupersededReviewError) && + errors.length === 0) ) { throw error; } @@ -223,20 +226,30 @@ async function main() { "CI and applicable security review requirements passed", ); } catch (error) { - if (error instanceof GitHubRateLimitError || error instanceof SupersededReviewError) { + if ( + error instanceof GitHubRateLimitError || + error instanceof GitHubStatusPublicationError || + error instanceof SupersededReviewError + ) { throw error; } await publishGuardStatus( review, "failure", "CI or security review failed; see workflow details", + ).catch( + /** @param {unknown} publicationError */ (publicationError) => { + console.error( + publicationError instanceof Error ? publicationError.message : String(publicationError), + ); + }, ); throw error; } } if (import.meta.url === `file://${process.argv[1]}`) { - withGitHubRateLimitRecovery(main).catch( + withSecurityReviewRecovery(main).catch( /** @param {unknown} error */ (error) => { if (error instanceof SupersededReviewError) { console.log(error.message); diff --git a/test/fixtures/github-guard-fetch.mjs b/test/fixtures/github-guard-fetch.mjs index 87f1836b3f59..5d57797e0117 100644 --- a/test/fixtures/github-guard-fetch.mjs +++ b/test/fixtures/github-guard-fetch.mjs @@ -35,6 +35,12 @@ globalThis.fetch = async (url, options = {}) => { ? route.responses.shift() : route.responses[0] : route; + if (value?.recordStatusBeforeError) recordStatus(); + if (value?.transportError) { + throw new TypeError("fetch failed", { + cause: Object.assign(new Error("Fixture connection failure"), { code: value.transportError }), + }); + } if (value?.httpError) { return new Response(JSON.stringify({ message: value.message ?? "Fixture API failure" }), { status: value.httpError, diff --git a/test/scripts/dependency-guard-script.test.ts b/test/scripts/dependency-guard-script.test.ts index d801e18f731d..d7717b0839ae 100644 --- a/test/scripts/dependency-guard-script.test.ts +++ b/test/scripts/dependency-guard-script.test.ts @@ -991,20 +991,26 @@ describe("dependency guard script", () => { expect(fetchImpl).toHaveBeenCalledTimes(1); }); - it("does not retry a connection error after the caller aborts", async () => { - const controller = new AbortController(); - const error = new TypeError("fetch failed", { - cause: Object.assign(new Error(), { code: "ECONNRESET" }), - }); - const fetchImpl = vi.fn().mockImplementation(async () => { - controller.abort(); - throw error; - }); - await expect( - githubApi("token", { fetchImpl }).request(pullPath, { signal: controller.signal }), - ).rejects.toMatchObject({ cause: error }); - expect(fetchImpl).toHaveBeenCalledTimes(1); - }); + it.each(["GET", "POST"])( + "does not retry or mark an aborted %s connection error for recovery", + async (method) => { + const controller = new AbortController(); + const error = new TypeError("fetch failed", { + cause: Object.assign(new Error(), { code: "ECONNRESET" }), + }); + const fetchImpl = vi.fn().mockImplementation(async () => { + controller.abort(); + throw error; + }); + const request = githubApi("token", { fetchImpl }).request(pullPath, { + method, + signal: controller.signal, + }); + await expect(request).rejects.toMatchObject({ cause: error }); + await expect(request).rejects.not.toHaveProperty("code"); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }, + ); it("bounds successful GitHub API response bodies", async () => { const request = githubApi("token", { diff --git a/test/scripts/security-review-event.test.ts b/test/scripts/security-review-event.test.ts index f6f219f9f068..e194d8bb3919 100644 --- a/test/scripts/security-review-event.test.ts +++ b/test/scripts/security-review-event.test.ts @@ -163,6 +163,33 @@ describe("automatic security review event resolution", () => { expect(result.requests.map(({ method }) => method)).toEqual(["GET", "WAIT", "GET", "POST"]); }); + it("resolves the fresh head after a failed pending status before scheduling review", () => { + const nextHead = "b".repeat(40); + const result = evaluate({ + eventName: "pull_request_target", + event: { action: "opened", pull_request: { number: 42 } }, + responses: { + [`${prefix}/pulls/42`]: [ + { body: pullRequest }, + { body: { ...pullRequest, head: { ...pullRequest.head, sha: nextHead } } }, + ], + [`${prefix}/statuses/${head}`]: { status: 500, body: { message: "Server error" } }, + }, + }); + expect(result.status, result.error).toBe(0); + expect(result.waits).toEqual([1_000]); + expect(result.matrix).toEqual({ include: [{ pr: 42, head: nextHead }] }); + expect(result.published.map(({ path }) => path)).toEqual([ + `${prefix}/statuses/${head}`, + `${prefix}/statuses/${nextHead}`, + ]); + expect(result.published.every(({ hadOutput }) => !hadOutput)).toBe(true); + expect(result.published.at(-1)?.body?.state).toBe("pending"); + expect(result.output).toBe( + `matrix={"include":[{"pr":42,"head":"${nextHead}"}]}\nhas-prs=true\n`, + ); + }); + it.each(["pull_request_target", "issue_comment"])( "resolves %s through current PR metadata", (eventName) => { @@ -209,6 +236,7 @@ describe("automatic security review event resolution", () => { }, }); expect(result).toMatchObject({ status: 1, output: "" }); + expect(result.waits).toEqual([]); expect(result.published).toHaveLength(1); expect(result.published[0]?.hadOutput).toBe(false); }); diff --git a/test/scripts/security-review-script.test.ts b/test/scripts/security-review-script.test.ts index b8e03a764a56..1f0eacb082c5 100644 --- a/test/scripts/security-review-script.test.ts +++ b/test/scripts/security-review-script.test.ts @@ -42,6 +42,7 @@ const jobs = { jobs: [{ name: "openclaw/ci-gate", status: "completed", conclusion: "success" }], }; const rolePath = "GET /repos/openclaw/openclaw/collaborators/maintainer/permission"; +const statusPath = `POST /repos/openclaw/openclaw/statuses/${head}`; const runsPath = `GET ${actions}/workflows/ci.yml/runs`; const jobsPath = `GET ${actions}/runs/10/attempts/1/jobs`; const files = [ @@ -251,11 +252,32 @@ describe("combined security review entry point", () => { }, ); - it("rereads authority after a rate-limited success write instead of replaying it", () => { + it.each([ + { httpError: 500 }, + { httpError: 502 }, + { httpError: 503 }, + { httpError: 504 }, + { transportError: "ECONNRESET" }, + ])("restarts evaluation after a transient status publication failure: %j", (failure) => { + const result = evaluate({ [statusPath]: { responses: [failure, {}] } }); + expect(result.status, result.stderr).toBe(0); + expect(result.waits).toEqual([1_000]); + const afterWait = result.requests.slice( + result.requests.findIndex((entry) => entry.method === "WAIT") + 1, + ); + expect(afterWait[0]).toMatchObject({ method: "GET", path: pullPath }); + expect(result.combined.at(-1)).toBe("success"); + }); + + it.each([ + { httpError: 429 }, + { httpError: 500, recordStatusBeforeError: true }, + { transportError: "ECONNRESET" }, + ])("rereads authority after a failed success write instead of replaying it: %j", (failure) => { const result = evaluate({ // First POST records pending; the dependency guard then records failure and success. - [`POST /repos/openclaw/openclaw/statuses/${head}`]: { - responses: [{}, {}, { httpError: 429 }, {}], + [statusPath]: { + responses: [{}, {}, failure, {}], }, [rolePath]: { responses: [{ role_name: "maintain" }, { role_name: "maintain" }, { role_name: "read" }], @@ -271,6 +293,83 @@ describe("combined security review entry point", () => { expect(result.combined.at(-1)).toBe("failure"); }); + it("stops after the head changes while recovering a failed success write", () => { + const result = evaluate({ + [statusPath]: { responses: [{}, {}, { httpError: 500 }, {}] }, + [`GET ${pullPath}`]: { + responses: [pr, pr, pr, { ...pr, head: { ...pr.head, sha: "d".repeat(40) } }], + }, + }); + expect(result.status, result.stderr).toBe(0); + expect(result.waits).toEqual([1_000]); + expect(result.stdout).toContain("Superseded"); + const afterWait = result.requests.slice( + result.requests.findIndex((entry) => entry.method === "WAIT") + 1, + ); + expect(afterWait.every((entry) => entry.method === "GET")).toBe(true); + expect(result.combined).not.toContain("success"); + }); + + it("rereads CI after a failed combined-success publication", () => { + const result = evaluate({ + [statusPath]: { responses: [{}, {}, {}, {}, {}, { httpError: 500 }, {}] }, + [jobsPath]: { + responses: [jobs, { ...jobs, jobs: [{ ...jobs.jobs[0], conclusion: "failure" }] }], + }, + }); + expect(result.status, result.stderr).toBe(0); + expect(result.waits).toEqual([1_000]); + expect(result.combined.at(-1)).toBe("failure"); + expect(result.requests.filter((entry) => `GET ${entry.path}` === jobsPath)).toHaveLength(2); + }); + + it("bounds persistent status publication failures without granting approval", () => { + const result = evaluate({ [statusPath]: { httpError: 500 } }); + expect(result.status).toBe(1); + expect(result.waits).toEqual([1_000, 2_000, 4_000]); + expect(result.requests.filter((entry) => entry.method === "POST")).toHaveLength(4); + expect(result.stderr).toContain("recovery budget exhausted"); + expect(result.requests.some((entry) => entry.body?.state === "success")).toBe(false); + }); + + it("shares three evaluation restarts between status failures and rate limits", () => { + const result = evaluate({ + [statusPath]: { + responses: [{ httpError: 500 }, { httpError: 429 }, { httpError: 500 }], + }, + }); + expect(result.status).toBe(1); + expect(result.waits).toHaveLength(3); + expect(result.waits[0]).toBe(1_000); + expect(result.waits[1]).toBeGreaterThanOrEqual(120_000); + expect(result.waits[1]).toBeLessThan(137_000); + expect(result.waits[2]).toBe(4_000); + expect(result.requests.filter((entry) => entry.method === "POST")).toHaveLength(4); + expect(result.stderr).toContain("recovery budget exhausted"); + expect(result.combined).not.toContain("success"); + }); + + it.each([ + { name: "comment only", statusReplies: [{}] }, + { name: "failure report also fails", statusReplies: [{}, {}, {}, {}, {}, { httpError: 500 }] }, + { name: "sibling status also fails", statusReplies: [{}, {}, {}, { httpError: 500 }, {}] }, + ])("does not restart evaluation for a failed comment publication: $name", ({ statusReplies }) => { + const commentPath = "/repos/openclaw/openclaw/issues/7/comments"; + const result = evaluate({ + [statusPath]: { responses: statusReplies }, + [`GET ${pullPath}`]: { ...pr, changed_files: 1 }, + [`GET ${pullPath}/files`]: [files[1]], + [`POST ${commentPath}`]: { httpError: 500 }, + }); + expect(result.status).toBe(1); + expect(result.waits).toEqual([]); + expect( + result.requests.filter((entry) => entry.method === "POST" && entry.path === commentPath), + ).toHaveLength(1); + expect(result.combined.at(-1)).toBe("failure"); + expect(result.stderr).toContain(`GitHub API POST ${commentPath} failed: 500`); + }); + it("recovers rate-limited notice writes instead of treating them as missing permissions", () => { const result = evaluate({ "POST /repos/openclaw/openclaw/issues/7/labels": { @@ -294,24 +393,30 @@ describe("combined security review entry point", () => { expect(result.requests.some((entry) => entry.body?.state === "success")).toBe(false); }); - it("does not shorten a server wait to fit the shared recovery deadline", () => { - const result = evaluate( - { [`GET ${pullPath}`]: { httpError: 429, headers: { "retry-after": "120" } } }, - "enforce", - Date.parse("2026-01-02T00:01:00Z"), - ); + it.each([ + { + route: `GET ${pullPath}`, + failure: { httpError: 429, headers: { "retry-after": "120" } }, + deadline: "2026-01-02T00:01:00Z", + }, + { route: statusPath, failure: { httpError: 500 }, deadline: "2026-01-02T00:00:30Z" }, + ])("keeps $route recovery within the shared deadline", ({ route, failure, deadline }) => { + const result = evaluate({ [route]: failure }, "enforce", Date.parse(deadline)); expect(result.status).toBe(1); expect(result.stderr).toContain("recovery budget exhausted"); expect(result.waits).toEqual([]); - expect(result.combined).toEqual([]); + expect(result.combined).not.toContain("success"); }); - it("does not retry an ordinary permission rejection", () => { - const result = evaluate({ [rolePath]: { httpError: 403 } }); - expect(result.status).toBe(1); - expect(result.waits).toEqual([]); - expect(result.combined.at(-1)).toBe("failure"); - }); + it.each([rolePath, statusPath])( + "does not retry an ordinary permission rejection: %s", + (route) => { + const result = evaluate({ [route]: { httpError: 403 } }); + expect(result.status).toBe(1); + expect(result.waits).toEqual([]); + expect(result.combined).toEqual(route === rolePath ? ["pending", "failure"] : ["pending"]); + }, + ); it("requires successful CI and both guard decisions on the actual PR head", () => { const result = evaluate();