Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions apps/server/src/mcp/OrchestratorMcpService.activity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import * as SecretRequests from "../secrets/SecretRequests.ts";
import * as ThreadLaunchService from "../orchestration-v2/ThreadLaunchService.ts";
import * as GitVcsDriver from "../vcs/GitVcsDriver.ts";
import * as RemoteDelegation from "../peer/RemoteDelegation.ts";
import * as Orchestrator from "../orchestration-v2/Orchestrator.ts";
import * as ThreadManagementService from "../orchestration-v2/ThreadManagementService.ts";
import type * as McpInvocationContext from "./McpInvocationContext.ts";
import * as OrchestratorMcpService from "./OrchestratorMcpService.ts";
Expand Down Expand Up @@ -116,7 +117,8 @@ function makeRun(input: {
};
}

it("readThread prefers activity-run status over a newer cancelled queued run", async () => {
it("readThread preserves caller access errors and prefers activity-run status over a newer cancelled queued run", async () => {
let callerUnavailable = false;
const projection = {
thread: baseThread({
threadId: parentThreadId,
Expand All @@ -142,15 +144,24 @@ it("readThread prefers activity-run status over a newer cancelled queued run", a
Layer.mock(ThreadManagementService.ThreadManagementService)({
getTimelinePage: () => Effect.succeed({ items: [], totalItems: 0, hasMore: false }),
getThreadRecords: (threadId) =>
threadId === parentThreadId
? Effect.succeed(projection)
: Effect.die(`unexpected thread ${threadId}`),
callerUnavailable
? Effect.fail(
new Orchestrator.OrchestratorProjectionError({
threadId,
cause: "Caller unavailable",
}),
)
: threadId === parentThreadId
? Effect.succeed(projection)
: Effect.die(`unexpected thread ${threadId}`),
getThreadShell: (threadId) =>
Effect.succeed(
threadId === parentThreadId
? (projection.thread as unknown as OrchestrationV2ThreadShell)
: null,
),
callerUnavailable
? Effect.die("A failed caller read must not reach the target")
: Effect.succeed(
threadId === parentThreadId
? (projection.thread as unknown as OrchestrationV2ThreadShell)
: null,
),
getProjectThreadRecords: (input) =>
input.threadId === parentThreadId
? Effect.succeed(projection)
Expand Down Expand Up @@ -180,6 +191,17 @@ it("readThread prefers activity-run status over a newer cancelled queued run", a
expect(result.thread.status).toBe("running");
expect(result.thread.latestRunId).toBe(cancelledRunId);
expect(result.thread.activeRunId).toBe(activeRunId);

callerUnavailable = true;
const denied = yield* service
.readThread({ ...makeScope(), capabilities: new Set() }, { threadId: parentThreadId })
.pipe(Effect.flip);
expect(denied.code).toBe("capability_denied");
const missingCaller = yield* service
.readThread(makeScope(), { threadId: parentThreadId })
.pipe(Effect.flip);
expect(missingCaller.code).toBe("orchestration_error");
expect(missingCaller.message).toContain(`Unable to read thread ${parentThreadId}:`);
}).pipe(Effect.provide(layer), Effect.runPromise);
});

Expand Down
30 changes: 19 additions & 11 deletions apps/server/src/mcp/OrchestratorMcpService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,7 @@ const make = Effect.gen(function* () {
),
);

const loadProjection = (threadId: ThreadId) =>
const loadProjection = (threadId: ThreadId, includeUserMessages = true) =>
threadManagement
.getThreadRecords(
threadId,
Expand All @@ -1023,7 +1023,7 @@ const make = Effect.gen(function* () {
"contextTransfers",
"turnItems",
],
{ turnItemTypes: [], messageRoles: ["user"] },
{ turnItemTypes: [], messageRoles: includeUserMessages ? ["user"] : [] },
)
.pipe(
Effect.mapError((error) =>
Expand Down Expand Up @@ -1073,6 +1073,7 @@ const make = Effect.gen(function* () {

const loadCaller = Effect.fn("OrchestratorMcpService.loadCaller")(function* (
scope: McpInvocationScope,
includeUserMessages = true,
) {
yield* requireCapability(scope);
if (scope.thread === undefined) {
Expand All @@ -1081,7 +1082,7 @@ const make = Effect.gen(function* () {
limits: clientModeCeiling(scope.client),
} as const;
}
const parent = yield* loadProjection(scope.thread.threadId);
const parent = yield* loadProjection(scope.thread.threadId, includeUserMessages);
return {
parent,
limits: {
Expand Down Expand Up @@ -1190,7 +1191,8 @@ const make = Effect.gen(function* () {
scope: McpInvocationScope,
threadId: ThreadId,
) {
const { parent } = yield* loadCaller(scope);
// Caller prompts are not used to read a timeline or acknowledge a child result.
const { parent } = yield* loadCaller(scope, false);
const shell = yield* threadManagement
.getThreadShell(threadId)
.pipe(Effect.mapError(threadManagementFailure));
Expand All @@ -1201,13 +1203,19 @@ const make = Effect.gen(function* () {
`Thread ${threadId} is no longer available.`,
);
}
const target = yield* threadManagement
.getProjectThreadRecords({ projectId: shell.projectId, threadId }, [
"runs",
"runtimeRequests",
"contextTransfers",
])
.pipe(Effect.mapError(threadManagementFailure));
const input = { projectId: shell.projectId, threadId };
// Transfers are needed only to acknowledge a caller's child-task result.
const records =
parent === undefined
? threadManagement
.getProjectThreadRecords(input, ["runs", "runtimeRequests"])
.pipe(Effect.map((target) => ({ ...target, contextTransfers: [] })))
: threadManagement.getProjectThreadRecords(input, [
"runs",
"runtimeRequests",
"contextTransfers",
]);
const target = yield* records.pipe(Effect.mapError(threadManagementFailure));
return { parent, target, shell } as const;
});

Expand Down
22 changes: 17 additions & 5 deletions apps/server/src/mcp/OrchestratorMcpToolkit.integration.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as NodeURL from "node:url";
import * as NodeServices from "@effect/platform-node/NodeServices";
import { describe, expect, it } from "@effect/vitest";
import {
Expand Down Expand Up @@ -96,9 +97,7 @@ const readDelegatedTaskStatusTranscript = Effect.fn("readDelegatedTaskStatusTran
followupResult: string,
) {
const fs = yield* FileSystem.FileSystem;
const text = yield* fs.readFileString(
decodeURIComponent(delegatedTaskStatusTranscriptFile.pathname),
);
const text = yield* fs.readFileString(NodeURL.fileURLToPath(delegatedTaskStatusTranscriptFile));
const transcript = yield* decodeProviderReplayNdjson(
text.replaceAll(queuedFollowupResult, followupResult),
);
Expand Down Expand Up @@ -315,11 +314,11 @@ describe("orchestrator MCP toolkit", () => {
capabilities: new Set(["orchestration"]),
issuedAt: 1,
};
const invoke = (name: string, args: Record<string, unknown>) =>
const invoke = (name: string, args: Record<string, unknown>, scope = invocation) =>
server
.callTool({ name, arguments: args })
.pipe(
Effect.provideService(McpInvocationContext.McpInvocationContext, invocation),
Effect.provideService(McpInvocationContext.McpInvocationContext, scope),
Effect.provideService(McpSchema.McpServerClient, client),
);

Expand Down Expand Up @@ -596,11 +595,24 @@ describe("orchestrator MCP toolkit", () => {
expect(finalStatus.latestTerminalResultContextTransferId).not.toBe(
delegated.resultContextTransferId,
);
const parentlessRead = yield* invoke(
"t3_thread_read",
{ threadId: delegated.childThreadId, limit: 100 },
{ ...invocation, thread: undefined },
);
expect(parentlessRead.isError).not.toBe(true);
const afterParentlessRead = yield* orchestrator.getThreadProjection(parentThreadId);
expect(
afterParentlessRead.subagents.find((task) => task.id === queuedFollowup.taskId)
?.completionDelivery?.state,
).toBe("claimed");

const read = yield* invoke("t3_thread_read", {
threadId: delegated.childThreadId,
limit: 100,
});
expect(read.isError).not.toBe(true);
expect(read.structuredContent).toEqual(parentlessRead.structuredContent);
const afterRead = yield* orchestrator.getThreadProjection(parentThreadId);
expect(
afterRead.subagents.find((task) => task.id === queuedFollowup.taskId)
Expand Down
Loading