diff --git a/packages/loopover-miner/lib/attempt-cli.ts b/packages/loopover-miner/lib/attempt-cli.ts index 643af871fa..f880cb99d6 100644 --- a/packages/loopover-miner/lib/attempt-cli.ts +++ b/packages/loopover-miner/lib/attempt-cli.ts @@ -43,7 +43,7 @@ import type { GovernorLedger } from "./governor-ledger.js"; import { openWorktreeAllocator } from "./worktree-allocator.js"; import type { WorktreeAllocation, WorktreeAllocator } from "./worktree-allocator.js"; import { isValidRepoSegment } from "./repo-clone.js"; -import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, resolveRejectionSignaled } from "./rejection-signal.js"; +import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, resolveOwnOpenPrForIssue, resolveRejectionSignaled } from "./rejection-signal.js"; import type { resolveRejectionSignaled as ResolveRejectionSignaledFn } from "./rejection-signal.js"; import { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; import type { @@ -141,6 +141,8 @@ export type RunAttemptOptions = { initSignalTrackingStore?: () => SignalStore; buildAttemptDeps?: typeof buildAttemptDeps; resolveRejectionSignaled?: typeof ResolveRejectionSignaledFn; + // #8808: injection seam for the own-open-PR idempotency guard, mirroring resolveRejectionSignaled above. + resolveOwnOpenPrForIssue?: typeof resolveOwnOpenPrForIssue; fetchImpl?: SelfReviewContextFetch; prepareAttemptWorktree?: typeof PrepareAttemptWorktreeFn; cleanupAttemptWorktree?: typeof CleanupAttemptWorktreeFn; @@ -450,6 +452,51 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} return 5; } + // #8808: idempotency against this miner's OWN already-open PR for this exact issue — the crash-retry + // double-open guard. Checked before a worktree slot is consumed, mirroring the rejection-signal block + // above. Fail-open by construction (resolveOwnOpenPrForIssue returns null on any read/fetch failure), so + // a hiccup never blocks a legitimate attempt; refusing (not adopting) is the safe disposition — the + // still-open PR is already doing this issue's job. + const ownOpenPr = await (options.resolveOwnOpenPrForIssue ?? resolveOwnOpenPrForIssue)(parsed.repoFullName, parsed.issueNumber, { + fetchImpl: options.fetchImpl, + } as Parameters[2]); + if (ownOpenPr !== null) { + const reason = "own_open_pr_for_issue"; + attemptLog.appendAttemptLogEvent({ + eventType: "attempt_aborted", + attemptId, + actionClass: "open_pr", + mode, + reason, + payload: { repoFullName: parsed.repoFullName, issueNumber: parsed.issueNumber, existingPullRequestNumber: ownOpenPr }, + }); + eventLedger.appendEvent({ + type: "attempt_blocked", + repoFullName: parsed.repoFullName, + payload: { issueNumber: parsed.issueNumber, reason, existingPullRequestNumber: ownOpenPr }, + }); + const duplicateResult = { + outcome: "blocked_own_open_pr", + reason, + repoFullName: parsed.repoFullName, + issueNumber: parsed.issueNumber, + existingPullRequestNumber: ownOpenPr, + minerLogin: parsed.minerLogin, + base: parsed.base, + mode, + attemptId, + }; + if (parsed.json) { + console.log(JSON.stringify(duplicateResult, null, 2)); + } else { + console.error( + `Attempt for ${parsed.repoFullName}#${parsed.issueNumber} is blocked: this miner already has open PR #${ownOpenPr} for this issue (a crash-retry duplicate would double-open). Close it or wait for its outcome first.`, + ); + } + options.onResult?.(duplicateResult as AttemptCliResult); + return 5; + } + allocation = allocator.acquire(attemptId, parsed.repoFullName); // #7858: only when the operator has configured Neon (dbForkConfig !== null) -- otherwise a complete diff --git a/packages/loopover-miner/lib/rejection-signal.ts b/packages/loopover-miner/lib/rejection-signal.ts index b6d52ad3c6..18363e0878 100644 --- a/packages/loopover-miner/lib/rejection-signal.ts +++ b/packages/loopover-miner/lib/rejection-signal.ts @@ -49,7 +49,7 @@ export type RejectionSignalFetch = ( type RejectionSignalResponse = Awaited>; -type OwnRejectionHistorySubmission = { pullRequestNumber?: number | null | undefined }; +type OwnRejectionHistorySubmission = { pullRequestNumber?: number | null | undefined; issueNumber?: number | null | undefined }; type ListOwnSubmissions = (filter: { repoFullName?: string }) => OwnRejectionHistorySubmission[]; @@ -152,11 +152,13 @@ async function fetchPullRequestPayload( * fetch/parse failure is skipped so it never blocks the others. Consumes both upstream modules without modifying * either. Every dependency is injectable for testing. */ -export async function resolveOwnRejectionHistory(repoFullName: string, options: OwnRejectionHistoryOptions = {}): Promise { - const target = parseRepoFullName(repoFullName); - if (!target) return false; - const listSubmissions = options.listSubmissions ?? listRecentOwnSubmissions; - const resolved = { +function resolveHistoryOptions(options: OwnRejectionHistoryOptions): { + fetchImpl: RejectionSignalFetch; + githubToken: string; + githubApiBaseUrl: string; + maxChecks: number; +} { + return { fetchImpl: options.fetchImpl ?? (fetch as unknown as RejectionSignalFetch), githubToken: typeof options.githubToken === "string" ? options.githubToken.trim() : (process.env.GITHUB_TOKEN ?? ""), githubApiBaseUrl: @@ -166,6 +168,13 @@ export async function resolveOwnRejectionHistory(repoFullName: string, options: ? (options.maxRejectionHistoryChecks as number) : DEFAULT_MAX_REJECTION_HISTORY_CHECKS, }; +} + +export async function resolveOwnRejectionHistory(repoFullName: string, options: OwnRejectionHistoryOptions = {}): Promise { + const target = parseRepoFullName(repoFullName); + if (!target) return false; + const listSubmissions = options.listSubmissions ?? listRecentOwnSubmissions; + const resolved = resolveHistoryOptions(options); let submissions: OwnRejectionHistorySubmission[]; try { @@ -193,6 +202,52 @@ export async function resolveOwnRejectionHistory(repoFullName: string, options: return false; } +/** + * #8808: does THIS miner already have an OPEN PR for THIS exact issue on THIS repo? The crash-retry + * double-open guard: claim-conflict resolution deliberately never treats the miner's own sibling PR as a + * competing claim, and freshness excludes same-author PRs -- so nothing downstream refuses a duplicate + * attempt. Reads the miner's own recorded submissions (issue-tagged since #8172-era rows), live-checks the + * most recent candidates' PR state (bounded by the same maxRejectionHistoryChecks fan-out cap), and returns + * the first still-OPEN PR number -- or null. FULLY FAIL-OPEN: any read/fetch failure returns null (an + * idempotency guard must never block a legitimate attempt on a hiccup; submission-freshness + the claim + * ledger remain the downstream backstops). + */ +export async function resolveOwnOpenPrForIssue( + repoFullName: string, + issueNumber: number, + options: OwnRejectionHistoryOptions = {}, +): Promise { + const target = parseRepoFullName(repoFullName); + if (!target || !Number.isInteger(issueNumber) || issueNumber <= 0) return null; + const listSubmissions = options.listSubmissions ?? listRecentOwnSubmissions; + const resolved = resolveHistoryOptions(options); + + let submissions: OwnRejectionHistorySubmission[]; + try { + submissions = listSubmissions({ repoFullName }); + } catch { + return null; // wholesale read failure -- fail open, never block an attempt on it + } + const candidates = (Array.isArray(submissions) ? submissions : []) + .filter( + (submission) => + submission && + submission.issueNumber === issueNumber && + Number.isInteger(submission.pullRequestNumber) && + (submission.pullRequestNumber as number) > 0, + ) + .slice(0, resolved.maxChecks); + for (const candidate of candidates) { + try { + const payload = (await fetchPullRequestPayload(target, candidate.pullRequestNumber as number, resolved)) as { state?: unknown } | null; + if (payload && payload.state === "open") return candidate.pullRequestNumber as number; + } catch { + // Individual fetch failure -- skip (fail open), keep checking the rest. + } + } + return null; +} + /** * Resolve whether the target repo has signaled it does not want automated/AI-authored contributions -- * either trigger documented above. Returns `false` (never throws) on any fetch/parse failure for the policy diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index 97bc0cd399..526de20e71 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -1571,6 +1571,70 @@ describe("runAttempt (#5132)", () => { expect(error).toHaveBeenCalledWith(expect.stringContaining("AI-usage policy bans automated/AI-authored contributions")); }); + it("#8808: refuses a duplicate attempt when this miner already has an open PR for the exact issue (crash-retry double-open guard)", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const appendAttemptLogEventSpy = vi.spyOn(attemptLog, "appendAttemptLogEvent"); + const acquireSpy = vi.spyOn(allocator, "acquire"); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + resolveOwnOpenPrForIssue: async () => 42, + }); + + expect(exitCode).toBe(5); + expect(acquireSpy).not.toHaveBeenCalled(); // refused BEFORE consuming a worktree slot + expect(appendAttemptLogEventSpy).toHaveBeenCalledWith( + expect.objectContaining({ eventType: "attempt_aborted", reason: "own_open_pr_for_issue" }), + ); + const payload = JSON.parse(String(log.mock.calls.at(-1)?.[0])); + expect(payload).toMatchObject({ outcome: "blocked_own_open_pr", reason: "own_open_pr_for_issue", existingPullRequestNumber: 42 }); + }); + + it("#8808: the non-json refusal names the existing PR on stderr", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + resolveOwnOpenPrForIssue: async () => 42, + }); + + expect(exitCode).toBe(5); + expect(error).toHaveBeenCalledWith(expect.stringContaining("already has open PR #42")); + }); + + it("#8808: a null resolution (no open PR / fail-open hiccup) proceeds to the worktree slot exactly as before", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const acquireSpy = vi.spyOn(allocator, "acquire"); + + await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + resolveOwnOpenPrForIssue: async () => null, + }); + + expect(acquireSpy).toHaveBeenCalled(); // the guard let the attempt through + }); + it("REGRESSION (#6055): labels own-rejection-history aborts as own_submission_rejected in --json output", async () => { const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); const log = vi.spyOn(console, "log").mockImplementation(() => undefined); diff --git a/test/unit/miner-rejection-signal.test.ts b/test/unit/miner-rejection-signal.test.ts index aff1e69c42..07bc4f575b 100644 --- a/test/unit/miner-rejection-signal.test.ts +++ b/test/unit/miner-rejection-signal.test.ts @@ -7,6 +7,7 @@ vi.mock("@loopover/engine", async () => { import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, + resolveOwnOpenPrForIssue, resolveOwnRejectionHistory, resolveRejectionSignaled, } from "../../packages/loopover-miner/lib/rejection-signal.js"; @@ -463,3 +464,64 @@ describe("resolveRejectionSignaled combines both triggers (#5655)", () => { expect(result).toBe(false); }); }); + +describe("resolveOwnOpenPrForIssue (#8808)", () => { + it("returns the still-open PR number when this miner already has one for the exact issue", async () => { + const fetchImpl = vi.fn(async (_url: string, _init?: unknown) => jsonResponse({ state: "open" })); + const result = await resolveOwnOpenPrForIssue("acme/widgets", 12, { + listSubmissions: () => [{ pullRequestNumber: 42, issueNumber: 12 }], + fetchImpl, + }); + expect(result).toBe(42); + expect(String(fetchImpl.mock.calls[0]?.[0])).toContain("/repos/acme/widgets/pulls/42"); + }); + + it("returns null (no fetch) when no recorded submission matches the issue — other issues' PRs never block", async () => { + const fetchImpl = vi.fn(); + const result = await resolveOwnOpenPrForIssue("acme/widgets", 12, { + listSubmissions: () => [{ pullRequestNumber: 41, issueNumber: 11 }, { pullRequestNumber: null, issueNumber: 12 }, {}], + fetchImpl, + }); + expect(result).toBeNull(); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("a CLOSED/merged prior PR for the issue does not block a fresh attempt", async () => { + const fetchImpl = vi.fn(async () => jsonResponse({ state: "closed" })); + const result = await resolveOwnOpenPrForIssue("acme/widgets", 12, { + listSubmissions: () => [{ pullRequestNumber: 42, issueNumber: 12 }], + fetchImpl, + }); + expect(result).toBeNull(); + }); + + it("FAIL-OPEN: a submissions read failure or a fetch rejection returns null — never blocks an attempt on a hiccup", async () => { + expect( + await resolveOwnOpenPrForIssue("acme/widgets", 12, { + listSubmissions: () => { + throw new Error("store down"); + }, + fetchImpl: vi.fn(), + }), + ).toBeNull(); + expect( + await resolveOwnOpenPrForIssue("acme/widgets", 12, { + listSubmissions: () => [{ pullRequestNumber: 42, issueNumber: 12 }], + fetchImpl: vi.fn(async () => { + throw new Error("network"); + }), + }), + ).toBeNull(); + }); + + it("bounds the live checks to maxRejectionHistoryChecks and rejects degenerate inputs", async () => { + const fetchImpl = vi.fn(async () => jsonResponse({ state: "closed" })); + const submissions = Array.from({ length: 15 }, (_, i) => ({ pullRequestNumber: i + 1, issueNumber: 12 })); + expect(await resolveOwnOpenPrForIssue("acme/widgets", 12, { listSubmissions: () => submissions, fetchImpl, maxRejectionHistoryChecks: 3 })).toBeNull(); + expect(fetchImpl).toHaveBeenCalledTimes(3); + expect(await resolveOwnOpenPrForIssue("not-a-repo", 12, { listSubmissions: () => submissions, fetchImpl })).toBeNull(); + expect(await resolveOwnOpenPrForIssue("acme/widgets", 0, { listSubmissions: () => submissions, fetchImpl })).toBeNull(); + // Defensive non-array return (mirrors the sibling resolver's own guard). + expect(await resolveOwnOpenPrForIssue("acme/widgets", 12, { listSubmissions: (() => null) as never, fetchImpl })).toBeNull(); + }); +});