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
17 changes: 17 additions & 0 deletions .loopover.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,23 @@ settings:
# abuse pattern.
# draftPrClosePolicy: off # off | close. Default: off.

# One-shot synchronize-amendment close policy (#synchronize-close-policy): distinct from
# reviewEvasionProtection and draftPrClosePolicy above -- those enforce on closing/converting-to-draft or on
# draft usage; this one enforces on the contributor's OWN PR receiving an ADDITIONAL commit (a
# `synchronize` push) before the PR has been merged or closed, regardless of what CI/review state that push
# interrupts. This repo's review is one-shot: the PR must be correct as originally opened, not iterated on
# while the first push's CI/review is still working -- "close" closes the PR immediately on that next push
# rather than letting a contributor use a slow CI run as a free window to land fixups. OFF BY DEFAULT
# (opt-in, unlike reviewEvasionProtection's default-close): it can catch ordinary, well-intentioned
# contributors who simply push a follow-up commit with no gaming intent, so choose this deliberately. Never
# fires for a push that isn't from the PR's own author (the engine's own rebase-if-behind push never
# matches), nor for the repo owner/admin, a protected automation author, or anyone with write+ collaborator
# access. Shares reviewEvasionLabel/reviewEvasionComment and autoCloseExemptLogins with the family above.
# Deliberately does NOT record a moderation strike (unlike reviewEvasionProtection) -- this is a blanket
# policy against an otherwise-ordinary GitHub action, not a detected abuse pattern. Config-as-code only --
# no dashboard/DB column.
# synchronizeClosePolicy: off # off | close. Default: off.

