From 18656a3402459d9d145bb3fa5767723531b27ac7 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 11 Oct 2026 03:29:26 -0700 Subject: [PATCH] feat(mcp): agents can write and read pull request review guides (#18143) Co-authored-by: Claude Opus 5.5 --- apps/server/src/mcp/McpHttpServer.test.ts | 2 + apps/server/src/mcp/McpToolAccess.test.ts | 8 + apps/server/src/mcp/McpToolAccess.ts | 8 + .../toolkits/pullRequests/handlers.test.ts | 190 +++++++++++++++++- .../src/mcp/toolkits/pullRequests/handlers.ts | 183 +++++++++++++++++ .../src/mcp/toolkits/pullRequests/tools.ts | 94 +++++++++ .../toolkits/worktree/registration.test.ts | 2 + .../PullRequestReviewGuideService.test.ts | 4 +- .../PullRequestReviewGuideService.ts | 12 +- docs/user/source-control.md | 3 + .../client-runtime/src/t3ToolSummary.test.ts | 16 ++ packages/client-runtime/src/t3ToolSummary.ts | 28 ++- packages/shared/src/t3McpToolPresentation.ts | 12 ++ 13 files changed, 555 insertions(+), 7 deletions(-) diff --git a/apps/server/src/mcp/McpHttpServer.test.ts b/apps/server/src/mcp/McpHttpServer.test.ts index e7b5e90fbd88ecc289eca6cb70d7b0a2f387407c..d1a48708c699d1ba6181d78814290a7fdb36e139 100644 --- a/apps/server/src/mcp/McpHttpServer.test.ts +++ b/apps/server/src/mcp/McpHttpServer.test.ts @@ -27,6 +27,7 @@ import { } from "effect/http"; import * as ProjectService from "../project/ProjectService.ts"; +import * as PullRequestReviewGuideService from "../pullRequest/PullRequestReviewGuideService.ts"; import * as ServerConfig from "../config.ts"; import * as McpHttpServer from "./McpHttpServer.ts"; import * as McpToolAccess from "./McpToolAccess.ts"; @@ -77,6 +78,7 @@ const layerPullRequestsTest = McpHttpServer.layerPullRequestsToolkit.pipe( Layer.provide( Layer.mergeAll( Layer.mock(ProjectService.ProjectService)({}), + Layer.mock(PullRequestReviewGuideService.PullRequestReviewGuideService)({}), Layer.mock(Orchestrator.OrchestratorV2)({}), Layer.mock(ProjectionStore.ProjectionStoreV2)({}), NodeServices.layer, diff --git a/apps/server/src/mcp/McpToolAccess.test.ts b/apps/server/src/mcp/McpToolAccess.test.ts index 04ac576b0679eac06b58d729096544719f8f254f..6a96fd580767711bdb211bc8868519bb2ce27b55 100644 --- a/apps/server/src/mcp/McpToolAccess.test.ts +++ b/apps/server/src/mcp/McpToolAccess.test.ts @@ -87,6 +87,7 @@ const ProbeToolkit = Toolkit.make( Tool.make("reads_as_caller", probe), Tool.make("acts_as_caller", probe), Tool.make("writes", probe), + Tool.make("writes_with_capability", probe), Tool.make("writes_threads", { ...probe, parameters: Schema.Struct({ threadId: Schema.optional(ThreadId) }), @@ -106,6 +107,7 @@ const probeHandlers: McpToolAccess.Handlers = { reads_as_caller: McpToolAccess.readsAsCaller(() => ran), acts_as_caller: McpToolAccess.actsAsCaller(() => ran), writes: McpToolAccess.writes(() => ran), + writes_with_capability: McpToolAccess.writesWithCapability(() => ran), writes_threads: McpToolAccess.writesThreads( (input) => [input.threadId], () => ran, @@ -152,6 +154,7 @@ export const refusedAtCompileTime = () => { reads_as_caller: unchecked, acts_as_caller: unchecked, writes: unchecked, + writes_with_capability: unchecked, writes_threads: unchecked, starts_threads: unchecked, writes_environment: unchecked, @@ -224,6 +227,11 @@ it.effect.each([ ["writes", previewOnly, {}, "capability_denied"], ["writes_environment", previewOnly, {}, "capability_denied"], + // A tool's own capability stands in for thread control; the rest of a change still applies. + ["writes_with_capability", previewOnly, {}, "ran"], + ["writes_with_capability", readOnlyClient, {}, "capability_denied"], + ["writes_with_capability", ended, {}, "parent_not_active"], + // Changing a thread: only one that runs within the caller's modes. ["writes_threads", supervised, {}, "ran"], ["writes_threads", supervised, { threadId: planThreadId }, "ran"], diff --git a/apps/server/src/mcp/McpToolAccess.ts b/apps/server/src/mcp/McpToolAccess.ts index 6e882629bbba10a63bff2d42bf2ee1735f615e4e..33323501965ac9dd0c952690fd10f10d6802d43f 100644 --- a/apps/server/src/mcp/McpToolAccess.ts +++ b/apps/server/src/mcp/McpToolAccess.ts @@ -157,6 +157,14 @@ export const actsAsCaller = (handle: (params: P) => Effect.Effect(handle: (params: P) => Effect.Effect) => declare((params: P) => orchestratingCaller.pipe(Effect.flatMap(() => handle(params)))); +/** + * Changes something that belongs to no thread under a capability of the tool's own, such as + * writing a pull request's review guide. Unlike `writes` it does not need thread control; the + * handler checks its capability, as the capability toolkits do. + */ +export const writesWithCapability = (handle: (params: P) => Effect.Effect) => + declare((params: P) => writingCaller.pipe(Effect.flatMap(() => handle(params)))); + /** * Changes the threads `threads` names. An omitted id is the caller's own * thread; any other thread must run within the caller's modes, checked here diff --git a/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts b/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts index 6101727cd89c71fc20cd02bcd2d74d5b2ad730c7..a292ebf05a7e7c6c8de8fc29691ac76991cc9f48 100644 --- a/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts +++ b/apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts @@ -4,7 +4,10 @@ import { ProviderInstanceId, ThreadId, type OrchestrationV2ServerCommand as OrchestrationCommand, + PullRequestOperationError, type OrchestrationProjectShell, + type PullRequestRef, + type PullRequestReviewGuideResult, type ThreadPullRequestLink, } from "@t3tools/contracts"; import { describe, expect, it } from "@effect/vitest"; @@ -22,6 +25,7 @@ import { v2PullRequestThread, } from "../../../orchestration-v2/testkit/pullRequestFixtures.ts"; import * as ProjectService from "../../../project/ProjectService.ts"; +import * as PullRequestReviewGuideService from "../../../pullRequest/PullRequestReviewGuideService.ts"; import * as McpInvocationContext from "../../McpInvocationContext.ts"; import * as McpToolAccess from "../../McpToolAccess.ts"; import * as McpToolAccessTestkit from "../../McpToolAccess.testkit.ts"; @@ -135,12 +139,24 @@ interface HarnessOptions { readonly project?: OrchestrationProjectShell | null; /** A rejection the orchestrator reports as the dispatch error's cause. */ readonly reject?: (command: OrchestrationCommand) => string | null; + /** Every project in the environment; defaults to the thread's own. */ + readonly projects?: ReadonlyArray; + /** What the guide service answers for a pull request it is asked to write. */ + readonly generate?: ( + input: PullRequestRef, + ) => Effect.Effect< + PullRequestReviewGuideResult & { started: boolean }, + PullRequestOperationError + >; } +const NO_GUIDE: PullRequestReviewGuideResult = { guide: null, run: null, currentHeadSha: "head-1" }; + const makeHarness = Effect.fn("makePullRequestsToolkitHarness")(function* ( options: HarnessOptions = {}, ) { const commands = yield* Ref.make>([]); + const guideRequests = yield* Ref.make>([]); const thread = options.thread === undefined ? makeThread([]) : options.thread; const project = options.project === undefined ? makeProject() : options.project; const dispatch: Orchestrator.OrchestratorV2Shape["dispatch"] = (command) => @@ -158,6 +174,25 @@ const makeHarness = Effect.fn("makePullRequestsToolkitHarness")(function* ( const layerDependencies = Layer.mergeAll( Layer.mock(ProjectService.ProjectService)({ getShell: () => Effect.succeed(Option.fromNullishOr(project)), + listShells: () => Effect.succeed(options.projects ?? (project ? [project] : [])), + }), + Layer.mock(PullRequestReviewGuideService.PullRequestReviewGuideService)({ + get: () => Effect.succeed(NO_GUIDE), + generate: (input) => + Ref.update(guideRequests, (recorded) => [...recorded, input]).pipe( + Effect.andThen( + options.generate?.(input) ?? + Effect.succeed({ + ...NO_GUIDE, + run: { + _tag: "Running" as const, + headSha: "head-1", + startedAt: "2026-10-11T00:00:00.000Z", + }, + started: true, + }), + ), + ), }), Layer.mock(Orchestrator.OrchestratorV2)({ getThreadShell: (id) => @@ -189,7 +224,7 @@ const makeHarness = Effect.fn("makePullRequestsToolkitHarness")(function* ( Effect.provideService(McpInvocationContext.McpInvocationContext, invocation(capabilities)), Effect.provide(layerDependencies), ); - return { commands, call }; + return { commands, guideRequests, call }; }); describe("pull request toolkit handlers", () => { @@ -546,6 +581,159 @@ describe("listThreadPullRequests", () => { }); }); +describe("pull request guide tools", () => { + it.effect( + "writes guides through the project whose checkout is each pull request's repository", + () => + Effect.gen(function* () { + const other = { + ...makeProject({ + canonicalKey: "github.com/acme/web", + locator: { + source: "git-remote", + remoteName: "origin", + remoteUrl: "git@github.com:acme/web.git", + }, + provider: "github", + displayName: "acme/web", + owner: "acme", + name: "web", + }), + id: ProjectId.make("project-web"), + }; + const harness = yield* makeHarness({ projects: [makeProject(), other] }); + const result = yield* harness.call("write_pull_request_guides", { + pullRequests: [ + { url: "https://github.com/acme/web/pull/7" }, + { repository: "t3tools/t3code", number: 12 }, + { url: "https://github.com/someone/else/pull/1" }, + ], + }); + expect( + result.guides.map((guide) => [guide.repository, guide.number, guide.outcome]), + ).toEqual([ + ["acme/web", 7, "started"], + ["t3tools/t3code", 12, "started"], + ["someone/else", 1, "failed"], + ]); + expect(result.guides[2]?.detail).toContain("No project in this environment"); + expect( + (yield* Ref.get(harness.guideRequests)).map((ref) => [ref.projectId, ref.number]), + ).toEqual([ + ["project-web", 7], + [PROJECT_ID, 12], + ]); + }), + ); + + it.effect("reports a refused run on its own pull request without failing the rest", () => + Effect.gen(function* () { + const harness = yield* makeHarness({ + generate: (input) => + input.number === 1 + ? Effect.fail( + new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "Review guides are off. Turn them on in Settings → Source Control.", + }), + ) + : Effect.succeed({ + ...NO_GUIDE, + run: { + _tag: "Running" as const, + headSha: "head-1", + startedAt: "2026-10-11T00:00:00.000Z", + }, + started: true, + }), + }); + const result = yield* harness.call("write_pull_request_guides", { + pullRequests: [ + { repository: "t3tools/t3code", number: 1 }, + { repository: "t3tools/t3code", number: 2 }, + ], + }); + expect(result.guides.map((guide) => [guide.number, guide.outcome, guide.detail])).toEqual([ + [1, "failed", "Review guides are off. Turn them on in Settings → Source Control."], + [2, "started", null], + ]); + }), + ); + + it.effect("reports an unreadable entry on its own and still writes the rest of the batch", () => + Effect.gen(function* () { + const harness = yield* makeHarness(); + const result = yield* harness.call("write_pull_request_guides", { + pullRequests: [ + { url: "https://example.com/not-a-pull-request" }, + { repository: "t3tools/t3code" }, + { repository: "t3tools/t3code", number: 5 }, + ], + }); + expect(result.guides.map((guide) => guide.outcome)).toEqual(["failed", "failed", "started"]); + expect(result.guides[0]?.detail).toContain("not a recognised pull request URL"); + expect(result.guides[1]?.detail).toContain("Pass either url"); + expect((yield* Ref.get(harness.guideRequests)).map((ref) => ref.number)).toEqual([5]); + }), + ); + + it.effect("reports a run the service joined as running, not as a new one", () => + Effect.gen(function* () { + const harness = yield* makeHarness({ + generate: () => + Effect.succeed({ + ...NO_GUIDE, + run: { _tag: "Running", headSha: "head-1", startedAt: "2026-10-11T00:00:00.000Z" }, + started: false, + }), + }); + const result = yield* harness.call("write_pull_request_guides", { + pullRequests: [{ repository: "t3tools/t3code", number: 4 }], + }); + expect(result.guides.map((guide) => guide.outcome)).toEqual(["running"]); + }), + ); + + it.effect("finds an Azure DevOps checkout for a pull request named by its full path", () => + Effect.gen(function* () { + const azure = { + ...makeProject({ + canonicalKey: "dev.azure.com/org/project/_git/web", + locator: { + source: "git-remote", + remoteName: "origin", + remoteUrl: "https://dev.azure.com/org/project/_git/web", + }, + provider: "azure-devops", + displayName: "org/project/_git/web", + owner: "org", + name: "web", + }), + id: ProjectId.make("project-azure"), + }; + const harness = yield* makeHarness({ projects: [makeProject(), azure] }); + const result = yield* harness.call("write_pull_request_guides", { + pullRequests: [{ host: "dev.azure.com", repository: "org/project/_git/web", number: 9 }], + }); + expect(result.guides.map((guide) => guide.outcome)).toEqual(["started"]); + expect((yield* Ref.get(harness.guideRequests)).map((ref) => ref.projectId)).toEqual([ + "project-azure", + ]); + }), + ); + + it.effect("reads a stored guide and says when a push left it behind", () => + Effect.gen(function* () { + const harness = yield* makeHarness(); + const result = yield* harness.call("read_pull_request_guide", { + repository: "t3tools/t3code", + number: 3, + }); + expect(result).toMatchObject({ number: 3, guide: null, run: null, stale: false }); + }), + ); +}); + it("keeps failure diagnostics as the cause rather than exposing them in the tool message", () => { const cause = new Error("database internals"); const failure = new PullRequestLinkFailedError({ cause }); diff --git a/apps/server/src/mcp/toolkits/pullRequests/handlers.ts b/apps/server/src/mcp/toolkits/pullRequests/handlers.ts index cced337670546fe95b9b69d39059273787735f77..c7213e782c813099756bdd3f2b855813f541826e 100644 --- a/apps/server/src/mcp/toolkits/pullRequests/handlers.ts +++ b/apps/server/src/mcp/toolkits/pullRequests/handlers.ts @@ -12,6 +12,7 @@ import { type ThreadPullRequestLink, } from "@t3tools/contracts"; import { changeRequestUrlFor, parseChangeRequestUrl } from "@t3tools/shared/changeRequestUrl"; +import { sourceControlRepositorySelector } from "@t3tools/shared/sourceControl"; import { normalizeThreadPullRequestKey, resolveThreadPullRequestChains, @@ -25,6 +26,8 @@ import * as Option from "effect/Option"; import * as Orchestrator from "../../../orchestration-v2/Orchestrator.ts"; import * as ProjectService from "../../../project/ProjectService.ts"; +import * as PullRequestReviewGuideService from "../../../pullRequest/PullRequestReviewGuideService.ts"; +import { pullRequestSyncKey } from "../../../pullRequest/pullRequestSyncKey.ts"; import * as McpInvocationContext from "../../McpInvocationContext.ts"; import * as McpToolAccess from "../../McpToolAccess.ts"; import { @@ -41,6 +44,8 @@ import { PullRequestWatchFromSubagentError, PullRequestThreadNotFoundError, PullRequestThreadRequiredError, + PullRequestGuideFailedError, + PullRequestProjectNotFoundError, PullRequestsToolkit, type ThreadPullRequestEntry, } from "./tools.ts"; @@ -153,8 +158,48 @@ export function listThreadPullRequests( }; } +/** + * The project a pull request is read through: the caller's own when its checkout is that + * repository, else the first project here whose checkout is. Host reads are scoped to a project, + * so a repository with no checkout in this environment cannot be guided from it. + */ +function projectForPullRequest( + projects: ReadonlyArray, + target: { readonly host: string; readonly repository: string }, + preferred: OrchestrationProjectShell | undefined, +): OrchestrationProjectShell | undefined { + // The same rule the guide service uses: a pull request reads through a project whose checkout + // names it with the same host-level key, which also maps Azure DevOps' org/project/_git/repo. + const matches = (project: OrchestrationProjectShell) => { + const own = pullRequestSyncKey( + { projectId: project.id, host: target.host, repository: target.repository, number: 1 }, + project.repositoryIdentity ?? undefined, + ); + const projectKey = + project.repositoryIdentity == null + ? null + : pullRequestSyncKey( + { + projectId: project.id, + repository: sourceControlRepositorySelector(project.repositoryIdentity) ?? "", + number: 1, + }, + project.repositoryIdentity, + ); + return ( + own !== null && + projectKey !== null && + own.host === projectKey.host && + own.repository === projectKey.repository + ); + }; + if (preferred !== undefined && matches(preferred)) return preferred; + return projects.find(matches); +} + const make = Effect.gen(function* () { const engine = yield* Orchestrator.OrchestratorV2; + const reviewGuides = yield* PullRequestReviewGuideService.PullRequestReviewGuideService; const projects = yield* ProjectService.ProjectService; const crypto = yield* Crypto.Crypto; @@ -264,6 +309,56 @@ const make = Effect.gen(function* () { }; }); + /** + * Resolves each requested pull request and the project it is read through. The caller's own + * project wins where it is a checkout of that repository; with no thread, any project here may. + */ + const locateAll = Effect.fn("PullRequestsToolkit.locateAll")(function* ( + targets: ReadonlyArray>, + ) { + const scope = yield* McpInvocationContext.requireMcpCapability("pull-requests"); + const callerThread = + scope.thread === undefined + ? undefined + : yield* engine.getThreadShell(scope.thread.threadId).pipe( + Effect.orElseSucceed(() => null), + Effect.map((shell) => shell ?? undefined), + ); + const known = yield* projects + .listShells() + .pipe(Effect.mapError((cause) => new PullRequestListFailedError({ cause }))); + const callerProject = known.find((project) => project.id === callerThread?.projectId); + // Each target resolves on its own, so one bad URL in a batch is that entry's failure only. + return yield* Effect.forEach(targets, (input) => + resolveTarget(input, callerProject).pipe( + Effect.map((target) => ({ + input, + target, + project: projectForPullRequest(known, target, callerProject), + failure: null, + })), + Effect.catch((error) => + Effect.succeed({ input, target: null, project: undefined, failure: error.message }), + ), + ), + ); + }); + + /** The service reads a pull request through one project's checkout and credentials. */ + const guideReference = ( + project: OrchestrationProjectShell, + target: { readonly host: string; readonly repository: string; readonly number: number }, + ) => ({ + projectId: project.id, + host: target.host, + repository: target.repository, + number: target.number, + }); + + /** A host or service error, said as its own detail rather than its operation's wrapper. */ + const guideFailureDetail = (error: { readonly message: string }) => + "detail" in error && typeof error.detail === "string" ? error.detail : error.message; + /** A tool that changes `threadId`, or the caller's own thread when it is omitted. */ const writesThread =

( handle: (params: P) => Effect.Effect, @@ -342,6 +437,94 @@ const make = Effect.gen(function* () { ), watch_pull_request: writesThread((input) => setWatching(input, true)), unwatch_pull_request: writesThread((input) => setWatching(input, false)), + write_pull_request_guides: McpToolAccess.writesWithCapability((input) => + Effect.gen(function* () { + const located = yield* locateAll(input.pullRequests); + // One pull request that cannot be guided does not stop the rest; it reports why. + const guides = yield* Effect.forEach( + located, + (entry) => + Effect.gen(function* () { + if (entry.target === null) { + return { + host: entry.input.host ?? "", + repository: entry.input.repository ?? "", + number: entry.input.number ?? 0, + url: entry.input.url ?? "", + outcome: "failed" as const, + detail: entry.failure, + }; + } + const identity = { + host: entry.target.host, + repository: entry.target.repository, + number: entry.target.number, + url: entry.target.url, + }; + if (entry.project === undefined) { + return { + ...identity, + outcome: "failed" as const, + detail: new PullRequestProjectNotFoundError({ + repository: entry.target.repository, + }).message, + }; + } + return yield* reviewGuides.generate(guideReference(entry.project, entry.target)).pipe( + Effect.map((result) => ({ + ...identity, + outcome: result.started ? ("started" as const) : ("running" as const), + detail: null, + })), + Effect.catch((error) => + Effect.succeed({ + ...identity, + outcome: "failed" as const, + detail: guideFailureDetail(error), + }), + ), + ); + }), + { concurrency: 4 }, + ); + return { guides }; + }), + ), + read_pull_request_guide: McpToolAccess.reads((input) => + Effect.gen(function* () { + const [entry] = yield* locateAll([input]); + if (entry !== undefined && entry.target === null) { + return yield* new PullRequestGuideFailedError({ detail: entry.failure ?? "" }); + } + if (entry === undefined || entry.target === null || entry.project === undefined) { + return yield* new PullRequestProjectNotFoundError({ + repository: entry?.target.repository ?? input.repository ?? "", + }); + } + const result = yield* reviewGuides.get(guideReference(entry.project, entry.target)).pipe( + Effect.mapError( + (error) => + new PullRequestGuideFailedError({ + detail: guideFailureDetail(error), + cause: error, + }), + ), + ); + return { + host: entry.target.host, + repository: entry.target.repository, + number: entry.target.number, + url: entry.target.url, + guide: result.guide, + run: result.run, + currentHeadSha: result.currentHeadSha, + stale: + result.guide !== null && + result.currentHeadSha !== null && + result.guide.headSha !== result.currentHeadSha, + }; + }), + ), } satisfies McpToolAccess.Handlers; }); diff --git a/apps/server/src/mcp/toolkits/pullRequests/tools.ts b/apps/server/src/mcp/toolkits/pullRequests/tools.ts index e5c3d53503ffb8e224847873a1613d1d9b567fe4..35b13f3d2dcfdfc74805fb6d0cdde711e29147f9 100644 --- a/apps/server/src/mcp/toolkits/pullRequests/tools.ts +++ b/apps/server/src/mcp/toolkits/pullRequests/tools.ts @@ -2,6 +2,8 @@ import { McpCapabilityUnavailableError, OrchestratorMcpFailure, PositiveInt, + PullRequestReviewGuide, + PullRequestReviewGuideRun, PullRequestState, ThreadPullRequestLinkSource, TrimmedNonEmptyString, @@ -15,6 +17,7 @@ import * as McpInvocationContext from "../../McpInvocationContext.ts"; import * as Orchestrator from "../../../orchestration-v2/Orchestrator.ts"; import * as ThreadManagementService from "../../../orchestration-v2/ThreadManagementService.ts"; import * as ProjectService from "../../../project/ProjectService.ts"; +import * as PullRequestReviewGuideService from "../../../pullRequest/PullRequestReviewGuideService.ts"; const dependencies = [ McpInvocationContext.McpInvocationContext, @@ -23,6 +26,11 @@ const dependencies = [ ProjectService.ProjectService, ]; +const guideDependencies = [ + ...dependencies, + PullRequestReviewGuideService.PullRequestReviewGuideService, +]; + const REGISTER_EVERY_PR = "Register every pull request you open for this thread, including each layer of a stack, right after creating it."; @@ -162,6 +170,24 @@ export class PullRequestListFailedError extends Schema.TaggedError()( + "PullRequestProjectNotFoundError", + { repository: Schema.String }, +) { + override get message(): string { + return `No project in this environment is a checkout of ${this.repository}, so its pull requests cannot be read here. Add one with t3_project_create.`; + } +} + +export class PullRequestGuideFailedError extends Schema.TaggedError()( + "PullRequestGuideFailedError", + { detail: Schema.String, cause: Schema.optional(Schema.Defect()) }, +) { + override get message(): string { + return this.detail; + } +} + export const PullRequestToolError = Schema.Union([ OrchestratorMcpFailure, McpCapabilityUnavailableError, @@ -176,6 +202,8 @@ export const PullRequestToolError = Schema.Union([ PullRequestWatchFailedError, PullRequestNotOpenError, PullRequestWatchFromSubagentError, + PullRequestProjectNotFoundError, + PullRequestGuideFailedError, ]); export type PullRequestToolError = typeof PullRequestToolError.Type; @@ -319,10 +347,76 @@ const UnwatchPullRequestTool = Tool.make("unwatch_pull_request", { .annotate(Tool.Idempotent, true) .annotate(Tool.OpenWorld, false); +/** A pull request to write or read a guide for, by URL or by repository and number. */ +const GuideTarget = Schema.Struct({ + url: PullRequestTargetInput.fields.url, + repository: PullRequestTargetInput.fields.repository, + number: PullRequestTargetInput.fields.number, + host: PullRequestTargetInput.fields.host, +}); + +const GuideOutcome = Schema.Struct({ + ...PullRequestIdentity, + outcome: Schema.Literals(["started", "running", "failed"]).annotate({ + description: + "started: a run began. running: one was already going for this head and the call joined it. failed: this pull request could not be guided; see detail.", + }), + detail: Schema.NullOr(Schema.String), +}); + +export const WritePullRequestGuidesResult = Schema.Struct({ + guides: Schema.Array(GuideOutcome), +}); +export type WritePullRequestGuidesResult = typeof WritePullRequestGuidesResult.Type; + +export const ReadPullRequestGuideResult = Schema.Struct({ + ...PullRequestIdentity, + guide: Schema.NullOr(PullRequestReviewGuide), + run: Schema.NullOr(PullRequestReviewGuideRun), + currentHeadSha: Schema.NullOr(Schema.String), + stale: Schema.Boolean.annotate({ + description: + "True when the guide was written for an earlier head than the pull request has now.", + }), +}); +export type ReadPullRequestGuideResult = typeof ReadPullRequestGuideResult.Type; + +const WritePullRequestGuidesTool = Tool.make("write_pull_request_guides", { + description: + "Write review guides for up to 20 pull requests, the tour a maintainer reads in a pull request's Guide tab. Each run reads its change in a temporary checkout and takes minutes, so this returns once the runs are under way; read a guide with read_pull_request_guide to see when it is written. A guide for the same head that is already running is joined, not restarted; a pull request whose guide is current is written again. Each pull request is read through the project in this environment whose checkout has that repository. Fails while review guides are off on this server.", + parameters: Schema.Struct({ + pullRequests: Schema.Array(GuideTarget).check(Schema.isMinLength(1), Schema.isMaxLength(20)), + }), + success: WritePullRequestGuidesResult, + failure: PullRequestToolError, + dependencies: guideDependencies, +}) + .annotate(Tool.Title, "Write pull request guides") + .annotate(Tool.Readonly, false) + .annotate(Tool.Destructive, false) + .annotate(Tool.Idempotent, false) + .annotate(Tool.OpenWorld, true); + +const ReadPullRequestGuideTool = Tool.make("read_pull_request_guide", { + description: + "Read a pull request's stored review guide and its run: run.Running while one is being written, run.Failed with the reason when the last one failed, guide null when none was written. stale is true after a push the guide has not caught up with. Never starts a run.", + parameters: GuideTarget, + success: ReadPullRequestGuideResult, + failure: PullRequestToolError, + dependencies: guideDependencies, +}) + .annotate(Tool.Title, "Read pull request guide") + .annotate(Tool.Readonly, true) + .annotate(Tool.Destructive, false) + .annotate(Tool.Idempotent, true) + .annotate(Tool.OpenWorld, true); + export const PullRequestsToolkit = Toolkit.make( LinkPullRequestTool, UnlinkPullRequestTool, ListThreadPullRequestsTool, WatchPullRequestTool, UnwatchPullRequestTool, + WritePullRequestGuidesTool, + ReadPullRequestGuideTool, ); diff --git a/apps/server/src/mcp/toolkits/worktree/registration.test.ts b/apps/server/src/mcp/toolkits/worktree/registration.test.ts index 91257c2fdb809607383dd60a32f156aabc6cc9ad..c0006f7fe171d2f8e577d7ca7401f5c548accea6 100644 --- a/apps/server/src/mcp/toolkits/worktree/registration.test.ts +++ b/apps/server/src/mcp/toolkits/worktree/registration.test.ts @@ -16,6 +16,7 @@ import * as GitWorkflowService from "../../../git/GitWorkflowService.ts"; import * as ProviderAdapterRegistry from "../../../orchestration-v2/ProviderAdapterRegistry.ts"; import * as ThreadManagementService from "../../../orchestration-v2/ThreadManagementService.ts"; import * as ProjectService from "../../../project/ProjectService.ts"; +import * as PullRequestReviewGuideService from "../../../pullRequest/PullRequestReviewGuideService.ts"; import * as ProjectSetupScriptRunner from "../../../project/ProjectSetupScriptRunner.ts"; import * as ProviderRegistry from "../../../provider/ProviderRegistry.ts"; import * as ScheduledTaskService from "../../../scheduledTasks/ScheduledTaskService.ts"; @@ -44,6 +45,7 @@ const layerStubServices = Layer.mergeAll( Layer.mock(ScheduledTaskService.ScheduledTaskService)({}), Layer.mock(SecretRequests.SecretRequests)({}), Layer.mock(ProjectService.ProjectService)({}), + Layer.mock(PullRequestReviewGuideService.PullRequestReviewGuideService)({}), ServerSettings.layerTest({}), Layer.mock(GitWorkflowService.GitWorkflowService)({}), Layer.mock(ProjectSetupScriptRunner.ProjectSetupScriptRunner)({}), diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts index aa41705db29d8237491ec6b0f9918d8f8b7d62c3..51591f031f1c4729b95ab064e68cc9addc58c59c 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts @@ -303,6 +303,7 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic const started = yield* service.generate(reference); assert.strictEqual(started.run?._tag, "Running"); assert.strictEqual(started.guide, null); + assert.isTrue(started.started); // A run that fails before reaching the model reports why rather than leaving this waiting. const failedRun = service.get(reference).pipe( @@ -329,7 +330,8 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic // A second press while running joins the run instead of starting another. const joined = yield* service.generate(reference); - assert.strictEqual(joined.run?._tag, "Running"); + assert.strictEqual(joined.run, started.run); + assert.isFalse(joined.started); yield* Deferred.succeed(finish, undefined); yield* Deferred.await(stored); diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts index b181926f4e8d375844129119652a57007f3fb9b5..2063804b48ef36831d99dd4a1a00e8e2f75bc37a 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts @@ -62,11 +62,15 @@ export class PullRequestReviewGuideService extends Context.Service< /** * Start writing a guide for the current head and return once it is under way. Joins a run * already going for that head. A stored guide stays readable until the new one replaces it, - * so a reader can rewrite one after changing the guide model or instructions. + * so a reader can rewrite one after changing the guide model or instructions. `started` says + * whether this call began the run or joined one already going. */ readonly generate: ( input: PullRequestRef, - ) => Effect.Effect; + ) => Effect.Effect< + PullRequestReviewGuideResult & { readonly started: boolean }, + PullRequestService.PullRequestError + >; } >()("t3/pullRequest/PullRequestReviewGuideService") {} @@ -439,7 +443,7 @@ const make = Effect.gen(function* () { ), ), ); - if (claimed !== running) return { guide: stored, run: claimed, currentHeadSha }; + if (claimed !== running) return { guide: stored, run: claimed, currentHeadSha, started: false }; // The run is identified by its own Running value: a later claim replaces it, and from then // on this run's completion leaves the newer run's state and guide alone. @@ -475,7 +479,7 @@ const make = Effect.gen(function* () { ), Effect.forkIn(serviceScope), ); - return { guide: stored, run: running, currentHeadSha }; + return { guide: stored, run: running, currentHeadSha, started: true }; }); return PullRequestReviewGuideService.of({ get, generate }); diff --git a/docs/user/source-control.md b/docs/user/source-control.md index 4c3fe884bd1acdf1fa50eac349a24de1d43f5e20..d9b0293537efc96cad7498f0fec4c8343c763a18 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -209,6 +209,9 @@ about them; comments wait with the ones from the **Code** tab until you send the suggested question, or **Ask about this stop**, to open a thread that already knows which stop you are on. **Finish review** on the last stop opens the review with everything you collected. +Agents can write guides too: ask one to prepare guides for a set of pull requests, and it starts +them for you. Each is read through the project whose checkout has that repository. + To shape guides to your taste, write **Review guide instructions** in Settings → Source Control, such as what to show first or what to skip; projects can override them. They change emphasis and ordering, not the guide's layout or which files it covers. diff --git a/packages/client-runtime/src/t3ToolSummary.test.ts b/packages/client-runtime/src/t3ToolSummary.test.ts index 45868cc70fdc10bf4b78c8611a75575b5934cda0..7cde36c19e62ecae745a45cd6eba3ff4904027a8 100644 --- a/packages/client-runtime/src/t3ToolSummary.test.ts +++ b/packages/client-runtime/src/t3ToolSummary.test.ts @@ -28,6 +28,22 @@ describe("summarizeT3ToolCalls", () => { ).toBe("Created 1 thread"); }); + it("reports review guides by outcome, and a call where none started as a failure", () => { + const guides = (...outcomes: ReadonlyArray) => ({ + guides: outcomes.map((outcome, index) => ({ number: index + 1, outcome })), + }); + expect( + summarizeT3ToolCalls("write-pr-guides", [ + completed({}, guides("started", "running", "failed")), + ]), + ).toEqual({ label: "Started 2 review guides, 1 failed", failedCount: 1 }); + const allFailed = summarizeT3ToolCalls("write-pr-guides", [ + completed({}, guides("failed", "failed")), + ]); + expect(allFailed.label).toBe("Could not start 2 review guides"); + expect(allFailed.failedCount).toBe(2); + }); + it.each([ ["queue-read", "Read 1 queued message"], ["queue-edit", "Edited 1 queued message"], diff --git a/packages/client-runtime/src/t3ToolSummary.ts b/packages/client-runtime/src/t3ToolSummary.ts index 101950dd6a5de96a0f6f6b59e5cec026796356fc..276a4dc18e15f4d4bd49b69d9b8b827f9be3a859 100644 --- a/packages/client-runtime/src/t3ToolSummary.ts +++ b/packages/client-runtime/src/t3ToolSummary.ts @@ -94,7 +94,7 @@ export function summarizeT3ToolCalls( }; }); const completed = results.filter((call) => call.outcome === "completed"); - const failedCount = results.filter((call) => call.outcome === "failed").length; + let failedCount = results.filter((call) => call.outcome === "failed").length; const selected = completed.length > 0 ? completed : results; const times = quantity(selected.length, "time"); const phrase = (past: string, infinitive: string, object: string) => @@ -399,6 +399,32 @@ export function summarizeT3ToolCalls( `linked pull requests${selected.length === 1 ? "" : ` ${times}`}`, ); break; + case "write-pr-guides": { + // One call writes many guides, and reports a refusal per guide rather than failing. + const outcomes = selected.flatMap((call) => + Array.isArray(call.output?.guides) + ? call.output.guides.map((guide) => asRecord(guide)?.outcome) + : [], + ); + const started = outcomes.filter((outcome) => outcome !== "failed").length; + const failed = outcomes.length - started; + failedCount += failed; + if (outcomes.length > 0 && started === 0) { + label = `Could not start ${quantity(failed, "review guide")}`; + } else { + label = phrase( + "Started", + "start", + outcomes.length === 0 + ? `review guides ${times}` + : `${quantity(started, "review guide")}${failed > 0 ? `, ${failed} failed` : ""}`, + ); + } + break; + } + case "read-pr-guide": + label = phrase("Read", "read", quantity(selected.length, "review guide")); + break; case "browser": label = phrase("Used", "use", `browser ${times}`); break; diff --git a/packages/shared/src/t3McpToolPresentation.ts b/packages/shared/src/t3McpToolPresentation.ts index 5557d13bd9dc7c5a83db8bcaca01668874a7f79d..32a5b6fa65edbd4f17d663bbfcf1a55454d3019e 100644 --- a/packages/shared/src/t3McpToolPresentation.ts +++ b/packages/shared/src/t3McpToolPresentation.ts @@ -59,6 +59,8 @@ export type T3McpToolSummaryAction = | "list-prs" | "watch-pr" | "unwatch-pr" + | "write-pr-guides" + | "read-pr-guide" | "browser" | "device" | "html-preview" @@ -109,6 +111,16 @@ const T3_MCP_TOOLS: Readonly> = { "unwatch-pr", "pull-request", ), + write_pull_request_guides: tool( + ["Write", "Writing", "Started", "review guides"], + "write-pr-guides", + "pull-request", + ), + read_pull_request_guide: tool( + ["Read", "Reading", "Read", "a review guide"], + "read-pr-guide", + "pull-request", + ), orchestrator_capabilities: tool( ["Get", "Getting", "Got", "orchestration capabilities"], "capabilities", -- 2.54.0