Skip to content

Commit 1cfc847

Browse files
authored
fix(mcp): align read report tools with requireRepoAccess (#8383)
* fix(mcp): align read report tools with requireRepoAccess Closes #8338. getMaintainerNoise, getAmsMinerCohort, and getActivationPreview now use requireRepoAccess like their REST mirrors and sibling read tools, instead of the live-write requireRepoApprovalQueueAccess gate reserved for approval-queue writes. * test(mcp): expect requireRepoAccess denial on read report tools Update mcp-output-schemas member-deny cases for #8338 so they assert the requireRepoAccess message instead of the old approval-queue maintainer-access string (fixes shard failure / empty patch coverage).
1 parent ce5f5aa commit 1cfc847

3 files changed

Lines changed: 120 additions & 9 deletions

File tree

src/mcp/server.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3456,8 +3456,10 @@ export class LoopoverMcp {
34563456
}
34573457

34583458
private async getMaintainerNoise(input: { owner: string; repo: string }): Promise<ToolPayload> {
3459+
// (#8338) Mirrors GET /v1/repos/:owner/:repo/maintainer-noise: same requireRepoAccess gate as sibling
3460+
// read-only maintainer reports (and REST requireRepoMaintainer) — not the live-write approval-queue gate.
34593461
const fullName = `${input.owner}/${input.repo}`;
3460-
await this.requireRepoApprovalQueueAccess(fullName);
3462+
await this.requireRepoAccess(fullName);
34613463
const report = await loadMaintainerNoiseReport(this.env, fullName);
34623464
return {
34633465
summary: maintainerNoiseSummary(report),
@@ -3466,10 +3468,11 @@ export class LoopoverMcp {
34663468
}
34673469

34683470
private async getAmsMinerCohort(input: { owner: string; repo: string }): Promise<ToolPayload> {
3469-
// Mirrors GET /v1/repos/:owner/:repo/ams-miner-cohort: same maintainer gate as getMaintainerNoise
3470-
// (requireRepoApprovalQueueAccess) and the same buildAmsMinerCohortComparison service the REST route uses.
3471+
// (#8338) Mirrors GET /v1/repos/:owner/:repo/ams-miner-cohort: same requireRepoAccess gate as
3472+
// getMaintainerNoise / other read-only maintainer reports, and the same buildAmsMinerCohortComparison
3473+
// service the REST route uses.
34713474
const fullName = `${input.owner}/${input.repo}`;
3472-
await this.requireRepoApprovalQueueAccess(fullName);
3475+
await this.requireRepoAccess(fullName);
34733476
const report = await buildAmsMinerCohortComparison(this.env, fullName);
34743477
// Single summary template (no present-branch) so patch coverage stays complete under the 99% gate; the
34753478
// structured payload still carries `present` for clients that need the empty vs populated distinction.
@@ -3492,12 +3495,13 @@ export class LoopoverMcp {
34923495
};
34933496
}
34943497

3495-
// (#7799) MCP surface for GET /v1/repos/:owner/:repo/activation-preview. Assembles the same inputs the REST
3498+
// (#7799/#8338) MCP surface for GET /v1/repos/:owner/:repo/activation-preview. Same requireRepoAccess gate
3499+
// as sibling read-only maintainer reports (REST requireRepoMaintainer). Assembles the same inputs the REST
34963500
// route does (getRepository + resolveRepositorySettings + listPullRequests) and defers to the guarded
34973501
// buildMaintainerActivationPreview service. Deterministic and advisory-only -- never runs AI.
34983502
private async getActivationPreview(input: { owner: string; repo: string }): Promise<ToolPayload> {
34993503
const fullName = `${input.owner}/${input.repo}`;
3500-
await this.requireRepoApprovalQueueAccess(fullName);
3504+
await this.requireRepoAccess(fullName);
35013505
const [repo, settings, pullRequests] = await Promise.all([
35023506
getRepository(this.env, fullName),
35033507
resolveRepositorySettings(this.env, fullName),

test/unit/mcp-output-schemas.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,8 @@ describe("MCP tool calls return schema-valid structured content", () => {
343343
});
344344
const result = await client.callTool({ name: "loopover_get_ams_miner_cohort", arguments: { owner: "victim-org", repo: "private-repo" } });
345345
expect(result.isError).toBe(true);
346-
expect(JSON.stringify(result.content)).toContain("maintainer access is required");
346+
// (#8338) requireRepoAccess denial — matches sibling read tools (not the live-write approval-queue message).
347+
expect(JSON.stringify(result.content)).toContain("session cannot access this repository");
347348
expect(result.structuredContent).toBeUndefined();
348349
});
349350

@@ -515,7 +516,8 @@ describe("MCP tool calls return schema-valid structured content", () => {
515516
const result = await client.callTool({ name: "loopover_get_activation_preview", arguments: { owner: "victim-org", repo: "private-repo" } });
516517

517518
expect(result.isError).toBe(true);
518-
expect(JSON.stringify(result.content)).toContain("maintainer access is required");
519+
// (#8338) requireRepoAccess denial — matches sibling read tools (not the live-write approval-queue message).
520+
expect(JSON.stringify(result.content)).toContain("session cannot access this repository");
519521
expect(result.structuredContent).toBeUndefined();
520522
});
521523

@@ -547,7 +549,8 @@ describe("MCP tool calls return schema-valid structured content", () => {
547549
const result = await client.callTool({ name: "loopover_get_maintainer_noise", arguments: { owner: "victim-org", repo: "private-repo" } });
548550

549551
expect(result.isError).toBe(true);
550-
expect(JSON.stringify(result.content)).toContain("maintainer access is required");
552+
// (#8338) MEMBER on a non-installed repo does not grant requireRepoAccess scope; message matches sibling reads.
553+
expect(JSON.stringify(result.content)).toContain("session cannot access this repository");
551554
expect(result.structuredContent).toBeUndefined();
552555
});
553556

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
2+
import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
3+
import { beforeEach, describe, expect, it, vi } from "vitest";
4+
import { LoopoverMcp } from "../../src/mcp/server";
5+
import { getRepositoryCollaboratorPermission } from "../../src/github/app";
6+
import { upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories";
7+
import type { AuthIdentity } from "../../src/auth/security";
8+
import { createTestEnv } from "../helpers/d1";
9+
10+
// #8338: getMaintainerNoise / getAmsMinerCohort / getActivationPreview must use requireRepoAccess
11+
// (cached maintainer/owner/operator scope), matching their REST mirrors and sibling read tools — not the
12+
// live-write requireRepoApprovalQueueAccess gate reserved for approval-queue / write tools.
13+
14+
vi.mock("../../src/github/app", async (importOriginal) => ({
15+
...(await importOriginal<typeof import("../../src/github/app")>()),
16+
getRepositoryCollaboratorPermission: vi.fn(),
17+
createInstallationToken: vi.fn(async () => "test-installation-token"),
18+
}));
19+
20+
const mockedPermission = vi.mocked(getRepositoryCollaboratorPermission);
21+
22+
const READ_REPORT_TOOLS = ["loopover_get_maintainer_noise", "loopover_get_ams_miner_cohort", "loopover_get_activation_preview"] as const;
23+
24+
beforeEach(() => {
25+
mockedPermission.mockReset();
26+
// Fail closed on live collaborator lookup — the regression is that these three tools used to deny
27+
// when this failed even though cached maintainer scope was enough for every sibling read tool.
28+
mockedPermission.mockRejectedValue(new Error("github collaborator lookup unavailable"));
29+
});
30+
31+
async function connect(env: Env, identity?: AuthIdentity) {
32+
const server = (identity ? new LoopoverMcp(env, identity) : new LoopoverMcp(env)).createServer();
33+
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
34+
await server.connect(serverTransport);
35+
const client = new Client({ name: "loopover-read-report-gate-test", version: "0.1.0" }, { capabilities: {} });
36+
await client.connect(clientTransport);
37+
return client;
38+
}
39+
40+
async function seedOwnedRepo(env: Env): Promise<void> {
41+
await upsertInstallation(env, {
42+
installation: {
43+
id: 5,
44+
account: { login: "owner", id: 1, type: "User" },
45+
repository_selection: "selected",
46+
permissions: { metadata: "read", contents: "read", pull_requests: "read", issues: "read" },
47+
events: ["pull_request"],
48+
},
49+
repositories: [{ name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }],
50+
});
51+
await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5);
52+
}
53+
54+
describe("MCP read-only maintainer report gate parity (#8338)", () => {
55+
it("REGRESSION (#8338): cached maintainer scope succeeds even when live collaborator lookup fails", async () => {
56+
const env = createTestEnv();
57+
await seedOwnedRepo(env);
58+
// Cached COLLABORATOR association grants maintainer scope via canLoginAccessRepo / requireRepoAccess,
59+
// but is intentionally insufficient for requireRepoApprovalQueueAccess (live write required).
60+
await upsertPullRequestFromGitHub(env, "owner/repo", {
61+
number: 7,
62+
title: "x",
63+
state: "open",
64+
user: { login: "reader" },
65+
author_association: "COLLABORATOR",
66+
head: { sha: "sha" },
67+
});
68+
69+
const client = await connect(env, { kind: "session", actor: "reader" } as AuthIdentity);
70+
for (const name of READ_REPORT_TOOLS) {
71+
const result = await client.callTool({ name, arguments: { owner: "owner", repo: "repo" } });
72+
expect(result.isError, name).toBeFalsy();
73+
const text = JSON.stringify(result);
74+
expect(text, name).toContain("owner/repo");
75+
expect(text, name).not.toMatch(/wallet|hotkey|raw trust|payout|reward estimate/i);
76+
}
77+
// Live lookup must not be required for these read tools after the fix (sibling write tools still use it).
78+
expect(mockedPermission).not.toHaveBeenCalled();
79+
});
80+
81+
it("rejects a session with no cached maintainer/owner scope on all three tools", async () => {
82+
const env = createTestEnv();
83+
await seedOwnedRepo(env);
84+
85+
const client = await connect(env, { kind: "session", actor: "rando" } as AuthIdentity);
86+
for (const name of READ_REPORT_TOOLS) {
87+
const result = await client.callTool({ name, arguments: { owner: "owner", repo: "repo" } });
88+
expect(result.isError, name).toBe(true);
89+
expect(JSON.stringify(result), name).toMatch(/Forbidden: session cannot access this repository/i);
90+
}
91+
});
92+
93+
it("allows the owning session when live collaborator lookup is unavailable", async () => {
94+
const env = createTestEnv();
95+
await seedOwnedRepo(env);
96+
97+
const client = await connect(env, { kind: "session", actor: "owner" } as AuthIdentity);
98+
for (const name of READ_REPORT_TOOLS) {
99+
const result = await client.callTool({ name, arguments: { owner: "owner", repo: "repo" } });
100+
expect(result.isError, name).toBeFalsy();
101+
}
102+
expect(mockedPermission).not.toHaveBeenCalled();
103+
});
104+
});

0 commit comments

Comments
 (0)