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
72 changes: 63 additions & 9 deletions packages/loopover-engine/src/advisory/gate-advisory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,19 @@ function addRepoFindings(repo: RepositoryRecord, findings: AdvisoryFinding[]): v
}
}

/** #9129 (host-parity, src/rules/advisory.ts): corroboration for a duplicate-issue-link finding, beyond
* authored body text alone -- see the host copy's own doc comment for the full rationale. Kept in lock-step
* with the host's `hasDuplicateOverlapCorroboration` by the live-gate-parity contract test. */
function hasDuplicateOverlapCorroboration(pr: PullRequestRecord, otherPr: PullRequestRecord): boolean {
const mineFiles = pr.changedFiles;
const theirsFiles = otherPr.changedFiles;
if (mineFiles && mineFiles.length > 0 && theirsFiles && theirsFiles.length > 0) {
const mine = new Set(mineFiles);
if (theirsFiles.some((path) => mine.has(path))) return true;
}
return Boolean(theirsFiles && theirsFiles.length > 0);
}

function addPullRequestFindings(
repo: RepositoryRecord | null,
pr: PullRequestRecord,
Expand Down Expand Up @@ -312,13 +325,30 @@ function addPullRequestFindings(
// suppressing duplicate evidence with arbitrary PR-number ordering.
// Flag-OFF (default) short-circuits ⇒ the finding is pushed exactly as before (byte-identical).
if (overlappingPrs.length > 0 && !(duplicateWinnerEnabled && isDuplicateClusterWinnerByClaim(pr, overlappingPrs))) {
findings.push({
code: "duplicate_pr_risk",
severity: "warning",
title: "Linked issue overlaps another open PR",
detail: `Other open pull requests reference the same linked issue set: ${overlappingPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`,
action: "Review the related PRs before spending reviewer time on duplicate work.",
});
// #9129 (host-parity): split by corroboration -- see hasDuplicateOverlapCorroboration + the host's own
// doc comment. A CONCRETE finding code (`duplicate_pr_risk`) is reserved for a sibling with real
// corroborating evidence beyond body text; a PURELY body-text overlap gets the separate, always-non-
// blocking `duplicate_pr_risk_unconfirmed` code (see resolveConfiguredGateMode below).
const corroboratedPrs = overlappingPrs.filter((otherPr) => hasDuplicateOverlapCorroboration(pr, otherPr));
const uncorroboratedPrs = overlappingPrs.filter((otherPr) => !hasDuplicateOverlapCorroboration(pr, otherPr));
if (corroboratedPrs.length > 0) {
findings.push({
code: "duplicate_pr_risk",
severity: "warning",
title: "Linked issue overlaps another open PR with corroborating changes",
detail: `Other open pull requests reference the same linked issue set AND show corroborating changed-file overlap or a non-trivial diff: ${corroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`,
action: "This looks like a genuine race for the same issue. Coordinate with the other contributor, or wait for a maintainer to triage before assuming priority.",
});
}
if (uncorroboratedPrs.length > 0) {
findings.push({
code: "duplicate_pr_risk_unconfirmed",
severity: "info",
title: "Linked issue is also cited by another open PR",
detail: `Other open pull requests cite the same linked issue number, with no corroborating changed-file evidence yet: ${uncorroboratedPrs.map((otherPr) => `#${otherPr.number}`).join(", ")}.`,
action: "This may be coincidental, or an early race that hasn't produced comparable code yet — verify manually before assuming it's a genuine duplicate.",
});
}
}
}
// Self-authored linked-issue detection: the PR author also filed the linked issue. Raised when at least
Expand Down Expand Up @@ -566,6 +596,21 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy
warnings: gateWarnings,
};
}
// Duplicate-only HOLD (#9129, host-parity): a gate that would otherwise FAIL solely because of a
// same-linked-issue duplicate_pr_risk blocker is HELD for a human instead of closed outright — see the
// host copy's own doc comment for the full rationale. There is no duplicatePrGateMode configuration that
// can close a PR through this finding anymore. A blocker set that mixes it with a genuinely critical
// finding still falls through to the unconditional failure below.
if (blockers.every((blocker) => blocker.code === "duplicate_pr_risk")) {
return {
enabled: true,
conclusion: "neutral",
title: `${LOOPOVER_GATE_CHECK_NAME} — held for manual review`,
summary: blockers.map((finding) => sanitizeForCheckRun(finding.title)).join("; "),
blockers: [],
warnings: [...gateWarnings, ...blockers],
};
}
// Name the exact blocker(s) + fix in the title so the contributor sees WHY at a glance.
const firstBlocker = blockers[0];
const titleDetail = blockers.length === 1 && firstBlocker ? sanitizeForCheckRun(firstBlocker.title) : `${blockers.length} blockers`;
Expand Down Expand Up @@ -603,9 +648,18 @@ function gatePolicyBlocks(mode: GateRuleMode | undefined, defaultMode: GateRuleM
function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPolicy): boolean {
const code = finding.code;
// Missing linked issue defaults to ADVISORY — issues aren't always available, so it only blocks when a
// repo explicitly opts in with linkedIssueGateMode: "block". Duplicates still default to blocking.
// repo explicitly opts in with linkedIssueGateMode: "block".
if (code === "missing_linked_issue") return gatePolicyBlocks(policy.linkedIssueGateMode, "advisory");
if (code === "duplicate_pr_risk") return gatePolicyBlocks(policy.duplicatePrGateMode, "block");
// #9129 (host-parity): default changed from "block" to "advisory" -- this finding is derived from another
// contributor's own PR body text, so blocking-by-default let anyone force-close a rival's PR for free. A
// maintainer who explicitly opts into "block" still gets real effect: evaluateGateCheckCore HOLDS (never
// closes) a gate that fails solely on this finding (see the duplicate-only hold there). Only ever produced
// for a CORROBORATED overlap (see hasDuplicateOverlapCorroboration) -- a purely body-text overlap uses the
// separate, always-non-blocking duplicate_pr_risk_unconfirmed code below.
if (code === "duplicate_pr_risk") return gatePolicyBlocks(policy.duplicatePrGateMode, "advisory");
// #9129 (host-parity): an UNCORROBORATED duplicate-issue citation is NEVER a configured gate blocker,
// regardless of duplicatePrGateMode -- an adversary can manufacture this signal for free.
if (code === "duplicate_pr_risk_unconfirmed") return false;
// A dual-model AI consensus defect blocks ONLY when the maintainer opted into aiReview: block. It is the
// most conservative AI signal (two independent models) but still confirmed-contributor gated by
// evaluateGateCheck, and advisory by default.
Expand Down
21 changes: 18 additions & 3 deletions src/github/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { recordGitHubRateLimitObservation, updateInstallationPermissions } from
import { recordClockSkewFromResponse } from "../selfhost/clock-skew";
import {
clearGitHubResponseCacheForTest,
forcedSelfhostMode,
githubHeaders,
githubRateLimitAdmissionKeyForInstallation,
makeInstallationOctokit,
Expand Down Expand Up @@ -514,11 +515,15 @@ export async function getGithubUserCreatedAt(

/** Sentinel result for cancelInFlightWorkflowRunsForHeadSha (#2462) -- mirrors CheckRunOutcome's shape (a
* typed "degraded, not thrown" result) so a missing `actions: write` grant never has to be distinguished
* from a genuine network/API failure by the caller via exception type-narrowing. */
* from a genuine network/API failure by the caller via exception type-narrowing.
* `suppressed` (#9130): the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch forced this call to no-op --
* see cancelInFlightWorkflowRunsForHeadSha's own doc comment. Distinct from every other kind: no network call
* was even attempted, so a caller must never treat it as either a success or a failure worth alerting on. */
export type CancelWorkflowRunsOutcome =
| { kind: "cancelled"; cancelledCount: number; totalFound: number }
| { kind: "permission_missing"; warning: string }
| { kind: "error"; warning: string };
| { kind: "error"; warning: string }
| { kind: "suppressed" };

// A rate-limit / secondary-limit 403 is NOT a permission gap -- mirrors isCheckRunPermissionError's own
// exclusion (src/github/app.ts, isRateLimitedError check) so a burst-load 403 is never misrecorded as a
Expand Down Expand Up @@ -625,14 +630,24 @@ async function cancelOneWorkflowRun(
* Needs `actions: write` (list needs `actions: read`, effectively granted alongside write) -- an
* installation that hasn't granted it gets a typed `permission_missing` result, never a thrown error, so
* this can run as a best-effort side effect AFTER a close has already succeeded without risking that
* success being misrecorded as a failure. Greenfield: no existing Actions-API wrapper to extend. */
* success being misrecorded as a failure. Greenfield: no existing Actions-API wrapper to extend.
*
* #9130: this is the last installation-scoped write that ran on raw `timeoutFetch`, entirely OUTSIDE
* `makeInstallationOctokit`'s own suppression hook -- a direct sibling of the #9067 gap. Every other write this
* executor performs is suppressed the instant the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch is set
* (the octokit hook, or -- since #9130 -- the executor's own `mode` never even reaching the "9) live" branch
* that calls this). This one didn't go through either mechanism, so a suppressed instance still cancelled a
* contributor's real CI run. Consulting `forcedSelfhostMode(env)` directly, the SAME chokepoint
* `makeInstallationOctokit` uses, closes that gap even for a future caller that reaches this function without
* going through executeAgentMaintenanceActions's own (now instance-mode-aware) gate. */
export async function cancelInFlightWorkflowRunsForHeadSha(
env: Env,
installationId: number,
repoFullName: string,
headSha: string,
pullNumber: number,
): Promise<CancelWorkflowRunsOutcome> {
if (forcedSelfhostMode(env) !== null) return { kind: "suppressed" };
const parsed = parseRepoFullNameStrict(repoFullName);
if (!parsed) return { kind: "error", warning: `Invalid repository full name: ${repoFullName}` };
const { owner, repo } = parsed;
Expand Down
1 change: 1 addition & 0 deletions src/github/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,7 @@ export async function resolveRepoActionMode(env: Env, settings: Pick<RepositoryS
const { isGlobalAgentFrozen } = await import("../db/repositories");
return resolveAgentActionMode({
globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)),
instanceMode: forcedSelfhostMode(env),
agentPaused: settings?.agentPaused,
agentDryRun: settings?.agentDryRun,
});
Expand Down
16 changes: 12 additions & 4 deletions src/github/pr-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,22 @@ export async function createPullRequestReviewComments(
}

/** Merge a pull request with the configured method. Pass `sha` to make the merge fail (409) if the head moved
* since we evaluated it — a guard against merging a PR that changed under us. */
* since we evaluated it — a guard against merging a PR that changed under us.
*
* `suppressed` (#9130): true when the instance-wide SELFHOST_DEPLOYMENT_MODE kill switch (or a per-call
* non-"live" mode) suppressed this write at the makeInstallationOctokit hook, so `merged: true` is a SYNTHETIC
* shadow response, never a real GitHub mutation. Under the normal production call path (executor gates on its
* own resolved mode BEFORE ever calling this, #9130) this is structurally unreachable -- included as defense
* in depth so a future caller that reaches this function without going through that gate can never mistake a
* suppressed shadow for a genuine merge. The caller MUST check this before recording a terminal outcome,
* sending a notification, or escalating moderation off this result. */
export async function mergePullRequest(
env: Env,
installationId: number,
repoFullName: string,
pullNumber: number,
options: { mergeMethod: AutoMergeMethod; sha?: string | undefined },
): Promise<{ merged: boolean; sha: string | null }> {
): Promise<{ merged: boolean; sha: string | null; suppressed: boolean }> {
const { owner, repo } = splitRepo(repoFullName);
return withInstallationTokenRetry(env, installationId, async (token) => {
const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId));
Expand All @@ -116,8 +124,8 @@ export async function mergePullRequest(
merge_method: options.mergeMethod,
...(options.sha ? { sha: options.sha } : {}),
});
const data = response.data as { merged?: boolean; sha?: string };
return { merged: data.merged ?? true, sha: data.sha ?? null };
const data = response.data as { merged?: boolean; sha?: string; dryRunSuppressed?: boolean };
return { merged: data.merged ?? true, sha: data.sha ?? null, suppressed: data.dryRunSuppressed === true };
});
}

Expand Down
Loading
Loading