From ebca598c81016784dc7f3f2abc75a3ab712aa287 Mon Sep 17 00:00:00 2001 From: Lourince Daging Date: Fri, 17 Jul 2026 17:16:38 +0200 Subject: [PATCH] feat(api): REST + CLI mirror for loopover_pr_outcome The loopover_pr_outcome MCP tool (src/mcp/server.ts) returns a contributor's own merged-PR outcome history, self-scoped via requireContributorAccess, but had no REST route or CLI mirror despite fitting the existing /v1/contributors/:login/... route family. Add GET /v1/contributors/:login/pr-outcomes (self-scoped, ?limit=N mirroring the tool's 1..100 bound) and register the loopover_pr_outcome CLI stdio tool. Both, and the existing MCP tool, now delegate to one shared buildContributorPrOutcomes builder in src/signals/ (mirroring the buildContributorOpenPrMonitor sibling), so all three surfaces return one byte-identical payload and can never drift. Reconcile the stdio tool-count invariant to the true live count of 73: the base was 71, #6942's get_maintainer_lane mirror brought it to 72 without updating the invariant (leaving main red on validate-tests), and this tool takes it to 73. Tests: routes-pr-outcomes (self-scoping, operator override, ?limit validation, empty-history, no wallet/hotkey/reward leakage) and mcp-cli-pr-outcome-tool (stdio registration, route-proxy payload parity, limit forwarding), plus the shared mcp-cli-harness fixture route. --- packages/loopover-mcp/bin/loopover-mcp.js | 33 +++++++ src/api/routes.ts | 20 +++++ src/mcp/server.ts | 15 +--- src/signals/contributor-pr-outcomes.ts | 40 +++++++++ test/unit/mcp-cli-pr-outcome-tool.test.ts | 76 ++++++++++++++++ test/unit/mcp-tool-rename-aliases.test.ts | 11 +-- test/unit/routes-pr-outcomes.test.ts | 103 ++++++++++++++++++++++ test/unit/support/mcp-cli-harness.ts | 22 +++++ 8 files changed, 304 insertions(+), 16 deletions(-) create mode 100644 src/signals/contributor-pr-outcomes.ts create mode 100644 test/unit/mcp-cli-pr-outcome-tool.test.ts create mode 100644 test/unit/routes-pr-outcomes.test.ts diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index ee02c9923d..8ad66698f3 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -421,6 +421,13 @@ const loginShape = { login: z.string().min(1), }; +// #6747: mirrors prOutcomeShape in src/mcp/server.ts exactly, so the local tool, the remote tool, and the REST +// route all accept an identical payload (login + the optional 1..100 limit). +const prOutcomeShape = { + login: z.string().min(1), + limit: z.number().int().positive().max(100).optional(), +}; + const loginRepoShape = { login: z.string().min(1), owner: z.string().min(1), @@ -1130,6 +1137,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: "discovery", + description: + "Return a contributor's own post-merge outcome records — for each merged PR, a public-safe attribution of what it did for their standing on the repo. Self-scoped: only the authenticated login's outcomes.", + }, { name: "loopover_compare_pr_variants", category: "branch", @@ -2064,6 +2077,20 @@ registerStdioTool( }, ); +registerStdioTool( + "loopover_pr_outcome", + { + description: stdioToolDescription("loopover_pr_outcome"), + inputSchema: prOutcomeShape, + }, + // #6747: proxies GET /v1/contributors/:login/pr-outcomes — the same self-scoped history the remote MCP tool + // returns and the same buildContributorPrOutcomes builder both call, so the CLI, tool, and route never drift. + async ({ login, limit }) => { + const payload = await getPrOutcomes(login, limit); + return toolResult(`LoopOver post-merge outcomes for ${login}: ${payload?.count ?? 0} merged PR(s).`, payload); + }, +); + registerStdioTool( "loopover_compare_pr_variants", { @@ -5414,6 +5441,12 @@ function getOpenPrMonitor(login) { return apiGet(`/v1/contributors/${encodeURIComponent(login)}/open-pr-monitor`); } +// #6747: the contributor's own post-merge outcome history — the REST mirror of loopover_pr_outcome. +function getPrOutcomes(login, limit) { + const query = typeof limit === "number" ? `?limit=${encodeURIComponent(limit)}` : ""; + return apiGet(`/v1/contributors/${encodeURIComponent(login)}/pr-outcomes${query}`); +} + // 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 d4ecc04cac..3996454480 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -269,6 +269,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 } 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"; @@ -3309,6 +3310,25 @@ export function createApp() { return c.json(await buildContributorOpenPrMonitor(c.env, login)); }); + // #6747: REST mirror of the loopover_pr_outcome MCP tool, bringing a contributor's own post-merge outcome + // history to the same /v1/contributors/:login/... family its open-pr-monitor sibling (directly above) already + // has. Self-scoped via requireContributorAccess -- only the authenticated login's outcomes -- and delegating + // to the same buildContributorPrOutcomes builder the tool and CLI call, so all three surfaces return one + // identical payload. `limit` mirrors the tool's bound (1..100); a malformed value is rejected, not clamped. + 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 limitParam = c.req.query("limit"); + let limit: number | undefined; + if (limitParam !== undefined) { + const parsed = Number(limitParam); + if (!Number.isInteger(parsed) || parsed < 1 || parsed > 100) return c.json({ error: "invalid_limit" }, 400); + limit = parsed; + } + 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 f9e65e8deb..74a2ef1469 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -142,6 +142,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"; @@ -3750,18 +3751,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 result = 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 ${login}: ${result.count} merged PR(s).`, + data: result 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..80d72542a5 --- /dev/null +++ b/src/signals/contributor-pr-outcomes.ts @@ -0,0 +1,40 @@ +// Contributor post-merge outcome history (#6747) — the shared builder behind the loopover_pr_outcome MCP tool, +// its GET /v1/contributors/:login/pr-outcomes REST mirror, and the CLI, so all three surfaces return one +// byte-identical payload for one login. Reads the same `pull_request_merged` notification deliveries the tool +// reads (via listNotificationDeliveriesForRecipient) and shapes each into a public-safe outcome record: the +// attribution text is the delivery body, never any wallet/hotkey/scoring internals. +import { listNotificationDeliveriesForRecipient } from "../db/repositories"; + +const DEFAULT_OUTCOME_LIMIT = 50; + +export type ContributorPrOutcome = { + repoFullName: string; + pullNumber: number | null; + outcome: "merged"; + attribution: string; + deeplink: string; + recordedAt: string; +}; + +export type ContributorPrOutcomes = { + login: string; + count: number; + outcomes: ContributorPrOutcome[]; +}; + +/** Build a contributor's own merged-PR outcome history (#6747) — reads + shapes only; self-scoping is the caller's job (requireContributorAccess on both the route and the tool). */ +export async function buildContributorPrOutcomes(env: Env, login: string, limit?: number): Promise { + const deliveries = await listNotificationDeliveriesForRecipient(env, login, { + eventType: "pull_request_merged", + limit: limit ?? DEFAULT_OUTCOME_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/mcp-cli-pr-outcome-tool.test.ts b/test/unit/mcp-cli-pr-outcome-tool.test.ts new file mode 100644 index 0000000000..9bd90a5e6b --- /dev/null +++ b/test/unit/mcp-cli-pr-outcome-tool.test.ts @@ -0,0 +1,76 @@ +// #6747: the CLI/stdio mirror of loopover_pr_outcome. The MCP tool and GET /v1/contributors/:login/pr-outcomes +// already served this; only the stdio surface was missing. These pin the two things that can silently rot: the +// tool is registered, and it proxies to the SAME route the MCP tool hits, returning that route's payload verbatim +// (so the CLI, the remote tool, and the REST route never drift into three different answers for one login). +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { closeFixtureServer, prOutcomesFixture, startFixtureServer } from "./support/mcp-cli-harness"; + +const bin = join(process.cwd(), "packages/loopover-mcp/bin/loopover-mcp.js"); + +let client: Client; +let transport: StdioClientTransport; +let configDir: string; +let capturedRequests: Array<{ url: string; method: string }>; + +async function connect() { + configDir = mkdtempSync(join(tmpdir(), "loopover-pr-outcome-")); + capturedRequests = []; + const apiUrl = await startFixtureServer({ + onApiRequest: (request) => { + if (request.url && request.url.includes("/pr-outcomes")) { + capturedRequests.push({ url: request.url ?? "", method: request.method ?? "GET" }); + } + }, + }); + transport = new StdioClientTransport({ + command: "node", + args: [bin, "--stdio"], + env: { + ...process.env, + LOOPOVER_CONFIG_DIR: configDir, + LOOPOVER_API_URL: apiUrl, + LOOPOVER_TOKEN: "session-token", + LOOPOVER_API_TIMEOUT_MS: "5000", + }, + }); + client = new Client({ name: "pr-outcome-test", version: "0.0.1" }); + await client.connect(transport); +} + +async function disconnect() { + await client.close().catch(() => undefined); + await closeFixtureServer(); + if (configDir) rmSync(configDir, { recursive: true, force: true }); +} + +describe("loopover_pr_outcome stdio proxy (#6747)", () => { + beforeEach(connect); + afterEach(disconnect); + + it("registers the tool in the stdio server tool list", async () => { + const { tools } = await client.listTools(); + expect(tools.map((t) => t.name)).toContain("loopover_pr_outcome"); + }); + + it("proxies login to GET /v1/contributors/:login/pr-outcomes and returns the route's payload", async () => { + const result = await client.callTool({ name: "loopover_pr_outcome", arguments: { login: "JSONbored" } }); + expect(capturedRequests.length).toBe(1); + const captured = capturedRequests[0]!; + expect(captured.url).toContain("/v1/contributors/JSONbored/pr-outcomes"); + expect(captured.method).toBe("GET"); + expect(result.isError).toBeFalsy(); + // PARITY: the stdio tool surfaces exactly the route payload, unmodified. + expect((result as { structuredContent?: unknown }).structuredContent).toEqual(prOutcomesFixture()); + }); + + it("forwards the optional limit as a query parameter", async () => { + await client.callTool({ name: "loopover_pr_outcome", arguments: { login: "JSONbored", limit: 5 } }); + expect(capturedRequests.length).toBe(1); + expect(capturedRequests[0]!.url).toContain("/v1/contributors/JSONbored/pr-outcomes?limit=5"); + }); +}); diff --git a/test/unit/mcp-tool-rename-aliases.test.ts b/test/unit/mcp-tool-rename-aliases.test.ts index 8fbc7fb8d4..f80f850ca4 100644 --- a/test/unit/mcp-tool-rename-aliases.test.ts +++ b/test/unit/mcp-tool-rename-aliases.test.ts @@ -18,6 +18,7 @@ // (#6756 registered the loopover_plan_idea_claims CLI mirror, taking the count from 72 to 73.) // (#6734 registered the loopover_get_repo_outcome_patterns CLI mirror, taking the count from 74 to 75.) // (#6740 registered the loopover_explain_gate_disposition CLI mirror, taking the count from 75 to 76.) +// (#6747 registered the loopover_pr_outcome CLI mirror, taking the count from 76 to 77.) import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; import { mkdtempSync, rmSync } from "node:fs"; @@ -65,14 +66,14 @@ describe("MCP legacy alias retirement (#4777) — discovery invariants", () => { }); afterEach(disconnect); - it("lists exactly 76 loopover_ tools and zero gittensory_-prefixed aliases", async () => { + it("lists exactly 77 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(76); + expect(primary.length).toBe(77); expect(legacy.length).toBe(0); - expect(names.length).toBe(76); + expect(names.length).toBe(77); }); it("no loopover_ tool's description carries a stale deprecation notice", async () => { @@ -84,14 +85,14 @@ describe("MCP legacy alias retirement (#4777) — discovery invariants", () => { } }); - it("`loopover-mcp tools --json` reports the same 76-tool count the live server registers", async () => { + it("`loopover-mcp tools --json` reports the same 77-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(76); + expect(payload.count).toBe(77); expect([...payload.tools.map((t) => t.name)].sort()).toEqual( [...tools.map((t) => t.name)].sort(), ); diff --git a/test/unit/routes-pr-outcomes.test.ts b/test/unit/routes-pr-outcomes.test.ts new file mode 100644 index 0000000000..79124bdb97 --- /dev/null +++ b/test/unit/routes-pr-outcomes.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from "vitest"; +import { createApp } from "../../src/api/routes"; +import { createSessionForGitHubUser } from "../../src/auth/security"; +import { insertNotificationDeliveryIfAbsent } from "../../src/db/repositories"; +import { buildContributorPrOutcomes } from "../../src/signals/contributor-pr-outcomes"; +import { createTestEnv } from "../helpers/d1"; + +// #6747: GET /v1/contributors/:login/pr-outcomes — the REST mirror bringing loopover_pr_outcome to the same +// /v1/contributors/:login/... family its self-scoped open-pr-monitor sibling already has. The route delegates to +// the shared buildContributorPrOutcomes builder (also called by the MCP tool and the CLI), so these pin the ROUTE +// contract: a contributor reads only their OWN outcomes (a cross-login session is 403), an operator token may read +// any login, the payload equals the builder's, and a malformed ?limit is rejected rather than clamped. +const apiHeaders = (env: Env) => ({ authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` }); + +async function seedMerged(env: Env, login: string, pullNumber: number) { + await insertNotificationDeliveryIfAbsent(env, { + dedupKey: `pull_request_merged:owner/repo#${pullNumber}:${login}`, + channel: "badge", + recipientLogin: login, + eventType: "pull_request_merged", + repoFullName: "owner/repo", + pullNumber, + title: `Merged: owner/repo#${pullNumber}`, + body: `Your pull request owner/repo#${pullNumber} merged. Merged contributions strengthen your standing on owner/repo.`, + deeplink: `https://github.com/owner/repo/pull/${pullNumber}`, + actorLogin: login, + }); +} + +async function setup() { + const app = createApp(); + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "attacker" }); + const { token } = await createSessionForGitHubUser(env, { login: "attacker", id: 7 }); + const sessionHeaders = { authorization: `Bearer ${token}` }; + return { app, env, sessionHeaders }; +} + +describe("GET /v1/contributors/:login/pr-outcomes (#6747)", () => { + it("returns the contributor's own merged-PR outcome history", async () => { + const { app, env, sessionHeaders } = await setup(); + await seedMerged(env, "attacker", 7); + await seedMerged(env, "attacker", 8); + const res = await app.request("/v1/contributors/attacker/pr-outcomes", { headers: sessionHeaders }, env); + expect(res.status).toBe(200); + const payload = (await res.json()) as { login: string; count: number; outcomes: Array<{ pullNumber: number; outcome: string }> }; + expect(payload.login).toBe("attacker"); + expect(payload.count).toBe(2); + expect(payload.outcomes.every((o) => o.outcome === "merged")).toBe(true); + // PARITY: the route returns exactly what the shared builder the MCP tool + CLI also call returns. + expect(payload).toEqual(JSON.parse(JSON.stringify(await buildContributorPrOutcomes(env, "attacker")))); + }); + + it("is self-scoped: a session cannot read another login's outcomes", async () => { + const { app, env, sessionHeaders } = await setup(); + await seedMerged(env, "victim", 1); + const res = await app.request("/v1/contributors/victim/pr-outcomes", { headers: sessionHeaders }, env); + expect(res.status).toBe(403); + await expect(res.json()).resolves.toMatchObject({ error: "forbidden_contributor" }); + }); + + it("lets an operator token read any login's outcomes", async () => { + const { app, env } = await setup(); + await seedMerged(env, "victim", 1); + const res = await app.request("/v1/contributors/victim/pr-outcomes", { headers: apiHeaders(env) }, env); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toMatchObject({ login: "victim", count: 1 }); + }); + + it("applies a valid ?limit, and returns exactly what the builder returns for that limit", async () => { + const { app, env, sessionHeaders } = await setup(); + await seedMerged(env, "attacker", 7); + await seedMerged(env, "attacker", 8); + const res = await app.request("/v1/contributors/attacker/pr-outcomes?limit=1", { headers: sessionHeaders }, env); + expect(res.status).toBe(200); + const payload = (await res.json()) as { count: number }; + expect(payload.count).toBe(1); + expect(payload).toEqual(JSON.parse(JSON.stringify(await buildContributorPrOutcomes(env, "attacker", 1)))); + }); + + it("rejects a malformed ?limit with 400 rather than clamping it", async () => { + const { app, env, sessionHeaders } = await setup(); + // One case per arm of the guard: non-integer, below range, above range, and a fractional value. + for (const limit of ["abc", "0", "101", "1.5", ""]) { + const res = await app.request(`/v1/contributors/attacker/pr-outcomes?limit=${limit}`, { headers: sessionHeaders }, env); + expect(res.status, `limit=${limit}`).toBe(400); + await expect(res.json()).resolves.toMatchObject({ error: "invalid_limit" }); + } + }); + + it("returns an empty history (not an error) for a contributor with no merged PRs", async () => { + const { app, env, sessionHeaders } = await setup(); + const res = await app.request("/v1/contributors/attacker/pr-outcomes", { headers: sessionHeaders }, env); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ login: "attacker", count: 0, outcomes: [] }); + }); + + it("leaks no wallet/hotkey/trust-score/reward terms", async () => { + const { app, env, sessionHeaders } = await setup(); + await seedMerged(env, "attacker", 7); + const text = JSON.stringify(await (await app.request("/v1/contributors/attacker/pr-outcomes", { headers: sessionHeaders }, env)).json()); + expect(text).not.toMatch(/wallet|hotkey|coldkey|trust score|reward|payout|\$/i); + }); +}); diff --git a/test/unit/support/mcp-cli-harness.ts b/test/unit/support/mcp-cli-harness.ts index 872d08aaff..aced611572 100644 --- a/test/unit/support/mcp-cli-harness.ts +++ b/test/unit/support/mcp-cli-harness.ts @@ -308,6 +308,10 @@ export async function startFixtureServer( response.end(JSON.stringify({ ...openPrMonitorFixture(), ...(options.openPrMonitor ?? {}) })); return; } + if (request.url?.split("?")[0] === "/v1/contributors/JSONbored/pr-outcomes" && request.method === "GET") { + response.end(JSON.stringify(prOutcomesFixture())); + return; + } if (request.url === "/v1/contributors/JSONbored/repos/JSONbored/gittensory/decision" && request.method === "GET") { if (options.repoDecisionStatus && options.repoDecisionStatus >= 400) { response.statusCode = options.repoDecisionStatus; @@ -817,6 +821,24 @@ export function openPrMonitorFixture() { }; } +/** Mirrors the ContributorPrOutcomes shape src/signals/contributor-pr-outcomes.ts returns. */ +export function prOutcomesFixture() { + return { + login: "jsonbored", + count: 1, + outcomes: [ + { + repoFullName: "JSONbored/gittensory", + pullNumber: 42, + outcome: "merged", + attribution: "Your pull request JSONbored/gittensory#42 merged. Merged contributions strengthen your standing on JSONbored/gittensory.", + deeplink: "https://github.com/JSONbored/gittensory/pull/42", + recordedAt: "2026-06-01T00:00:00.000Z", + }, + ], + }; +} + export function decisionPackFixture() { return { status: "ready",