vrr/openclaw
2
0
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-10-03 09:39:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 30

fix(ci): keep security review pending while CI runs (#153005)

Browse source 
Distinguish missing or active CI from failed CI. Keep the required combined status pending without failing the waiting review job, and fail visibly on approval, CI, metadata, or evaluation errors. Preserve workflow, head, attempt, and authority checks.
This commit is contained in:
Peter Steinberger 2026-09-19 12:24:53 -07:00 • committed by GitHub
parent ba113dc711
commit 3a722d4c62
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 203 additions and 109 deletions
Download patch file Download diff file Expand all files Collapse all files

11
docs/ci/pipeline.md
View file

@ -156,13 +156,14 @@ It publishes a commit status named `openclaw/ci-gate` that requires both the
applicable approvals and a successful native CI gate from the latest CI run for
the current PR head. The existing CI job retains its check with the same name.
GitHub requires both the check and the commit status when both share a required
context. Missing approval or incomplete CI fails the review status; CI completion
context. Missing approval, failed CI, or evaluation errors fail the review status.
Missing or running CI leaves it pending and keeps merging blocked. CI completion
automatically evaluates it again. Approval comments do not rerun the test suite.
The Security Review Actions job succeeds when evaluation completes, including
when the required commit status blocks merging for missing approval or CI that
has not passed. This prevents an earlier evaluation from leaving a stale failed
job after automatic reevaluation clears the status. Evaluation errors still fail
the job and keep the required status closed. No pending statuses are published.
when the required commit status blocks merging for missing approval or failed CI.
This prevents an earlier evaluation from leaving a stale failed job after automatic
reevaluation clears the status. Evaluation errors still fail the job and keep the
required status closed.
The **Security Sensitive Guard** publishes `openclaw/security-sensitive-review`.
Its inventory in `.github/security-review-policy.yml` covers Gateway

2
scripts/github/security-review-event.mjs
View file

@ -191,7 +191,7 @@ async function main() {
context: "openclaw/ci-gate",
runUrl: `https://github.com/${owner}/${repo}/actions/runs/${GITHUB_RUN_ID}`,
},
"failure",
"pending",
"Review scheduled; CI and security review have not completed",
);
}

216
scripts/github/security-review.mjs
View file

