From 3da0fd1c00d8dd592cd03ae73d8bb8f2fbc7cc67 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Tue, 10 Nov 2026 21:36:01 +0100 Subject: [PATCH 1/2] fix(pr-review): a pull request session reads its own repository A review pinned its diff tools to the pull request, but the sandbox and source tools read whichever connected repository the model named, and a reply session pinned nothing. Both now hold every repository-naming tool to the pull request's repository. A session whose pull request cannot be looked up is offered none of them, and a review no longer lists the other repositories. --- apps/ai/src/chat/code-mode.ts | 1 + apps/ai/src/chat/maple-toolkit.test.ts | 56 ++++++++++++++++++- apps/ai/src/chat/maple-toolkit.ts | 32 +++++++++-- apps/ai/src/chat/permissions.ts | 1 - apps/ai/src/chat/run.ts | 9 ++- apps/ai/src/chat/turn-runner.ts | 33 +++++++++-- .../PrReviewConversationService.test.ts | 12 ++++ .../pr-review/PrReviewConversationService.ts | 25 +++++++++ 8 files changed, 151 insertions(+), 18 deletions(-) diff --git a/apps/ai/src/chat/code-mode.ts b/apps/ai/src/chat/code-mode.ts index 09a9fdd179..ac998f28cf 100644 --- a/apps/ai/src/chat/code-mode.ts +++ b/apps/ai/src/chat/code-mode.ts @@ -51,6 +51,7 @@ export const CODE_MODE_TOOLS: ReadonlySet = new Set([ "list_integrations", "list_security_audit_runs", "list_security_findings", + "list_source_repositories", "mine_log_patterns", "query_data", "run_sql", diff --git a/apps/ai/src/chat/maple-toolkit.test.ts b/apps/ai/src/chat/maple-toolkit.test.ts index 12930d3a99..d2261281ab 100644 --- a/apps/ai/src/chat/maple-toolkit.test.ts +++ b/apps/ai/src/chat/maple-toolkit.test.ts @@ -58,7 +58,7 @@ describe("buildChatToolkit", () => { ]) }) - it("leaves every other tool's arguments as the model sent them", async () => { + it("reads the session's repository whatever repository the model names", async () => { const { executor, calls } = recordingExecutor() const { handlers } = buildChatToolkit( executor, @@ -73,8 +73,60 @@ describe("buildChatToolkit", () => { const read = { repository: "MapleTechLabs/other", ref: "main", path: "a.ts" } assert.isDefined(handlers.sandbox_read_file) + assert.isDefined(handlers.search_source_code) await Effect.runPromise(handlers.sandbox_read_file(read, {})) + await Effect.runPromise( + handlers.search_source_code({ repository: "octo/private", query: "token" }, {}), + ) + + assert.deepEqual(calls, [ + { name: "sandbox_read_file", params: { ...read, repository: PULL_REQUEST.repository } }, + { name: "search_source_code", params: { repository: PULL_REQUEST.repository, query: "token" } }, + ]) + }) + + it("leaves a telemetry tool's arguments as the model sent them", async () => { + const { executor, calls } = recordingExecutor() + const { handlers } = buildChatToolkit( + executor, + tenantFor(human), + PR_REVIEW_RULESET, + "chat", + undefined, + undefined, + undefined, + PULL_REQUEST, + ) + + assert.isDefined(handlers.list_services) + await Effect.runPromise(handlers.list_services({ environment: "production" }, {})) + + assert.deepEqual(calls, [{ name: "list_services", params: { environment: "production" } }]) + }) + + it("offers no tool that names a repository when the pull request could not be read", () => { + const { executor } = recordingExecutor() + const { handlers } = buildChatToolkit( + executor, + tenantFor(human), + PR_REVIEW_RULESET, + "chat", + undefined, + undefined, + undefined, + "unreadable", + ) + + assert.isUndefined(handlers.pr_file_diff) + assert.isUndefined(handlers.sandbox_exec) + assert.isUndefined(handlers.read_source_file) + assert.isDefined(handlers.list_services) + }) + + it("does not let a review list the organization's other repositories", () => { + const { executor } = recordingExecutor() + const { handlers } = buildChatToolkit(executor, tenantFor(human), PR_REVIEW_RULESET, "chat") - assert.deepEqual(calls, [{ name: "sandbox_read_file", params: read }]) + assert.isUndefined(handlers.list_source_repositories) }) }) diff --git a/apps/ai/src/chat/maple-toolkit.ts b/apps/ai/src/chat/maple-toolkit.ts index 54b5864a67..27f089099b 100644 --- a/apps/ai/src/chat/maple-toolkit.ts +++ b/apps/ai/src/chat/maple-toolkit.ts @@ -10,14 +10,32 @@ import type { McpToolSurface } from "@maple/domain/mcp-manifest" import type { McpToolExecutorApi } from "../mcp/dispatcher" import { buildMapleToolkit, type ToolUiPayload, type UiSource } from "../mcp/tools/llm-tools" -/** The pull request a review session is about, which its diff tools always read. */ +/** The pull request a review or reply session is about: the one its tools read, whatever the model names. */ export interface SessionPullRequest { readonly repository: string readonly number: number } +/** + * What a session about a pull request may read. `unreadable` is one whose pull request could not + * be looked up: it is offered no tool that names a repository, rather than every repository. + */ +export type SessionRepositoryScope = SessionPullRequest | "unreadable" + const PULL_REQUEST_TOOLS: ReadonlySet = new Set(["pr_changed_files", "pr_context", "pr_file_diff"]) +/** Tools that read whichever connected repository their `repository` argument names. */ +const REPOSITORY_TOOLS: ReadonlySet = new Set([ + "read_source_file", + "sandbox_exec", + "sandbox_grep", + "sandbox_list_files", + "sandbox_read_file", + "search_source_code", +]) + +const namesRepository = (tool: string): boolean => PULL_REQUEST_TOOLS.has(tool) || REPOSITORY_TOOLS.has(tool) + /** * All Maple tools, with mutating ones gated. * @@ -33,23 +51,27 @@ export const buildChatToolkit = ( sessionAttributes?: Readonly>, onAnswer?: (tool: string, answer: string) => void, onUi?: (toolCallId: string, ui: ToolUiPayload, call: UiSource) => void, - pullRequest?: SessionPullRequest, + pullRequest?: SessionRepositoryScope, ) => buildMapleToolkit(executor, tenant, { surface, ...(sessionAttributes === undefined ? undefined : { sessionAttributes }), ...(onAnswer === undefined ? undefined : { onAnswer }), ...(onUi === undefined ? undefined : { onUi }), - ...(pullRequest === undefined + ...(pullRequest === undefined || pullRequest === "unreadable" ? undefined : { pinned: (name: string) => PULL_REQUEST_TOOLS.has(name) ? { repository: pullRequest.repository, number: pullRequest.number } - : undefined, + : REPOSITORY_TOOLS.has(name) + ? { repository: pullRequest.repository } + : undefined, }), // `deny` means the model never sees the tool. That is a stronger guarantee than refusing the // call afterwards, and it is free — an unoffered tool cannot be called. - include: (name) => evaluatePermission(ruleset, name) !== "deny", + include: (name) => + evaluatePermission(ruleset, name) !== "deny" && + !(pullRequest === "unreadable" && namesRepository(name)), gate: (name) => evaluatePermission(ruleset, name) === "ask", }) diff --git a/apps/ai/src/chat/permissions.ts b/apps/ai/src/chat/permissions.ts index 2848a2272e..4397c6d12a 100644 --- a/apps/ai/src/chat/permissions.ts +++ b/apps/ai/src/chat/permissions.ts @@ -65,7 +65,6 @@ export const PR_REVIEW_TOOLS: ReadonlyArray = [ "sandbox_list_files", "sandbox_read_file", "sandbox_exec", - "list_source_repositories", "search_source_code", "read_source_file", "list_services", diff --git a/apps/ai/src/chat/run.ts b/apps/ai/src/chat/run.ts index 03984f9c47..3477d22bb8 100644 --- a/apps/ai/src/chat/run.ts +++ b/apps/ai/src/chat/run.ts @@ -45,7 +45,7 @@ import { } from "./events" import { buildReviewFanout } from "./review/fanout" import { buildCodeMode, CODE_MODE_GUIDANCE, CODE_MODE_TOOLS } from "./code-mode" -import { buildChatToolkit, type SessionPullRequest } from "./maple-toolkit" +import { buildChatToolkit, type SessionRepositoryScope } from "./maple-toolkit" import { accumulateUsage, type RunUsage } from "./usage" import type { MakeCompletion } from "./completion" import type { TenantContext } from "@maple/domain/http" @@ -111,11 +111,10 @@ export interface ChatRunInput { /** This run is an autonomous pass's close-out: a report it files is a partial. */ readonly closeOut?: boolean /** - * The pull request a review session is about. Its diff tools read this one whatever - * repository or number the model passes: a wrong guess there failed the call, and enough of - * those stopped the review. + * The pull request a review or reply session is about. Its diff and repository tools read + * this one whatever repository or number the model passes; `unreadable` withholds them. */ - readonly pullRequest?: SessionPullRequest + readonly pullRequest?: SessionRepositoryScope /** The agent to run as; defaults to the session's. The local review runner passes a variant. */ readonly agent?: AgentDefinition /** Runs `run_code` programs for an agent that offers it; absent, the tool is not offered. */ diff --git a/apps/ai/src/chat/turn-runner.ts b/apps/ai/src/chat/turn-runner.ts index 1e6809b097..580412114f 100644 --- a/apps/ai/src/chat/turn-runner.ts +++ b/apps/ai/src/chat/turn-runner.ts @@ -54,6 +54,7 @@ import { type PhaseTurn, runAutonomousPhases } from "./autonomous-phases" import { makeUnfinishedSettler, runAutonomousTurn, settleAbandonedTurn, settleRow } from "./autonomous-settle" import { AGENTS, isAutonomousTurn, sessionOf } from "./agents" import { profileForTurn } from "./profiles" +import type { SessionRepositoryScope } from "./maple-toolkit" import { runChatTurn } from "./run" import { toTenantContext, turnTenant } from "./turn-actor" import { dataSourcesPromptSection } from "../mcp/lib/data-sources" @@ -361,7 +362,7 @@ export const runChatSessionTurn = async (input: RunChatSessionTurnInput): Promis input.session.endTurn(input.messageId, { reason: "error", error: refusal.value.message }) return yield* annotateTurn() } - // Read before the run: the diff tools are pinned to it. An unreadable target reviews unpinned. + // Read before the run: the diff and repository tools are held to it. const target = prReviewId === undefined ? Option.none() @@ -376,6 +377,31 @@ export const runChatSessionTurn = async (input: RunChatSessionTurnInput): Promis ), ), ) + const replyTarget = + prReplyId === undefined + ? Option.none() + : yield* conversations.replyTarget(tenant.orgId, prReplyId).pipe( + Effect.catch((error) => + Effect.logInfo("Could not read the reply's target").pipe( + Effect.annotateLogs({ + sessionId: input.sessionId, + error: error.message, + }), + Effect.as(Option.none()), + ), + ), + ) + // A session about a pull request reads that repository, or none when it cannot be named. + const pullRequest: SessionRepositoryScope | undefined = + prReviewId === undefined && prReplyId === undefined + ? undefined + : Option.match( + Option.orElse(target, () => replyTarget), + { + onNone: (): SessionRepositoryScope => "unreadable", + onSome: ({ repository, number }) => ({ repository, number }), + }, + ) // Clone the commit while the model reads the diff, rather than when its first source tool // asks and waits on it. A child of the turn (no request Scope here, so `forkRequestScoped` // parents it on this fiber): a clone still running when the turn ends carries on in the container. @@ -564,10 +590,7 @@ export const runChatSessionTurn = async (input: RunChatSessionTurnInput): Promis ...(turn.closeOut === true ? { closeOut: true } : undefined), ...(turn.agent === undefined ? undefined : { agent: turn.agent }), ...(codeExecutor === undefined ? undefined : { codeExecutor }), - ...Option.match(target, { - onNone: () => undefined, - onSome: ({ repository, number }) => ({ pullRequest: { repository, number } }), - }), + ...(pullRequest === undefined ? undefined : { pullRequest }), text: turn.text, history: turn.history, usage, diff --git a/packages/backend/src/services/pr-review/PrReviewConversationService.test.ts b/packages/backend/src/services/pr-review/PrReviewConversationService.test.ts index a516bb0d90..ad23986927 100644 --- a/packages/backend/src/services/pr-review/PrReviewConversationService.test.ts +++ b/packages/backend/src/services/pr-review/PrReviewConversationService.test.ts @@ -365,6 +365,18 @@ describe("PrReviewConversationService", () => { assert.deepEqual(recorded.posted, [ { body: "Because two writers race on the cursor.", threadRootId: "800" }, ]) + // The session's tools are held to this pull request, whatever its comments name. + assert.deepEqual( + yield* conversations.replyTarget(orgId, outcome.replyId!), + Option.some({ + repository: "octo/repo", + number: 612, + }), + ) + assert.deepEqual( + yield* conversations.replyTarget(asOrgId("org_other"), outcome.replyId!), + Option.none(), + ) // Nothing staged on an ask, and a second submission posts nothing. assert.include( yield* conversations.stageEdit(orgId, outcome.replyId!, { diff --git a/packages/backend/src/services/pr-review/PrReviewConversationService.ts b/packages/backend/src/services/pr-review/PrReviewConversationService.ts index 6d8018f484..dcb595c338 100644 --- a/packages/backend/src/services/pr-review/PrReviewConversationService.ts +++ b/packages/backend/src/services/pr-review/PrReviewConversationService.ts @@ -87,6 +87,14 @@ export interface PrReviewConversationServiceApi { * pull request's head is still the run's, its review starts as a trusted author's would. Never fails. */ readonly onCiApproved: (orgId: OrgId, job: PullRequestCiApprovedJob) => Effect.Effect + /** The pull request a reply answers on, which its session's tools are held to. */ + readonly replyTarget: ( + orgId: OrgId, + replyId: PrReviewReplyId, + ) => Effect.Effect< + Option.Option<{ readonly repository: string; readonly number: number }>, + PrReviewPersistenceError + > /** Post the agent's answer, committing a `fix`'s staged edits first. */ readonly submitReply: ( orgId: OrgId, @@ -303,6 +311,22 @@ export class PrReviewConversationService extends Context.Service< ), ) + const replyTarget: PrReviewConversationServiceApi["replyTarget"] = Effect.fn( + "PrReviewConversationService.replyTarget", + )(function* (orgId, replyId) { + const row = yield* getReply(orgId, replyId).pipe( + Effect.asSome, + Effect.catchTag("@maple/http/pr-review/PrReviewReplyNotFoundError", () => + Effect.succeed(Option.none()), + ), + ) + if (Option.isNone(row)) return Option.none() + const repo = yield* repositories + .getRepositoryById(orgId, row.value.repositoryId) + .pipe(Effect.mapError(toPersistence)) + return Option.map(repo, ({ fullName }) => ({ repository: fullName, number: row.value.number })) + }) + /** Post on the pull request where the row says; a failure is logged, never thrown into the turn. */ const post = Effect.fn("PrReviewConversationService.post")(function* ( orgId: OrgId, @@ -840,6 +864,7 @@ export class PrReviewConversationService extends Context.Service< return { onPullRequestComment, onCiApproved, + replyTarget, submitReply, stageEdit, failReply, From 8896f533de5bed81faee0668ecdc18ec6175cb34 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Tue, 10 Nov 2026 21:47:32 +0100 Subject: [PATCH 2/2] fix(pr-review): fail a pass whose pull request cannot be read, refuse spaced search paths An unattended pass with no readable pull request now fails as a pass that could not start, instead of running without its diff tools. A code search path with whitespace is refused, since the path is written into the search query as qualifiers. --- apps/ai/src/chat/turn-runner.ts | 9 +++++++++ apps/ai/src/mcp/tools/source-code.test.ts | 20 ++++++++++++++++++++ apps/ai/src/mcp/tools/source-code.ts | 5 +++-- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/apps/ai/src/chat/turn-runner.ts b/apps/ai/src/chat/turn-runner.ts index 580412114f..cd45bc1af5 100644 --- a/apps/ai/src/chat/turn-runner.ts +++ b/apps/ai/src/chat/turn-runner.ts @@ -100,6 +100,7 @@ const METERING_TIMEOUT = "5 seconds" /** What the row records when the pass and its close-out both ended in prose or on an error. */ const NO_DIAGNOSIS_ERROR = "no_diagnosis: the agent ended its pass without submitting a diagnosis; retry" const NO_REPLY_ERROR = "no_reply: the agent ended its pass without submitting an answer" +const PULL_REQUEST_UNREADABLE = "The pull request this session is about could not be read." /** * Meter what this turn spent into the org's AI usage. @@ -402,6 +403,14 @@ export const runChatSessionTurn = async (input: RunChatSessionTurnInput): Promis onSome: ({ repository, number }) => ({ repository, number }), }, ) + // An unattended pass that cannot name its pull request has nothing it may read. + if (pullRequest === "unreadable" && autonomous) { + observability.outcome = "error" + yield* failUnfinished("start_failed", PULL_REQUEST_UNREADABLE) + observability.failureReason = "PullRequestUnreadable" + input.session.endTurn(input.messageId, { reason: "error", error: PULL_REQUEST_UNREADABLE }) + return yield* annotateTurn() + } // Clone the commit while the model reads the diff, rather than when its first source tool // asks and waits on it. A child of the turn (no request Scope here, so `forkRequestScoped` // parents it on this fiber): a clone still running when the turn ends carries on in the container. diff --git a/apps/ai/src/mcp/tools/source-code.test.ts b/apps/ai/src/mcp/tools/source-code.test.ts index c381db13f4..5c6d6747dd 100644 --- a/apps/ai/src/mcp/tools/source-code.test.ts +++ b/apps/ai/src/mcp/tools/source-code.test.ts @@ -131,6 +131,26 @@ describe("the source-code tools", () => { }), ) + it.effect("refuses a search path that would add a qualifier of its own", () => + Effect.gen(function* () { + const searched: Array = [] + const error = yield* Effect.flip( + call( + "search_source_code", + { repository: "octo/shop", query: "three", path: "src repo:octo/other" }, + { + searchCode: (_org, repo) => { + searched.push(repo) + return Effect.succeed([]) + }, + }, + ), + ) + assert.strictEqual(error._tag, "@maple/mcp/errors/McpInvalidInputError") + assert.deepStrictEqual(searched, []) + }), + ) + it("splits search paths and finds match lines", () => { assert.deepStrictEqual(splitSearchPath("apps/api/"), { dir: "apps/api" }) assert.deepStrictEqual(splitSearchPath("README.md"), { filename: "README.md" }) diff --git a/apps/ai/src/mcp/tools/source-code.ts b/apps/ai/src/mcp/tools/source-code.ts index ad91d25b2e..2dc3acf6ff 100644 --- a/apps/ai/src/mcp/tools/source-code.ts +++ b/apps/ai/src/mcp/tools/source-code.ts @@ -133,9 +133,10 @@ export function registerSourceCodeTools(server: McpToolRegistrar) { parameter: "query", }) } - if (path !== undefined && unsafePath(path)) { + // The path becomes search qualifiers, so a space in it would start a qualifier of its own. + if (path !== undefined && (unsafePath(path) || /\s/.test(Str.trim(path)))) { return yield* new McpInvalidInputError({ - message: "path must be repository-relative", + message: "path must be repository-relative and contain no whitespace", parameter: "path", }) }