diff --git a/.loopover.yml.example b/.loopover.yml.example index 3f63ec3f77..71d43f9c41 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -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 diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 3162ead20f..0d0c8919c2 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -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": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index f91a848423..139544a79f 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -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 diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index e3032acd82..1967addd69 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -628,6 +628,7 @@ export type FocusManifestSettings = Partial< | "reviewEvasionProtection" | "reviewEvasionLabel" | "reviewEvasionComment" + | "synchronizeClosePolicy" | "mergeTrainMode" > > & { @@ -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; diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index bde465395b..e704c92116 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -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; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 068f897539..b0d6b97e8f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -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: [] }, }; @@ -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, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 7298dfec4b..7a1030d623 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -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({ diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 74dc0d7def..46add1566f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -393,6 +393,7 @@ import { maybeCloseRepeatedDraftCycling, maybeCloseReviewEvasionDraftConversion, maybeCloseReviewEvasionSelfClose, + maybeCloseSynchronizeAmendment, maybeRecloseDisallowedReopen, type ReopenRecloseOutcome, } from "./review-evasion"; @@ -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 diff --git a/src/queue/review-evasion.ts b/src/queue/review-evasion.ts index ccd449e903..20f16ef6c2 100644 --- a/src/queue/review-evasion.ts +++ b/src/queue/review-evasion.ts @@ -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, @@ -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 @@ -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 { + 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 { + 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, + ); +} diff --git a/src/types.ts b/src/types.ts index 61fface11d..b99f5c31fb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1354,6 +1354,28 @@ export type RepositorySettings = { * label/comment conventions, no need for separate config). See `queue/review-evasion.ts`'s * `maybeCloseDraftPr`. */ draftPrClosePolicy?: "off" | "close" | undefined; + /** One-shot synchronize-amendment close policy (#synchronize-close-policy): distinct from {@link + * reviewEvasionProtection} and {@link draftPrClosePolicy} above -- those families 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` webhook) 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 instead of letting a contributor use a slow CI run as a free + * window to land fixups (each restart paying the full suite duration again). `"off"` (the default) is + * unchanged behavior -- like {@link draftPrClosePolicy}, this is opt-in (not default-on like + * reviewEvasionProtection): it can catch ordinary, well-intentioned contributors who simply push a + * follow-up commit with no gaming intent, so a maintainer chooses it deliberately per 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; 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 against an otherwise + * completely ordinary GitHub action, not a detected abuse pattern. Shares `autoCloseExemptLogins` and + * `reviewEvasionLabel`/`reviewEvasionComment` with the `reviewEvasionProtection` family (same anti-abuse + * label/comment conventions, no need for separate config). Config-as-code only -- no DB column; set via + * `.loopover.yml settings.synchronizeClosePolicy`. See `queue/review-evasion.ts`'s + * `maybeCloseSynchronizeAmendment`. */ + synchronizeClosePolicy?: "off" | "close" | undefined; /** 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. `"off"` (the default) is unchanged behavior. diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 8f141f1814..baa24d4425 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -373,6 +373,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { reviewEvasionProtection: "reviewEvasionProtection:", reviewEvasionLabel: "reviewEvasionLabel:", reviewEvasionComment: "reviewEvasionComment:", + synchronizeClosePolicy: "synchronizeClosePolicy:", mergeTrainMode: "mergeTrainMode:", typeLabels: "typeLabels:", issuePlanEnabled: "issuePlanEnabled:", diff --git a/test/unit/queue-lifecycle-guards.test.ts b/test/unit/queue-lifecycle-guards.test.ts index ca15f9cc08..77544b44cb 100644 --- a/test/unit/queue-lifecycle-guards.test.ts +++ b/test/unit/queue-lifecycle-guards.test.ts @@ -2016,7 +2016,7 @@ describe("review-evasion protection (#review-evasion-protection)", () => { // the DB write below so a per-test `{ reviewEvasionProtection: "off" }`/`{ reviewEvasionComment: false }`/ // `{ autoCloseExemptLogins: [...] }` still takes effect via the manifest overlay instead of being // silently outranked by the hardcoded default. - const { reviewEvasionProtection, reviewEvasionComment, autoCloseExemptLogins, ...dbOverrides } = overrides; + const { reviewEvasionProtection, reviewEvasionComment, autoCloseExemptLogins, synchronizeClosePolicy, ...dbOverrides } = overrides; await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto" }, @@ -2031,6 +2031,10 @@ describe("review-evasion protection (#review-evasion-protection)", () => { reviewEvasionProtection: (reviewEvasionProtection as "off" | "close" | undefined) ?? "close", ...(reviewEvasionComment !== undefined ? { reviewEvasionComment: reviewEvasionComment as boolean } : {}), ...(autoCloseExemptLogins !== undefined ? { autoCloseExemptLogins: autoCloseExemptLogins as string[] } : {}), + // synchronizeClosePolicy (#synchronize-close-policy) is manifest-only too -- pulled out here for the + // same reason as the three fields above, so a per-test override actually reaches the manifest + // overlay instead of being silently dropped by the DB write (there is no column for it). + ...(synchronizeClosePolicy !== undefined ? { synchronizeClosePolicy: synchronizeClosePolicy as "off" | "close" } : {}), }, }); } @@ -3864,6 +3868,264 @@ describe("review-evasion protection (#review-evasion-protection)", () => { expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); }); }); + + describe("one-shot synchronize-amendment close policy (#synchronize-close-policy)", () => { + function synchronizePayload(sender: string, author = sender, headSha = "def456"): any { + return { + action: "synchronize", + installation: { id: 123 }, + repository: { id: 1, name: "gittensory", full_name: "JSONbored/gittensory", private: false, default_branch: "main", owner: { login: "JSONbored" } }, + sender: { login: sender, type: "User" }, + pull_request: { + id: 4242, + number: 42, + state: "open", + title: "Some PR", + body: "Body.", + user: { login: author }, + head: { sha: headSha, ref: "fix", repo: { full_name: `${author}/gittensory`, owner: { login: author } } }, + base: { sha: "base123", ref: "main", repo: { full_name: "JSONbored/gittensory", owner: { login: "JSONbored" } } }, + draft: false, + merged: false, + mergeable_state: "clean", + created_at: "2026-05-27T00:00:00Z", + updated_at: "2026-05-27T00:00:00Z", + }, + }; + } + + it("does nothing when synchronizeClosePolicy is off (the default) -- an ordinary follow-up push is unaffected", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off" }); // synchronizeClosePolicy defaults to "off" + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-off", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("closes immediately when the PR's own author pushes an additional commit", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-close", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(true); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.synchronize_amend_closed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("completed"); + expect(audit?.detail).toContain("contributor"); + }); + + it("does NOT record a moderation strike -- this is a blanket policy against an ordinary push, not a detected abuse pattern", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + await repositoriesModule.upsertGlobalModerationConfig(env, { enabled: true, rules: ["review_evasion"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-no-strike", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + const strike = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("moderation.violation.review_evasion").first<{ n: number }>(); + expect(strike?.n).toBe(0); + }); + + it("audits an error and does NOT record a strike when the close API call fails", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls, { onPatch: () => new Response("server error", { status: 500 }) }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + await repositoriesModule.upsertGlobalModerationConfig(env, { enabled: true, rules: ["review_evasion"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-close-fail", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.synchronize_amend_closed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("error"); + expect(audit?.detail).toContain("FAILED to close"); + const strike = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("moderation.violation.review_evasion").first<{ n: number }>(); + expect(strike?.n).toBe(0); + }); + + it("honors settings.autoCloseExemptLogins -- the shared allowlist the review-evasion family already uses", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close", autoCloseExemptLogins: ["contributor"] }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-exempt", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing when the author holds write collaborator permission", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls, { collaboratorPermission: "write" }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-maintainer", eventName: "pull_request", payload: synchronizePayload("write-collaborator") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing for a protected automation author (e.g. dependabot[bot])", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-bot-author", eventName: "pull_request", payload: synchronizePayload("dependabot[bot]") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing when a THIRD PARTY pushes to someone else's PR branch -- an ordinary maintainer action, not the author amending their own PR", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls, { collaboratorPermission: "write" }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + const payload = synchronizePayload("contributor"); + payload.sender = { login: "a-maintainer", type: "User" }; + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-third-party", eventName: "pull_request", payload }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("REGRESSION (#synchronize-close-policy): does nothing when the push is the engine's OWN rebase-if-behind, not a genuine contributor amendment", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + const payload = synchronizePayload("contributor"); + // prReadyForReview's forceUpdateBranch (rebase-if-behind) pushes via the installation token; GitHub + // attributes the resulting synchronize webhook's sender to the App's own bot identity, never the + // PR author -- this must never be mistaken for the author amending their own PR. + payload.sender = { login: "loopover-orb[bot]", type: "Bot" }; + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-own-rebase", eventName: "pull_request", payload }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing when the webhook payload has no sender", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + const payload = synchronizePayload("contributor"); + payload.sender = undefined; + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-no-sender", eventName: "pull_request", payload }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing when the PR record has no author (a deleted-account PR)", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + const payload = synchronizePayload("contributor"); + payload.pull_request.user = null; + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-no-author", eventName: "pull_request", payload }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("does nothing when the PR record has no headSha", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + + const payload = synchronizePayload("contributor"); + payload.pull_request.head = null; + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-no-head-sha", eventName: "pull_request", payload }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("denies enforcement when the agent is paused for this repo", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close", agentPaused: true }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-paused", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.synchronize_amend_closed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("paused"); + }); + + it("skips the courtesy comment when reviewEvasionComment is explicitly false", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close", reviewEvasionComment: false }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-no-comment", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + }); + + it("posts the courtesy comment when reviewEvasionComment is unset (undefined, not just a stored default)", async () => { + const calls: Array<{ url: string; method: string }> = []; + stubEvasionFetch(calls); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + const baseSettings = await repositorySettingsModule.resolveRepositorySettings(env, "JSONbored/gittensory"); + vi.spyOn(repositorySettingsModule, "resolveRepositorySettings").mockResolvedValue({ ...baseSettings, reviewEvasionComment: undefined }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-comment-unset", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(true); + }); + + it("applies no label when reviewEvasionLabel is explicitly null (a .loopover.yml-only 'no label' override)", async () => { + const calls: Array<{ url: string; method: string }> = []; + const labelPostBodies: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.includes("/collaborators/")) return Response.json({ permission: "read" }); + if (method === "PATCH" && url.endsWith("/pulls/42")) return Response.json({ state: "closed" }); + if (method === "POST" && url.endsWith("/issues/42/comments")) return Response.json({ id: 1 }, { status: 201 }); + if (method === "POST" && url.endsWith("/issues/42/labels")) { + labelPostBodies.push(String(init?.body ?? "")); + return Response.json([], { status: 200 }); + } + if (url.includes("/labels")) return Response.json([]); // dedup probe: no labels on the issue yet + if (url.includes("/pulls/42/files")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "loopover-orb" }); + await setupEvasionRepo(env, { reviewEvasionProtection: "off", synchronizeClosePolicy: "close" }); + const baseSettings = await repositorySettingsModule.resolveRepositorySettings(env, "JSONbored/gittensory"); + vi.spyOn(repositorySettingsModule, "resolveRepositorySettings").mockResolvedValue({ ...baseSettings, reviewEvasionLabel: null }); + + await processJob(env, { type: "github-webhook", deliveryId: "sync-policy-label-null", eventName: "pull_request", payload: synchronizePayload("contributor") }); + + expect(labelPostBodies.some((b) => b.includes("review-evasion"))).toBe(false); + const audit = await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("github_app.synchronize_amend_closed").first<{ outcome: string }>(); + expect(audit?.outcome).toBe("completed"); + }); + }); }); describe("markPullRequestLinkedIssueHardRuleViolated (#linked-issue-hard-rule-persistence)", () => {