diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 60cf184e26..5cf5f84626 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -73,15 +73,56 @@ jobs: } const statusSha = context.payload.sha; - const associatedPrs = await github.paginate( - github.rest.repos.listPullRequestsAssociatedWithCommit, - { owner, repo, commit_sha: statusSha, per_page: 100 } - ); - const candidates = associatedPrs.filter( - candidate => - candidate.state === "open" && - candidate.head?.sha === statusSha - ); + // Primary authority: GitHub's commit-to-PR index. This read can + // lag a fresh head push, so a non-match is not proof of absence. + let candidates = []; + try { + const associatedPrs = await github.paginate( + github.rest.repos.listPullRequestsAssociatedWithCommit, + { owner, repo, commit_sha: statusSha, per_page: 100 } + ); + candidates = associatedPrs.filter( + candidate => + candidate.state === "open" && + candidate.head?.sha === statusSha + ); + } catch (error) { + core.warning( + `Could not list PRs associated with commit ${statusSha}: ${error.message}` + ); + } + + if (candidates.length !== 1) { + // Fallback: reconcile directly against the live head SHA. The + // association index can be stale or empty for a very recent + // head (seen on PR #1441), so an empty/ambiguous index result + // must not silently drop the revalidation. Matching on the + // head SHA is the same authoritative identity the write gate + // uses, and `pulls.list` is a read — compatible with this + // job's `pull-requests: read` permission. + const priorCount = candidates.length; + try { + const openPrs = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: "open", + per_page: 100 + }); + candidates = openPrs.filter( + pr => + pr.state === "open" && + pr.head?.sha === statusSha + ); + core.info( + `Associated-index fallback: ${priorCount} index match(es), ${candidates.length} open PR(s) match head ${statusSha}.` + ); + } catch (error) { + core.warning( + `Could not list open PRs for head-${statusSha} fallback: ${error.message}` + ); + } + } + if (candidates.length !== 1) { core.info( `CodeRabbit status ${statusSha} maps to ${candidates.length} open current-head PRs; skipping ambiguous/stale revalidation.` diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index abfeff81e5..b755c0425c 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1256,6 +1256,29 @@ describe("GitHub Actions hardening", () => { return runEnforcePrTarget(script, options); } + /** + * Run the read-only `resolve-pr` job's SHA-to-PR resolver in the same + * harness scope the write gate gets. The resolver is a separate inline + * script from `enforce-target`; compiling it on its own lets a test pin + * what `pull-number` it publishes for a given associated-PR / open-PR + * state. This is the exact surface where the stale-commit-index bug + * (PR #1441) lived, so the head-SHA fallback is asserted here. + */ + async function runResolver(options: Parameters[1]) { + const text = await readText(".github/workflows/enforce-pr-target.yml"); + const workflow = Bun.YAML.parse(text) as { + jobs?: Record }>; + }; + const step = workflow.jobs?.["resolve-pr"]?.steps?.find( + s => s.name === "Resolve trusted gate event to PR", + ); + const resolverScript = step?.with?.script; + if (typeof resolverScript !== "string") { + throw new Error("resolve-pr step has no inline script"); + } + return runEnforcePrTarget(stripComments(resolverScript), options); + } + /** * Run an arbitrary body in the same scope the workflow script gets, and * hand back what it returned. @@ -2890,6 +2913,127 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toContain("issues.listComments"); }); + test("the resolver falls back to the head SHA when the commit-PR index is empty", async () => { + const headSha = "6c42d17f213a632fc2def56053f0cd574b13d459"; + const result = await runResolver({ + pr: { base: { ref: "dev" }, number: 4242, head: { sha: headSha } }, + eventName: "status", + statusSha: headSha, + // GitHub's commit-to-PR index can lag a fresh push (seen on #1441): + // the association endpoint returns no PR for a genuine current head. + associatedPullRequests: [], + openPulls: [ + { number: 4242, state: "open", head: { sha: headSha } }, + { number: 9999, state: "open", head: { sha: "other" } }, + { number: 100, state: "closed", head: { sha: headSha } }, + ], + }); + + expect(result.outputs).toEqual([{ name: "pull-number", value: "4242" }]); + // The fallback must consult the live open-PR list exactly once. + expect(callsTo(result, "repos.listPullRequestsAssociatedWithCommit")).toHaveLength(1); + expect(callsTo(result, "pulls.list")).toHaveLength(1); + expect(result.logs.join(" ")).toContain("Associated-index fallback"); + // A closed PR with the same head must not count as a candidate. + expect(result.logs.join(" ")).toContain("1 open PR(s) match head"); + }); + + test("the resolver skips when the fallback finds no unique open head match", async () => { + const headSha = "6c42d17f213a632fc2def56053f0cd574b13d459"; + const result = await runResolver({ + pr: { base: { ref: "dev" }, number: 4242, head: { sha: headSha } }, + eventName: "status", + statusSha: headSha, + associatedPullRequests: [], + // No open PR carries this head; the stale index is not a match. + openPulls: [ + { number: 9999, state: "open", head: { sha: "other" } }, + ], + }); + + expect(result.outputs).toEqual([]); + // The fallback must actually have run: the resolver consults the live + // open-PR list exactly once. Without this assertion the test would still + // pass if the fallback were removed (the empty association index by + // itself already skips), silently losing coverage of the head-SHA + // reconciliation path. + expect(callsTo(result, "pulls.list")).toHaveLength(1); + expect(callsTo(result, "pulls.list")[0]).toMatchObject({ + owner: "lidge-jun", + repo: "opencodex", + state: "open", + }); + expect(result.logs.join(" ")).toContain("skipping ambiguous/stale revalidation"); + }); + + test("the resolver fails closed when two open PRs share the head SHA", async () => { + const headSha = "6c42d17f213a632fc2def56053f0cd574b13d459"; + const result = await runResolver({ + pr: { base: { ref: "dev" }, number: 4242, head: { sha: headSha } }, + eventName: "status", + statusSha: headSha, + associatedPullRequests: [], + // Two open PRs on the same head: the live lookup is ambiguous, so the + // resolver must fail closed rather than guess which PR to revalidate. + openPulls: [ + { number: 4242, state: "open", head: { sha: headSha } }, + { number: 7777, state: "open", head: { sha: headSha } }, + ], + }); + + expect(result.outputs).toEqual([]); + // The live fallback must actually have run before the ambiguity is + // detected: the resolver consults the open-PR list exactly once and + // weighs both same-head candidates before failing closed. Without this + // assertion the test would still pass if the fallback were removed (the + // empty association index by itself already skips), silently losing + // coverage of the head-SHA reconciliation path. + expect(callsTo(result, "pulls.list")).toHaveLength(1); + expect(callsTo(result, "pulls.list")[0]).toMatchObject({ + owner: "lidge-jun", + repo: "opencodex", + state: "open", + }); + expect(result.logs.join(" ")).toContain("skipping ambiguous/stale revalidation"); + }); + + test("the resolver resolves via the association index when it is already fresh", async () => { + const headSha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + const result = await runResolver({ + pr: { base: { ref: "dev" }, number: 42, head: { sha: headSha } }, + eventName: "status", + statusSha: headSha, + associatedPullRequests: [ + { number: 42, state: "open", head: { sha: headSha } }, + ], + }); + + expect(result.outputs).toEqual([{ name: "pull-number", value: "42" }]); + // A unique index hit must not need the open-PR fallback. + expect(callsTo(result, "pulls.list")).toEqual([]); + }); + + test("the resolver fails closed when both resolution paths error", async () => { + const headSha = "6c42d17f213a632fc2def56053f0cd574b13d459"; + const result = await runResolver({ + pr: { base: { ref: "dev" }, number: 4242, head: { sha: headSha } }, + eventName: "status", + statusSha: headSha, + associatedPullRequests: [], + failOn: [ + "repos.listPullRequestsAssociatedWithCommit", + "pulls.list", + ], + }); + + expect(result.outputs).toEqual([]); + expect(result.logs.join(" ")).toContain("skipping ambiguous/stale revalidation"); + expect(result.warnings.join(" ")).toContain( + "Could not list PRs associated with commit", + ); + expect(result.warnings.join(" ")).toContain("Could not list open PRs"); + }); + test("a non-maintainer issue_comment does not re-run the gate", async () => { // The `issue_comment` trigger must only re-run for maintainer comments // (OWNER / COLLABORATOR / MEMBER). A random comment from a contributor diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index e1e69d68f2..b6d90da52f 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -22,6 +22,13 @@ export type RecordedCall = { method: string; args: unknown }; export type HarnessResult = { calls: RecordedCall[]; + /** + * Values the script wrote through `core.setOutput`, in write order. The + * write-capable gate consumes `RESOLVED_PULL_NUMBER` from the resolver, and + * the client sets it as a step output; exposing it lets a test assert the + * SHA-to-PR resolution directly instead of inferring it from later calls. + */ + outputs: Array<{ name: string; value: unknown }>; /** * Paths the script read through its `node:fs` stub. Kept separate from * `calls` so exact method-sequence assertions stay stable while the fs @@ -1197,6 +1204,7 @@ export async function runEnforcePrTarget( return { calls, + outputs, fsReads, logs, warnings,