From e4c29f6c7c7e6dfc9ddd2ce6ef35a70a6576458b Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 11 Aug 2026 04:32:39 +0200 Subject: [PATCH 1/4] fix(ci): reconcile PR from head SHA when the commit-PR index lags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolve-pr job mapped a trusted CodeRabbit status SHA back to a PR via the commit-to-PR association index. That index can lag a fresh head push (seen on PR #1441), returning zero matches for a genuine current head, so revalidation was silently skipped and new unresolved findings never re-drafted the PR. When the index yields no unique match, fall back to reconciling directly against the live open-PR list by head.sha — the same authoritative identity the write gate uses. Resolution still requires exactly one open match, so ambiguity remains fail-closed. The fallback is a read and stays within the resolver's pull-requests:read permission. Add behavioural harness tests for the resolver: empty-index fallback, no-match skip, fresh-index direct resolve, and fail-closed on error. --- .github/workflows/enforce-pr-target.yml | 59 +++++++++++-- tests/ci-workflows.test.ts | 96 ++++++++++++++++++++++ tests/helpers/enforce-pr-target-harness.ts | 8 ++ 3 files changed, 154 insertions(+), 9 deletions(-) diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 60cf184e2..5cf5f8462 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 abfeff81e..1b255ec79 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,79 @@ 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([]); + 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: ["pulls.list"], + }); + + expect(result.outputs).toEqual([]); + expect(result.logs.join(" ")).toContain("skipping ambiguous/stale revalidation"); + 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 e1e69d68f..b6d90da52 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, From 793a54585852a3240b9d402ca9b32146cb5be359 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 11 Aug 2026 04:38:54 +0200 Subject: [PATCH 2/4] test(ci): cover ambiguous fallback and fail-closed index error Address CodeRabbit feedback on the resolver tests: - Add a regression test for the ambiguous case: two open PRs sharing the same head SHA must fail closed (no pull-number output), instead of only covering zero and one matches. - Correct the fail-closed test so both resolution calls reject. It previously only failed pulls.list, so the association-index error path was never exercised; assert both warning messages now. --- tests/ci-workflows.test.ts | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 1b255ec79..f92376884 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -2955,6 +2955,25 @@ describe("GitHub Actions hardening", () => { 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([]); + 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({ @@ -2978,11 +2997,17 @@ describe("GitHub Actions hardening", () => { eventName: "status", statusSha: headSha, associatedPullRequests: [], - failOn: ["pulls.list"], + 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"); }); From 1b1d7d9eea2585012dfd3ad616b7250115e456d0 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 11 Aug 2026 04:51:33 +0200 Subject: [PATCH 3/4] test(ci): assert the fallback lookup runs in the ambiguous-head case Address CodeRabbit feedback on the ambiguous fallback test: assert that the resolver actually consults the live open-PR list (one pulls.list call with the open-state filter) so the test cannot pass without the head-SHA reconciliation path. The empty-output and ambiguous/stale log assertions are unchanged. --- tests/ci-workflows.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index f92376884..bbb5da3bd 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -2952,6 +2952,18 @@ describe("GitHub Actions hardening", () => { }); expect(result.outputs).toEqual([]); + // The fallback must actually have run: the resolver consults the live + // open-PR list exactly once, and both same-head candidates are weighed + // before failing closed as ambiguous. 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"); }); From 7f4de367ea7151c26aa8663c55366251df2e2594 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 11 Aug 2026 05:24:00 +0200 Subject: [PATCH 4/4] test(ci): prove the ambiguous-head fallback runs pulls.list The ambiguous-head regression (two open PRs sharing the same head.sha) only asserted empty output and the skip log. Without a pulls.list assertion it would pass even if the head-SHA fallback were removed, because the empty association index by itself already causes the skip. Assert the live fallback ran: exactly one pulls.list call with the open-state filter, alongside the retained empty-output and skip-log assertions. The neighbouring zero-match test keeps its own independent fallback assertion. --- tests/ci-workflows.test.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index bbb5da3bd..b755c0425 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -2953,11 +2953,10 @@ describe("GitHub Actions hardening", () => { expect(result.outputs).toEqual([]); // The fallback must actually have run: the resolver consults the live - // open-PR list exactly once, and both same-head candidates are weighed - // before failing closed as ambiguous. 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. + // 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", @@ -2983,6 +2982,18 @@ describe("GitHub Actions hardening", () => { }); 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"); });