# Merge-train FIFO gate (#selfhost-merge-train): without this, a PR merges the instant its OWN gate clears,
# with zero awareness of an older sibling PR still open in the same repo -- proven live to cause out-of-order
# merges and the conflicts that follow. "audit" logs what the gate WOULD hold, without actually holding
Expand Down
8 changes: 8 additions & 0 deletions apps/loopover-ui/public/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -9746,6 +9746,14 @@
"minimum": 0,
"exclusiveMinimum": true
},
"synchronizeClosePolicy": {
"type": "string",
"enum": [
"off",
"close"
],
"description": "Off by default (opt-in, config-as-code only -- no dashboard/DB column). \"close\" closes a contributor's own PR immediately when they push an additional commit (synchronize) before it's been merged or closed -- this repo's review is one-shot, so the first push is the only push. Never fires for a push that isn't from the PR's own author (e.g. the engine's own rebase-if-behind), nor for the repo owner/admin, a protected automation author, or anyone with write+ collaborator access."
},
"contentLaneDeliverableGateMode": {
"type": "string",
"enum": [
Expand Down
17 changes: 17 additions & 0 deletions config/examples/loopover.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,23 @@ settings:
# abuse pattern.
# draftPrClosePolicy: off # off | close. Default: off.

# One-shot synchronize-amendment close policy (#synchronize-close-policy): distinct from
# reviewEvasionProtection and draftPrClosePolicy above -- those enforce on closing/converting-to-draft or on
# draft usage; this one enforces on the contributor's OWN PR receiving an ADDITIONAL commit (a
# `synchronize` push) before the PR has been merged or closed, regardless of what CI/review state that push
# interrupts. This repo's review is one-shot: the PR must be correct as originally opened, not iterated on
# while the first push's CI/review is still working -- "close" closes the PR immediately on that next push
# rather than letting a contributor use a slow CI run as a free window to land fixups. OFF BY DEFAULT
# (opt-in, unlike reviewEvasionProtection's default-close): it can catch ordinary, well-intentioned
# contributors who simply push a follow-up commit with no gaming intent, so choose this deliberately. Never
# fires for a push that isn't from the PR's own author (the engine's own rebase-if-behind push never
# matches), nor for the repo owner/admin, a protected automation author, or anyone with write+ collaborator
# access. Shares reviewEvasionLabel/reviewEvasionComment and autoCloseExemptLogins with the family above.
# Deliberately does NOT record a moderation strike (unlike reviewEvasionProtection) -- this is a blanket
# policy against an otherwise-ordinary GitHub action, not a detected abuse pattern. Config-as-code only --
# no dashboard/DB column.
# synchronizeClosePolicy: off # off | close. Default: off.

# Merge-train FIFO gate (#selfhost-merge-train): without this, a PR merges the instant its OWN gate clears,
# with zero awareness of an older sibling PR still open in the same repo -- proven live to cause out-of-order
# merges and the conflicts that follow. "audit" logs what the gate WOULD hold, without actually holding
Expand Down
6 changes: 6 additions & 0 deletions packages/loopover-engine/src/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ export type FocusManifestSettings = Partial<
| "reviewEvasionProtection"
| "reviewEvasionLabel"
| "reviewEvasionComment"
| "synchronizeClosePolicy"
| "mergeTrainMode"
>
> & {
Expand Down Expand Up @@ -2854,6 +2855,11 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[])
}
const reviewEvasionComment = normalizeOptionalBoolean(r.reviewEvasionComment, "settings.reviewEvasionComment", warnings);
if (reviewEvasionComment !== null) out.reviewEvasionComment = reviewEvasionComment;
// One-shot synchronize-amendment close policy (#synchronize-close-policy): a contributor pushing an
// additional commit to their own still-open PR before it's merged/closed is amending a one-shot review,
// not making an ordinary follow-up push.
const synchronizeClosePolicy = normalizeOptionalEnum(r.synchronizeClosePolicy, "settings.synchronizeClosePolicy", ["off", "close"] as const, warnings);
if (synchronizeClosePolicy !== null) out.synchronizeClosePolicy = synchronizeClosePolicy;
const mergeTrainMode = normalizeOptionalEnum(r.mergeTrainMode, "settings.mergeTrainMode", ["off", "audit", "enforce"] as const, warnings);
if (mergeTrainMode !== null) out.mergeTrainMode = mergeTrainMode;
return out;
Expand Down
7 changes: 7 additions & 0 deletions packages/loopover-engine/src/types/manifest-deps-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,13 @@ export type RepositorySettings = {
/** Review-evasion protection: whether to post the public explanation comment before the enforcement close.
* Default true. */
reviewEvasionComment?: boolean | undefined;
/** One-shot synchronize-amendment close policy (#synchronize-close-policy): distinct from
* reviewEvasionProtection above -- that one enforces on closing/converting-to-draft; this one enforces on
* the contributor's OWN PR receiving an ADDITIONAL commit before it's been merged or closed. `"off"` (the
* default) disables detection entirely; `"close"` closes the PR immediately on that next push. Only fires
* when the pusher is the PR's own author (never the engine's own rebase-if-behind, never a maintainer
* pushing to someone else's branch). */
synchronizeClosePolicy?: "off" | "close" | undefined;
/** Merge-train FIFO gate (#selfhost-merge-train): `"off"` keeps current behavior, `"audit"` logs would-hold
* decisions, and `"enforce"` defers a merge behind a still-viable older sibling. */
mergeTrainMode?: "off" | "audit" | "enforce" | undefined;
Expand Down
6 changes: 6 additions & 0 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,9 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise
reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL,
reviewEvasionComment: true,
draftPrClosePolicy: "off",
// Config-as-code only (#synchronize-close-policy): no DB column, matching reviewEvasionProtection's
// pattern above -- only .loopover.yml settings.synchronizeClosePolicy can set this.
synchronizeClosePolicy: "off",
mergeTrainMode: "off",
screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] },
};
Expand Down Expand Up @@ -756,6 +759,9 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise
reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL,
reviewEvasionComment: true,
draftPrClosePolicy: normalizeDraftPrClosePolicy(row.draftPrClosePolicy),
// Config-as-code only (#synchronize-close-policy): no DB column, matching reviewEvasionProtection's
// pattern above -- only .loopover.yml settings.synchronizeClosePolicy can set this.
synchronizeClosePolicy: "off",
mergeTrainMode: "off",
screenshotTableGate: parseScreenshotTableGateRow(row),
createdAt: row.createdAt,
Expand Down
6 changes: 6 additions & 0 deletions src/openapi/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,12 @@ export const RepositorySettingsSchema = z
.describe(
"Off by default (opt-in, unlike reviewEvasionProtection's default-close). \"close\" enforces on ANY draft PR, including the very first one, before a review pass has had a chance to run -- distinct from reviewEvasionProtection's family, which only enforces after a review already ran or on the 2nd+ draft conversion.",
),
synchronizeClosePolicy: z
.enum(["off", "close"])
.optional()
.describe(
"Off by default (opt-in, config-as-code only -- no dashboard/DB column). \"close\" closes a contributor's own PR immediately when they push an additional commit (synchronize) before it's been merged or closed -- this repo's review is one-shot, so the first push is the only push. Never fires for a push that isn't from the PR's own author (e.g. the engine's own rebase-if-behind), nor for the repo owner/admin, a protected automation author, or anyone with write+ collaborator access.",
),
mergeTrainMode: z.enum(["off", "audit", "enforce"]).optional(),
screenshotTableGate: z
.object({
Expand Down
10 changes: 10 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ import {
maybeCloseRepeatedDraftCycling,
maybeCloseReviewEvasionDraftConversion,
maybeCloseReviewEvasionSelfClose,
maybeCloseSynchronizeAmendment,
maybeRecloseDisallowedReopen,
type ReopenRecloseOutcome,
} from "./review-evasion";
Expand Down Expand Up @@ -6051,6 +6052,15 @@ async function handlePullRequestWebhookEvent(
// Resolve settings first so the self-authored + open-reference live-fetch fallbacks only fire when their
// respective gates are in block mode.
const settings = await resolveRepositorySettings(env, repoFullName);
// One-shot synchronize-amendment close (#synchronize-close-policy, resource-waste ordering -- mirrors the
// #7284-fix contributor-cap-on-open short-circuit below): a cheap, opt-in check dispatched BEFORE any of
// the expensive work further down (automation-bot-skip's own audit write, the Promise.all fetch, CI-wait,
// AI review, etc.) ever runs for a PR this policy is about to close anyway. maybeCloseSynchronizeAmendment
// itself does the real work (config check, then author/permission/bot exemptions) -- this call site only
// decides WHEN to ask, same division of labor as every other guard in this file.
if (payload.action === "synchronize" && installationId) {
await maybeCloseSynchronizeAmendment(env, deliveryId, installationId, repoFullName, pr, payload, settings);
}
// Waste elimination for known automation authors (settings/automation-bot-skip.ts): a PR/event genuinely
// triggered by release-please's github-actions[bot], Renovate, or Dependabot never needs AI review, gate
// evaluation, or a public-surface publish. Checked here (not earlier) because it needs `settings` for the
Expand Down
140 changes: 139 additions & 1 deletion src/queue/review-evasion.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// Review-evasion / close-enforcement guards (#4013 step 5 -- extracted from processors.ts, fifth step of
// the file's own module-split sequence, after transient-locks.ts, signal-snapshot.ts,
// duplicate-detection.ts, and slop-detection.ts). Pure move; only the 5 top-level "maybe*" entry points are
// duplicate-detection.ts, and slop-detection.ts). Only the top-level "maybe*" entry points are
// exported (each called from exactly one webhook-handler call site still in processors.ts) -- every other
// function/type/constant here (withPrActuationLock, evaluateCloseEnforcementGate, hasMaintainerOrOwnerPermission,
// the "close*If*" implementations, ReopenRecloseOutcome, REVIEW_EVASION_CLOSED_EVENT_TYPE) is private to this
// file, since none of them had any caller outside this cluster in the original file either.
// maybeCloseSynchronizeAmendment (#synchronize-close-policy) is a later, 6th addition alongside the original
// 5 extracted here -- same shape and reasoning as its siblings, added directly to this module rather than
// growing processors.ts again.

import {
getGateBlockOutcome,
Expand Down Expand Up @@ -530,6 +533,12 @@ const REVIEW_EVASION_CLOSED_EVENT_TYPE = "github_app.review_evasion_closed";
// review-evasion family -- keeping it a distinct audit category lets an operator query the two apart.
const DRAFT_PR_CLOSED_EVENT_TYPE = "github_app.draft_pr_closed";

// Separate eventType again (#synchronize-close-policy): same blanket-repo-POLICY reasoning as
// DRAFT_PR_CLOSED_EVENT_TYPE above, not the review-evasion family's detected-abuse-PATTERN framing -- an
// additional push is an otherwise-ordinary GitHub action this repo has chosen to forbid, not a caught
// gaming attempt, so it gets its own audit category too.
const SYNCHRONIZE_AMEND_CLOSED_EVENT_TYPE = "github_app.synchronize_amend_closed";

// Whether `login` holds a maintainer-equivalent permission on repoFullName -- the owner, an ADMIN_GITHUB_LOGINS
// entry, or a collaborator with admin/maintain/write access. Shared by both review-evasion guards below;
// mirrors recloseDisallowedReopenIfNeeded's identical `hasMaintainerPermission` closure (kept as a standalone
Expand Down Expand Up @@ -1228,3 +1237,132 @@ async function closeDraftPrIfPolicyEnabled(
/* v8 ignore next -- best-effort: the guarded CAS update never rejects against a healthy D1, and a cleanup failure here must never block the webhook. */
await terminalizeActiveReviewTracking(env, repoFullName, pr.number, { onlyIfHeadSha: pr.headSha }).catch(() => undefined);
}

/** One-shot synchronize-amendment close policy (#synchronize-close-policy): distinct from the four review-
* evasion guards above (which key off a review having ALREADY run) and from draftPrClosePolicy (which keys
* off draft state) -- this guard enforces on the contributor's OWN PR receiving an ADDITIONAL commit
* (synchronize) before the PR has been merged or closed, regardless of what CI/review state that push
* interrupts. This repo's review is one-shot: the PR must be correct as opened. Off by default
* (`settings.synchronizeClosePolicy !== "close"` bails immediately) -- unlike reviewEvasionProtection's
* default-close, this is opt-in: it can catch ordinary, well-intentioned contributors who simply push a
* follow-up commit with no gaming intent, so a maintainer chooses it deliberately for a specific repo.
* Only fires when the ACTOR who pushed is the PR's own author -- an engine-initiated rebase-if-behind push
* (prReadyForReview's forceUpdateBranch) is attributed to the App's own bot identity, never the author, so
* it can never match here; a maintainer pushing to someone else's branch is an ordinary maintainer action,
* not the author amending their own PR. Deliberately does NOT record a moderation strike (unlike the
* review-evasion family) -- this is a blanket repo policy applied to an otherwise-completely-ordinary
* GitHub action (pushing a follow-up commit), not a detected abuse pattern. Per-PR actuation-locked like
* its siblings. */
export async function maybeCloseSynchronizeAmendment(
env: Env,
deliveryId: string,
installationId: number,
repoFullName: string,
pr: PullRequestRecord,
payload: GitHubWebhookPayload,
settings: RepositorySettings,
): Promise<void> {
if (settings.synchronizeClosePolicy !== "close") return;
await withPrActuationLock(env, repoFullName, pr.number, "synchronize-close-policy", () =>
closeSynchronizeAmendmentIfPolicyEnabled(env, deliveryId, installationId, repoFullName, pr, payload, settings),
);
}

async function closeSynchronizeAmendmentIfPolicyEnabled(
env: Env,
deliveryId: string,
installationId: number,
repoFullName: string,
pr: PullRequestRecord,
payload: GitHubWebhookPayload,
settings: RepositorySettings,
): Promise<void> {
const actorLogin = (payload.sender?.login ?? "").toLowerCase();
const authorLogin = (pr.authorLogin ?? "").toLowerCase();
if (!actorLogin || !authorLogin || actorLogin !== authorLogin) return;
if (isProtectedAutomationAuthor(pr.authorLogin)) return;
if (isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) return;
if (!pr.headSha) return;
const headSha = pr.headSha;
if (await hasMaintainerOrOwnerPermission(env, installationId, repoFullName, authorLogin)) return;

const targetKey = `${repoFullName}#${pr.number}`;
const gateMetadata = { deliveryId, repoFullName, headSha };
const gate = await evaluateCloseEnforcementGate({
env,
installationId,
repoFullName,
pr,
settings,
eventType: SYNCHRONIZE_AMEND_CLOSED_EVENT_TYPE,
targetKey,
actionLabel: "synchronize close policy",
actor: String(pr.authorLogin),
metadata: gateMetadata,
dryRun: {
detail: `dry-run: would close PR amended by ${pr.authorLogin} after opening (synchronizeClosePolicy)`,
metadata: { ...gateMetadata, mode: "dry_run" },
},
paused: {
detail: `agent actions paused -- synchronize close policy not enforced for ${pr.authorLogin}`,
metadata: gateMetadata,
},
permissionReadiness: {
detail: `denied synchronize close for ${pr.authorLogin} -- pull_requests: write not granted`,
metadata: gateMetadata,
},
freshness: {
detailSuffix: " -- synchronize close not executed",
metadata: gateMetadata,
},
});
if (!gate.proceed) return;

const closeError = await closePullRequest(env, installationId, repoFullName, pr.number)
.then(() => null)
.catch((error: unknown) => error);
if (closeError !== null) {
await recordAuditEvent(env, {
eventType: SYNCHRONIZE_AMEND_CLOSED_EVENT_TYPE,
actor: "loopover",
targetKey,
outcome: "error",
detail: `FAILED to close PR amended by ${pr.authorLogin} -- the close API call did not succeed; the PR may still be open`,
metadata: { ...gateMetadata, error: errorMessage(closeError) },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
);
return;
}

const shouldPostComment = settings.reviewEvasionComment ?? true;
if (shouldPostComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"This repository reviews pull requests one-shot: the PR must be correct as originally opened. Pushing an additional commit closes it automatically instead of restarting review — open a fresh pull request with every fix included.",
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
/* v8 ignore next -- fail-safe: a label-application failure never blocks the handler (the enforcement close already happened). */
await ensurePullRequestLabel(env, installationId, repoFullName, pr.number, label, { createMissingLabel: true }).catch(() => undefined);
}
await recordAuditEvent(env, {
eventType: SYNCHRONIZE_AMEND_CLOSED_EVENT_TYPE,
actor: "loopover",
targetKey,
outcome: "completed",
detail: `closed PR by ${pr.authorLogin} for pushing an additional commit after opening -- synchronizeClosePolicy is "close"`,
metadata: gateMetadata,
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
);
}
Loading
Loading