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
33 changes: 9 additions & 24 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7465,28 +7465,8 @@ async function maybePublishPrPublicSurface(
reviewEligibility.skipReason,
webhook.deliveryId,
);
if (gateEnabled) {
const gateCheckResult = await createOrUpdateSkippedGateCheckRun(
env,
installationId,
repoFullName,
advisory,
"Review skipped: ignored author.",
mode,
);
/* v8 ignore next -- permission-missing audit behavior mirrors the existing skipped-check path above. */
if (gateCheckResult?.kind === "permission_missing") {
await auditGateCheckPermissionMissing(
env,
author,
repoFullName,
pr.number,
webhook.deliveryId,
gateCheckResult.warning,
);
}
}
return undefined;
publicSurfaceSkipped = true;
if (!shouldEvaluateGate) return undefined;
}
// A missing author already forces publicSurfaceSkipped=true above (decidePublicSurface's own
// "missing_author" skip), so the guard just above already returns undefined whenever `!author` combines with
Expand Down Expand Up @@ -8960,7 +8940,12 @@ async function maybePublishPrPublicSurface(
decisionOutcome: gateEvaluation?.conclusion,
},
() =>
autoReviewSkipReason
// #3698: a benign auto-review skip (draft, WIP title, too-large, docs-only, base-branch,
// auto-pause) shows the quiet "skipped (reason)" status -- but an IGNORED-AUTHOR skip also
// trips publicSurfaceSkipped, and that path exists specifically so the deterministic gate
// (e.g. the linked-issue hard rule) still shows its REAL, truthful conclusion instead of a
// "skipped" veneer that would let an ignored/excluded author's PR silently bypass it.
autoReviewSkipReason && !publicSurfaceSkipped
? createOrUpdateSkippedGateCheckRun(
env,
installationId,
Expand Down Expand Up @@ -8995,7 +8980,7 @@ async function maybePublishPrPublicSurface(
headSha: advisory.headSha,
checkRunId: gateCheckResult.id,
/* v8 ignore next -- gate-enabled publication always has a gate evaluation. */
conclusion: autoReviewSkipReason ? "skipped" : (gateEvaluation?.conclusion ?? null),
conclusion: autoReviewSkipReason && !publicSurfaceSkipped ? "skipped" : (gateEvaluation?.conclusion ?? null),
detailsUrl: gateCheckResult.html_url,
deliveryId: webhook.deliveryId,
}).catch((error) => {
Expand Down
28 changes: 16 additions & 12 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13765,7 +13765,7 @@ describe("queue processors", () => {
expect(audit?.detail).toBe("bot_author");
});

it("publishes a skipped review check and no gate failure for ignored authors", async () => {
it("evaluates the gate while suppressing public review output for ignored authors", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await persistRegistrySnapshot(
env,
Expand All @@ -13785,7 +13785,7 @@ describe("queue processors", () => {
gateCheckMode: "enabled",
linkedIssueGateMode: "block",
});
const calls = { skippedChecks: 0, comments: 0, minerList: 0 };
const calls = { gateChecks: 0, comments: 0, minerList: 0 };
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
const url = input.toString();
const method = init?.method ?? "GET";
Expand All @@ -13796,21 +13796,25 @@ describe("queue processors", () => {
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.includes("/commits/ignoredauthor123/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
if (url.includes("/issues/56/comments")) {
calls.comments += 1;
if (method !== "GET") calls.comments += 1;
return Response.json([]);
}
if (url.includes("/check-runs") && method === "POST") {
const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string; output?: { title?: string; summary?: string } };
const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string; output?: { title?: string } };
expect(body).toMatchObject({ status: "in_progress", output: { title: "Gittensory Orb Review Agent is evaluating" } });
expect(body.conclusion).toBeUndefined();
calls.gateChecks += 1;
return Response.json({ id: 930 }, { status: 201 });
}
if (url.includes("/check-runs/930") && method === "PATCH") {
const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string; output?: { title?: string } };
expect(body).toMatchObject({
status: "completed",
conclusion: "skipped",
output: {
title: "Gittensory Orb Review Agent skipped",
summary: "Review skipped: ignored author.",
},
conclusion: "failure",
output: { title: "Gittensory Orb Review Agent: No linked issue detected" },
});
calls.skippedChecks += 1;
return Response.json({ id: 930 }, { status: 201 });
calls.gateChecks += 1;
return Response.json({ id: 930 });
}
return new Response("not found", { status: 404 });
});
Expand All @@ -13831,7 +13835,7 @@ describe("queue processors", () => {
},
});

expect(calls).toEqual({ skippedChecks: 1, comments: 0, minerList: 0 });
expect(calls).toEqual({ gateChecks: 2, comments: 1, minerList: 0 });
const visibilitySkip = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ? and target_key = ?")
.bind("github_app.pr_visibility_skipped", "JSONbored/gittensory#56")
.first<{ detail: string; metadata_json: string }>();
Expand Down