Skip to content

Commit e26b44a

Browse files
committed
ci: harden merge-gate (fail-closed, rerun-safe, robust PR resolution)
Address review of the ci/merge-gate convergence: - Extract shared logic to .github/workflows/scripts/merge-gate.js; both entry points use it (no drift). - merge-gate.yml now also listens to 'External PR credential-free validation' and triggers on in_progress + completed, forcing a re-running workflow's own input to pending so an earlier success can't linger during a same-SHA rerun. - Fork side persists the approved Cypress result as the ci/fork-cypress commit status and an early mark-fork-gate-pending job brackets the rerun window; the gate is recomputed on both fork inputs, closing the stale-success / stuck-pending holes. - Both entry points validate open + base==release + head.sha==written-SHA before writing (no stale writes, no reuse after base retarget). - Robust PR resolution: workflow_run.pull_requests -> head owner:branch -> commit association, unique exact-SHA match only, no arbitrary fallback; null head.repo treated as fork. - ci/fork-cypress requires exactly one Cypress result job to have run. Fail-closed throughout: skipped/neutral/missing/in-flight => pending. Linear: https://linear.app/appsmith/issue/APP-15921
1 parent 076731e commit e26b44a

3 files changed

Lines changed: 350 additions & 167 deletions

File tree

.github/workflows/build-client-server.yml

Lines changed: 72 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@ concurrency:
1010

1111
permissions:
1212
actions: read
13+
checks: read
1314
contents: read
1415
issues: write
1516
packages: write
1617
pull-requests: write
18+
statuses: write
1719

1820
jobs:
1921
file-check:
@@ -766,11 +768,52 @@ jobs:
766768
if: needs.ci-test-limited-existing-docker-image.result != 'success'
767769
run: exit 1
768770

