From e7268b874119b99f024ff9d26d4379bf7a987e9b Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 02:57:47 +0530 Subject: [PATCH 01/10] fix(ci): fail closed when review evaluation is incomplete Signed-off-by: Tapish Khandelwal --- .github/workflows/review-gate.yml | 7 +- scripts/check-pr-review-gate.mjs | 199 ++++++++++++++++++++++++++---- 2 files changed, 177 insertions(+), 29 deletions(-) diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index db6bacd0..ed575bd7 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -12,6 +12,8 @@ on: types: [submitted, edited, dismissed] pull_request_review_comment: types: [created, edited, deleted] + issue_comment: + types: [created, edited, deleted] permissions: contents: read @@ -20,6 +22,7 @@ permissions: jobs: review-gate: name: Review gate + if: github.event_name != 'issue_comment' || github.event.issue.pull_request runs-on: ubuntu-latest timeout-minutes: 8 @@ -30,7 +33,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} EVENT_NAME: ${{ github.event_name }} - PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr }} EVENT_HEAD_REF: ${{ github.event.pull_request.head.ref }} EVENT_BASE_REF: ${{ github.event.pull_request.base.ref }} EVENT_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} @@ -43,7 +46,7 @@ jobs: exit 1 fi - if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then + if [ "${EVENT_NAME}" = "workflow_dispatch" ] || [ "${EVENT_NAME}" = "issue_comment" ]; then mapfile -t pr_metadata < <( gh pr view "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" \ --json headRefName,baseRefName,headRepository,headRefOid \ diff --git a/scripts/check-pr-review-gate.mjs b/scripts/check-pr-review-gate.mjs index 09b24143..6d764725 100644 --- a/scripts/check-pr-review-gate.mjs +++ b/scripts/check-pr-review-gate.mjs @@ -3,31 +3,56 @@ import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; +const BLOCKING_STATE_EXIT_CODE = 1; +const EVALUATION_FAILURE_EXIT_CODE = 2; +const DEFAULT_GH_RETRY_ATTEMPTS = 6; +const DEFAULT_GH_RETRY_BACKOFF_MS = 10_000; +const MAX_GH_RETRY_BACKOFF_MS = 60_000; +const DEFAULT_PR_FINDING_AUTHOR = "chatgpt-codex-connector"; +const ALLOWED_MISSING_HEAD_REVIEW_MARKER = "review-gate:allowed-missing-head-review"; +// Codex uses this shields.io badge markup for findings in both inline threads and PR-level +// comments, as observed on Pack PRs #126, #142, and #144. Clean reviews and setup notices omit it. +const CODEX_SEVERITY_BADGE_PATTERN = + /!\[P[0-3] Badge\]\(https:\/\/img\.shields\.io\/badge\/P[0-3]-[^)\s]+\)/u; +// gh does not expose structured HTTP/network failure details through execFileSync, so keep the +// complete stderr and process-signal fallback list here as the single transience classifier. +const TRANSIENT_GH_FAILURE_PATTERNS = [ + /\bHTTP\s+(?:429|500|502|503|504)\b/iu, + /\b(?:secondary\s+)?rate limit(?:ed|ing)?\b/iu, + /\b(?:ETIMEDOUT|timeout|timed out|context deadline exceeded|deadline exceeded)\b/iu, + /\b(?:ECONNRESET|connection reset)\b/iu, + /\b(?:ENOTFOUND|EAI_AGAIN|getaddrinfo|could not resolve host|no such host|temporary failure in name resolution)\b/iu, +]; + const rawArgs = process.argv.slice(2); const args = new Set(rawArgs); const strictHeadReview = args.has("--strict-head-review"); const allowMissingHeadReview = args.has("--allow-missing-head-review"); const waitHeadReviewMs = readNonNegativeIntegerArg("--wait-head-review-ms", 0); const pollIntervalMs = readNonNegativeIntegerArg("--poll-interval-ms", 10_000); +const retryAttempts = readPositiveIntegerArg("--retry-attempts", DEFAULT_GH_RETRY_ATTEMPTS); +const retryBackoffMs = readNonNegativeIntegerArg("--retry-backoff-ms", DEFAULT_GH_RETRY_BACKOFF_MS); const fixturePaths = readFixturePaths(); const requiredReviewAuthor = readArgValue("--required-review-author"); +const prFindingAuthor = requiredReviewAuthor ?? DEFAULT_PR_FINDING_AUTHOR; const expectedHeadOid = readArgValue("--expected-head-oid"); const explicitRepo = readArgValue("--repo"); const explicitPr = readArgValue("--pr"); let fixtureIndex = 0; -const ALLOWED_MISSING_HEAD_REVIEW_MARKER = "review-gate:allowed-missing-head-review"; const repo = explicitRepo ?? runText(["repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner"]); const prNumber = Number(explicitPr ?? runText(["pr", "view", "--json", "number", "-q", ".number"])); -if (!repo || !repo.includes("/")) fail("Could not determine GitHub repo. Pass --repo owner/name."); +if (!repo || !repo.includes("/")) + failEvaluation("Could not determine GitHub repo. Pass --repo owner/name."); if (!Number.isInteger(prNumber) || prNumber < 1) - fail("Could not determine PR number. Pass --pr ."); + failEvaluation("Could not determine PR number. Pass --pr ."); -const { pr, unresolvedThreads, blockingReviews, headReviews } = await fetchEvaluatedPr(); +const { pr, unresolvedThreads, blockingReviews, blockingComments, headReviews } = + await fetchEvaluatedPr(); const bodyIssues = evaluatePullRequestBody(pr); -reportBlockingState({ unresolvedThreads, blockingReviews }); +reportBlockingState({ unresolvedThreads, blockingReviews, blockingComments }); const missingHeadReview = strictHeadReview && headReviews.length === 0; if (missingHeadReview) { @@ -43,6 +68,7 @@ if (missingHeadReview) { if ( unresolvedThreads.length > 0 || blockingReviews.length > 0 || + blockingComments.length > 0 || bodyIssues.length > 0 || (missingHeadReview && !allowMissingHeadReview) ) { @@ -50,7 +76,7 @@ if ( console.error(`PR body workflow/template issues on ${repo}#${prNumber}:`); for (const issue of bodyIssues) console.error(`- ${issue}`); } - process.exit(1); + process.exit(BLOCKING_STATE_EXIT_CODE); } const latestReview = pr.reviews.nodes.at(-1); @@ -67,10 +93,10 @@ async function fetchEvaluatedPr() { while (true) { const result = fetchReviewGraph(); - const pr = result.data?.repository?.pullRequest; - if (!pr) fail(`Could not fetch PR #${prNumber} from ${repo}.`); + const pr = result?.data?.repository?.pullRequest; + if (!pr) failEvaluation(`Could not fetch PR #${prNumber} from ${repo}.`); if (expectedHeadOid && pr.headRefOid !== expectedHeadOid) { - fail( + failEvaluation( `PR head changed while evaluating ${repo}#${prNumber}: expected ${expectedHeadOid}, found ${pr.headRefOid}.`, ); } @@ -93,6 +119,12 @@ function evaluatePullRequestReviewState(pr) { const unresolvedThreads = pr.reviewThreads.nodes.filter( (thread) => !thread.isResolved && !thread.isOutdated, ); + const blockingComments = pr.comments.nodes.filter( + (comment) => + !comment.isMinimized && + normaliseAuthorLogin(comment.author?.login) === normaliseAuthorLogin(prFindingAuthor) && + CODEX_SEVERITY_BADGE_PATTERN.test(comment.body ?? ""), + ); const authorStates = reduceSubmittedCurrentHeadReviewsByAuthor(pr.reviews.nodes, pr.headRefOid); const blockingReviews = Array.from(authorStates.values()) .map((state) => state.blockingReview) @@ -105,7 +137,7 @@ function evaluatePullRequestReviewState(pr) { !requiredReviewAuthor || normaliseAuthorLogin(review.author?.login) === normaliseAuthorLogin(requiredReviewAuthor), ); - return { unresolvedThreads, blockingReviews, headReviews }; + return { unresolvedThreads, blockingReviews, blockingComments, headReviews }; } function reduceSubmittedCurrentHeadReviewsByAuthor(reviews, headRefOid) { @@ -206,7 +238,7 @@ function evaluatePullRequestBody(pr) { return issues; } -function reportBlockingState({ unresolvedThreads, blockingReviews }) { +function reportBlockingState({ unresolvedThreads, blockingReviews, blockingComments }) { if (unresolvedThreads.length > 0) { console.error(`Unresolved review threads on ${repo}#${prNumber}:`); for (const thread of unresolvedThreads) { @@ -216,6 +248,17 @@ function reportBlockingState({ unresolvedThreads, blockingReviews }) { } } + if (blockingComments.length > 0) { + console.error(`Unresolved PR-level review findings on ${repo}#${prNumber}:`); + for (const comment of blockingComments) { + console.error(`- ${comment.url ?? comment.id}`); + console.error(` author: ${comment.author?.login ?? "unknown"}`); + } + console.error( + "Minimize each finding with GitHub Hide → Resolved after dispositioning it to clear the gate.", + ); + } + if (blockingReviews.length > 0) { console.error(`Requested-changes reviews on ${repo}#${prNumber}:`); for (const review of blockingReviews) { @@ -233,24 +276,38 @@ function fetchReviewGraph() { function fetchPaginatedReviewGraph() { const merged = fetchReviewGraphPage(); - const mergedPr = merged.data?.repository?.pullRequest; + const mergedPr = merged?.data?.repository?.pullRequest; if (!mergedPr) return merged; + ensureReviewConnections(mergedPr); let reviewThreadsPageInfo = mergedPr.reviewThreads.pageInfo; while (reviewThreadsPageInfo?.hasNextPage) { const page = fetchReviewThreadsGraphPage(reviewThreadsPageInfo.endCursor); - const pr = page.data?.repository?.pullRequest; - if (!pr) break; + const pr = page?.data?.repository?.pullRequest; + if (!pr) failEvaluation(`Could not fetch the next review-thread page for ${repo}#${prNumber}.`); + ensureReviewConnections(pr); mergedPr.reviewThreads.nodes.push(...pr.reviewThreads.nodes); reviewThreadsPageInfo = pr.reviewThreads.pageInfo; mergedPr.reviewThreads.pageInfo = reviewThreadsPageInfo; } + let commentsPageInfo = mergedPr.comments.pageInfo; + while (commentsPageInfo?.hasNextPage) { + const page = fetchCommentsGraphPage(commentsPageInfo.endCursor); + const pr = page?.data?.repository?.pullRequest; + if (!pr) failEvaluation(`Could not fetch the next PR-comment page for ${repo}#${prNumber}.`); + ensureReviewConnections(pr); + mergedPr.comments.nodes.push(...pr.comments.nodes); + commentsPageInfo = pr.comments.pageInfo; + mergedPr.comments.pageInfo = commentsPageInfo; + } + let reviewsPageInfo = mergedPr.reviews.pageInfo; while (reviewsPageInfo?.hasNextPage) { const page = fetchReviewsGraphPage(reviewsPageInfo.endCursor); - const pr = page.data?.repository?.pullRequest; - if (!pr) break; + const pr = page?.data?.repository?.pullRequest; + if (!pr) failEvaluation(`Could not fetch the next review page for ${repo}#${prNumber}.`); + ensureReviewConnections(pr); mergedPr.reviews.nodes.push(...pr.reviews.nodes); reviewsPageInfo = pr.reviews.pageInfo; mergedPr.reviews.pageInfo = reviewsPageInfo; @@ -271,11 +328,29 @@ function fetchReviewGraphPage() { "-F", `number=${prNumber}`, "-f", - "query=query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){body headRefName baseRefName headRepository{nameWithOwner} headRefOid reviewThreads(first:100){pageInfo{hasNextPage endCursor} nodes{id isResolved isOutdated path line comments(first:1){nodes{url author{login} body}}}} reviews(first:100){pageInfo{hasNextPage endCursor} nodes{state submittedAt url author{login} commit{oid}}}}}}", + "query=query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){body headRefName baseRefName headRepository{nameWithOwner} headRefOid comments(first:100){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized author{login} body}} reviewThreads(first:100){pageInfo{hasNextPage endCursor} nodes{id isResolved isOutdated path line comments(first:1){nodes{url author{login} body}}}} reviews(first:100){pageInfo{hasNextPage endCursor} nodes{state submittedAt url author{login} commit{oid}}}}}}", ]; return runJson(commandArgs); } +function fetchCommentsGraphPage(after) { + const [owner, name] = repo.split("/"); + return runJson([ + "api", + "graphql", + "-F", + `owner=${owner}`, + "-F", + `name=${name}`, + "-F", + `number=${prNumber}`, + "-F", + `after=${after}`, + "-f", + "query=query($owner:String!,$name:String!,$number:Int!,$after:String!){repository(owner:$owner,name:$name){pullRequest(number:$number){comments(first:100,after:$after){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized author{login} body}}}}}", + ]); +} + function fetchReviewThreadsGraphPage(after) { const [owner, name] = repo.split("/"); return runJson([ @@ -332,11 +407,15 @@ function mergeReviewGraphPages(pages) { const merged = JSON.parse(JSON.stringify(firstPage)); const mergedPr = merged.data?.repository?.pullRequest; if (!mergedPr) return merged; + ensureReviewConnections(mergedPr); for (const page of remainingPages) { const pr = page.data?.repository?.pullRequest; if (!pr) continue; + ensureReviewConnections(pr); + mergedPr.comments.nodes.push(...pr.comments.nodes); mergedPr.reviewThreads.nodes.push(...pr.reviewThreads.nodes); mergedPr.reviews.nodes.push(...pr.reviews.nodes); + mergedPr.comments.pageInfo = pr.comments.pageInfo; mergedPr.reviewThreads.pageInfo = pr.reviewThreads.pageInfo; mergedPr.reviews.pageInfo = pr.reviews.pageInfo; } @@ -346,9 +425,20 @@ function mergeReviewGraphPages(pages) { function annotateReviewHeadRef(result) { const pr = result?.data?.repository?.pullRequest; if (!pr) return result; + ensureReviewConnections(pr); + return result; +} + +function ensureReviewConnections(pr) { + pr.comments ??= { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }; + pr.comments.nodes ??= []; + pr.comments.pageInfo ??= { hasNextPage: false, endCursor: null }; + pr.reviewThreads ??= { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }; + pr.reviewThreads.nodes ??= []; pr.reviewThreads.pageInfo ??= { hasNextPage: false, endCursor: null }; + pr.reviews ??= { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }; + pr.reviews.nodes ??= []; pr.reviews.pageInfo ??= { hasNextPage: false, endCursor: null }; - return result; } function readFixturePaths() { @@ -367,7 +457,14 @@ function readNonNegativeIntegerArg(name, defaultValue) { const rawValue = readArgValue(name); if (rawValue === null) return defaultValue; const value = Number(rawValue); - if (!Number.isInteger(value) || value < 0) fail(`${name} must be a non-negative integer.`); + if (!Number.isInteger(value) || value < 0) + failEvaluation(`${name} must be a non-negative integer.`); + return value; +} + +function readPositiveIntegerArg(name, defaultValue) { + const value = readNonNegativeIntegerArg(name, defaultValue); + if (value < 1) failEvaluation(`${name} must be a positive integer.`); return value; } @@ -376,19 +473,67 @@ function normaliseAuthorLogin(login) { } function runText(commandArgs) { - return execFileSync("gh", commandArgs, { - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); + for (let attempt = 1; attempt <= retryAttempts; attempt += 1) { + try { + return execFileSync("gh", commandArgs, { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + } catch (error) { + const detail = formatGhFailure(error); + const transient = isTransientGhFailure(error, detail); + if (!transient) { + failEvaluation(`GitHub CLI failed without a retryable error: ${detail}`); + } + if (attempt >= retryAttempts) { + failEvaluation(`GitHub CLI evaluation failed after ${retryAttempts} attempts: ${detail}`); + } + + const backoff = Math.min( + retryBackoffMs * 2 ** Math.min(attempt - 1, 20), + MAX_GH_RETRY_BACKOFF_MS, + ); + console.warn( + `GitHub CLI transient failure on attempt ${attempt}/${retryAttempts}: ${detail}. Retrying in ${backoff}ms.`, + ); + sleepSync(backoff); + } + } + + failEvaluation("GitHub CLI evaluation ended without a result."); } function runJson(commandArgs) { - return JSON.parse(runText(commandArgs)); + const output = runText(commandArgs); + try { + return JSON.parse(output); + } catch (error) { + failEvaluation(`GitHub CLI returned malformed JSON: ${formatErrorMessage(error)}`); + } +} + +function isTransientGhFailure(error, detail) { + const evidence = [error?.code, error?.signal, detail].filter(Boolean).join("\n"); + return TRANSIENT_GH_FAILURE_PATTERNS.some((pattern) => pattern.test(evidence)); +} + +function formatGhFailure(error) { + const stderr = String(error?.stderr ?? "").trim(); + return stderr || formatErrorMessage(error); +} + +function formatErrorMessage(error) { + return error instanceof Error ? error.message : String(error); +} + +function failEvaluation(message) { + console.error(`Review gate could not evaluate: ${message}`); + process.exit(EVALUATION_FAILURE_EXIT_CODE); } -function fail(message) { - console.error(message); - process.exit(1); +function sleepSync(ms) { + if (ms <= 0) return; + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); } function sleep(ms) { From 4b9899dbb9b69c8bb8cb788e4046f9d9b73bd82b Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 02:57:51 +0530 Subject: [PATCH 02/10] test(ci): cover review gate visibility failures Signed-off-by: Tapish Khandelwal --- tests/extension/ci-workflow.test.ts | 10 +- tests/scripts/check-pr-review-gate.test.ts | 307 ++++++++++++++++++++- 2 files changed, 313 insertions(+), 4 deletions(-) diff --git a/tests/extension/ci-workflow.test.ts b/tests/extension/ci-workflow.test.ts index c262c8be..6250a743 100644 --- a/tests/extension/ci-workflow.test.ts +++ b/tests/extension/ci-workflow.test.ts @@ -47,10 +47,16 @@ describe("Pack CI workflow", () => { expect(workflow).toContain("pull_request:"); expect(workflow).toContain("pull_request_review:"); expect(workflow).toContain("pull_request_review_comment:"); + expect(workflow).toContain("issue_comment:"); expect(workflow).not.toContain("pull_request_target:"); expect(workflow).not.toContain("schedule:"); - expect(workflow).not.toContain("issue_comment:"); - expect(workflow).not.toContain("github.event.issue"); + expect(workflow).toContain( + "if: github.event_name != 'issue_comment' || github.event.issue.pull_request", + ); + expect(workflow).toContain("github.event.issue.number"); + expect(workflow).toContain( + '[ "${EVENT_NAME}" = "workflow_dispatch" ] || [ "${EVENT_NAME}" = "issue_comment" ]', + ); expect(workflow).not.toContain("/review-gate"); expect(workflow).toContain("name: Review findings gate"); expect(workflow).toContain("name: Review gate"); diff --git a/tests/scripts/check-pr-review-gate.test.ts b/tests/scripts/check-pr-review-gate.test.ts index 8f5fbf6e..60d74c29 100644 --- a/tests/scripts/check-pr-review-gate.test.ts +++ b/tests/scripts/check-pr-review-gate.test.ts @@ -1,5 +1,5 @@ -import { execFileSync } from "node:child_process"; -import { mkdtempSync, writeFileSync } from "node:fs"; +import { execFileSync, spawnSync } from "node:child_process"; +import { chmodSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; @@ -8,6 +8,236 @@ const rootDir = process.cwd(); const scriptPath = path.join(rootDir, "scripts", "check-pr-review-gate.mjs"); describe("PR review gate", () => { + it("fails when the required reviewer has an unresolved PR-level finding", () => { + const fixturePath = writeFixture( + "unresolved-pr-level-finding", + reviewFixture({ + headRefOid: "head-sha", + comments: [ + { + id: "comment-1", + url: "https://github.com/lamemustafa/pack/pull/14#issuecomment-1", + createdAt: "2026-08-17T12:00:00Z", + isMinimized: false, + author: { login: "chatgpt-codex-connector[bot]" }, + body: "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", + }, + ], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const result = spawnSync( + process.execPath, + [ + scriptPath, + "--repo", + "lamemustafa/pack", + "--pr", + "14", + "--fixture", + fixturePath, + "--strict-head-review", + ], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Unresolved PR-level review findings"); + expect(result.stderr).toContain("chatgpt-codex-connector[bot]"); + expect(result.stderr).toContain("Minimize each finding"); + }); + + it("does not block a minimized PR-level finding", () => { + const fixturePath = writeFixture( + "minimized-pr-level-finding", + reviewFixture({ + headRefOid: "head-sha", + comments: [prFindingComment({ isMinimized: true })], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const output = execFileSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(output).toContain("PR review gate passed"); + }); + + it("does not treat clean-review notes or setup notices as PR-level findings", () => { + const fixturePath = writeFixture( + "non-finding-pr-level-comments", + reviewFixture({ + headRefOid: "head-sha", + comments: [ + prFindingComment({ + id: "clean-review", + body: "Codex Review: Didn't find any major issues. Breezy!", + }), + prFindingComment({ + id: "setup-notice", + body: "To use Codex here, create an environment for this repo.", + }), + ], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const output = execFileSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(output).toContain("PR review gate passed"); + }); + + it("does not block a finding-shaped comment from an unrelated author", () => { + const fixturePath = writeFixture( + "unrelated-author-pr-level-finding", + reviewFixture({ + headRefOid: "head-sha", + comments: [prFindingComment({ author: "external-reviewer" })], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const output = execFileSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(output).toContain("PR review gate passed"); + }); + + it("evaluates PR-level findings from paginated fixture pages", () => { + const fixturePath = writeFixture("paginated-pr-level-findings", { + pages: [ + reviewFixture({ + headRefOid: "head-sha", + comments: [], + commentsPageInfo: { hasNextPage: true, endCursor: "comments-page-1" }, + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + reviewFixture({ + headRefOid: "head-sha", + comments: [prFindingComment()], + reviews: [], + }), + ], + }); + + const result = spawnSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Unresolved PR-level review findings"); + }); + + it("fails as could-not-evaluate when a live pagination page has no PR data", () => { + const firstPage = reviewFixture({ + headRefOid: "head-sha", + commentsPageInfo: { hasNextPage: true, endCursor: "comments-page-1" }, + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }); + const missingPrPage = { data: { repository: { pullRequest: null } } }; + const { result, attempts } = runGateWithFakeGh("missing-paginated-pr", [ + { status: 0, stdout: JSON.stringify(firstPage) }, + { status: 0, stdout: JSON.stringify(missingPrPage) }, + ]); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("Review gate could not evaluate"); + expect(result.stderr).toContain("next PR-comment page"); + expect(attempts).toBe(2); + }); + + it("retries a transient GitHub CLI failure and then succeeds", () => { + const fixture = reviewFixture({ + headRefOid: "head-sha", + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }); + const { result, attempts } = runGateWithFakeGh("transient-then-success", [ + { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, + { status: 0, stdout: JSON.stringify(fixture) }, + ]); + + expect(result.status).toBe(0); + expect(result.stdout).toContain("PR review gate passed"); + expect(result.stderr).toContain("transient failure on attempt 1/2"); + expect(result.stderr).toContain("Retrying in 0ms"); + expect(attempts).toBe(2); + }); + + it.each([ + ["http-429", "gh: HTTP 429: too many requests\n"], + ["http-500", "gh: HTTP 500: internal server error\n"], + ["http-502", "gh: HTTP 502: bad gateway\n"], + ["http-504", "gh: HTTP 504: gateway timeout\n"], + ["rate-limit", "gh: API rate limit exceeded\n"], + ["timeout", "gh: request timed out\n"], + ["connection-reset", "gh: read: connection reset by peer\n"], + ["dns", "gh: dial tcp: lookup api.github.com: no such host\n"], + ])("retries the allowed %s transient failure", (name, stderr) => { + const fixture = reviewFixture({ + headRefOid: "head-sha", + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }); + const { result, attempts } = runGateWithFakeGh(name, [ + { status: 1, stderr }, + { status: 0, stdout: JSON.stringify(fixture) }, + ]); + + expect(result.status).toBe(0); + expect(result.stderr).toContain("Retrying in 0ms"); + expect(attempts).toBe(2); + }); + + it("uses the could-not-evaluate exit code after transient retries are exhausted", () => { + const { result, attempts } = runGateWithFakeGh( + "transient-exhaustion", + [ + { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, + { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, + { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, + ], + ["--retry-attempts", "3"], + ); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("Review gate could not evaluate"); + expect(result.stderr).toContain("failed after 3 attempts"); + expect(result.stderr).toContain("HTTP 503"); + expect(result.stderr).not.toContain("Unresolved review threads"); + expect(attempts).toBe(3); + }); + + it("does not retry a non-transient GitHub CLI failure", () => { + const { result, attempts } = runGateWithFakeGh( + "non-transient", + [ + { + status: 1, + stderr: "gh: GraphQL: Could not resolve to a Repository with the name pack\n", + }, + ], + ["--retry-attempts", "3"], + ); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("Review gate could not evaluate"); + expect(result.stderr).toContain("Could not resolve to a Repository"); + expect(result.stderr).not.toContain("Retrying in"); + expect(attempts).toBe(1); + }); + it("fails when unresolved review threads are present", () => { const fixturePath = writeFixture( "unresolved-thread", @@ -865,6 +1095,8 @@ function reviewFixture({ baseRefName = "master", headRepository = { nameWithOwner: "lamemustafa/pack" }, body = packPrBody(), + comments = [], + commentsPageInfo = { hasNextPage: false, endCursor: null }, reviewThreads = [], reviewThreadsPageInfo = { hasNextPage: false, endCursor: null }, reviews, @@ -875,6 +1107,8 @@ function reviewFixture({ baseRefName?: string; headRepository?: { nameWithOwner: string }; body?: string; + comments?: unknown[]; + commentsPageInfo?: { hasNextPage: boolean; endCursor: string | null }; reviewThreads?: unknown[]; reviewThreadsPageInfo?: { hasNextPage: boolean; endCursor: string | null }; reviews: Array>; @@ -889,6 +1123,10 @@ function reviewFixture({ baseRefName, headRepository, headRefOid, + comments: { + nodes: comments, + pageInfo: commentsPageInfo, + }, reviewThreads: { nodes: reviewThreads, pageInfo: reviewThreadsPageInfo }, reviews: { nodes: reviews, pageInfo: reviewsPageInfo }, }, @@ -897,6 +1135,71 @@ function reviewFixture({ }; } +function prFindingComment({ + id = "comment-1", + isMinimized = false, + author = "chatgpt-codex-connector[bot]", + body = "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", +}: { + id?: string; + isMinimized?: boolean; + author?: string; + body?: string; +} = {}) { + return { + id, + url: `https://github.com/lamemustafa/pack/pull/14#issuecomment-${id}`, + createdAt: "2026-08-17T12:00:00Z", + isMinimized, + author: { login: author }, + body, + }; +} + +function runGateWithFakeGh( + name: string, + responses: Array<{ status: number; stdout?: string; stderr?: string }>, + extraArgs: string[] = ["--retry-attempts", String(responses.length)], +) { + const directory = mkdtempSync(path.join(tmpdir(), "pack-review-gate-gh-")); + const statePath = path.join(directory, `${name}-attempts.txt`); + const fakeGhPath = path.join(directory, "gh"); + const fakeGhSource = `#!/usr/bin/env node +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +const responses = ${JSON.stringify(responses)}; +const statePath = ${JSON.stringify(statePath)}; +const attempt = existsSync(statePath) ? Number(readFileSync(statePath, "utf8")) : 0; +writeFileSync(statePath, String(attempt + 1), "utf8"); +const response = responses[Math.min(attempt, responses.length - 1)]; +if (response.stdout) process.stdout.write(response.stdout); +if (response.stderr) process.stderr.write(response.stderr); +process.exit(response.status); +`; + writeFileSync(fakeGhPath, fakeGhSource, "utf8"); + chmodSync(fakeGhPath, 0o755); + + const result = spawnSync( + process.execPath, + [ + scriptPath, + "--repo", + "lamemustafa/pack", + "--pr", + "14", + "--retry-backoff-ms", + "0", + ...extraArgs, + ], + { + cwd: rootDir, + encoding: "utf8", + env: { ...process.env, PATH: `${directory}${path.delimiter}${process.env.PATH ?? ""}` }, + }, + ); + + return { result, attempts: Number(readFileSync(statePath, "utf8")) }; +} + function packPrBody() { return [ "## Summary", From c26db85bf23ec296b06b1b9687c8be8390c55ed9 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 04:07:04 +0530 Subject: [PATCH 03/10] fix(ci): publish review verdict on pull request head Signed-off-by: Tapish Khandelwal --- .github/workflows/review-gate.yml | 81 +++++++++++++++++++++++++-- scripts/publish-review-gate-check.mjs | 74 ++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 scripts/publish-review-gate-check.mjs diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index ed575bd7..ff8901b2 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -25,6 +25,10 @@ jobs: if: github.event_name != 'issue_comment' || github.event.issue.pull_request runs-on: ubuntu-latest timeout-minutes: 8 + outputs: + head_repo: ${{ steps.resolve-pr.outputs.head_repo }} + head_sha: ${{ steps.resolve-pr.outputs.head_sha }} + gate_exit_code: ${{ steps.evaluate.outputs.exit_code || '2' }} steps: - name: Resolve PR metadata @@ -76,6 +80,14 @@ jobs: echo "head_sha=${head_sha}" } >> "${GITHUB_OUTPUT}" + - name: Report fork PR limitation + if: >- + github.event_name == 'issue_comment' && + steps.resolve-pr.outputs.head_repo != github.repository + run: >- + echo "::warning::PR-head Review gate publication is unavailable for fork pull requests; + this comment-triggered run remains read-only and cannot replace the required check on the fork head." + - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 with: @@ -106,10 +118,69 @@ jobs: --head-repo "${{ steps.resolve-pr.outputs.head_repo }}" - name: Check current-head review state - run: >- - pnpm review:gate -- --repo "${{ github.repository }}" --pr "${{ steps.resolve-pr.outputs.pr }}" - --strict-head-review --required-review-author chatgpt-codex-connector - --wait-head-review-ms 180000 --poll-interval-ms 10000 --allow-missing-head-review - --expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}" + id: evaluate + shell: bash + run: | + set +e + pnpm review:gate -- --repo "${{ github.repository }}" --pr "${{ steps.resolve-pr.outputs.pr }}" \ + --strict-head-review --required-review-author chatgpt-codex-connector \ + --wait-head-review-ms 180000 --poll-interval-ms 10000 --allow-missing-head-review \ + --expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}" + gate_exit_code=$? + set -e + + case "${gate_exit_code}" in + 0|1|2) ;; + *) + echo "Unexpected review gate exit ${gate_exit_code}; recording could-not-evaluate." >&2 + gate_exit_code=2 + ;; + esac + + echo "exit_code=${gate_exit_code}" >> "${GITHUB_OUTPUT}" env: GH_TOKEN: ${{ github.token }} + + - name: Preserve review gate conclusion + if: always() + shell: bash + env: + REVIEW_GATE_EXIT_CODE: ${{ steps.evaluate.outputs.exit_code || '2' }} + run: | + case "${REVIEW_GATE_EXIT_CODE}" in + 0) exit 0 ;; + 1|2) exit "${REVIEW_GATE_EXIT_CODE}" ;; + *) exit 2 ;; + esac + + publish-pr-head-check: + name: Publish Review gate verdict + needs: review-gate + if: >- + always() && + github.event_name == 'issue_comment' && + needs.review-gate.outputs.head_repo == github.repository && + needs.review-gate.outputs.head_sha != '' + runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: + contents: read + checks: write + + steps: + - name: Checkout trusted publisher + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + fetch-depth: 1 + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + + - name: Publish verdict on pull request head + env: + GH_TOKEN: ${{ github.token }} + run: >- + node scripts/publish-review-gate-check.mjs + --repo "${{ github.repository }}" + --head-sha "${{ needs.review-gate.outputs.head_sha }}" + --exit-code "${{ needs.review-gate.outputs.gate_exit_code }}" + --details-url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs new file mode 100644 index 00000000..e0846756 --- /dev/null +++ b/scripts/publish-review-gate-check.mjs @@ -0,0 +1,74 @@ +#!/usr/bin/env node + +import { execFileSync } from "node:child_process"; + +const CHECK_RUN_NAME = "Review gate"; +const EXIT_VERDICTS = new Map([ + [0, { conclusion: "success", title: "Review gate passed" }], + [1, { conclusion: "failure", title: "Review gate found blocking review state" }], + [2, { conclusion: "action_required", title: "Review gate could not evaluate" }], +]); + +const rawArgs = process.argv.slice(2); +const repo = readRequiredArg("--repo"); +const headSha = readRequiredArg("--head-sha"); +const detailsUrl = readRequiredArg("--details-url"); +const exitCode = Number(readRequiredArg("--exit-code")); +const verdict = EXIT_VERDICTS.get(exitCode); + +if (!repo.includes("/")) fail("--repo must be owner/name."); +if (!/^[0-9a-f]{40}$/iu.test(headSha)) fail("--head-sha must be a full commit SHA."); +if (!verdict) fail("--exit-code must be 0, 1, or 2."); + +const summary = + exitCode === 0 + ? "The review gate evaluated the pull request head and found no blocking state." + : exitCode === 1 + ? "The review gate evaluated the pull request head and found a blocking state." + : "The review gate could not evaluate the complete pull request review state."; + +runText([ + "api", + "-X", + "POST", + `repos/${repo}/check-runs`, + "-H", + "X-GitHub-Api-Version: 2022-11-28", + "-f", + `name=${CHECK_RUN_NAME}`, + "-f", + `head_sha=${headSha}`, + "-f", + "status=completed", + "-f", + `conclusion=${verdict.conclusion}`, + "-f", + `details_url=${detailsUrl}`, + "-f", + `output[title]=${verdict.title}`, + "-f", + `output[summary]=${summary}`, +]); + +console.log( + `${CHECK_RUN_NAME} check created for ${headSha} with conclusion ${verdict.conclusion}.`, +); + +function readRequiredArg(name) { + const index = rawArgs.indexOf(name); + const value = index >= 0 ? rawArgs[index + 1] : null; + if (!value || value.startsWith("--")) fail(`Pass ${name} .`); + return value; +} + +function runText(commandArgs) { + return execFileSync("gh", commandArgs, { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); +} + +function fail(message) { + console.error(message); + process.exit(1); +} From 44ce29bca8eded48baace726b076a74c153c38c5 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 04:07:04 +0530 Subject: [PATCH 04/10] fix(ci): require resolved comment minimization Signed-off-by: Tapish Khandelwal --- scripts/check-pr-review-gate.mjs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/check-pr-review-gate.mjs b/scripts/check-pr-review-gate.mjs index 6d764725..e31123ef 100644 --- a/scripts/check-pr-review-gate.mjs +++ b/scripts/check-pr-review-gate.mjs @@ -9,13 +9,15 @@ const DEFAULT_GH_RETRY_ATTEMPTS = 6; const DEFAULT_GH_RETRY_BACKOFF_MS = 10_000; const MAX_GH_RETRY_BACKOFF_MS = 60_000; const DEFAULT_PR_FINDING_AUTHOR = "chatgpt-codex-connector"; +// GitHub's live GraphQL API returns this lowercase string for a RESOLVED minimization classifier. +const RESOLVED_MINIMIZED_REASON = "resolved"; const ALLOWED_MISSING_HEAD_REVIEW_MARKER = "review-gate:allowed-missing-head-review"; // Codex uses this shields.io badge markup for findings in both inline threads and PR-level // comments, as observed on Pack PRs #126, #142, and #144. Clean reviews and setup notices omit it. const CODEX_SEVERITY_BADGE_PATTERN = /!\[P[0-3] Badge\]\(https:\/\/img\.shields\.io\/badge\/P[0-3]-[^)\s]+\)/u; // gh does not expose structured HTTP/network failure details through execFileSync, so keep the -// complete stderr and process-signal fallback list here as the single transience classifier. +// complete stderr and process-error fallback list here as the single transience classifier. const TRANSIENT_GH_FAILURE_PATTERNS = [ /\bHTTP\s+(?:429|500|502|503|504)\b/iu, /\b(?:secondary\s+)?rate limit(?:ed|ing)?\b/iu, @@ -121,7 +123,7 @@ function evaluatePullRequestReviewState(pr) { ); const blockingComments = pr.comments.nodes.filter( (comment) => - !comment.isMinimized && + (!comment.isMinimized || comment.minimizedReason !== RESOLVED_MINIMIZED_REASON) && normaliseAuthorLogin(comment.author?.login) === normaliseAuthorLogin(prFindingAuthor) && CODEX_SEVERITY_BADGE_PATTERN.test(comment.body ?? ""), ); @@ -328,7 +330,7 @@ function fetchReviewGraphPage() { "-F", `number=${prNumber}`, "-f", - "query=query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){body headRefName baseRefName headRepository{nameWithOwner} headRefOid comments(first:100){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized author{login} body}} reviewThreads(first:100){pageInfo{hasNextPage endCursor} nodes{id isResolved isOutdated path line comments(first:1){nodes{url author{login} body}}}} reviews(first:100){pageInfo{hasNextPage endCursor} nodes{state submittedAt url author{login} commit{oid}}}}}}", + "query=query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){body headRefName baseRefName headRepository{nameWithOwner} headRefOid comments(first:100){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized minimizedReason author{login} body}} reviewThreads(first:100){pageInfo{hasNextPage endCursor} nodes{id isResolved isOutdated path line comments(first:1){nodes{url author{login} body}}}} reviews(first:100){pageInfo{hasNextPage endCursor} nodes{state submittedAt url author{login} commit{oid}}}}}}", ]; return runJson(commandArgs); } @@ -347,7 +349,7 @@ function fetchCommentsGraphPage(after) { "-F", `after=${after}`, "-f", - "query=query($owner:String!,$name:String!,$number:Int!,$after:String!){repository(owner:$owner,name:$name){pullRequest(number:$number){comments(first:100,after:$after){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized author{login} body}}}}}", + "query=query($owner:String!,$name:String!,$number:Int!,$after:String!){repository(owner:$owner,name:$name){pullRequest(number:$number){comments(first:100,after:$after){pageInfo{hasNextPage endCursor} nodes{id url createdAt isMinimized minimizedReason author{login} body}}}}}", ]); } From 1806e9478baa9c5f1c78791c21389213275d3f01 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 04:07:04 +0530 Subject: [PATCH 05/10] test(ci): cover review verdict publication Signed-off-by: Tapish Khandelwal --- tests/extension/ci-workflow.test.ts | 12 ++- tests/scripts/check-pr-review-gate.test.ts | 53 ++++++++++++- .../scripts/publish-review-gate-check.test.ts | 77 +++++++++++++++++++ 3 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 tests/scripts/publish-review-gate-check.test.ts diff --git a/tests/extension/ci-workflow.test.ts b/tests/extension/ci-workflow.test.ts index 6250a743..29112299 100644 --- a/tests/extension/ci-workflow.test.ts +++ b/tests/extension/ci-workflow.test.ts @@ -37,7 +37,7 @@ describe("Pack CI workflow", () => { expect(workflow).not.toContain("actions/upload-artifact"); }); - it("runs a read-only current-head review findings gate", async () => { + it("runs the review gate and publishes comment-triggered verdicts on the PR head", async () => { const workflow = await readFile( path.join(rootDir, ".github", "workflows", "review-gate.yml"), "utf8", @@ -60,7 +60,13 @@ describe("Pack CI workflow", () => { expect(workflow).not.toContain("/review-gate"); expect(workflow).toContain("name: Review findings gate"); expect(workflow).toContain("name: Review gate"); + expect(workflow).toContain("name: Publish Review gate verdict"); expect(workflow).toContain("pull-requests: read"); + expect(workflow.match(/checks: write/g)).toHaveLength(1); + expect(workflow).toContain("permissions:\n contents: read\n pull-requests: read\n\njobs:"); + expect(workflow).toMatch( + /publish-pr-head-check:[\s\S]*?permissions:\n\s+contents: read\n\s+checks: write/, + ); expect(workflow).not.toContain("statuses: write"); expect(workflow).toContain("GH_TOKEN: ${{ github.token }}"); expect(workflow).toContain("repository: ${{ steps.resolve-pr.outputs.head_repo }}"); @@ -74,6 +80,10 @@ describe("Pack CI workflow", () => { expect(workflow).toContain("--wait-head-review-ms 180000"); expect(workflow).toContain("--allow-missing-head-review"); expect(workflow).toContain('--expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}"'); + expect(workflow).toContain("steps.evaluate.outputs.exit_code || '2'"); + expect(workflow).toContain("scripts/publish-review-gate-check.mjs"); + expect(workflow).toContain("PR-head Review gate publication is unavailable for fork"); + expect(workflow).toContain("needs.review-gate.outputs.head_repo == github.repository"); }); it("keeps every workflow action reference within the repository selected-actions policy", async () => { diff --git a/tests/scripts/check-pr-review-gate.test.ts b/tests/scripts/check-pr-review-gate.test.ts index 60d74c29..ad983af9 100644 --- a/tests/scripts/check-pr-review-gate.test.ts +++ b/tests/scripts/check-pr-review-gate.test.ts @@ -8,6 +8,12 @@ const rootDir = process.cwd(); const scriptPath = path.join(rootDir, "scripts", "check-pr-review-gate.mjs"); describe("PR review gate", () => { + it("fetches the minimization reason on initial and paginated PR comments", () => { + const script = readFileSync(scriptPath, "utf8"); + + expect(script.match(/isMinimized minimizedReason/g)).toHaveLength(2); + }); + it("fails when the required reviewer has an unresolved PR-level finding", () => { const fixturePath = writeFixture( "unresolved-pr-level-finding", @@ -48,12 +54,12 @@ describe("PR review gate", () => { expect(result.stderr).toContain("Minimize each finding"); }); - it("does not block a minimized PR-level finding", () => { + it("does not block a PR-level finding minimized as resolved", () => { const fixturePath = writeFixture( "minimized-pr-level-finding", reviewFixture({ headRefOid: "head-sha", - comments: [prFindingComment({ isMinimized: true })], + comments: [prFindingComment({ isMinimized: true, minimizedReason: "resolved" })], reviews: [review({ state: "COMMENTED", commit: "head-sha" })], }), ); @@ -67,6 +73,46 @@ describe("PR review gate", () => { expect(output).toContain("PR review gate passed"); }); + it("blocks a PR-level finding minimized as off-topic", () => { + const fixturePath = writeFixture( + "off-topic-pr-level-finding", + reviewFixture({ + headRefOid: "head-sha", + comments: [prFindingComment({ isMinimized: true, minimizedReason: "off-topic" })], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const result = spawnSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Unresolved PR-level review findings"); + }); + + it("blocks a PR-level finding minimized as outdated", () => { + const fixturePath = writeFixture( + "outdated-pr-level-finding", + reviewFixture({ + headRefOid: "head-sha", + comments: [prFindingComment({ isMinimized: true, minimizedReason: "outdated" })], + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + + const result = spawnSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], + { cwd: rootDir, encoding: "utf8" }, + ); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Unresolved PR-level review findings"); + }); + it("does not treat clean-review notes or setup notices as PR-level findings", () => { const fixturePath = writeFixture( "non-finding-pr-level-comments", @@ -1138,11 +1184,13 @@ function reviewFixture({ function prFindingComment({ id = "comment-1", isMinimized = false, + minimizedReason = null, author = "chatgpt-codex-connector[bot]", body = "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", }: { id?: string; isMinimized?: boolean; + minimizedReason?: string | null; author?: string; body?: string; } = {}) { @@ -1151,6 +1199,7 @@ function prFindingComment({ url: `https://github.com/lamemustafa/pack/pull/14#issuecomment-${id}`, createdAt: "2026-08-17T12:00:00Z", isMinimized, + minimizedReason, author: { login: author }, body, }; diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts new file mode 100644 index 00000000..9be11bf8 --- /dev/null +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -0,0 +1,77 @@ +import { spawnSync } from "node:child_process"; +import { chmodSync, existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +const rootDir = process.cwd(); +const scriptPath = path.join(rootDir, "scripts", "publish-review-gate-check.mjs"); +const headSha = "a".repeat(40); + +describe("PR-head Review gate check publisher", () => { + it.each([ + [0, "success"], + [1, "failure"], + [2, "action_required"], + ])("maps exit %i to the %s conclusion", (exitCode, conclusion) => { + const { result, calls } = runPublisher(exitCode); + + expect(result.status).toBe(0); + expect(result.stdout).toContain(`Review gate check created for ${headSha}`); + expect(calls).toHaveLength(1); + expect(calls[0]).toContain("repos/lamemustafa/pack/check-runs"); + expect(calls[0]).toContain("name=Review gate"); + expect(calls[0]).toContain(`head_sha=${headSha}`); + expect(calls[0]).toContain("status=completed"); + expect(calls[0]).toContain(`conclusion=${conclusion}`); + }); + + it("rejects an unknown evaluator exit code without writing a check", () => { + const { result, calls } = runPublisher(3); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("--exit-code must be 0, 1, or 2"); + expect(calls).toEqual([]); + }); +}); + +function runPublisher(exitCode: number) { + const directory = mkdtempSync(path.join(tmpdir(), "pack-review-check-publisher-")); + const callsPath = path.join(directory, "calls.json"); + const fakeGhPath = path.join(directory, "gh"); + const fakeGhSource = `#!/usr/bin/env node +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +const callsPath = ${JSON.stringify(callsPath)}; +const calls = existsSync(callsPath) ? JSON.parse(readFileSync(callsPath, "utf8")) : []; +calls.push(process.argv.slice(2)); +writeFileSync(callsPath, JSON.stringify(calls), "utf8"); +process.stdout.write(JSON.stringify({ id: 1 })); +`; + writeFileSync(fakeGhPath, fakeGhSource, "utf8"); + chmodSync(fakeGhPath, 0o755); + + const result = spawnSync( + process.execPath, + [ + scriptPath, + "--repo", + "lamemustafa/pack", + "--head-sha", + headSha, + "--exit-code", + String(exitCode), + "--details-url", + "https://github.com/lamemustafa/pack/actions/runs/1", + ], + { + cwd: rootDir, + encoding: "utf8", + env: { ...process.env, PATH: `${directory}${path.delimiter}${process.env.PATH ?? ""}` }, + }, + ); + + const calls = existsSync(callsPath) + ? (JSON.parse(readFileSync(callsPath, "utf8")) as string[][]) + : []; + return { result, calls }; +} From ebd14c5154e00099768e2d2caf0fb46137c00f9d Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 05:07:13 +0530 Subject: [PATCH 06/10] fix(ci): reconcile review checks on a schedule Signed-off-by: Tapish Khandelwal --- .github/workflows/review-gate.yml | 86 ++++-------- scripts/check-pr-review-gate.mjs | 73 ++-------- scripts/lib/github-cli-retry.mjs | 39 ++++++ scripts/publish-review-gate-check.mjs | 185 +++++++++++++++++++------- 4 files changed, 212 insertions(+), 171 deletions(-) create mode 100644 scripts/lib/github-cli-retry.mjs diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index ff8901b2..f1929409 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -1,6 +1,8 @@ name: Review findings gate on: + schedule: + - cron: "*/15 * * * *" workflow_dispatch: inputs: pr: @@ -12,8 +14,6 @@ on: types: [submitted, edited, dismissed] pull_request_review_comment: types: [created, edited, deleted] - issue_comment: - types: [created, edited, deleted] permissions: contents: read @@ -22,13 +22,9 @@ permissions: jobs: review-gate: name: Review gate - if: github.event_name != 'issue_comment' || github.event.issue.pull_request + if: github.event_name != 'schedule' runs-on: ubuntu-latest timeout-minutes: 8 - outputs: - head_repo: ${{ steps.resolve-pr.outputs.head_repo }} - head_sha: ${{ steps.resolve-pr.outputs.head_sha }} - gate_exit_code: ${{ steps.evaluate.outputs.exit_code || '2' }} steps: - name: Resolve PR metadata @@ -37,7 +33,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} EVENT_NAME: ${{ github.event_name }} - PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr }} EVENT_HEAD_REF: ${{ github.event.pull_request.head.ref }} EVENT_BASE_REF: ${{ github.event.pull_request.base.ref }} EVENT_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} @@ -50,7 +46,7 @@ jobs: exit 1 fi - if [ "${EVENT_NAME}" = "workflow_dispatch" ] || [ "${EVENT_NAME}" = "issue_comment" ]; then + if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then mapfile -t pr_metadata < <( gh pr view "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" \ --json headRefName,baseRefName,headRepository,headRefOid \ @@ -80,14 +76,6 @@ jobs: echo "head_sha=${head_sha}" } >> "${GITHUB_OUTPUT}" - - name: Report fork PR limitation - if: >- - github.event_name == 'issue_comment' && - steps.resolve-pr.outputs.head_repo != github.repository - run: >- - echo "::warning::PR-head Review gate publication is unavailable for fork pull requests; - this comment-triggered run remains read-only and cannot replace the required check on the fork head." - - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 with: @@ -118,69 +106,43 @@ jobs: --head-repo "${{ steps.resolve-pr.outputs.head_repo }}" - name: Check current-head review state - id: evaluate - shell: bash - run: | - set +e - pnpm review:gate -- --repo "${{ github.repository }}" --pr "${{ steps.resolve-pr.outputs.pr }}" \ - --strict-head-review --required-review-author chatgpt-codex-connector \ - --wait-head-review-ms 180000 --poll-interval-ms 10000 --allow-missing-head-review \ - --expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}" - gate_exit_code=$? - set -e - - case "${gate_exit_code}" in - 0|1|2) ;; - *) - echo "Unexpected review gate exit ${gate_exit_code}; recording could-not-evaluate." >&2 - gate_exit_code=2 - ;; - esac - - echo "exit_code=${gate_exit_code}" >> "${GITHUB_OUTPUT}" + run: >- + pnpm review:gate -- --repo "${{ github.repository }}" --pr "${{ steps.resolve-pr.outputs.pr }}" + --strict-head-review --required-review-author chatgpt-codex-connector + --wait-head-review-ms 180000 --poll-interval-ms 10000 --allow-missing-head-review + --expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}" env: GH_TOKEN: ${{ github.token }} - - name: Preserve review gate conclusion - if: always() - shell: bash - env: - REVIEW_GATE_EXIT_CODE: ${{ steps.evaluate.outputs.exit_code || '2' }} - run: | - case "${REVIEW_GATE_EXIT_CODE}" in - 0) exit 0 ;; - 1|2) exit "${REVIEW_GATE_EXIT_CODE}" ;; - *) exit 2 ;; - esac - - publish-pr-head-check: - name: Publish Review gate verdict - needs: review-gate - if: >- - always() && - github.event_name == 'issue_comment' && - needs.review-gate.outputs.head_repo == github.repository && - needs.review-gate.outputs.head_sha != '' + scheduled-review-gate: + name: Reconcile Review gate checks + if: github.event_name == 'schedule' runs-on: ubuntu-latest - timeout-minutes: 2 + timeout-minutes: 14 permissions: contents: read + pull-requests: read checks: write steps: - - name: Checkout trusted publisher + - name: Checkout trusted default-branch code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 with: fetch-depth: 1 ref: ${{ github.event.repository.default_branch }} persist-credentials: false - - name: Publish verdict on pull request head + - name: Set up Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 + with: + node-version: 22.13.0 + + - name: Reconcile open pull request checks env: GH_TOKEN: ${{ github.token }} run: >- node scripts/publish-review-gate-check.mjs --repo "${{ github.repository }}" - --head-sha "${{ needs.review-gate.outputs.head_sha }}" - --exit-code "${{ needs.review-gate.outputs.gate_exit_code }}" + --reconcile-open-prs + --max-prs 25 --details-url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" diff --git a/scripts/check-pr-review-gate.mjs b/scripts/check-pr-review-gate.mjs index e31123ef..d60c9ed9 100644 --- a/scripts/check-pr-review-gate.mjs +++ b/scripts/check-pr-review-gate.mjs @@ -1,30 +1,19 @@ #!/usr/bin/env node -import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; +import { + DEFAULT_GH_RETRY_ATTEMPTS, + DEFAULT_GH_RETRY_BACKOFF_MS, + runGhText, +} from "./lib/github-cli-retry.mjs"; const BLOCKING_STATE_EXIT_CODE = 1; const EVALUATION_FAILURE_EXIT_CODE = 2; -const DEFAULT_GH_RETRY_ATTEMPTS = 6; -const DEFAULT_GH_RETRY_BACKOFF_MS = 10_000; -const MAX_GH_RETRY_BACKOFF_MS = 60_000; const DEFAULT_PR_FINDING_AUTHOR = "chatgpt-codex-connector"; -// GitHub's live GraphQL API returns this lowercase string for a RESOLVED minimization classifier. const RESOLVED_MINIMIZED_REASON = "resolved"; const ALLOWED_MISSING_HEAD_REVIEW_MARKER = "review-gate:allowed-missing-head-review"; -// Codex uses this shields.io badge markup for findings in both inline threads and PR-level -// comments, as observed on Pack PRs #126, #142, and #144. Clean reviews and setup notices omit it. const CODEX_SEVERITY_BADGE_PATTERN = /!\[P[0-3] Badge\]\(https:\/\/img\.shields\.io\/badge\/P[0-3]-[^)\s]+\)/u; -// gh does not expose structured HTTP/network failure details through execFileSync, so keep the -// complete stderr and process-error fallback list here as the single transience classifier. -const TRANSIENT_GH_FAILURE_PATTERNS = [ - /\bHTTP\s+(?:429|500|502|503|504)\b/iu, - /\b(?:secondary\s+)?rate limit(?:ed|ing)?\b/iu, - /\b(?:ETIMEDOUT|timeout|timed out|context deadline exceeded|deadline exceeded)\b/iu, - /\b(?:ECONNRESET|connection reset)\b/iu, - /\b(?:ENOTFOUND|EAI_AGAIN|getaddrinfo|could not resolve host|no such host|temporary failure in name resolution)\b/iu, -]; const rawArgs = process.argv.slice(2); const args = new Set(rawArgs); @@ -257,7 +246,7 @@ function reportBlockingState({ unresolvedThreads, blockingReviews, blockingComme console.error(` author: ${comment.author?.login ?? "unknown"}`); } console.error( - "Minimize each finding with GitHub Hide → Resolved after dispositioning it to clear the gate.", + "Minimize each finding with GitHub Hide → Resolved after dispositioning it. The next scheduled Review gate run will clear the check; the change is not immediate.", ); } @@ -475,34 +464,15 @@ function normaliseAuthorLogin(login) { } function runText(commandArgs) { - for (let attempt = 1; attempt <= retryAttempts; attempt += 1) { - try { - return execFileSync("gh", commandArgs, { - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); - } catch (error) { - const detail = formatGhFailure(error); - const transient = isTransientGhFailure(error, detail); - if (!transient) { - failEvaluation(`GitHub CLI failed without a retryable error: ${detail}`); - } - if (attempt >= retryAttempts) { - failEvaluation(`GitHub CLI evaluation failed after ${retryAttempts} attempts: ${detail}`); - } - - const backoff = Math.min( - retryBackoffMs * 2 ** Math.min(attempt - 1, 20), - MAX_GH_RETRY_BACKOFF_MS, - ); - console.warn( - `GitHub CLI transient failure on attempt ${attempt}/${retryAttempts}: ${detail}. Retrying in ${backoff}ms.`, - ); - sleepSync(backoff); - } + try { + return runGhText(commandArgs, { + attempts: retryAttempts, + backoffMs: retryBackoffMs, + operation: "evaluation", + }); + } catch (error) { + failEvaluation(formatErrorMessage(error)); } - - failEvaluation("GitHub CLI evaluation ended without a result."); } function runJson(commandArgs) { @@ -514,16 +484,6 @@ function runJson(commandArgs) { } } -function isTransientGhFailure(error, detail) { - const evidence = [error?.code, error?.signal, detail].filter(Boolean).join("\n"); - return TRANSIENT_GH_FAILURE_PATTERNS.some((pattern) => pattern.test(evidence)); -} - -function formatGhFailure(error) { - const stderr = String(error?.stderr ?? "").trim(); - return stderr || formatErrorMessage(error); -} - function formatErrorMessage(error) { return error instanceof Error ? error.message : String(error); } @@ -533,11 +493,6 @@ function failEvaluation(message) { process.exit(EVALUATION_FAILURE_EXIT_CODE); } -function sleepSync(ms) { - if (ms <= 0) return; - Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); -} - function sleep(ms) { return new Promise((resolve) => globalThis.setTimeout(resolve, ms)); } diff --git a/scripts/lib/github-cli-retry.mjs b/scripts/lib/github-cli-retry.mjs new file mode 100644 index 00000000..4b7b1c76 --- /dev/null +++ b/scripts/lib/github-cli-retry.mjs @@ -0,0 +1,39 @@ +import { execFileSync } from "node:child_process"; +export const DEFAULT_GH_RETRY_ATTEMPTS = 6; +export const DEFAULT_GH_RETRY_BACKOFF_MS = 10_000; +const MAX_GH_RETRY_BACKOFF_MS = 60_000; +const TRANSIENT_GH_FAILURE_PATTERN = + /\b(?:HTTP\s+(?:429|500|502|503|504)|(?:secondary\s+)?rate limit(?:ed|ing)?|ETIMEDOUT|timeout|timed out|context deadline exceeded|deadline exceeded|ECONNRESET|connection reset|ENOTFOUND|EAI_AGAIN|getaddrinfo|could not resolve host|no such host|temporary failure in name resolution)\b/iu; + +export function runGhText(commandArgs, options = {}) { + const { + attempts = DEFAULT_GH_RETRY_ATTEMPTS, + backoffMs = DEFAULT_GH_RETRY_BACKOFF_MS, + operation = "operation", + } = options; + for (let attempt = 1; attempt <= attempts; attempt += 1) { + try { + return execFileSync("gh", commandArgs, { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + } catch (error) { + const detail = + String(error?.stderr ?? "").trim() || + (error instanceof Error ? error.message : String(error)); + const evidence = [error?.code, error?.signal, detail].filter(Boolean).join("\n"); + if (!TRANSIENT_GH_FAILURE_PATTERN.test(evidence)) { + throw new Error(`GitHub CLI failed without a retryable error: ${detail}`); + } + if (attempt >= attempts) { + throw new Error(`GitHub CLI ${operation} failed after ${attempts} attempts: ${detail}`); + } + const delay = Math.min(backoffMs * 2 ** Math.min(attempt - 1, 20), MAX_GH_RETRY_BACKOFF_MS); + console.warn( + `GitHub CLI transient failure on attempt ${attempt}/${attempts}: ${detail}. Retrying in ${delay}ms.`, + ); + if (delay > 0) Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, delay); + } + } + throw new Error(`GitHub CLI ${operation} ended without a result.`); +} diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index e0846756..255404ca 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -1,6 +1,12 @@ #!/usr/bin/env node -import { execFileSync } from "node:child_process"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath, URL } from "node:url"; +import { + DEFAULT_GH_RETRY_ATTEMPTS, + DEFAULT_GH_RETRY_BACKOFF_MS, + runGhText, +} from "./lib/github-cli-retry.mjs"; const CHECK_RUN_NAME = "Review gate"; const EXIT_VERDICTS = new Map([ @@ -8,67 +14,146 @@ const EXIT_VERDICTS = new Map([ [1, { conclusion: "failure", title: "Review gate found blocking review state" }], [2, { conclusion: "action_required", title: "Review gate could not evaluate" }], ]); - const rawArgs = process.argv.slice(2); -const repo = readRequiredArg("--repo"); -const headSha = readRequiredArg("--head-sha"); -const detailsUrl = readRequiredArg("--details-url"); -const exitCode = Number(readRequiredArg("--exit-code")); -const verdict = EXIT_VERDICTS.get(exitCode); - +const repo = readArg("--repo", true); +const detailsUrl = readArg("--details-url", true); +const retryAttempts = readIntegerArg("--retry-attempts", DEFAULT_GH_RETRY_ATTEMPTS, 1); +const retryBackoffMs = readIntegerArg("--retry-backoff-ms", DEFAULT_GH_RETRY_BACKOFF_MS, 0); if (!repo.includes("/")) fail("--repo must be owner/name."); -if (!/^[0-9a-f]{40}$/iu.test(headSha)) fail("--head-sha must be a full commit SHA."); -if (!verdict) fail("--exit-code must be 0, 1, or 2."); +try { + if (rawArgs.includes("--reconcile-open-prs")) { + reconcileOpenPullRequests(); + } else { + publishCheck(readArg("--head-sha", true), Number(readArg("--exit-code", true))); + } +} catch (error) { + const detail = error instanceof Error ? error.message : String(error); + fail(`Review gate publication could not complete: ${detail}`, 2); +} +function reconcileOpenPullRequests() { + const maxPrs = readIntegerArg("--max-prs", 25, 1); + const pages = JSON.parse( + runGithub( + ["api", "--paginate", "--slurp", `repos/${repo}/pulls?state=open&per_page=100`], + "pull request discovery", + ), + ); + const pulls = (Array.isArray(pages?.[0]) ? pages.flat() : pages).filter(Boolean); + const eligible = []; -const summary = - exitCode === 0 - ? "The review gate evaluated the pull request head and found no blocking state." - : exitCode === 1 - ? "The review gate evaluated the pull request head and found a blocking state." - : "The review gate could not evaluate the complete pull request review state."; + for (const pr of pulls) { + const label = `#${pr.number ?? "unknown"}`; + const reason = + String(pr.state).toLowerCase() !== "open" + ? "pull request is not open" + : pr.draft + ? "pull request is a draft" + : pr.head?.repo?.full_name !== repo + ? `fork head ${pr.head?.repo?.full_name ?? "unknown"} cannot receive a trusted scheduled Review gate check` + : !Number.isInteger(pr.number) || !/^[0-9a-f]{40}$/iu.test(pr.head?.sha ?? "") + ? "pull request metadata is incomplete" + : null; + if (reason) console.log(`Skipping ${label}: ${reason}.`); + else eligible.push(pr); + } -runText([ - "api", - "-X", - "POST", - `repos/${repo}/check-runs`, - "-H", - "X-GitHub-Api-Version: 2022-11-28", - "-f", - `name=${CHECK_RUN_NAME}`, - "-f", - `head_sha=${headSha}`, - "-f", - "status=completed", - "-f", - `conclusion=${verdict.conclusion}`, - "-f", - `details_url=${detailsUrl}`, - "-f", - `output[title]=${verdict.title}`, - "-f", - `output[summary]=${summary}`, -]); + const selected = eligible.slice(0, maxPrs); + if (eligible.length > maxPrs) { + console.warn( + `Review gate schedule cap hit: processing ${maxPrs} of ${eligible.length} eligible pull requests.`, + ); + } -console.log( - `${CHECK_RUN_NAME} check created for ${headSha} with conclusion ${verdict.conclusion}.`, -); + for (const pr of selected) { + publishCheck(pr.head.sha, evaluatePullRequest(pr)); + } -function readRequiredArg(name) { + console.log(`Scheduled Review gate evaluated ${selected.length} pull request(s).`); +} + +function evaluatePullRequest(pr) { + const evaluator = fileURLToPath(new URL("./check-pr-review-gate.mjs", import.meta.url)); + const result = spawnSync( + process.execPath, + [ + evaluator, + "--repo", + repo, + "--pr", + String(pr.number), + "--strict-head-review", + "--required-review-author", + "chatgpt-codex-connector", + "--allow-missing-head-review", + "--expected-head-oid", + pr.head.sha, + ], + { encoding: "utf8", env: process.env }, + ); + if (result.stdout) process.stdout.write(result.stdout); + if (result.stderr) process.stderr.write(result.stderr); + return [0, 1, 2].includes(result.status) ? result.status : 2; +} + +function publishCheck(headSha, exitCode) { + const verdict = EXIT_VERDICTS.get(exitCode); + if (!/^[0-9a-f]{40}$/iu.test(headSha)) fail("--head-sha must be a full commit SHA."); + if (!verdict) fail("--exit-code must be 0, 1, or 2."); + const summary = + exitCode === 0 + ? "The review gate evaluated the pull request head and found no blocking state." + : exitCode === 1 + ? "The review gate evaluated the pull request head and found a blocking state." + : "The review gate could not evaluate the complete pull request review state."; + const fields = { + name: CHECK_RUN_NAME, + head_sha: headSha, + status: "completed", + conclusion: verdict.conclusion, + details_url: detailsUrl, + "output[title]": verdict.title, + "output[summary]": summary, + }; + const formArgs = Object.entries(fields).flatMap(([name, value]) => ["-f", `${name}=${value}`]); + runGithub( + [ + "api", + "-X", + "POST", + `repos/${repo}/check-runs`, + "-H", + "X-GitHub-Api-Version: 2022-11-28", + ...formArgs, + ], + "check publication", + ); + console.log(`${CHECK_RUN_NAME} ${verdict.conclusion} check created for ${headSha}.`); +} + +function runGithub(commandArgs, operation) { + return runGhText(commandArgs, { + attempts: retryAttempts, + backoffMs: retryBackoffMs, + operation, + }); +} + +function readArg(name, required = false) { const index = rawArgs.indexOf(name); const value = index >= 0 ? rawArgs[index + 1] : null; - if (!value || value.startsWith("--")) fail(`Pass ${name} .`); + if (required && (!value || value.startsWith("--"))) fail(`Pass ${name} .`); return value; } -function runText(commandArgs) { - return execFileSync("gh", commandArgs, { - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); +function readIntegerArg(name, defaultValue, minimum) { + const rawValue = readArg(name); + if (rawValue === null) return defaultValue; + const value = Number(rawValue); + if (!Number.isInteger(value) || value < minimum) fail(`${name} must be at least ${minimum}.`); + return value; } -function fail(message) { +function fail(message, exitCode = 1) { console.error(message); - process.exit(1); + process.exit(exitCode); } From 01d6b2464d682123ca702cf14caa6ae9fa010bc4 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 05:07:13 +0530 Subject: [PATCH 07/10] test(ci): cover scheduled review reconciliation Signed-off-by: Tapish Khandelwal --- tests/extension/ci-workflow.test.ts | 34 +-- tests/scripts/check-pr-review-gate.test.ts | 286 ++++++------------ .../scripts/publish-review-gate-check.test.ts | 147 ++++++--- 3 files changed, 212 insertions(+), 255 deletions(-) diff --git a/tests/extension/ci-workflow.test.ts b/tests/extension/ci-workflow.test.ts index 29112299..29b4b5d0 100644 --- a/tests/extension/ci-workflow.test.ts +++ b/tests/extension/ci-workflow.test.ts @@ -37,35 +37,31 @@ describe("Pack CI workflow", () => { expect(workflow).not.toContain("actions/upload-artifact"); }); - it("runs the review gate and publishes comment-triggered verdicts on the PR head", async () => { + it("keeps event gates read-only and reconciles PR-head checks from scheduled trusted code", async () => { const workflow = await readFile( path.join(rootDir, ".github", "workflows", "review-gate.yml"), "utf8", ); expect(workflow).toContain("workflow_dispatch:"); - expect(workflow).toContain("pull_request:"); - expect(workflow).toContain("pull_request_review:"); - expect(workflow).toContain("pull_request_review_comment:"); - expect(workflow).toContain("issue_comment:"); - expect(workflow).not.toContain("pull_request_target:"); - expect(workflow).not.toContain("schedule:"); expect(workflow).toContain( - "if: github.event_name != 'issue_comment' || github.event.issue.pull_request", + "pull_request:\n types: [opened, reopened, synchronize, ready_for_review, edited]", ); - expect(workflow).toContain("github.event.issue.number"); + expect(workflow).toContain("pull_request_review:\n types: [submitted, edited, dismissed]"); expect(workflow).toContain( - '[ "${EVENT_NAME}" = "workflow_dispatch" ] || [ "${EVENT_NAME}" = "issue_comment" ]', + "pull_request_review_comment:\n types: [created, edited, deleted]", ); + expect(workflow).not.toContain("pull_request_target:"); + expect(workflow).not.toContain("issue_comment:"); + expect(workflow).not.toContain("github.event.issue"); + expect(workflow).toContain('schedule:\n - cron: "*/15 * * * *"'); expect(workflow).not.toContain("/review-gate"); - expect(workflow).toContain("name: Review findings gate"); expect(workflow).toContain("name: Review gate"); - expect(workflow).toContain("name: Publish Review gate verdict"); - expect(workflow).toContain("pull-requests: read"); + expect(workflow).toContain("if: github.event_name != 'schedule'"); + expect(workflow).toContain("if: github.event_name == 'schedule'"); expect(workflow.match(/checks: write/g)).toHaveLength(1); - expect(workflow).toContain("permissions:\n contents: read\n pull-requests: read\n\njobs:"); expect(workflow).toMatch( - /publish-pr-head-check:[\s\S]*?permissions:\n\s+contents: read\n\s+checks: write/, + /scheduled-review-gate:[\s\S]*?permissions:\n\s+contents: read\n\s+pull-requests: read\n\s+checks: write/, ); expect(workflow).not.toContain("statuses: write"); expect(workflow).toContain("GH_TOKEN: ${{ github.token }}"); @@ -80,10 +76,12 @@ describe("Pack CI workflow", () => { expect(workflow).toContain("--wait-head-review-ms 180000"); expect(workflow).toContain("--allow-missing-head-review"); expect(workflow).toContain('--expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}"'); - expect(workflow).toContain("steps.evaluate.outputs.exit_code || '2'"); expect(workflow).toContain("scripts/publish-review-gate-check.mjs"); - expect(workflow).toContain("PR-head Review gate publication is unavailable for fork"); - expect(workflow).toContain("needs.review-gate.outputs.head_repo == github.repository"); + expect(workflow).toContain("--reconcile-open-prs"); + expect(workflow).toContain("--max-prs 25"); + expect(workflow).toMatch( + /scheduled-review-gate:[\s\S]*?ref: \$\{\{ github\.event\.repository\.default_branch \}\}/, + ); }); it("keeps every workflow action reference within the repository selected-actions policy", async () => { diff --git a/tests/scripts/check-pr-review-gate.test.ts b/tests/scripts/check-pr-review-gate.test.ts index ad983af9..54055e91 100644 --- a/tests/scripts/check-pr-review-gate.test.ts +++ b/tests/scripts/check-pr-review-gate.test.ts @@ -14,150 +14,38 @@ describe("PR review gate", () => { expect(script.match(/isMinimized minimizedReason/g)).toHaveLength(2); }); - it("fails when the required reviewer has an unresolved PR-level finding", () => { - const fixturePath = writeFixture( - "unresolved-pr-level-finding", - reviewFixture({ - headRefOid: "head-sha", - comments: [ - { - id: "comment-1", - url: "https://github.com/lamemustafa/pack/pull/14#issuecomment-1", - createdAt: "2026-08-17T12:00:00Z", - isMinimized: false, - author: { login: "chatgpt-codex-connector[bot]" }, - body: "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", - }, - ], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const result = spawnSync( - process.execPath, + it.each([ + ["unresolved finding", [prFindingComment()], 1], + ["resolved finding", [prFindingComment({ isMinimized: true, minimizedReason: "resolved" })], 0], + [ + "off-topic minimization", + [prFindingComment({ isMinimized: true, minimizedReason: "off-topic" })], + 1, + ], + [ + "outdated minimization", + [prFindingComment({ isMinimized: true, minimizedReason: "outdated" })], + 1, + ], + ["unrelated author", [prFindingComment({ author: "external-reviewer" })], 0], + [ + "non-finding notes", [ - scriptPath, - "--repo", - "lamemustafa/pack", - "--pr", - "14", - "--fixture", - fixturePath, - "--strict-head-review", + prFindingComment({ body: "Codex Review: Didn't find any major issues. Breezy!" }), + prFindingComment({ body: "To use Codex here, create an environment for this repo." }), ], - { cwd: rootDir, encoding: "utf8" }, - ); - - expect(result.status).toBe(1); - expect(result.stderr).toContain("Unresolved PR-level review findings"); - expect(result.stderr).toContain("chatgpt-codex-connector[bot]"); - expect(result.stderr).toContain("Minimize each finding"); - }); - - it("does not block a PR-level finding minimized as resolved", () => { - const fixturePath = writeFixture( - "minimized-pr-level-finding", - reviewFixture({ - headRefOid: "head-sha", - comments: [prFindingComment({ isMinimized: true, minimizedReason: "resolved" })], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const output = execFileSync( - process.execPath, - [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], - { cwd: rootDir, encoding: "utf8" }, - ); - - expect(output).toContain("PR review gate passed"); - }); - - it("blocks a PR-level finding minimized as off-topic", () => { - const fixturePath = writeFixture( - "off-topic-pr-level-finding", - reviewFixture({ - headRefOid: "head-sha", - comments: [prFindingComment({ isMinimized: true, minimizedReason: "off-topic" })], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const result = spawnSync( - process.execPath, - [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], - { cwd: rootDir, encoding: "utf8" }, - ); - - expect(result.status).toBe(1); - expect(result.stderr).toContain("Unresolved PR-level review findings"); - }); - - it("blocks a PR-level finding minimized as outdated", () => { - const fixturePath = writeFixture( - "outdated-pr-level-finding", - reviewFixture({ - headRefOid: "head-sha", - comments: [prFindingComment({ isMinimized: true, minimizedReason: "outdated" })], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const result = spawnSync( - process.execPath, - [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], - { cwd: rootDir, encoding: "utf8" }, - ); - - expect(result.status).toBe(1); - expect(result.stderr).toContain("Unresolved PR-level review findings"); - }); - - it("does not treat clean-review notes or setup notices as PR-level findings", () => { - const fixturePath = writeFixture( - "non-finding-pr-level-comments", - reviewFixture({ - headRefOid: "head-sha", - comments: [ - prFindingComment({ - id: "clean-review", - body: "Codex Review: Didn't find any major issues. Breezy!", - }), - prFindingComment({ - id: "setup-notice", - body: "To use Codex here, create an environment for this repo.", - }), - ], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const output = execFileSync( - process.execPath, - [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], - { cwd: rootDir, encoding: "utf8" }, + 0, + ], + ])("maps PR-level %s", (name, comments, expectedExit) => { + const result = runGateFixture(name, comments); + expect(result.status).toBe(expectedExit); + expect(expectedExit === 0 ? result.stdout : result.stderr).toContain( + expectedExit === 0 ? "PR review gate passed" : "Unresolved PR-level review findings", ); - - expect(output).toContain("PR review gate passed"); - }); - - it("does not block a finding-shaped comment from an unrelated author", () => { - const fixturePath = writeFixture( - "unrelated-author-pr-level-finding", - reviewFixture({ - headRefOid: "head-sha", - comments: [prFindingComment({ author: "external-reviewer" })], - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }), - ); - - const output = execFileSync( - process.execPath, - [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixturePath], - { cwd: rootDir, encoding: "utf8" }, - ); - - expect(output).toContain("PR review gate passed"); + if (name === "unresolved finding") { + expect(result.stderr).toContain("Hide → Resolved"); + expect(result.stderr).toContain("next scheduled Review gate run"); + } }); it("evaluates PR-level findings from paginated fixture pages", () => { @@ -194,7 +82,7 @@ describe("PR review gate", () => { reviews: [review({ state: "COMMENTED", commit: "head-sha" })], }); const missingPrPage = { data: { repository: { pullRequest: null } } }; - const { result, attempts } = runGateWithFakeGh("missing-paginated-pr", [ + const { result, attempts } = runGateWithFakeGh([ { status: 0, stdout: JSON.stringify(firstPage) }, { status: 0, stdout: JSON.stringify(missingPrPage) }, ]); @@ -205,23 +93,6 @@ describe("PR review gate", () => { expect(attempts).toBe(2); }); - it("retries a transient GitHub CLI failure and then succeeds", () => { - const fixture = reviewFixture({ - headRefOid: "head-sha", - reviews: [review({ state: "COMMENTED", commit: "head-sha" })], - }); - const { result, attempts } = runGateWithFakeGh("transient-then-success", [ - { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, - { status: 0, stdout: JSON.stringify(fixture) }, - ]); - - expect(result.status).toBe(0); - expect(result.stdout).toContain("PR review gate passed"); - expect(result.stderr).toContain("transient failure on attempt 1/2"); - expect(result.stderr).toContain("Retrying in 0ms"); - expect(attempts).toBe(2); - }); - it.each([ ["http-429", "gh: HTTP 429: too many requests\n"], ["http-500", "gh: HTTP 500: internal server error\n"], @@ -231,12 +102,12 @@ describe("PR review gate", () => { ["timeout", "gh: request timed out\n"], ["connection-reset", "gh: read: connection reset by peer\n"], ["dns", "gh: dial tcp: lookup api.github.com: no such host\n"], - ])("retries the allowed %s transient failure", (name, stderr) => { + ])("retries the allowed %s transient failure", (_name, stderr) => { const fixture = reviewFixture({ headRefOid: "head-sha", reviews: [review({ state: "COMMENTED", commit: "head-sha" })], }); - const { result, attempts } = runGateWithFakeGh(name, [ + const { result, attempts } = runGateWithFakeGh([ { status: 1, stderr }, { status: 0, stdout: JSON.stringify(fixture) }, ]); @@ -246,42 +117,36 @@ describe("PR review gate", () => { expect(attempts).toBe(2); }); - it("uses the could-not-evaluate exit code after transient retries are exhausted", () => { - const { result, attempts } = runGateWithFakeGh( - "transient-exhaustion", + it.each([ + [ + "transient retry exhaustion", [ { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, { status: 1, stderr: "gh: Service Unavailable (HTTP 503)\n" }, ], - ["--retry-attempts", "3"], - ); - - expect(result.status).toBe(2); - expect(result.stderr).toContain("Review gate could not evaluate"); - expect(result.stderr).toContain("failed after 3 attempts"); - expect(result.stderr).toContain("HTTP 503"); - expect(result.stderr).not.toContain("Unresolved review threads"); - expect(attempts).toBe(3); - }); - - it("does not retry a non-transient GitHub CLI failure", () => { - const { result, attempts } = runGateWithFakeGh( - "non-transient", + 3, + "failed after 3 attempts", + 3, + ], + [ + "non-transient failure", [ { status: 1, stderr: "gh: GraphQL: Could not resolve to a Repository with the name pack\n", }, ], - ["--retry-attempts", "3"], - ); - + 3, + "Could not resolve to a Repository", + 1, + ], + ])("fails closed for %s", (_name, responses, retryAttempts, message, expectedAttempts) => { + const { result, attempts } = runGateWithFakeGh(responses, retryAttempts); expect(result.status).toBe(2); expect(result.stderr).toContain("Review gate could not evaluate"); - expect(result.stderr).toContain("Could not resolve to a Repository"); - expect(result.stderr).not.toContain("Retrying in"); - expect(attempts).toBe(1); + expect(result.stderr).toContain(message); + expect(attempts).toBe(expectedAttempts); }); it("fails when unresolved review threads are present", () => { @@ -1181,19 +1046,22 @@ function reviewFixture({ }; } -function prFindingComment({ - id = "comment-1", - isMinimized = false, - minimizedReason = null, - author = "chatgpt-codex-connector[bot]", - body = "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", -}: { - id?: string; - isMinimized?: boolean; - minimizedReason?: string | null; - author?: string; - body?: string; -} = {}) { +function prFindingComment( + options: { + id?: string; + isMinimized?: boolean; + minimizedReason?: string | null; + author?: string; + body?: string; + } = {}, +) { + const { + id = "comment-1", + isMinimized = false, + minimizedReason = null, + author = "chatgpt-codex-connector[bot]", + body = "![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Fix this.", + } = options; return { id, url: `https://github.com/lamemustafa/pack/pull/14#issuecomment-${id}`, @@ -1205,13 +1073,28 @@ function prFindingComment({ }; } +function runGateFixture(name: string, comments: unknown[]) { + const fixture = writeFixture( + `pr-finding-${name.replaceAll(" ", "-")}`, + reviewFixture({ + headRefOid: "head-sha", + comments, + reviews: [review({ state: "COMMENTED", commit: "head-sha" })], + }), + ); + return spawnSync( + process.execPath, + [scriptPath, "--repo", "lamemustafa/pack", "--pr", "14", "--fixture", fixture], + { cwd: rootDir, encoding: "utf8" }, + ); +} + function runGateWithFakeGh( - name: string, responses: Array<{ status: number; stdout?: string; stderr?: string }>, - extraArgs: string[] = ["--retry-attempts", String(responses.length)], + retryAttempts = responses.length, ) { const directory = mkdtempSync(path.join(tmpdir(), "pack-review-gate-gh-")); - const statePath = path.join(directory, `${name}-attempts.txt`); + const statePath = path.join(directory, "attempts.txt"); const fakeGhPath = path.join(directory, "gh"); const fakeGhSource = `#!/usr/bin/env node import { existsSync, readFileSync, writeFileSync } from "node:fs"; @@ -1237,7 +1120,8 @@ process.exit(response.status); "14", "--retry-backoff-ms", "0", - ...extraArgs, + "--retry-attempts", + String(retryAttempts), ], { cwd: rootDir, diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index 9be11bf8..1887c4d2 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process"; -import { chmodSync, existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; @@ -13,65 +13,140 @@ describe("PR-head Review gate check publisher", () => { [0, "success"], [1, "failure"], [2, "action_required"], - ])("maps exit %i to the %s conclusion", (exitCode, conclusion) => { - const { result, calls } = runPublisher(exitCode); - + ])("maps exit %i to the %s Review gate conclusion", (exitCode, conclusion) => { + const { result, calls } = runPublisher(exitCode, [{ status: 0 }]); expect(result.status).toBe(0); - expect(result.stdout).toContain(`Review gate check created for ${headSha}`); - expect(calls).toHaveLength(1); - expect(calls[0]).toContain("repos/lamemustafa/pack/check-runs"); - expect(calls[0]).toContain("name=Review gate"); - expect(calls[0]).toContain(`head_sha=${headSha}`); - expect(calls[0]).toContain("status=completed"); - expect(calls[0]).toContain(`conclusion=${conclusion}`); + const call = calls[0]?.join(" ") ?? ""; + expect(call).toContain("repos/lamemustafa/pack/check-runs"); + expect(call).toContain("name=Review gate"); + expect(call).toContain(`head_sha=${headSha}`); + expect(call).toContain(`conclusion=${conclusion}`); }); - it("rejects an unknown evaluator exit code without writing a check", () => { - const { result, calls } = runPublisher(3); + it.each([ + ["succeeds after retry", [1, 0], 0], + ["reports exhaustion", [1, 1], 2], + ])("%s for transient check publication", (_name, statuses, expectedExit) => { + const responses = statuses.map((status) => ({ + status, + stderr: status ? "gh: HTTP 503\n" : "", + })); + const { result, calls } = runPublisher(1, responses); + expect(result.status).toBe(expectedExit); + if (expectedExit === 2) expect(result.stderr).toContain("failed after 2 attempts"); + expect(calls).toHaveLength(2); + }); - expect(result.status).toBe(1); - expect(result.stderr).toContain("--exit-code must be 0, 1, or 2"); - expect(calls).toEqual([]); + it("selects only open non-draft same-repository PRs and logs skips and the cap", () => { + const pulls = [ + pull(1), + pull(2, { draft: true }), + pull(3, { state: "closed" }), + pull(4, { headRepo: "external/pack" }), + pull(5), + ]; + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1"], + pulls, + cleanReviewFixture(), + ); + const publications = calls.filter((call) => call.includes("repos/lamemustafa/pack/check-runs")); + + expect(result.status).toBe(0); + expect(result.stdout).toContain("Skipping #2: pull request is a draft"); + expect(result.stdout).toContain("Skipping #3: pull request is not open"); + expect(result.stdout).toContain("Skipping #4: fork head external/pack"); + expect(result.stderr).toContain("schedule cap hit: processing 1 of 2 eligible"); + expect(publications).toHaveLength(1); + expect(publications[0]).toContain(`head_sha=${headSha}`); }); }); -function runPublisher(exitCode: number) { - const directory = mkdtempSync(path.join(tmpdir(), "pack-review-check-publisher-")); +function runPublisher(exitCode: number, responses: Array<{ status: number; stderr?: string }>) { + return runScript(["--head-sha", headSha, "--exit-code", String(exitCode)], [], {}, responses); +} + +function runScript( + modeArgs: string[], + pulls: unknown[] = [], + fixture: unknown = {}, + responses: Array<{ status: number; stderr?: string }> = [{ status: 0 }], +) { + const directory = mkdtempSync(path.join(tmpdir(), "pack-review-publisher-")); const callsPath = path.join(directory, "calls.json"); const fakeGhPath = path.join(directory, "gh"); - const fakeGhSource = `#!/usr/bin/env node -import { existsSync, readFileSync, writeFileSync } from "node:fs"; -const callsPath = ${JSON.stringify(callsPath)}; -const calls = existsSync(callsPath) ? JSON.parse(readFileSync(callsPath, "utf8")) : []; -calls.push(process.argv.slice(2)); -writeFileSync(callsPath, JSON.stringify(calls), "utf8"); -process.stdout.write(JSON.stringify({ id: 1 })); -`; writeFileSync(fakeGhPath, fakeGhSource, "utf8"); chmodSync(fakeGhPath, 0o755); - const result = spawnSync( process.execPath, [ scriptPath, "--repo", "lamemustafa/pack", - "--head-sha", - headSha, - "--exit-code", - String(exitCode), "--details-url", "https://github.com/lamemustafa/pack/actions/runs/1", + ...modeArgs, + "--retry-attempts", + String(responses.length), + "--retry-backoff-ms", + "0", ], { cwd: rootDir, encoding: "utf8", - env: { ...process.env, PATH: `${directory}${path.delimiter}${process.env.PATH ?? ""}` }, + env: { + ...process.env, + PATH: `${directory}${path.delimiter}${process.env.PATH ?? ""}`, + FAKE_CALLS: callsPath, + FAKE_FIXTURE: JSON.stringify(fixture), + FAKE_PULLS: JSON.stringify([pulls]), + FAKE_RESPONSES: JSON.stringify(responses), + }, }, ); + const calls = readFileSync(callsPath, "utf8"); + return { result, calls: JSON.parse(calls) as string[][] }; +} + +const fakeGhSource = `#!/usr/bin/env node +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +const args = process.argv.slice(2); +const calls = existsSync(process.env.FAKE_CALLS) ? JSON.parse(readFileSync(process.env.FAKE_CALLS, "utf8")) : []; +calls.push(args); writeFileSync(process.env.FAKE_CALLS, JSON.stringify(calls), "utf8"); +const text = args.join(" "); +if (text.includes("pulls?state=open")) process.stdout.write(process.env.FAKE_PULLS); +else if (text.includes("graphql")) process.stdout.write(process.env.FAKE_FIXTURE); +else if (text.includes("check-runs")) { + const attempts = calls.filter((call) => call.includes("repos/lamemustafa/pack/check-runs")).length; + const responses = JSON.parse(process.env.FAKE_RESPONSES); + const response = responses[Math.min(attempts - 1, responses.length - 1)]; + if (response.stderr) process.stderr.write(response.stderr); + process.exit(response.status); +} else process.exit(1); +`; - const calls = existsSync(callsPath) - ? (JSON.parse(readFileSync(callsPath, "utf8")) as string[][]) - : []; - return { result, calls }; +function pull( + number: number, + { draft = false, state = "open", headRepo = "lamemustafa/pack" } = {}, +) { + const sha = number === 1 ? headSha : String(number).repeat(40); + return { + number, + state, + draft, + head: { sha, repo: { full_name: headRepo } }, + }; } + +const cleanReviewFixture = () => ({ + data: { + repository: { + pullRequest: { + body: "Pack Workflow Preflight\nPrivacy And Data-Flow Impact\nSensitive Surface Review\nVerification\nPR Review Follow-Up\npnpm workflow:preflight", + headRefName: "tapish-codex/test", + headRepository: { nameWithOwner: "lamemustafa/pack" }, + headRefOid: headSha, + }, + }, + }, +}); From a2354c3ac3f6d2577fc4d7c553f8f00c10ea36e5 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 05:30:01 +0530 Subject: [PATCH 08/10] fix(ci): isolate scheduled review reconciliation Signed-off-by: Tapish Khandelwal --- .github/workflows/review-gate.yml | 37 ++++++++------------------- scripts/publish-review-gate-check.mjs | 20 +++++++++++---- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index f1929409..e9758a79 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -4,10 +4,6 @@ on: schedule: - cron: "*/15 * * * *" workflow_dispatch: - inputs: - pr: - description: Pull request number to check - required: true pull_request: types: [opened, reopened, synchronize, ready_for_review, edited] pull_request_review: @@ -22,7 +18,10 @@ permissions: jobs: review-gate: name: Review gate - if: github.event_name != 'schedule' + if: >- + github.event_name == 'pull_request' || + github.event_name == 'pull_request_review' || + github.event_name == 'pull_request_review_comment' runs-on: ubuntu-latest timeout-minutes: 8 @@ -31,9 +30,7 @@ jobs: id: resolve-pr shell: bash env: - GH_TOKEN: ${{ github.token }} - EVENT_NAME: ${{ github.event_name }} - PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr }} + PR_NUMBER: ${{ github.event.pull_request.number }} EVENT_HEAD_REF: ${{ github.event.pull_request.head.ref }} EVENT_BASE_REF: ${{ github.event.pull_request.base.ref }} EVENT_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} @@ -46,22 +43,10 @@ jobs: exit 1 fi - if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then - mapfile -t pr_metadata < <( - gh pr view "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" \ - --json headRefName,baseRefName,headRepository,headRefOid \ - --jq '.headRefName, .baseRefName, .headRepository.nameWithOwner, .headRefOid' - ) - head_ref="${pr_metadata[0]:-}" - base_ref="${pr_metadata[1]:-}" - head_repo="${pr_metadata[2]:-}" - head_sha="${pr_metadata[3]:-}" - else - head_ref="${EVENT_HEAD_REF}" - base_ref="${EVENT_BASE_REF}" - head_repo="${EVENT_HEAD_REPO}" - head_sha="${EVENT_HEAD_SHA}" - fi + head_ref="${EVENT_HEAD_REF}" + base_ref="${EVENT_BASE_REF}" + head_repo="${EVENT_HEAD_REPO}" + head_sha="${EVENT_HEAD_SHA}" if [ -z "${head_ref}" ] || [ -z "${base_ref}" ] || [ -z "${head_repo}" ] || [ -z "${head_sha}" ]; then echo "Could not resolve complete pull request metadata for #${PR_NUMBER}." >&2 @@ -116,7 +101,7 @@ jobs: scheduled-review-gate: name: Reconcile Review gate checks - if: github.event_name == 'schedule' + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest timeout-minutes: 14 permissions: @@ -144,5 +129,5 @@ jobs: node scripts/publish-review-gate-check.mjs --repo "${{ github.repository }}" --reconcile-open-prs - --max-prs 25 + --max-prs 4 --details-url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" diff --git a/scripts/publish-review-gate-check.mjs b/scripts/publish-review-gate-check.mjs index 255404ca..0bb471ee 100644 --- a/scripts/publish-review-gate-check.mjs +++ b/scripts/publish-review-gate-check.mjs @@ -8,11 +8,11 @@ import { runGhText, } from "./lib/github-cli-retry.mjs"; -const CHECK_RUN_NAME = "Review gate"; +const CHECK_RUN_NAME = "Review gate (scheduled)"; const EXIT_VERDICTS = new Map([ - [0, { conclusion: "success", title: "Review gate passed" }], - [1, { conclusion: "failure", title: "Review gate found blocking review state" }], - [2, { conclusion: "action_required", title: "Review gate could not evaluate" }], + [0, { conclusion: "success", title: "Scheduled review gate passed" }], + [1, { conclusion: "failure", title: "Scheduled review gate found blocking review state" }], + [2, { conclusion: "action_required", title: "Scheduled review gate could not evaluate" }], ]); const rawArgs = process.argv.slice(2); const repo = readArg("--repo", true); @@ -32,6 +32,11 @@ try { } function reconcileOpenPullRequests() { const maxPrs = readIntegerArg("--max-prs", 25, 1); + const selectionOffset = readIntegerArg( + "--selection-offset", + Math.floor(Date.now() / (15 * 60 * 1000)), + 0, + ); const pages = JSON.parse( runGithub( ["api", "--paginate", "--slurp", `repos/${repo}/pulls?state=open&per_page=100`], @@ -57,7 +62,8 @@ function reconcileOpenPullRequests() { else eligible.push(pr); } - const selected = eligible.slice(0, maxPrs); + const start = eligible.length === 0 ? 0 : (selectionOffset * maxPrs) % eligible.length; + const selected = [...eligible.slice(start), ...eligible.slice(0, start)].slice(0, maxPrs); if (eligible.length > maxPrs) { console.warn( `Review gate schedule cap hit: processing ${maxPrs} of ${eligible.length} eligible pull requests.`, @@ -84,6 +90,10 @@ function evaluatePullRequest(pr) { "--strict-head-review", "--required-review-author", "chatgpt-codex-connector", + "--wait-head-review-ms", + "180000", + "--poll-interval-ms", + "10000", "--allow-missing-head-review", "--expected-head-oid", pr.head.sha, From 598f13e4928c1be1f389a0bfbb25034f3c139efe Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 05:30:10 +0530 Subject: [PATCH 09/10] test(ci): cover scheduled gate isolation Signed-off-by: Tapish Khandelwal --- tests/extension/ci-workflow.test.ts | 12 +++-- .../scripts/publish-review-gate-check.test.ts | 44 +++++++++++++++++-- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/tests/extension/ci-workflow.test.ts b/tests/extension/ci-workflow.test.ts index 29b4b5d0..4b633a4d 100644 --- a/tests/extension/ci-workflow.test.ts +++ b/tests/extension/ci-workflow.test.ts @@ -57,8 +57,14 @@ describe("Pack CI workflow", () => { expect(workflow).toContain('schedule:\n - cron: "*/15 * * * *"'); expect(workflow).not.toContain("/review-gate"); expect(workflow).toContain("name: Review gate"); - expect(workflow).toContain("if: github.event_name != 'schedule'"); - expect(workflow).toContain("if: github.event_name == 'schedule'"); + expect(workflow).toContain( + "if: >-\n github.event_name == 'pull_request' ||\n github.event_name == 'pull_request_review' ||\n github.event_name == 'pull_request_review_comment'", + ); + expect(workflow).toContain( + "if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch'", + ); + expect(workflow).not.toContain("github.event.inputs.pr"); + expect(workflow).not.toContain("EVENT_NAME: ${{ github.event_name }}"); expect(workflow.match(/checks: write/g)).toHaveLength(1); expect(workflow).toMatch( /scheduled-review-gate:[\s\S]*?permissions:\n\s+contents: read\n\s+pull-requests: read\n\s+checks: write/, @@ -78,7 +84,7 @@ describe("Pack CI workflow", () => { expect(workflow).toContain('--expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}"'); expect(workflow).toContain("scripts/publish-review-gate-check.mjs"); expect(workflow).toContain("--reconcile-open-prs"); - expect(workflow).toContain("--max-prs 25"); + expect(workflow).toContain("--max-prs 4"); expect(workflow).toMatch( /scheduled-review-gate:[\s\S]*?ref: \$\{\{ github\.event\.repository\.default_branch \}\}/, ); diff --git a/tests/scripts/publish-review-gate-check.test.ts b/tests/scripts/publish-review-gate-check.test.ts index 1887c4d2..1f6c3316 100644 --- a/tests/scripts/publish-review-gate-check.test.ts +++ b/tests/scripts/publish-review-gate-check.test.ts @@ -13,12 +13,12 @@ describe("PR-head Review gate check publisher", () => { [0, "success"], [1, "failure"], [2, "action_required"], - ])("maps exit %i to the %s Review gate conclusion", (exitCode, conclusion) => { + ])("maps exit %i to the %s scheduled Review gate conclusion", (exitCode, conclusion) => { const { result, calls } = runPublisher(exitCode, [{ status: 0 }]); expect(result.status).toBe(0); const call = calls[0]?.join(" ") ?? ""; expect(call).toContain("repos/lamemustafa/pack/check-runs"); - expect(call).toContain("name=Review gate"); + expect(call).toContain("name=Review gate (scheduled)"); expect(call).toContain(`head_sha=${headSha}`); expect(call).toContain(`conclusion=${conclusion}`); }); @@ -46,7 +46,7 @@ describe("PR-head Review gate check publisher", () => { pull(5), ]; const { result, calls } = runScript( - ["--reconcile-open-prs", "--max-prs", "1"], + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "0"], pulls, cleanReviewFixture(), ); @@ -60,6 +60,24 @@ describe("PR-head Review gate check publisher", () => { expect(publications).toHaveLength(1); expect(publications[0]).toContain(`head_sha=${headSha}`); }); + + it("rotates the capped selection across eligible pull requests", () => { + const { result, calls } = runScript( + ["--reconcile-open-prs", "--max-prs", "1", "--selection-offset", "1"], + [pull(1), pull(2)], + cleanReviewFixture(), + ); + const publication = calls.find((call) => call.includes("repos/lamemustafa/pack/check-runs")); + + expect(result.status).toBe(0); + expect(publication).toContain(`head_sha=${"2".repeat(40)}`); + }); + + it("preserves the event gate's current-head review wait for scheduled evaluation", () => { + const script = readFileSync(scriptPath, "utf8"); + expect(script).toMatch(/"--wait-head-review-ms",\s*"180000"/u); + expect(script).toMatch(/"--poll-interval-ms",\s*"10000"/u); + }); }); function runPublisher(exitCode: number, responses: Array<{ status: number; stderr?: string }>) { @@ -115,7 +133,14 @@ const calls = existsSync(process.env.FAKE_CALLS) ? JSON.parse(readFileSync(proce calls.push(args); writeFileSync(process.env.FAKE_CALLS, JSON.stringify(calls), "utf8"); const text = args.join(" "); if (text.includes("pulls?state=open")) process.stdout.write(process.env.FAKE_PULLS); -else if (text.includes("graphql")) process.stdout.write(process.env.FAKE_FIXTURE); +else if (text.includes("graphql")) { + const fixture = JSON.parse(process.env.FAKE_FIXTURE); + const number = Number(args.find((arg) => arg.startsWith("number="))?.split("=")[1]); + const pull = JSON.parse(process.env.FAKE_PULLS).flat().find((item) => item.number === number); + fixture.data.repository.pullRequest.headRefOid = pull.head.sha; + fixture.data.repository.pullRequest.reviews.nodes[0].commit.oid = pull.head.sha; + process.stdout.write(JSON.stringify(fixture)); +} else if (text.includes("check-runs")) { const attempts = calls.filter((call) => call.includes("repos/lamemustafa/pack/check-runs")).length; const responses = JSON.parse(process.env.FAKE_RESPONSES); @@ -146,6 +171,17 @@ const cleanReviewFixture = () => ({ headRefName: "tapish-codex/test", headRepository: { nameWithOwner: "lamemustafa/pack" }, headRefOid: headSha, + reviews: { + nodes: [ + { + state: "COMMENTED", + submittedAt: "2026-08-18T00:00:00Z", + author: { login: "chatgpt-codex-connector" }, + commit: { oid: headSha }, + }, + ], + pageInfo: { hasNextPage: false, endCursor: null }, + }, }, }, }, From 2942bb7e4f1916bd0a628f71ba9d0543793ccc54 Mon Sep 17 00:00:00 2001 From: Tapish Khandelwal Date: Tue, 18 Aug 2026 05:55:17 +0530 Subject: [PATCH 10/10] fix(ci): separate trusted review reconciliation Signed-off-by: Tapish Khandelwal --- .github/workflows/review-gate-reconcile.yml | 43 ++++++++++ .github/workflows/review-gate.yml | 36 --------- tests/extension/ci-workflow.test.ts | 89 ++++++++++++--------- 3 files changed, 93 insertions(+), 75 deletions(-) create mode 100644 .github/workflows/review-gate-reconcile.yml diff --git a/.github/workflows/review-gate-reconcile.yml b/.github/workflows/review-gate-reconcile.yml new file mode 100644 index 00000000..e0cb087e --- /dev/null +++ b/.github/workflows/review-gate-reconcile.yml @@ -0,0 +1,43 @@ +name: Reconcile review findings gate + +on: + schedule: + - cron: "*/15 * * * *" + workflow_dispatch: + +permissions: + contents: read + pull-requests: read + +jobs: + scheduled-review-gate: + name: Reconcile Review gate checks + runs-on: ubuntu-latest + timeout-minutes: 14 + permissions: + contents: read + pull-requests: read + checks: write + + steps: + - name: Checkout trusted default-branch code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + fetch-depth: 1 + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + + - name: Set up Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 + with: + node-version: 22.13.0 + + - name: Reconcile open pull request checks + env: + GH_TOKEN: ${{ github.token }} + run: >- + node scripts/publish-review-gate-check.mjs + --repo "${{ github.repository }}" + --reconcile-open-prs + --max-prs 4 + --details-url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index e9758a79..de4bd8f6 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -1,9 +1,6 @@ name: Review findings gate on: - schedule: - - cron: "*/15 * * * *" - workflow_dispatch: pull_request: types: [opened, reopened, synchronize, ready_for_review, edited] pull_request_review: @@ -98,36 +95,3 @@ jobs: --expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}" env: GH_TOKEN: ${{ github.token }} - - scheduled-review-gate: - name: Reconcile Review gate checks - if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' - runs-on: ubuntu-latest - timeout-minutes: 14 - permissions: - contents: read - pull-requests: read - checks: write - - steps: - - name: Checkout trusted default-branch code - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 - with: - fetch-depth: 1 - ref: ${{ github.event.repository.default_branch }} - persist-credentials: false - - - name: Set up Node.js - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 - with: - node-version: 22.13.0 - - - name: Reconcile open pull request checks - env: - GH_TOKEN: ${{ github.token }} - run: >- - node scripts/publish-review-gate-check.mjs - --repo "${{ github.repository }}" - --reconcile-open-prs - --max-prs 4 - --details-url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" diff --git a/tests/extension/ci-workflow.test.ts b/tests/extension/ci-workflow.test.ts index 4b633a4d..a201ff9e 100644 --- a/tests/extension/ci-workflow.test.ts +++ b/tests/extension/ci-workflow.test.ts @@ -37,55 +37,66 @@ describe("Pack CI workflow", () => { expect(workflow).not.toContain("actions/upload-artifact"); }); - it("keeps event gates read-only and reconciles PR-head checks from scheduled trusted code", async () => { - const workflow = await readFile( + it("isolates PR-head execution from trusted scheduled reconciliation", async () => { + const prWorkflow = await readFile( path.join(rootDir, ".github", "workflows", "review-gate.yml"), "utf8", ); + const trustedWorkflow = await readFile( + path.join(rootDir, ".github", "workflows", "review-gate-reconcile.yml"), + "utf8", + ); - expect(workflow).toContain("workflow_dispatch:"); - expect(workflow).toContain( + expect(prWorkflow).not.toContain("workflow_dispatch:"); + expect(prWorkflow).not.toContain("schedule:"); + expect(prWorkflow).toContain( "pull_request:\n types: [opened, reopened, synchronize, ready_for_review, edited]", ); - expect(workflow).toContain("pull_request_review:\n types: [submitted, edited, dismissed]"); - expect(workflow).toContain( + expect(prWorkflow).toContain("pull_request_review:\n types: [submitted, edited, dismissed]"); + expect(prWorkflow).toContain( "pull_request_review_comment:\n types: [created, edited, deleted]", ); - expect(workflow).not.toContain("pull_request_target:"); - expect(workflow).not.toContain("issue_comment:"); - expect(workflow).not.toContain("github.event.issue"); - expect(workflow).toContain('schedule:\n - cron: "*/15 * * * *"'); - expect(workflow).not.toContain("/review-gate"); - expect(workflow).toContain("name: Review gate"); - expect(workflow).toContain( - "if: >-\n github.event_name == 'pull_request' ||\n github.event_name == 'pull_request_review' ||\n github.event_name == 'pull_request_review_comment'", - ); - expect(workflow).toContain( - "if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch'", - ); - expect(workflow).not.toContain("github.event.inputs.pr"); - expect(workflow).not.toContain("EVENT_NAME: ${{ github.event_name }}"); - expect(workflow.match(/checks: write/g)).toHaveLength(1); - expect(workflow).toMatch( + expect(prWorkflow).not.toContain("pull_request_target:"); + expect(prWorkflow).not.toContain("issue_comment:"); + expect(prWorkflow).not.toContain("github.event.issue"); + expect(prWorkflow).not.toContain("/review-gate"); + expect(prWorkflow).toContain("name: Review gate"); + expect(prWorkflow).not.toContain("checks: write"); + expect(prWorkflow).not.toContain("statuses: write"); + expect(prWorkflow).toContain("GH_TOKEN: ${{ github.token }}"); + expect(prWorkflow).toContain("repository: ${{ steps.resolve-pr.outputs.head_repo }}"); + expect(prWorkflow).toContain("ref: ${{ steps.resolve-pr.outputs.head_sha }}"); + expect(prWorkflow).toContain("pnpm install --frozen-lockfile"); + expect(prWorkflow).toContain("pnpm workflow:preflight"); + expect(prWorkflow).toContain("pnpm review:gate"); + expect(prWorkflow).toContain("ready_for_review, edited"); + expect(prWorkflow).toContain("--strict-head-review"); + expect(prWorkflow).toContain("--required-review-author chatgpt-codex-connector"); + expect(prWorkflow).toContain("--wait-head-review-ms 180000"); + expect(prWorkflow).toContain("--allow-missing-head-review"); + expect(prWorkflow).toContain('--expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}"'); + + expect(trustedWorkflow).toContain("workflow_dispatch:"); + expect(trustedWorkflow).toContain('schedule:\n - cron: "*/15 * * * *"'); + expect(trustedWorkflow).not.toContain("pull_request:"); + expect(trustedWorkflow).not.toContain("pull_request_review:"); + expect(trustedWorkflow).not.toContain("pull_request_review_comment:"); + expect(trustedWorkflow).not.toContain("github.event.inputs.pr"); + expect(trustedWorkflow).not.toContain("EVENT_NAME: ${{ github.event_name }}"); + expect(trustedWorkflow.match(/checks: write/g)).toHaveLength(1); + expect(trustedWorkflow).toMatch( /scheduled-review-gate:[\s\S]*?permissions:\n\s+contents: read\n\s+pull-requests: read\n\s+checks: write/, ); - expect(workflow).not.toContain("statuses: write"); - expect(workflow).toContain("GH_TOKEN: ${{ github.token }}"); - expect(workflow).toContain("repository: ${{ steps.resolve-pr.outputs.head_repo }}"); - expect(workflow).toContain("ref: ${{ steps.resolve-pr.outputs.head_sha }}"); - expect(workflow).toContain("pnpm install --frozen-lockfile"); - expect(workflow).toContain("pnpm workflow:preflight"); - expect(workflow).toContain("pnpm review:gate"); - expect(workflow).toContain("ready_for_review, edited"); - expect(workflow).toContain("--strict-head-review"); - expect(workflow).toContain("--required-review-author chatgpt-codex-connector"); - expect(workflow).toContain("--wait-head-review-ms 180000"); - expect(workflow).toContain("--allow-missing-head-review"); - expect(workflow).toContain('--expected-head-oid "${{ steps.resolve-pr.outputs.head_sha }}"'); - expect(workflow).toContain("scripts/publish-review-gate-check.mjs"); - expect(workflow).toContain("--reconcile-open-prs"); - expect(workflow).toContain("--max-prs 4"); - expect(workflow).toMatch( + expect(trustedWorkflow).not.toContain("statuses: write"); + expect(trustedWorkflow).toContain("GH_TOKEN: ${{ github.token }}"); + expect(trustedWorkflow).not.toContain("steps.resolve-pr.outputs"); + expect(trustedWorkflow).not.toContain("cache:"); + expect(trustedWorkflow).not.toContain("pnpm install"); + expect(trustedWorkflow).not.toContain("pnpm workflow:preflight"); + expect(trustedWorkflow).toContain("scripts/publish-review-gate-check.mjs"); + expect(trustedWorkflow).toContain("--reconcile-open-prs"); + expect(trustedWorkflow).toContain("--max-prs 4"); + expect(trustedWorkflow).toMatch( /scheduled-review-gate:[\s\S]*?ref: \$\{\{ github\.event\.repository\.default_branch \}\}/, ); });