diff --git a/packages/gittensory-miner/lib/rejection-signal.d.ts b/packages/gittensory-miner/lib/rejection-signal.d.ts index 8548b955ec..f9506441b9 100644 --- a/packages/gittensory-miner/lib/rejection-signal.d.ts +++ b/packages/gittensory-miner/lib/rejection-signal.d.ts @@ -1,6 +1,27 @@ import type { SelfReviewContextFetch } from "./self-review-context.js"; +type OwnRejectionHistorySubmission = { pullRequestNumber?: number | null }; + +type ListOwnSubmissions = (filter: { repoFullName?: string }) => OwnRejectionHistorySubmission[]; + +export interface OwnRejectionHistoryOptions { + listSubmissions?: ListOwnSubmissions; + fetchImpl?: SelfReviewContextFetch; + githubToken?: string; + githubApiBaseUrl?: string; + maxRejectionHistoryChecks?: number; +} + +export interface RejectionSignaledOptions extends OwnRejectionHistoryOptions { + rawContentBaseUrl?: string; +} + export function resolveRejectionSignaled( repoFullName: string, - options?: { rawContentBaseUrl?: string; fetchImpl?: SelfReviewContextFetch }, + options?: RejectionSignaledOptions, +): Promise; + +export function resolveOwnRejectionHistory( + repoFullName: string, + options?: OwnRejectionHistoryOptions, ): Promise; diff --git a/packages/gittensory-miner/lib/rejection-signal.js b/packages/gittensory-miner/lib/rejection-signal.js index 6f5c225883..2fdd8945d2 100644 --- a/packages/gittensory-miner/lib/rejection-signal.js +++ b/packages/gittensory-miner/lib/rejection-signal.js @@ -1,4 +1,6 @@ import { resolveAiPolicyVerdict } from "@loopover/engine"; +import { listRecentOwnSubmissions } from "./governor-state.js"; +import { resolveRejection } from "./rejection-state-machine.js"; // Real rejectionSignaled resolver (#5132, Wave 3.5 follow-up). iterate-policy.ts's own doc comment: "True // when the target repo (or this contributor's history with it) has signaled it does not want automated/ @@ -8,16 +10,19 @@ import { resolveAiPolicyVerdict } from "@loopover/engine"; // fetched live and scanned via the engine's own resolveAiPolicyVerdict -- the same check // opportunity-fanout.js already runs during discovery, applied here at attempt time instead. // -// The SECOND trigger (a prior submission from this same miner was closed/rejected on this exact repo) is -// DELIBERATELY not resolved here: it would need each of this miner's recorded own-submissions -// (governor-state.js's listRecentOwnSubmissions, #5134) checked against its live PR outcome via -// rejection-state-machine.js's resolveRejection -- a second, separately-scoped fetch-and-classify pipeline. -// Not fabricated as "no rejection history" -- explicitly left as a known, documented gap for a follow-up, -// same discipline as SelfReviewContext's bounties/issueQuality (#5145) and this file's own callers should -// not assume a false result here means "no rejection signal of any kind." +// The SECOND trigger (a prior submission from this same miner was closed/rejected on this exact repo) is now +// resolved by resolveOwnRejectionHistory (#5655), closing the gap this header previously documented: it checks +// each of this miner's recorded own-submissions on the repo (governor-state.js's listRecentOwnSubmissions, +// #5134) against its live PR outcome via rejection-state-machine.js's resolveRejection (#4278) -- consuming both +// upstream modules without modifying either. resolveRejectionSignaled now returns true if EITHER trigger fires, +// so `rejectionSignaled` finally means what iterate-policy.ts's doc comment has always said. const DEFAULT_RAW_CONTENT_BASE_URL = "https://raw.githubusercontent.com"; const MAX_POLICY_DOC_BYTES = 128 * 1024; +const DEFAULT_GITHUB_API_BASE_URL = "https://api.github.com"; +// Bound the per-call PR-status fetch fan-out (#5655): a miner with a long submission history on one repo must +// not trigger an unbounded burst of GitHub API calls on every attempt -- only the N most recent are checked. +const DEFAULT_MAX_REJECTION_HISTORY_CHECKS = 10; function parseRepoFullName(repoFullName) { if (typeof repoFullName !== "string") return null; @@ -79,6 +84,70 @@ async function fetchPolicyDoc(target, path, resolved) { } } +async function fetchPullRequestPayload(target, prNumber, resolved) { + const url = `${resolved.githubApiBaseUrl}/repos/${encodeURIComponent(target.owner)}/${encodeURIComponent(target.repo)}/pulls/${prNumber}`; + const headers = { accept: "application/vnd.github+json", "user-agent": "loopover-miner" }; + if (resolved.githubToken) headers.authorization = `Bearer ${resolved.githubToken}`; + const response = await resolved.fetchImpl(url, { method: "GET", headers }); + if (!response.ok) return null; + return await response.json(); +} + +/** + * Resolve the SECOND `rejectionSignaled` trigger (#5655): has a prior submission from THIS miner on THIS exact + * repo already been closed/rejected? Reads this miner's own recorded submissions on the repo + * (`listRecentOwnSubmissions`, #5134), fetches each one's live PR state, and runs it through `resolveRejection` + * (#4278) -- returning `true` if ANY was closed without merge. Bounded (only the most recent + * `maxRejectionHistoryChecks` submissions with a real PR number are fetched) and fully fail-open: a wholesale + * failure to read submissions resolves to `false` (never fabricated as a rejection), and any single PR + * fetch/parse failure is skipped so it never blocks the others. Consumes both upstream modules without modifying + * either. Every dependency is injectable for testing. + * + * @param {string} repoFullName + * @param {{ listSubmissions?: typeof listRecentOwnSubmissions, fetchImpl?: typeof fetch, githubToken?: string, githubApiBaseUrl?: string, maxRejectionHistoryChecks?: number }} [options] + * @returns {Promise} + */ +export async function resolveOwnRejectionHistory(repoFullName, options = {}) { + const target = parseRepoFullName(repoFullName); + if (!target) return false; + const listSubmissions = options.listSubmissions ?? listRecentOwnSubmissions; + const resolved = { + fetchImpl: options.fetchImpl ?? fetch, + githubToken: typeof options.githubToken === "string" ? options.githubToken.trim() : (process.env.GITHUB_TOKEN ?? ""), + githubApiBaseUrl: + typeof options.githubApiBaseUrl === "string" && options.githubApiBaseUrl.trim() ? options.githubApiBaseUrl.trim() : DEFAULT_GITHUB_API_BASE_URL, + maxChecks: + Number.isInteger(options.maxRejectionHistoryChecks) && options.maxRejectionHistoryChecks > 0 + ? options.maxRejectionHistoryChecks + : DEFAULT_MAX_REJECTION_HISTORY_CHECKS, + }; + + let submissions; + try { + submissions = listSubmissions({ repoFullName }); + } catch { + return false; // wholesale failure to read own submissions -- fail open, never fabricate a rejection + } + const checkable = (Array.isArray(submissions) ? submissions : []) + .filter((submission) => submission && Number.isInteger(submission.pullRequestNumber) && submission.pullRequestNumber > 0) + .slice(0, resolved.maxChecks); + if (checkable.length === 0) return false; // no prior submissions on this repo -- no fetch attempted + + for (const submission of checkable) { + try { + const payload = await fetchPullRequestPayload(target, submission.pullRequestNumber, resolved); + if (!payload) continue; + // No signal (gate/duplicate context isn't available here) -- resolveRejection returns non-null only for a + // PR that is closed-without-merge, which is exactly the "was it rejected" question this check asks. + const rejection = resolveRejection(payload, undefined, { repoFullName, prNumber: submission.pullRequestNumber }); + if (rejection) return true; + } catch { + // Individual PR fetch/parse/classify failure -- skip this one, keep checking the rest (fail open). + } + } + return false; +} + /** * Resolve whether the target repo has an explicit, live AI-usage-policy ban -- the first of * `rejectionSignaled`'s two documented triggers. Returns `false` (never throws) on any fetch/parse failure, @@ -97,5 +166,8 @@ export async function resolveRejectionSignaled(repoFullName, options = {}) { const contributing = aiUsage && aiUsage.trim() ? null : await fetchPolicyDoc(target, "CONTRIBUTING.md", resolved); const verdict = resolveAiPolicyVerdict({ aiUsage, contributing }); - return !verdict.allowed; + // First trigger: an explicit live AI-usage-policy ban. A ban short-circuits -- no need to also check history. + if (!verdict.allowed) return true; + // Second trigger (#5655): a prior submission from this same miner on this exact repo was closed/rejected. + return resolveOwnRejectionHistory(repoFullName, options); } diff --git a/test/unit/miner-rejection-signal.test.ts b/test/unit/miner-rejection-signal.test.ts index 698eaebf69..207d1ccd5a 100644 --- a/test/unit/miner-rejection-signal.test.ts +++ b/test/unit/miner-rejection-signal.test.ts @@ -4,7 +4,10 @@ vi.mock("@loopover/engine", async () => { return import("../../packages/gittensory-engine/src/index"); }); -import { resolveRejectionSignaled } from "../../packages/gittensory-miner/lib/rejection-signal.js"; +import { + resolveOwnRejectionHistory, + resolveRejectionSignaled, +} from "../../packages/gittensory-miner/lib/rejection-signal.js"; // resolveRejectionSignaled fetches plain markdown text (AI-USAGE.md/CONTRIBUTING.md), never JSON, so // json() is never actually called -- it's here only to satisfy SelfReviewContextFetch's response shape. @@ -231,7 +234,7 @@ describe("resolveRejectionSignaled (#5132)", () => { const fetchSpy = vi.fn(async () => textResponse(null, 404)); globalThis.fetch = fetchSpy as unknown as typeof fetch; try { - const result = await resolveRejectionSignaled("acme/widgets"); + const result = await resolveRejectionSignaled("acme/widgets", { listSubmissions: () => [] }); expect(result).toBe(false); expect(fetchSpy).toHaveBeenCalled(); } finally { @@ -239,3 +242,177 @@ describe("resolveRejectionSignaled (#5132)", () => { } }); }); + +function jsonResponse(payload: unknown, status = 200) { + return { + ok: status >= 200 && status < 300, + status, + json: async () => payload, + text: async () => JSON.stringify(payload), + }; +} + +const CLOSED_WITHOUT_MERGE = { + state: "closed", + merged: false, + closed_at: "2026-07-01T00:00:00Z", + merged_at: null, +}; +const MERGED = { + state: "closed", + merged: true, + closed_at: "2026-07-01T00:00:00Z", + merged_at: "2026-07-01T00:00:00Z", +}; + +describe("resolveOwnRejectionHistory (#5655)", () => { + it("returns true when a prior submission on the repo resolves to a closed-without-merge PR", async () => { + const fetchImpl = vi.fn(async (_url: string, _init?: unknown) => jsonResponse(CLOSED_WITHOUT_MERGE)); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: 42 }], + fetchImpl, + }); + expect(result).toBe(true); + expect(String(fetchImpl.mock.calls[0]?.[0])).toContain("/repos/acme/widgets/pulls/42"); + }); + + it("returns false and fetches nothing when no prior submission on this repo has a real PR number", async () => { + const fetchImpl = vi.fn(); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: null }, { pullRequestNumber: 0 }, {}], + fetchImpl, + }); + expect(result).toBe(false); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("bounds the fetch count to maxRejectionHistoryChecks (no unbounded fan-out)", async () => { + const fetchImpl = vi.fn(async () => jsonResponse(MERGED)); // none rejected -> it checks up to the cap + const submissions = Array.from({ length: 15 }, (_, i) => ({ pullRequestNumber: i + 1 })); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => submissions, + fetchImpl, + maxRejectionHistoryChecks: 3, + }); + expect(result).toBe(false); + expect(fetchImpl).toHaveBeenCalledTimes(3); + }); + + it("fails open on an individual PR fetch failure while still checking the rest", async () => { + const fetchImpl = vi.fn(async (url: string) => { + if (url.includes("/pulls/1")) throw new Error("network unreachable"); + return jsonResponse(CLOSED_WITHOUT_MERGE); + }); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: 1 }, { pullRequestNumber: 2 }], + fetchImpl, + }); + expect(result).toBe(true); // PR 1 failed, but PR 2's rejection is still detected + }); + + it("treats a non-array submissions result as empty and fetches nothing", async () => { + const fetchImpl = vi.fn(); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: (() => null) as never, + fetchImpl, + }); + expect(result).toBe(false); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("fails open to false (never throws) on a wholesale failure to read submissions", async () => { + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => { + throw new Error("db unavailable"); + }, + fetchImpl: vi.fn(), + }); + expect(result).toBe(false); + }); + + it("treats a non-2xx PR response as not-a-rejection", async () => { + const fetchImpl = vi.fn(async () => jsonResponse({}, 404)); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: 7 }], + fetchImpl, + }); + expect(result).toBe(false); + }); + + it("does not treat a merged PR as a rejection", async () => { + const fetchImpl = vi.fn(async () => jsonResponse(MERGED)); + const result = await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: 9 }], + fetchImpl, + }); + expect(result).toBe(false); + }); + + it("sends the auth header and hits the configured API base when an auth value is provided", async () => { + const fetchImpl = vi.fn(async (_url: string, _init?: unknown) => jsonResponse(MERGED)); + const injectedAuthValue = "fake-auth-placeholder"; + await resolveOwnRejectionHistory("acme/widgets", { + listSubmissions: () => [{ pullRequestNumber: 9 }], + fetchImpl, + githubToken: injectedAuthValue, + githubApiBaseUrl: "https://api.example.internal", + }); + const init = fetchImpl.mock.calls[0]?.[1] as { headers?: Record }; + expect(init?.headers?.authorization).toBe(`Bearer ${injectedAuthValue}`); + expect(String(fetchImpl.mock.calls[0]?.[0])).toContain( + "https://api.example.internal/repos/acme/widgets/pulls/9", + ); + }); + + it("returns false for a malformed repoFullName without reading submissions", async () => { + const listSubmissions = vi.fn(); + const result = await resolveOwnRejectionHistory("not-a-repo", { listSubmissions, fetchImpl: vi.fn() }); + expect(result).toBe(false); + expect(listSubmissions).not.toHaveBeenCalled(); + }); +}); + +describe("resolveRejectionSignaled combines both triggers (#5655)", () => { + it("returns true from the policy-ban trigger even with a clean rejection history (short-circuits)", async () => { + const listSubmissions = vi.fn(() => []); + const result = await resolveRejectionSignaled("acme/widgets", { + fetchImpl: routedFetch({ + "AI-USAGE.md": () => textResponse("No AI-generated pull requests, please."), + "CONTRIBUTING.md": () => textResponse("Welcome, contributors!"), + }), + listSubmissions, + }); + expect(result).toBe(true); + expect(listSubmissions).not.toHaveBeenCalled(); + }); + + it("returns true from the own-rejection-history trigger when the policy docs are clean", async () => { + const policyFetch = routedFetch({ + "AI-USAGE.md": () => textResponse("AI contributions are welcome here."), + "CONTRIBUTING.md": () => textResponse("Welcome, contributors!"), + }); + const fetchImpl = vi.fn(async (url: string) => + url.includes("/pulls/") ? jsonResponse(CLOSED_WITHOUT_MERGE) : policyFetch(url), + ); + const result = await resolveRejectionSignaled("acme/widgets", { + fetchImpl, + listSubmissions: () => [{ pullRequestNumber: 42 }], + }); + expect(result).toBe(true); + }); + + it("returns false when neither trigger fires (clean policy + no prior rejection)", async () => { + const policyFetch = routedFetch({ + "AI-USAGE.md": () => textResponse("AI contributions are welcome here."), + "CONTRIBUTING.md": () => textResponse("Welcome, contributors!"), + }); + const fetchImpl = vi.fn(async (url: string) => + url.includes("/pulls/") ? jsonResponse(MERGED) : policyFetch(url), + ); + const result = await resolveRejectionSignaled("acme/widgets", { + fetchImpl, + listSubmissions: () => [{ pullRequestNumber: 42 }], + }); + expect(result).toBe(false); + }); +});