@ -7,7 +7,30 @@ import { publishGuardStatus } from "./guard-shared.mjs";
import { securityReviewRollout } from "./security-review-rollout.mjs";
import { reviewSecuritySensitiveChanges } from "./security-sensitive-guard.mjs";
async function ciPassed(review) {
function ciRunState(run) {
if (
!Number.isSafeInteger(run.id) ||
run.id <= 0 ||
!Number.isSafeInteger(run.run_attempt) ||
run.run_attempt <= 0
) {
throw new Error("CI returned an invalid run identity.");
}
switch (run.status) {
case "completed":
return "completed";
case "queued":
case "in_progress":
case "waiting":
case "requested":
case "pending":
return "pending";
default:
throw new Error("CI returned an invalid run status.");
}
}
async function ciState(review) {
const { api, owner, repo, pullRequest } = review;
const root = `/repos/${owner}/${repo}/actions`;
const response = await api.request(
@ -18,23 +41,23 @@ async function ciPassed(review) {
}
// A same-named status alone must never stand in for absent or unfinished CI.
// Use the latest real CI run, not arbitrary checks posted under its job name.
const run = response.workflow_runs
.filter(
(candidate) =>
(candidate.event === "pull_request" ||
(candidate.event === "workflow_dispatch" &&
candidate.display_title === `CI release gate ${pullRequest.head.sha}`)) &&
candidate.path === ".github/workflows/ci.yml" &&
candidate.head_sha === pullRequest.head.sha &&
candidate.head_branch === pullRequest.head.ref &&
candidate.repository?.id === pullRequest.base.repo.id,
)
.toSorted((left, right) => right.id - left.id)[0];
if (!run || run.status !== "completed") {
return false;
const candidates = response.workflow_runs.filter(
(candidate) =>
(candidate.event === "pull_request" ||
(candidate.event === "workflow_dispatch" &&
candidate.display_title === `CI release gate ${pullRequest.head.sha}`)) &&
candidate.path === ".github/workflows/ci.yml" &&
candidate.head_sha === pullRequest.head.sha &&
candidate.head_branch === pullRequest.head.ref &&
candidate.repository?.id === pullRequest.base.repo.id,
);
// Invalid IDs make latest-run ordering untrustworthy, even if an older run passed.
for (const candidate of candidates) {
ciRunState(candidate);
}
if (!Number.isSafeInteger(run.id) || !Number.isSafeInteger(run.run_attempt)) {
throw new Error("CI returned an invalid run identity.");
const run = candidates.toSorted((left, right) => right.id - left.id)[0];
if (!run || run.status !== "completed") {
return "pending";
}
const jobs = [];
for (let page = 1; ; page += 1) {
@ -52,16 +75,21 @@ async function ciPassed(review) {
throw new Error("CI did not return the complete job list.");
}
}
const gates = jobs.filter((job) => job.name === "openclaw/ci-gate");
if (gates.length !== 1 || gates[0].status !== "completed" || gates[0].conclusion !== "success") {
return false;
}
const current = await api.request(`${root}/runs/${run.id}`);
return (
current.head_sha === run.head_sha &&
current.run_attempt === run.run_attempt &&
current.status === "completed"
);
const currentState = ciRunState(current);
if (current.id !== run.id || current.head_sha !== run.head_sha) {
throw new Error("The CI run identity changed during security review.");
}
if (currentState === "pending") {
return "pending";
}
if (current.run_attempt !== run.run_attempt) {
throw new Error("The completed CI attempt changed during security review; rerun this review.");
}
const gates = jobs.filter((job) => job.name === "openclaw/ci-gate");
return gates.length === 1 && gates[0].status === "completed" && gates[0].conclusion === "success"
? "success"
: "failure";
}
async function main() {
@ -75,75 +103,97 @@ async function main() {
}
review.context = "openclaw/ci-gate";
review.guards = [];
await publishGuardStatus(review, "failure", "CI and security review have not completed");
review.rollout = await securityReviewRollout(review);
if (review.rollout.mode === "enforced") {
if (mode !== "enforce") {
await reviewDependencyChanges(review, mode);
return;
}
// Each guard must publish its own notice even when its sibling rejects a PR.
const errors = [];
let allowed = true;
for (const guard of [reviewDependencyChanges, reviewSecuritySensitiveChanges]) {
try {
if (!(await guard(review))) {
allowed = false;
await publishGuardStatus(review, "pending", "CI and security review have not completed");
try {
review.rollout = await securityReviewRollout(review);
if (review.rollout.mode === "enforced") {
if (mode !== "enforce") {
await reviewDependencyChanges(review, mode);
return;
}
// Each guard must publish its own notice even when its sibling rejects a PR.
const errors = [];
let allowed = true;
for (const guard of [reviewDependencyChanges, reviewSecuritySensitiveChanges]) {
try {
if (!(await guard(review))) {
allowed = false;
}
} catch (error) {
errors.push(error instanceof Error ? error.message : String(error));
}
} catch (error) {
errors.push(error instanceof Error ? error.message : String(error));
}
if (errors.length > 0) {
throw new Error(errors.join("\n"));
}
if (!allowed) {
await publishGuardStatus(
review,
"failure",
"A maintainer must approve the current PR revision",
);
console.log("Security review is awaiting maintainer approval.");
return;
}
} else {
const summary = `Security review: ${review.rollout.mode}; standalone review statuses are not published.`;
if (process.env.GITHUB_STEP_SUMMARY) {
await appendFile(process.env.GITHUB_STEP_SUMMARY, `${summary}\n`);
} else {
console.log(summary);
}
if (mode !== "enforce") {
return;
}
}
if (errors.length > 0) {
throw new Error(errors.join("\n"));
}
// Merge decisions live in commit statuses. Expected blocks must not leave
// failed Actions jobs behind after an automatic reevaluation succeeds.
if (!allowed) {
console.log("Security review is awaiting maintainer approval.");
const ci = await ciState(review);
if (ci === "pending") {
await assertGuardUnchanged(review);
await publishGuardStatus(review, "pending", "Waiting for CI; review updates automatically");
console.log("Waiting for CI. CI completion will automatically reevaluate security review.");
return;
}
} else {
const summary = `Security review: ${review.rollout.mode}; standalone review statuses are not published.`;
if (process.env.GITHUB_STEP_SUMMARY) {
await appendFile(process.env.GITHUB_STEP_SUMMARY, `${summary}\n`);
} else {
console.log(summary);
}
if (mode !== "enforce") {
if (ci === "failure") {
await publishGuardStatus(
review,
"failure",
"CI must complete successfully; review updates automatically",
);
console.log("The current CI gate did not pass. Review the CI workflow failures.");
return;
}
}
if (!(await ciPassed(review))) {
// Authority can be removed while CI metadata and the other guard are read.
// Revalidate both decisions immediately before publishing their combined result.
for (const guard of review.guards) {
if (guard.requiresApproval && !(await findMaintainerApproval(guard))) {
await publishGuardStatus(
guard,
"failure",
"A maintainer must approve the current PR revision",
);
await publishGuardStatus(
review,
"failure",
"Maintainer approval changed during security review",
);
console.log("Maintainer approval changed during security review.");
return;
}
}
await assertGuardUnchanged(review);
await publishGuardStatus(
review,
"success",
"CI and applicable security review requirements passed",
);
} catch (error) {
await publishGuardStatus(
review,
"failure",
"CI must complete successfully; review updates automatically",
"CI or security review failed; see workflow details",
);
console.log(
"The current CI gate has not passed. CI completion will automatically reevaluate security review.",
);
return;
throw error;
}
// Authority can be removed while CI metadata and the other guard are read.
// Revalidate both decisions immediately before publishing their combined result.
for (const guard of review.guards) {
if (guard.requiresApproval && !(await findMaintainerApproval(guard))) {
await publishGuardStatus(
guard,
"failure",
"A maintainer must approve the current PR revision",
);
console.log("Maintainer approval changed during security review.");
return;
}
}
await assertGuardUnchanged(review);
await publishGuardStatus(
review,
"success",
"CI and applicable security review requirements passed",
);
}
if (import.meta.url === `file://${process.argv[1]}`) {

14
test/scripts/security-review-event.test.ts
View file

@ -32,7 +32,7 @@ function recordedPullRequest(number: number) {
return {
context: "openclaw/ci-gate",
description: `PR #${number}: Checking security review`,
state: "failure",
state: "pending",
creator: { login: "github-actions[bot]", type: "Bot" },
};
}
@ -163,7 +163,7 @@ describe("automatic security review event resolution", () => {
hadOutput: false,
body: {
context: "openclaw/ci-gate",
state: "failure",
state: "pending",
description: "PR #42: Review scheduled; CI and security review have not completed",
target_url: "https://github.com/openclaw/openclaw/actions/runs/789",
},
@ -172,7 +172,7 @@ describe("automatic security review event resolution", () => {
},
);
it("does not schedule a review job when its initial failure cannot be recorded", () => {
it("does not schedule a review job when its initial pending status cannot be recorded", () => {
const result = evaluate({
eventName: "pull_request_target",
event: { action: "opened", pull_request: { number: 42 } },
@ -246,13 +246,13 @@ describe("automatic security review event resolution", () => {
{
path: `${prefix}/statuses/${nextHead}`,
description: "PR #42: Review scheduled; CI and security review have not completed",
state: "failure",
state: "pending",
hadOutput: false,
},
{
path: `${prefix}/statuses/${head}`,
description: "PR #43: Review scheduled; CI and security review have not completed",
state: "failure",
state: "pending",
hadOutput: false,
},
]);
@ -325,7 +325,7 @@ describe("automatic security review event resolution", () => {
).toMatchObject({
status: 0,
matrix: { include: [{ pr: 42, head }] },
published: [{ body: { context: "openclaw/ci-gate", state: "failure" } }],
published: [{ body: { context: "openclaw/ci-gate", state: "pending" } }],
});
});
@ -380,7 +380,7 @@ describe("automatic security review event resolution", () => {
]);
expect(result.requests.slice(0, -1).every((request) => request.method === "GET")).toBe(true);
expect(result.published).toHaveLength(1);
expect(result.published[0]?.body?.state).toBe("failure");
expect(result.published[0]?.body?.state).toBe("pending");
},
);

69
test/scripts/security-review-script.test.ts
View file

@ -129,12 +129,12 @@ describe("combined security review entry point", () => {
it("requires successful CI and both guard decisions on the actual PR head", () => {
const result = evaluate();
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
expect(result.reviews.filter((entry) => entry.body?.state === "success")).toHaveLength(2);
expect(
result.requests
.filter((entry) => entry.path.includes("/statuses/"))
.every((entry) => entry.path.endsWith(head) && entry.body?.state !== "pending"),
.every((entry) => entry.path.endsWith(head)),
).toBe(true);
});
@ -187,7 +187,7 @@ describe("combined security review entry point", () => {
"GET /repos/openclaw/openclaw/pulls/8": other,
});
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
});
it.each(["openclaw/ci-gate", "openclaw/dependency-review", "openclaw/security-sensitive-review"])(
@ -230,7 +230,7 @@ describe("combined security review entry point", () => {
[`GET ${actions}/runs/10`]: fallback,
});
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
});
it("does not accept a manual historical-target run as current PR proof", () => {
@ -261,7 +261,7 @@ describe("combined security review entry point", () => {
},
});
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
expect(result.requests.filter((entry) => `GET ${entry.path}` === jobsPath)).toHaveLength(2);
});
@ -289,9 +289,43 @@ describe("combined security review entry point", () => {
])("does not turn $name into a passing combined gate", ({ response }) => {
const result = evaluate({ [runsPath]: response });
expect(result.status, result.stderr).toBe(0);
expect(result.combined).not.toContain("success");
expect(result.combined).toEqual(["pending", "pending"]);
});
it.each([
{ field: "status", value: undefined },
{ field: "status", value: "unknown" },
{ field: "run_attempt", value: undefined },
{ field: "run_attempt", value: 0 },
{ field: "id", value: undefined },
{ field: "id", value: 0 },
])("fails malformed CI metadata instead of waiting: $field=$value", ({ field, value }) => {
const malformed = { ...run, [field]: value };
for (const routes of [
{ [runsPath]: { total_count: 1, workflow_runs: [malformed] } },
{ [`GET ${actions}/runs/10`]: malformed },
]) {
const result = evaluate(routes);
expect(result.status).toBe(1);
expect(result.combined).toEqual(["pending", "failure"]);
}
});
it.each([0, -1, undefined, "invalid"])(
"rejects malformed eligible run ID %s before selecting older successful CI",
(id) => {
const result = evaluate({
[runsPath]: {
total_count: 2,
workflow_runs: [run, { ...run, id, status: "queued" }],
},
});
expect(result.status).toBe(1);
expect(result.stderr).toContain("invalid run identity");
expect(result.combined).toEqual(["pending", "failure"]);
},
);
it.each(["failure", "cancelled", "skipped", "neutral"])(
"does not hide a %s CI gate",
(conclusion) => {
@ -306,7 +340,16 @@ describe("combined security review entry point", () => {
[`GET ${actions}/runs/10`]: { ...run, run_attempt: 2, status: "in_progress" },
});
expect(result.status, result.stderr).toBe(0);
expect(result.combined).not.toContain("success");
expect(result.combined).toEqual(["pending", "pending"]);
});
it("fails evaluation when a replacement CI attempt has already completed", () => {
const result = evaluate({
[`GET ${actions}/runs/10`]: { ...run, run_attempt: 2 },
});
expect(result.status).toBe(1);
expect(result.stderr).toContain("completed CI attempt changed");
expect(result.combined).toEqual(["pending", "failure"]);
});
it("settles after CI completes without leaving either evaluation failed", () => {
@ -315,7 +358,7 @@ describe("combined security review entry point", () => {
});
const completed = evaluate();
expect(waiting.status, waiting.stderr).toBe(0);
expect(waiting.combined.at(-1)).toBe("failure");
expect(waiting.combined.at(-1)).toBe("pending");
expect(completed.status, completed.stderr).toBe(0);
expect(completed.combined.at(-1)).toBe("success");
});
@ -366,7 +409,7 @@ describe("combined security review entry point", () => {
it("publishes both review notices and settles automatically after command approval", () => {
const result = evaluate({ [rolePath]: { role_name: "write" } });
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure"]);
expect(result.combined).toEqual(["pending", "failure"]);
const notices = result.requests.filter(
(entry) => entry.method === "POST" && entry.path.endsWith("/comments"),
);
@ -426,7 +469,7 @@ describe("combined security review entry point", () => {
},
});
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
expect(result.reviews.filter((entry) => entry.body?.state === "success")).toHaveLength(2);
});
@ -456,7 +499,7 @@ describe("combined security review entry point", () => {
it("grandfathers an old branch without issuing reusable standalone successes or notices", () => {
const result = evaluate(exemptRoutes);
expect(result.status, result.stderr).toBe(0);
expect(result.combined).toEqual(["failure", "success"]);
expect(result.combined).toEqual(["pending", "success"]);
expect(result.reviews).toEqual([]);
expect(result.requests.some((entry) => entry.path.includes("/issues/"))).toBe(false);
expect(result.requests.some((entry) => entry.path.endsWith("/files"))).toBe(false);
@ -465,7 +508,7 @@ describe("combined security review entry point", () => {
it("still requires real CI for a grandfathered PR", () => {
const result = evaluate({ ...exemptRoutes, [runsPath]: { total_count: 0, workflow_runs: [] } });
expect(result.status, result.stderr).toBe(0);
expect(result.combined).not.toContain("success");
expect(result.combined).toEqual(["pending", "pending"]);
expect(result.reviews).toEqual([]);
});
@ -497,7 +540,7 @@ describe("combined security review entry point", () => {
it("leaves the combined gate failed when rollout metadata cannot be read", () => {
const result = evaluate({ "GET /repos/openclaw/openclaw/pulls/152415": { httpError: 403 } });
expect(result.status).toBe(1);
expect(result.combined).toEqual(["failure"]);
expect(result.combined).toEqual(["pending", "failure"]);
expect(result.reviews).toEqual([]);
});
});