Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions .github/workflows/enforce-pr-target.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
Expand Down
144 changes: 144 additions & 0 deletions tests/ci-workflows.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof runEnforcePrTarget>[1]) {
const text = await readText(".github/workflows/enforce-pr-target.yml");
const workflow = Bun.YAML.parse(text) as {
jobs?: Record<string, { steps?: Array<{ name?: string; with?: { script?: string } }> }>;
};
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.
Expand Down Expand Up @@ -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");
});
Comment thread
Wibias marked this conversation as resolved.

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");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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");
});

Comment thread
Wibias marked this conversation as resolved.
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
Expand Down
8 changes: 8 additions & 0 deletions tests/helpers/enforce-pr-target-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1197,6 +1204,7 @@ export async function runEnforcePrTarget(

return {
calls,
outputs,
fsReads,
logs,
warnings,
Expand Down
Loading