From 0c3139eb55abbf8f12e7522c3b750882527be1e2 Mon Sep 17 00:00:00 2001 From: jaytbarimbao-collab <300663773+jaytbarimbao-collab@users.noreply.github.com> Date: Fri, 17 Jul 2026 11:59:31 -0400 Subject: [PATCH] feat: REST + CLI mirror for loopover_pr_outcome (#6747) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loopover_pr_outcome MCP tool returned a contributor's own merged-PR outcome history, but no REST route or CLI stdio tool exposed it — unlike its siblings in the /v1/contributors/:login/* family. - New src/signals/contributor-pr-outcomes.ts: buildContributorPrOutcomes(env, login, limit) — the single source of truth, mapping the contributor's pull_request_merged notification deliveries into outcome records. The MCP tool now delegates to it, so all three surfaces return identical data (mirrors how buildContributorOpenPrMonitor backs its own trio). - REST: GET /v1/contributors/:login/pr-outcomes[?limit=N] in routes.ts, gated by requireContributorAccess (same self-scoping as the tool), limit clamped. - CLI: loopover_pr_outcome stdio tool in loopover-mcp.js, hitting the new route (count guard 72 -> 73). Tests: builder unit coverage (mapping / event-type filter / empty / limit), REST parity (route output equals the shared builder byte-for-byte), limit query, and the forbidden_contributor guard for the shared MCP token. The existing loopover_pr_outcome MCP test still passes through the shared builder. Closes #6747 --- packages/loopover-mcp/bin/loopover-mcp.js | 32 +++++++ src/api/routes.ts | 15 +++ src/mcp/server.ts | 15 +-- src/signals/contributor-pr-outcomes.ts | 48 ++++++++++ test/unit/contributor-pr-outcomes.test.ts | 111 ++++++++++++++++++++++ test/unit/mcp-tool-rename-aliases.test.ts | 11 ++- 6 files changed, 216 insertions(+), 16 deletions(-) create mode 100644 src/signals/contributor-pr-outcomes.ts create mode 100644 test/unit/contributor-pr-outcomes.test.ts diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index 70db6f645f..78fcc1f2a9 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -412,6 +412,10 @@ const followUpIssueShape = { const loginShape = { login: z.string().min(1), }; +const prOutcomeShape = { + login: z.string().min(1), + limit: z.number().int().positive().max(200).optional(), +}; const loginRepoShape = { login: z.string().min(1), @@ -1098,6 +1102,12 @@ const STDIO_TOOL_DESCRIPTORS = [ description: "Inspect a contributor's open PRs on registered repos, classify queue state, and return public-safe next-step packets from cached metadata.", }, + { + name: "loopover_pr_outcome", + category: "review", + description: + "Return a contributor's own post-merge outcome history — for each merged PR, a public-safe attribution of what it did for their standing. Self-scoped to the authenticated login. Mirrors GET /v1/contributors/{login}/pr-outcomes.", + }, { name: "loopover_compare_pr_variants", category: "branch", @@ -1944,6 +1954,18 @@ registerStdioTool( }, ); +registerStdioTool( + "loopover_pr_outcome", + { + description: stdioToolDescription("loopover_pr_outcome"), + inputSchema: prOutcomeShape, + }, + async ({ login, limit }) => { + const payload = await getContributorPrOutcomes(login, limit); + return toolResult(prOutcomeToolSummary(login, payload), payload); + }, +); + registerStdioTool( "loopover_monitor_open_prs", { @@ -5198,6 +5220,16 @@ function getOpenPrMonitor(login) { return apiGet(`/v1/contributors/${encodeURIComponent(login)}/open-pr-monitor`); } +function getContributorPrOutcomes(login, limit) { + const query = limit ? `?limit=${encodeURIComponent(limit)}` : ""; + return apiGet(`/v1/contributors/${encodeURIComponent(login)}/pr-outcomes${query}`); +} + +function prOutcomeToolSummary(login, payload) { + const count = typeof payload?.count === "number" ? payload.count : (payload?.outcomes?.length ?? 0); + return `LoopOver post-merge outcomes for ${login}: ${count} merged PR(s).`; +} + // Mirror the API's own `summary` when it sends one, so the CLI and the loopover_monitor_open_prs MCP // tool (which returns monitor.summary verbatim) never drift into two different sentences for one payload. function openPrMonitorToolSummary(login, payload) { diff --git a/src/api/routes.ts b/src/api/routes.ts index b734dd6cb7..171809bd97 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -268,6 +268,7 @@ import { } from "../signals/extension-contributor-context"; import { attachDataQuality, buildCoreSignalFidelity, buildFreshnessSloReport, buildRepoDataQuality, buildSignalFidelity } from "../signals/data-quality"; import { buildContributorOpenPrMonitor } from "../signals/contributor-open-pr-monitor"; +import { buildContributorPrOutcomes, CONTRIBUTOR_PR_OUTCOMES_DEFAULT_LIMIT } from "../signals/contributor-pr-outcomes"; import { buildPullRequestReviewability, type PullRequestReviewability } from "../signals/reward-risk"; import { buildLocalBranchAnalysis, findCurrentBranchPullRequest } from "../signals/local-branch"; import { buildIssueSlopAssessment, ISSUE_SLOP_RUBRIC_MARKDOWN } from "../signals/issue-slop"; @@ -3259,6 +3260,20 @@ export function createApp() { return c.json(await buildContributorOpenPrMonitor(c.env, login)); }); + // #6747: REST mirror of the loopover_pr_outcome MCP tool — a contributor's own merged-PR outcome history. + // Self-scoped by requireContributorAccess (same gate as the tool); shares buildContributorPrOutcomes so the + // two surfaces can't drift. `limit` is clamped to a sane range, defaulting to the tool's own default. + app.get("/v1/contributors/:login/pr-outcomes", async (c) => { + const login = c.req.param("login"); + const unauthorized = await requireContributorAccess(c, login); + if (unauthorized) return unauthorized; + const limit = Math.max( + 1, + Math.min(200, Number(c.req.query("limit") ?? CONTRIBUTOR_PR_OUTCOMES_DEFAULT_LIMIT) || CONTRIBUTOR_PR_OUTCOMES_DEFAULT_LIMIT), + ); + return c.json(await buildContributorPrOutcomes(c.env, login, limit)); + }); + app.get("/v1/contributors/:login/repos/:owner/:repo/decision", async (c) => { const login = c.req.param("login"); const unauthorized = await requireContributorAccess(c, login); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 5310504255..82c97bc176 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -141,6 +141,7 @@ import { } from "../signals/engine"; import { PUBLIC_SURFACE_SKIP_REASONS, skippedPrAuditRemediation, type PublicSurfaceSkipReason } from "../signals/settings-preview"; import { buildContributorOpenPrMonitor } from "../signals/contributor-open-pr-monitor"; +import { buildContributorPrOutcomes } from "../signals/contributor-pr-outcomes"; import { buildLocalBranchAnalysis, findCurrentBranchPullRequest } from "../signals/local-branch"; import { computeLocalScorerTokens } from "../signals/local-scorer"; import { buildPullRequestReviewability, type PullRequestReviewability } from "../signals/reward-risk"; @@ -3762,18 +3763,10 @@ export class LoopoverMcp { private async prOutcomes(login: string, limit?: number): Promise { this.requireContributorAccess(login); - const deliveries = await listNotificationDeliveriesForRecipient(this.env, login, { eventType: "pull_request_merged", limit: limit ?? 50 }); - const outcomes = deliveries.map((delivery) => ({ - repoFullName: delivery.repoFullName, - pullNumber: delivery.pullNumber, - outcome: "merged" as const, - attribution: delivery.body, - deeplink: delivery.deeplink, - recordedAt: delivery.createdAt, - })); + const data = await buildContributorPrOutcomes(this.env, login, limit); return { - summary: `LoopOver post-merge outcomes for ${login}: ${outcomes.length} merged PR(s).`, - data: { login: login.toLowerCase(), count: outcomes.length, outcomes } as unknown as Record, + summary: `LoopOver post-merge outcomes for ${data.login}: ${data.count} merged PR(s).`, + data: data as unknown as Record, }; } diff --git a/src/signals/contributor-pr-outcomes.ts b/src/signals/contributor-pr-outcomes.ts new file mode 100644 index 0000000000..a2949781d7 --- /dev/null +++ b/src/signals/contributor-pr-outcomes.ts @@ -0,0 +1,48 @@ +import { listNotificationDeliveriesForRecipient } from "../db/repositories"; + +// A contributor's own post-merge outcome history (#6747): for each merged PR, the public-safe attribution that +// was delivered to them describing what it did for their standing on the repo. Sourced from the same +// `pull_request_merged` notification deliveries the `loopover_pr_outcome` MCP tool reads, so the MCP tool, the +// REST route (`GET /v1/contributors/:login/pr-outcomes`), and the CLI mirror all return identical data — this +// builder is the single source of truth, mirroring how `buildContributorOpenPrMonitor` backs its own trio. + +/** Default number of most-recent merged-PR outcomes returned when the caller doesn't specify a limit. */ +export const CONTRIBUTOR_PR_OUTCOMES_DEFAULT_LIMIT = 50; + +export type ContributorPrOutcome = { + repoFullName: string; + /** The merged PR's number; null only for a legacy delivery recorded without one. */ + pullNumber: number | null; + outcome: "merged"; + /** Public-safe attribution text — the delivered notification body, never raw internal scoring. */ + attribution: string; + deeplink: string; + recordedAt: string; +}; + +export type ContributorPrOutcomes = { + login: string; + count: number; + outcomes: ContributorPrOutcome[]; +}; + +/** + * Build a contributor's merged-PR outcome history from their `pull_request_merged` notification deliveries, + * newest first. Self-scoped by `login` — the caller (MCP tool / REST route) owns the access check. `login` is + * lowercased in the result so every surface reports a canonical form. + */ +export async function buildContributorPrOutcomes(env: Env, login: string, limit?: number): Promise { + const deliveries = await listNotificationDeliveriesForRecipient(env, login, { + eventType: "pull_request_merged", + limit: limit ?? CONTRIBUTOR_PR_OUTCOMES_DEFAULT_LIMIT, + }); + const outcomes: ContributorPrOutcome[] = deliveries.map((delivery) => ({ + repoFullName: delivery.repoFullName, + pullNumber: delivery.pullNumber, + outcome: "merged", + attribution: delivery.body, + deeplink: delivery.deeplink, + recordedAt: delivery.createdAt, + })); + return { login: login.toLowerCase(), count: outcomes.length, outcomes }; +} diff --git a/test/unit/contributor-pr-outcomes.test.ts b/test/unit/contributor-pr-outcomes.test.ts new file mode 100644 index 0000000000..b5fc7c650c --- /dev/null +++ b/test/unit/contributor-pr-outcomes.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from "vitest"; + +import { createApp } from "../../src/api/routes"; +import { insertNotificationDeliveryIfAbsent } from "../../src/db/repositories"; +import { buildContributorPrOutcomes } from "../../src/signals/contributor-pr-outcomes"; +import { createTestEnv } from "../helpers/d1"; + +async function seedMerged(env: Env, login: string, dedupKey: string, pullNumber: number, body: string): Promise { + await insertNotificationDeliveryIfAbsent(env, { + dedupKey, + channel: "badge", + recipientLogin: login, + eventType: "pull_request_merged", + repoFullName: "owner/repo", + pullNumber, + title: `Merged: owner/repo#${pullNumber}`, + body, + deeplink: `https://github.com/owner/repo/pull/${pullNumber}`, + actorLogin: login, + }); +} + +describe("buildContributorPrOutcomes (#6747)", () => { + it("maps a contributor's pull_request_merged deliveries to outcomes (newest first, login lowercased), excluding other event types", async () => { + const env = createTestEnv(); + await seedMerged(env, "Miner", "m1", 7, "PR #7 merged."); + await seedMerged(env, "Miner", "m2", 8, "PR #8 merged."); + // A non-merge delivery for the same login must NOT surface as an outcome. + await insertNotificationDeliveryIfAbsent(env, { + dedupKey: "changes:1", + channel: "badge", + recipientLogin: "Miner", + eventType: "pull_request_changes_requested", + repoFullName: "owner/repo", + pullNumber: 9, + title: "Changes requested", + body: "A reviewer requested changes.", + deeplink: "https://github.com/owner/repo/pull/9", + actorLogin: "reviewer", + }); + + const result = await buildContributorPrOutcomes(env, "Miner"); + expect(result.login).toBe("miner"); + expect(result.count).toBe(2); + expect(result.outcomes.map((o) => o.pullNumber)).toEqual([8, 7]); // deliveries are returned newest-first + expect(result.outcomes[0]).toMatchObject({ + repoFullName: "owner/repo", + pullNumber: 8, + outcome: "merged", + attribution: "PR #8 merged.", + deeplink: "https://github.com/owner/repo/pull/8", + }); + expect(typeof result.outcomes[0]!.recordedAt).toBe("string"); + }); + + it("returns an empty history for a contributor with no merged deliveries", async () => { + const env = createTestEnv(); + expect(await buildContributorPrOutcomes(env, "nobody")).toEqual({ login: "nobody", count: 0, outcomes: [] }); + }); + + it("honors an explicit limit", async () => { + const env = createTestEnv(); + for (let i = 1; i <= 3; i += 1) await seedMerged(env, "miner", `k${i}`, i, `PR #${i} merged.`); + expect((await buildContributorPrOutcomes(env, "miner", 2)).count).toBe(2); + }); +}); + +describe("GET /v1/contributors/:login/pr-outcomes (#6747) — REST mirror", () => { + it("returns byte-for-byte what the shared builder produces (parity with the MCP surface)", async () => { + const env = createTestEnv(); + await seedMerged(env, "miner", "r1", 7, "PR #7 merged."); + await seedMerged(env, "miner", "r2", 8, "PR #8 merged."); + const app = createApp(); + + // Operator (api) token: the contributor's private surface is reached over HTTP via a trusted token, not a + // browser session (contributor routes aren't in the session path allowlist). + const res = await app.request( + "/v1/contributors/miner/pr-outcomes", + { headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` } }, + env, + ); + expect(res.status).toBe(200); + expect(await res.json()).toEqual(await buildContributorPrOutcomes(env, "miner", 50)); + }); + + it("honors the limit query parameter", async () => { + const env = createTestEnv(); + for (let i = 1; i <= 3; i += 1) await seedMerged(env, "miner", `q${i}`, i, `PR #${i} merged.`); + const app = createApp(); + const res = await app.request( + "/v1/contributors/miner/pr-outcomes?limit=2", + { headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` } }, + env, + ); + expect(res.status).toBe(200); + expect(((await res.json()) as { count: number }).count).toBe(2); + }); + + it("refuses the shared, end-user MCP token reading an arbitrary contributor (forbidden_contributor)", async () => { + // A scoped MCP allowlist (not the wildcard opt-in) — the shared LOOPOVER_MCP_TOKEN must not read an + // arbitrary contributor's private history over HTTP, mirroring the MCP tool surface's own guard. + const env = createTestEnv({ MCP_READ_REPO_ALLOWLIST: "" }); + const app = createApp(); + const res = await app.request( + "/v1/contributors/miner/pr-outcomes", + { headers: { authorization: `Bearer ${env.LOOPOVER_MCP_TOKEN}` } }, + env, + ); + expect(res.status).toBe(403); + }); +}); diff --git a/test/unit/mcp-tool-rename-aliases.test.ts b/test/unit/mcp-tool-rename-aliases.test.ts index cb74d301b5..4582184841 100644 --- a/test/unit/mcp-tool-rename-aliases.test.ts +++ b/test/unit/mcp-tool-rename-aliases.test.ts @@ -3,6 +3,7 @@ // canonical loopover_-prefixed stdio tools are registered, none of their old gittensory_-prefixed // alias names resolve anymore, no description carries a stale deprecation notice, and the CLI's // `tools --json` listing stays in lockstep with what the live server actually registers. +// (#6747 registered the loopover_pr_outcome REST/CLI mirror, taking the count from 72 to 73.) // (#6754 registered the evaluate-escalation mirror, taking the count from 64 to 65.) // (#// (#6152 registered the 5 maintain-surface tools, taking the count from 42 to 47.) // (#6150 registered the local-scorer and plan-DAG/predict-gate tools, taking the count from 55 to 60.) @@ -57,14 +58,14 @@ describe("MCP legacy alias retirement (#4777) — discovery invariants", () => { }); afterEach(disconnect); - it("lists exactly 71 loopover_ tools and zero gittensory_-prefixed aliases", async () => { + it("lists exactly 73 loopover_ tools and zero gittensory_-prefixed aliases", async () => { const { tools } = await client.listTools(); const names = tools.map((t) => t.name); const primary = names.filter((n) => n.startsWith("loopover_")); const legacy = names.filter((n) => n.startsWith("gittensory_")); - expect(primary.length).toBe(71); + expect(primary.length).toBe(73); expect(legacy.length).toBe(0); - expect(names.length).toBe(71); + expect(names.length).toBe(73); }); it("no loopover_ tool's description carries a stale deprecation notice", async () => { @@ -74,11 +75,11 @@ describe("MCP legacy alias retirement (#4777) — discovery invariants", () => { } }); - it("`loopover-mcp tools --json` reports the same 71-tool count the live server registers", async () => { + it("`loopover-mcp tools --json` reports the same 73-tool count the live server registers", async () => { const { tools } = await client.listTools(); const payload = JSON.parse(run(["tools", "--json"])) as { count: number; tools: Array<{ name: string }> }; expect(payload.count).toBe(tools.length); - expect(payload.count).toBe(71); + expect(payload.count).toBe(73); expect([...payload.tools.map((t) => t.name)].sort()).toEqual([...tools.map((t) => t.name)].sort()); }); });