diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 83d3d82492..731c43aaa2 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -1,8 +1,9 @@ import { getInstallation, getPullRequest, getRepositorySettings, getPendingAgentAction, recordAuditEvent, setPendingAgentActionStatus } from "../db/repositories"; +import { createInstallationToken } from "../github/app"; +import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/linked-issue-hard-rules"; import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor"; -import { downgradeCloseToHold, downgradeMergeToHold, type PlannedAgentAction } from "../settings/agent-actions"; +import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; -import { createInstallationToken } from "../github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -148,6 +149,58 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de if (holdOnly) plan = downgradeMergeToHold(plan, true); if (closeHoldOnly) plan = downgradeCloseToHold(plan, true); + // Re-validate a staged MERGE against the CURRENT linked-issue hard-rule state (#2132). The hard rule is + // evaluated fresh on every planning pass and takes precedence over merge (see planAgentMaintenanceActions), + // but a staged merge only replays the PLAN-TIME snapshot — a maintainer relabeling/reassigning the linked + // issue between staging and accept (head SHA unchanged, so the check above doesn't catch it) would otherwise + // still merge a now-ineligible PR. Mirrors the planner's own owner/automation exemption (closeEligible) so an + // owner's staged merge, which the hard rule never blocks in the first place, is not wrongly denied here. + // Gated on the POST-downgrade `plan`, not `pending.actionClass`: the precision-breaker downgrade immediately + // above can already have replaced a staged merge with a needs-human-review label (downgradeMergeToHold) — that + // downgraded plan isn't going to merge anything, so a stale linked-issue violation must not reject the whole + // row and suppress the hold label; it only matters while a merge is still the thing about to execute. + if (plan.some((action) => action.actionClass === "merge") && pr) { + const repoOwner = pending.repoFullName.includes("/") ? pending.repoFullName.slice(0, pending.repoFullName.indexOf("/")) : ""; + const authorLogin = pr.authorLogin ?? ""; + const authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase(); + const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin); + const closeEligible = (!authorIsOwner && !authorIsAutomationBot) || (authorIsOwner && settings.closeOwnerAuthors === true); + if (closeEligible) { + const linkedIssueRulesConfig = await loadLinkedIssueHardRules(env, pending.repoFullName); + // Best-effort mint, same as the #2126 CI/mergeable/review re-check above: a failed mint here does NOT + // silently skip the recheck -- resolveLinkedIssueHardRule falls back to env.GITHUB_PUBLIC_TOKEN when + // ciToken is undefined and still attempts the fetch, only returning "not violated" if that ALSO can't + // gather issue facts. This is the same shared resolver + same fail-open contract the LIVE planning path + // (processors.ts) already relies on for the PRIMARY hard-rule decision; holding this SECONDARY, narrow- + // race-window recheck to a stricter fail-closed standard would deny otherwise-legitimate merges on every + // transient token-mint hiccup without closing a real gap (the executor mints its OWN token independently + // for the actual merge mutation, so a suspended/broken installation still fails there regardless). + const ciToken = await createInstallationToken(env, pending.installationId).catch(() => undefined); + const linkedIssueHardRule = await resolveLinkedIssueHardRule({ + env, + repoFullName: pending.repoFullName, + repoOwner, + config: linkedIssueRulesConfig, + body: pr.body, + linkedIssues: pr.linkedIssues, + ciToken, + installationId: pending.installationId, + }); + if (linkedIssueHardRule?.violated) { + await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); + await recordAuditEvent(env, { + eventType: "agent.pending_action.superseded", + actor: input.decidedBy, + targetKey, + outcome: "denied", + detail: `superseded merge: linked-issue hard rule now violated — ${linkedIssueHardRule.reason ?? "ineligible linked issue"}`, + metadata: { ...baseMetadata, linkedIssueReason: linkedIssueHardRule.reason }, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "linked_issue_hard_rule" }; + } + } + } + const outcomes = await executeAgentMaintenanceActions( env, { diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 8052462bb5..dcc68e05eb 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -34,11 +34,22 @@ vi.mock("../../src/github/backfill", async (importOriginal) => ({ fetchLivePullRequestMergeState: vi.fn(async () => "clean"), fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), })); +// resolveLinkedIssueHardRule defaults to the REAL implementation, which is a safe no-op here: loadLinkedIssueHardRules +// (also real, unmocked) always returns the all-off default config, so the real resolver returns undefined (not +// violated) without any GitHub fetch. Individual tests override it to exercise the accept-time recheck (#2132). +vi.mock("../../src/review/linked-issue-hard-rules", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + resolveLinkedIssueHardRule: vi.fn(actual.resolveLinkedIssueHardRule), + }; +}); import { createPullRequestReview, mergePullRequest } from "../../src/github/pr-actions"; import { ensurePullRequestLabel } from "../../src/github/labels"; import { createInstallationToken } from "../../src/github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../../src/github/backfill"; +import { resolveLinkedIssueHardRule } from "../../src/review/linked-issue-hard-rules"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; import { decidePendingAgentAction } from "../../src/services/agent-approval-queue"; import { @@ -357,6 +368,29 @@ describe("agent approval queue (#779)", () => { expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "gittensory:needs-human-review", { createMissingLabel: true }); }); + it("REGRESSION: a precision-breaker-downgraded merge still executes the hold/label plan even when the linked issue would now violate the hard rule", async () => { + // Before the fix, the linked-issue recheck gated on pending.actionClass (the ORIGINAL staged class), not the + // post-downgrade plan -- so a merge already downgraded to a needs-human-review label by the #2127 precision + // breaker above would still get its whole row rejected on a stale linked-issue violation, silently swallowing + // the hold label the breaker was supposed to guarantee. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval", label: "auto" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs." }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + // The merge-precision breaker engages fleet-wide AFTER this merge was staged — same as the #2127 test above. + await env.DB.prepare("INSERT INTO system_flags (key, value) VALUES (?, ?)").bind("holdonly:owner/repo", "true").run(); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).not.toHaveBeenCalled(); + expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "gittensory:needs-human-review", { createMissingLabel: true }); + // The recheck must not even run once the plan no longer contains a merge -- there's nothing left to validate. + expect(resolveLinkedIssueHardRule).not.toHaveBeenCalled(); + }); + it("accept executes a staged merge normally when the precision breaker is off", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); @@ -467,6 +501,108 @@ describe("agent approval queue (#779)", () => { expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 5, "owner/repo", 8, "gittensory:needs-human-review", { createMissingLabel: true }); }); + it("accept supersedes a staged merge when the linked issue trips a hard rule after staging (#2132)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + // Staged against a CONTRIBUTOR PR whose linked issue was eligible at plan time; between staging and accept + // another maintainer relabeled the linked issue (head SHA unchanged, so the freshness check above misses it). + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs." }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("linked_issue_hard_rule"); + expect(mergePullRequest).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("maintainer-only"); + }); + + it("accept executes a staged merge when the linked issue remains eligible", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: false, reason: null }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept still executes when the linked-issue recheck's own token mint fails — fails OPEN, ciToken passed as undefined (#2132)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: false, reason: null }); + // First call is the #2126 merge-live-recheck's own token mint (succeeds); the second is this new linked-issue + // recheck's token mint, which fails here. The executor mints its own token for the actual mutation + // independently, so this transient failure must fail open on THIS check specifically, not block the accept. + vi.mocked(createInstallationToken).mockResolvedValueOnce("test-installation-token").mockRejectedValueOnce(new Error("installation suspended")); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + expect(vi.mocked(resolveLinkedIssueHardRule)).toHaveBeenCalledWith(expect.objectContaining({ ciToken: undefined })); + }); + + it("accept supersedes with a fallback reason when the hard-rule result omits one", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: null }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("linked_issue_hard_rule"); + const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ detail: string }>(); + expect(audit?.detail).toContain("ineligible linked issue"); + }); + + it("accept tolerates a slash-less repoFullName and a missing PR author login (defensive fallbacks)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "solorepo", autonomy: { merge: "auto_with_approval" } }); + await upsertInstallation(env, { + installation: { id: 5, account: { login: "owner", id: 1, type: "User" }, repository_selection: "selected", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "solorepo", full_name: "solorepo", private: false, owner: { login: "owner" } }], + }); + // No `user` on the payload → authorLogin stored null; repoFullName has no "/" → repoOwner falls back to "". + await upsertPullRequestFromGitHub(env, "solorepo", { number: 7, title: "PR", state: "open", head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "solorepo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "solorepo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept does not consult the linked-issue hard rule for an owner-authored staged merge (mirrors the planner's closeEligible exemption)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + // Author IS the repo owner ("owner/repo" → owner login "owner"); closeOwnerAuthors defaults false, so the + // hard rule must never even be consulted for this PR, regardless of what it would say. + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "owner" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "would have violated, but must not even be checked" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + expect(resolveLinkedIssueHardRule).not.toHaveBeenCalled(); + }); + it("accept does not supersede when the PR record is absent (no live head to compare) — proceeds to the executor", async () => { const env = createTestEnv({}); // No PR seeded → getPullRequest returns null → pr?.headSha is undefined, so the staleness guard is skipped