769-
# Publish the single required merge gate (`ci/merge-gate`) for FORK PRs on the
770-
# maintainer-approved head SHA. This dispatch is triggered by `/approve-ci`, so
771-
# it runs trusted and may hold statuses: write. Internal PRs get ci/merge-gate
772-
# from merge-gate.yml instead; the fork-only guard below prevents both paths
773-
# from writing the same context on one PR. See merge-gate.yml for the rationale.
771+
# Fork PRs: mark the single required gate `pending` as soon as an approved-CI
772+
# run starts, so an earlier `success` cannot linger while the same commit is
773+
# re-tested. Internal PRs are owned by merge-gate.yml (guarded out below). No
774+
# checkout here (no fork code) — the PR number/head come from the trusted
775+
# dispatch payload.
776+
mark-fork-gate-pending:
777+
name: mark-fork-gate-pending
778+
if: github.event.action == 'approve-ci-command'
779+
runs-on: ubuntu-latest
780+
steps:
781+
- name: Set ci/merge-gate pending for the fork PR
782+
uses: actions/github-script@v7
783+
env:
784+
HEAD_SHA: ${{ github.event.client_payload.pull_request.head.sha }}
785+
PR_NUMBER: ${{ github.event.client_payload.pull_request.number }}
786+
with:
787+
script: |
788+
const { owner, repo } = context.repo;
789+
const { HEAD_SHA, PR_NUMBER } = process.env;
790+
const pr = (
791+
await github.rest.pulls.get({ owner, repo, pull_number: Number(PR_NUMBER) })
792+
).data;
793+
const isFork = !pr.head.repo || pr.head.repo.full_name !== `${owner}/${repo}`;
794+
if (!isFork) {
795+
core.info(`PR #${PR_NUMBER} is internal; merge-gate.yml owns it. Skipping.`);
796+
return;
797+
}
798+
if (pr.state !== "open" || pr.base.ref !== "release" || pr.head.sha !== HEAD_SHA) {
799+
core.info(`PR #${PR_NUMBER} not gatable / head moved; skipping.`);
800+
return;
801+
}
802+
await github.rest.repos.createCommitStatus({
803+
owner,
804+
repo,
805+
sha: HEAD_SHA,
806+
state: "pending",
807+
context: "ci/merge-gate",
808+
description: "Approved CI running…",
809+
target_url: `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}`,
810+
});
811+
812+
# Fork PRs: persist the approved Cypress result (`ci/fork-cypress`) on the
813+
# approved head SHA, then write the single required `ci/merge-gate`. Uses the
814+
# shared gate script, checked out from the trusted base repo (a
815+
# repository_dispatch run defaults to the default branch — never fork code).
816+
# Internal PRs are owned by merge-gate.yml (guarded out below).
774817
set-fork-merge-gate:
775818
name: set-fork-merge-gate
776819
needs:
@@ -782,92 +825,55 @@ jobs:
782825
]
783826
if: always() && github.event.action == 'approve-ci-command' && needs.file-check.outputs.pr != '0'
784827
runs-on: ubuntu-latest
785-
permissions:
786-
contents: read
787-
checks: read
788-
pull-requests: read
789-
statuses: write
790828
steps:
791-
- name: Set ci/merge-gate for the fork PR
829+
- name: Checkout base repo (trusted — for the shared gate script only)
830+
uses: actions/checkout@v4
831+
832+
- name: Write ci/merge-gate for the fork PR
792833
uses: actions/github-script@v7
793834
env:
835+
NODE_PATH: ${{ github.workspace }}/.github/workflows/scripts
794836
HEAD_SHA: ${{ github.event.client_payload.pull_request.head.sha }}
795837
PR_NUMBER: ${{ needs.file-check.outputs.pr }}
796838
CYPRESS_LIMITED: ${{ needs.ci-test-limited-result.result }}
797839
CYPRESS_FULL: ${{ needs.ci-test-full-result.result }}
798840
CYPRESS_EXISTING: ${{ needs.ci-test-limited-result-existing.result }}
799841
with:
800842
script: |
801-
const GATE = "ci/merge-gate";
843+
const gate = require("merge-gate.js");
802844
const { owner, repo } = context.repo;
803845
const { HEAD_SHA, PR_NUMBER, CYPRESS_LIMITED, CYPRESS_FULL, CYPRESS_EXISTING } = process.env;
846+
const runUrl = `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}`;
804847
805-
// Only fork PRs are gated here; internal PRs are owned by merge-gate.yml.
806848
const pr = (
807849
await github.rest.pulls.get({ owner, repo, pull_number: Number(PR_NUMBER) })
808850
).data;
809-
if (pr.head.repo.full_name === `${owner}/${repo}`) {
851+
852+
// Only fork PRs are gated here; internal PRs are owned by merge-gate.yml.
853+
if (!gate.isFork(pr, owner, repo)) {
810854
core.info(`PR #${PR_NUMBER} is internal; merge-gate.yml owns its status. Skipping.`);
811855
return;
812856
}
813857
814-
// Exactly one of the three Cypress result jobs runs per invocation;
815-
// the others report 'skipped'. Cypress is green only if the one that
816-
// actually ran succeeded.
817-
const cypressRuns = [CYPRESS_LIMITED, CYPRESS_FULL, CYPRESS_EXISTING].filter(
818-
(r) => r !== "skipped",
819-
);
820-
const cypress =
821-
cypressRuns.length === 0
822-
? "pending"
823-
: cypressRuns.every((r) => r === "success")
824-
? "success"
825-
: "failure";
826-
827-
// Credential-free validation (external-ci-result) runs in a separate
828-
// workflow on the same head SHA; read its latest conclusion.
829-
const checks = await github.paginate(github.rest.checks.listForRef, {
858+
// Persist the approved Cypress result on the approved head SHA, then
859+
// compute the gate from external-ci-result + that result. Passing the
860+
// just-computed state avoids a read-after-write race on the status.
861+
const forkCypressState = await gate.publishForkCypress({
862+
github,
830863
owner,
831864
repo,
832-
ref: HEAD_SHA,
833-
per_page: 100,
865+
sha: HEAD_SHA,
866+
results: [CYPRESS_LIMITED, CYPRESS_FULL, CYPRESS_EXISTING],
867+
runUrl,
834868
});
835-
const ext = checks
836-
.filter((c) => c.name === "external-ci-result")
837-
.sort((a, b) => new Date(b.started_at ?? 0) - new Date(a.started_at ?? 0))[0];
838-
let credFree = "pending";
839-
if (ext && ext.status === "completed") {
840-
credFree =
841-
ext.conclusion === "success"
842-
? "success"
843-
: ext.conclusion === "skipped" || ext.conclusion === "neutral"
844-
? "pending"
845-
: "failure";
846-
}
847869
848-
const parts = { "external-ci-result": credFree, "approved Cypress": cypress };
849-
const failed = Object.entries(parts).filter(([, v]) => v === "failure").map(([k]) => k);
850-
const pending = Object.entries(parts).filter(([, v]) => v === "pending").map(([k]) => k);
851-
852-
let state, description;
853-
if (failed.length) {
854-
state = "failure";
855-
description = `Failed: ${failed.join(", ")}`.slice(0, 140);
856-
} else if (pending.length) {
857-
state = "pending";
858-
description = `Waiting: ${pending.join(", ")}`.slice(0, 140);
859-
} else {
860-
state = "success";
861-
description = "Credential-free + approved Cypress passed";
862-
}
863-
864-
core.info(`Setting ${GATE}=${state} on ${HEAD_SHA} for fork PR #${PR_NUMBER} (${description})`);
865-
await github.rest.repos.createCommitStatus({
870+
await gate.evaluate({
871+
github,
872+
core,
866873
owner,
867874
repo,
868875
sha: HEAD_SHA,
869-
state,
870-
context: GATE,
871-
description,
872-
target_url: `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}`,
876+
pr,
877+
forkCypressState,
878+
runUrl,
873879
});

.github/workflows/merge-gate.yml

Lines changed: 37 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -2,31 +2,33 @@ name: Merge gate
22

33
# Single required status context (`ci/merge-gate`) that both internal and fork
44
# PRs to `release` can satisfy. Branch protection on `release` requires
5-
# `ci/merge-gate`; the per-flow checks it aggregates (qc-result,
6-
# `perform-test / ci-test-result`, external-ci-result, approved Cypress) are no
7-
# longer individually required — each PR type only ever produces one of the two
8-
# sets, and a raw required context that is skipped/absent on the other type
9-
# stays "Expected" forever and blocks the merge (this is exactly the bug that
10-
# left fork PRs unmergeable after the external-contributor flow shipped).
5+
# `ci/merge-gate`; the per-flow checks it aggregates (`qc-result`,
6+
# `perform-test / ci-test-result`, `external-ci-result`, approved Cypress) are
7+
# no longer individually required — each PR type only ever produces one of the
8+
# two sets, and a raw required context that is skipped/absent on the other type
9+
# stays "Expected" forever and blocks the merge (the bug this fixes).
1110
#
12-
# Ownership of the write is split by which flow knows the PR head SHA:
13-
# - Internal PRs: THIS workflow, triggered by the internal PR workflows'
14-
# completion (their head_sha is the PR head).
15-
# - Fork PRs: build-client-server.yml sets `ci/merge-gate` on `/approve-ci`,
16-
# where the approved head SHA is known. This workflow skips forks.
17-
# A PR is exactly one of the two, so the context is written by exactly one path
18-
# and the two setters never race.
11+
# This workflow recomputes the gate for the PR associated with a feeding
12+
# workflow's run, on both rerun start (`in_progress`) and `completed`:
13+
# - `in_progress` forces the feeding workflow's own input to `pending`, so an
14+
# earlier success cannot linger while that workflow re-runs on the same SHA.
15+
# - `completed` reads the head SHA's checks/statuses and writes success /
16+
# failure / pending (fail-closed: skipped/neutral/absent => pending).
17+
# It owns internal PRs outright and recomputes the fork credential-free side;
18+
# the fork Cypress side is written by build-client-server.yml on `/approve-ci`.
1919
#
20-
# Security: this workflow never checks out or runs PR code — it only calls the
21-
# GitHub API — and runs from the default branch in the base-repo context, so
22-
# holding `statuses: write` is safe even when the associated PR is from a fork.
20+
# Security: it never checks out or runs PR code — it only checks out the base
21+
# repo for the shared script and calls the GitHub API — and runs from the
22+
# default branch in the base-repo context, so holding `statuses: write` is safe
23+
# even when the associated PR is from a fork.
2324

2425
on:
2526
workflow_run:
2627
workflows:
2728
- "Quality checks"
2829
- "PR Automation test suite"
29-
types: [completed]
30+
- "External PR credential-free validation"
31+
types: [in_progress, completed]
3032

3133
permissions:
3234
contents: read
@@ -36,106 +38,40 @@ permissions:
3638

3739
concurrency:
3840
# Serialize evaluations per head SHA so concurrent triggers don't race on the
39-
# commit-status write; queue rather than cancel so the latest state wins.
41+
# status write; queue rather than cancel so the latest state always wins.
4042
group: merge-gate-${{ github.event.workflow_run.head_sha }}
4143
cancel-in-progress: false
4244

4345
jobs:
4446
evaluate:
4547
runs-on: ubuntu-latest
4648
steps:
47-
- name: Evaluate internal merge gate
49+
- name: Checkout base repo (trusted — for the shared gate script only)
50+
uses: actions/checkout@v4
51+
52+
- name: Evaluate merge gate
4853
uses: actions/github-script@v7
4954
env:
50-
HEAD_SHA: ${{ github.event.workflow_run.head_sha }}
55+
NODE_PATH: ${{ github.workspace }}/.github/workflows/scripts
5156
with:
5257
script: |
53-
const GATE = "ci/merge-gate";
58+
const gate = require("merge-gate.js");
5459
const { owner, repo } = context.repo;
55-
const sha = process.env.HEAD_SHA;
56-
if (!sha) {
57-
core.info("No head SHA on the triggering run; skipping.");
58-
return;
59-
}
60+
const run = context.payload.workflow_run;
61+
const sha = run.head_sha;
62+
const runUrl = `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}`;
6063
61-
// Resolve the open PR whose head is this SHA.
62-
const assoc = await github.paginate(
63-
github.rest.repos.listPullRequestsAssociatedWithCommit,
64-
{ owner, repo, commit_sha: sha, per_page: 100 },
65-
);
66-
const pr =
67-
assoc.find((p) => p.state === "open" && p.head.sha === sha) ??
68-
assoc.find((p) => p.state === "open");
64+
const pr = await gate.resolvePr({ github, owner, repo, sha, workflowRun: run });
6965
if (!pr) {
70-
core.info(`No open PR found for ${sha}; skipping.`);
71-
return;
72-
}
73-
74-
// Only gate PRs targeting release — the branch whose protection
75-
// requires ci/merge-gate.
76-
if (pr.base.ref !== "release") {
77-
core.info(`PR #${pr.number} targets ${pr.base.ref}, not release; skipping.`);
66+
core.info(`No unique open PR found for ${sha}; skipping.`);
7867
return;
7968
}
8069
81-
// Fork PRs get ci/merge-gate from build-client-server.yml on
82-
// /approve-ci; this workflow owns internal PRs only.
83-
if (pr.head.repo.full_name !== `${owner}/${repo}`) {
84-
core.info(`PR #${pr.number} is a fork; build-client-server.yml owns its status. Skipping.`);
85-
return;
86-
}
87-
88-
// Read the latest run of each required internal check on the head SHA.
89-
const checks = await github.paginate(github.rest.checks.listForRef, {
90-
owner,
91-
repo,
92-
ref: sha,
93-
per_page: 100,
94-
});
95-
const latest = new Map();
96-
for (const c of checks) {
97-
const prev = latest.get(c.name);
98-
if (!prev || new Date(c.started_at ?? 0) >= new Date(prev.started_at ?? 0)) {
99-
latest.set(c.name, c);
100-
}
101-
}
102-
// A skipped/neutral required check means "not satisfied yet" (e.g. an
103-
// internal PR without ok-to-test never ran Cypress) — treat as
104-
// pending so the gate blocks, matching pre-existing behavior.
105-
const outcome = (name) => {
106-
const c = latest.get(name);
107-
if (!c || c.status !== "completed") return "pending";
108-
if (c.conclusion === "success") return "success";
109-
if (c.conclusion === "skipped" || c.conclusion === "neutral") return "pending";
110-
return "failure";
111-
};
112-
113-
const parts = {
114-
"qc-result": outcome("qc-result"),
115-
"perform-test / ci-test-result": outcome("perform-test / ci-test-result"),
116-
};
117-
const failed = Object.entries(parts).filter(([, v]) => v === "failure").map(([k]) => k);
118-
const pending = Object.entries(parts).filter(([, v]) => v === "pending").map(([k]) => k);
119-
120-
let state, description;
121-
if (failed.length) {
122-
state = "failure";
123-
description = `Failed: ${failed.join(", ")}`.slice(0, 140);
124-
} else if (pending.length) {
125-
state = "pending";
126-
description = `Waiting: ${pending.join(", ")}`.slice(0, 140);
127-
} else {
128-
state = "success";
129-
description = "Quality checks + Cypress passed";
70+
// A rerun that just started means its result is in flight — force
71+
// that input to `pending` so an existing success can't linger.
72+
const pendingChecks = new Set();
73+
if (run.status === "in_progress" && gate.WORKFLOW_TO_CHECK[run.name]) {
74+
pendingChecks.add(gate.WORKFLOW_TO_CHECK[run.name]);
13075
}
13176
132-
core.info(`Setting ${GATE}=${state} on ${sha} for internal PR #${pr.number} (${description})`);
133-
await github.rest.repos.createCommitStatus({
134-
owner,
135-
repo,
136-
sha,
137-
state,
138-
context: GATE,
139-
description,
140-
target_url: `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}`,
141-
});
77+
await gate.evaluate({ github, core, owner, repo, sha, pr, pendingChecks, runUrl });

0 commit comments

Comments
 (0)