From f4a6bdfa8851b13fc1f69b7c205cd8912e2a6c10 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 20 Sep 2026 06:38:55 +0000 Subject: [PATCH] chore: sync MCP source from the monorepo at 2fbc58b Published by release/2026-09-19-001. Previous sync: 7ee9a90ba1c844bacebe1159f2b3228a7c488f6a. --- README.md | 1 + mcp-server/.synced-from | 2 +- mcp-server/src/http.test.ts | 40 ++++ mcp-server/src/http.ts | 95 +++++++- mcp-server/src/index.ts | 3 + mcp-server/src/lib/concurrency.test.ts | 119 ++++++++++ mcp-server/src/lib/concurrency.ts | 49 +++++ mcp-server/src/lib/instructions.test.ts | 22 ++ mcp-server/src/lib/instructions.ts | 30 +++ mcp-server/src/lib/request.test.ts | 116 ++++++++++ mcp-server/src/lib/request.ts | 92 +++++++- mcp-server/src/lib/toolResult.test.ts | 46 ++++ mcp-server/src/lib/toolResult.ts | 24 +- mcp-server/src/server.test.ts | 81 +++++-- mcp-server/src/server.ts | 19 +- mcp-server/src/skills.test.ts | 62 +++++- mcp-server/src/skills.ts | 53 ++++- .../tools/evidence/get-test-evidence.test.ts | 73 +++++++ .../src/tools/evidence/get-test-evidence.ts | 20 +- .../src/tools/sessions/create-session.test.ts | 206 ++++++++++++++++++ .../src/tools/sessions/create-session.ts | 190 ++++++++++++++++ 21 files changed, 1294 insertions(+), 49 deletions(-) create mode 100644 mcp-server/src/lib/concurrency.test.ts create mode 100644 mcp-server/src/lib/concurrency.ts create mode 100644 mcp-server/src/tools/sessions/create-session.test.ts create mode 100644 mcp-server/src/tools/sessions/create-session.ts diff --git a/README.md b/README.md index 5c0a87f..537ff84 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,7 @@ Give your AI coding agents full visibility into your CI test results. The Curren | `currents-get-errors-explorer` | Get aggregated error metrics for a project within a date range. | | `currents-get-test-evidence` | Collect evidence artifacts (screenshots, videos, traces, attachments) produced by tests in a CI run, with signed download URLs grouped per test. | | `currents-create-trace-link` | Create a shareable link that serves a test attempt's Playwright trace: a markdown digest of what the attempt did and what failed, a filmstrip, an animated screencast, DOM snapshots, network requests and attachments. | +| `currents-create-session` | Record a browser session you drove as a Currents run, so its evidence can be read and shared like a CI run's. | | `currents-list-webhooks` | List all webhooks for a project. | | `currents-create-webhook` | Create a new webhook for a project. | | `currents-get-webhook` | Get a single webhook by ID. | diff --git a/mcp-server/.synced-from b/mcp-server/.synced-from index 17d6224..95c2231 100644 --- a/mcp-server/.synced-from +++ b/mcp-server/.synced-from @@ -1 +1 @@ -7ee9a90ba1c844bacebe1159f2b3228a7c488f6a +2fbc58bf3d70f9b56d624f54c61e4c0ec79a5c65 diff --git a/mcp-server/src/http.test.ts b/mcp-server/src/http.test.ts index 8315e94..6e40e9e 100644 --- a/mcp-server/src/http.test.ts +++ b/mcp-server/src/http.test.ts @@ -5,6 +5,7 @@ import { AddressInfo } from 'node:net'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { handleMcpRequest } from './http'; import { ApiDispatch, RequestContext } from './lib/context'; +import { getSkills } from './skills'; vi.mock('./lib/logger', () => ({ logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn() }, @@ -88,6 +89,45 @@ describe('handleMcpRequest', () => { expect(JSON.stringify(result.content)).toContain('run-1'); }); + // Both halves over the transport, which is where a host meets them: + // `instructions` comes back from `initialize`, and `prompts/list` needs the + // capability the registration declares. + describe('skills', () => { + it('names them in the instructions the handshake returns', () => { + expect(client.getInstructions()).toContain('collect-evidence'); + }); + + it('lists one prompt per skill', async () => { + const expected = getSkills().map((skill) => skill.name); + // Both sides are empty if no skill shipped, which would pass without + // serving anything. + expect(expected.length).toBeGreaterThan(0); + + const { prompts } = await client.listPrompts(); + + expect(prompts.map((prompt) => prompt.name)).toEqual(expected); + }); + + // The references are the half a second fetch would lose. `skills.test.ts` + // covers the ordering against a fixture, which this cannot: the order + // `getSkills` returns depends on the collation of the machine it runs on. + it('carries the whole skill in the prompt, entry point first', async () => { + const skill = getSkills()[0]; + const { messages } = await client.getPrompt({ name: skill.name }); + const text = messages + .map((message) => + message.content.type === 'text' ? message.content.text : '' + ) + .join(''); + + for (const file of skill.files) { + expect(text).toContain(``); + expect(text).toContain(file.content); + } + expect(text.startsWith('')).toBe(true); + }); + }); + // Each exchange gets a server and a transport of its own, so nothing may // depend on the one before it. it('serves a second exchange with no session to carry', async () => { diff --git a/mcp-server/src/http.ts b/mcp-server/src/http.ts index c1f2a5e..a3396e7 100644 --- a/mcp-server/src/http.ts +++ b/mcp-server/src/http.ts @@ -1,4 +1,4 @@ -import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; +import { WebStandardStreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js'; import type { IncomingMessage, ServerResponse } from 'node:http'; import { RequestContext, requestContext } from './lib/context'; import { logger } from './lib/logger'; @@ -18,9 +18,18 @@ import { createMcpServer } from './server'; * call, because the tool handlers are reached from inside `handleRequest` and * `AsyncLocalStorage` is how they read it (`lib/context.ts`). * - * The caller has already parsed the body: the transport takes it as an argument - * rather than reading the stream, so the host's `express.json()` and this agree - * on one parse. + * The web-standard transport rather than the node one, which is a wrapper over + * this same class that converts the node request with `@hono/node-server`. That + * conversion needs a request backed by a real socket, and the api Lambda has + * none: `@vendia/serverless-express` builds a stand-in, and the conversion + * answered `400` with an empty body for every call while the identical chain on + * a socket answered `200`. Going through the web types directly is what the SDK + * documents for a host that is not a node HTTP server, and it removes the + * difference between the two runtimes rather than working around it. + * + * The request is rebuilt rather than forwarded because nothing here reads its + * stream: the host has already parsed the body, and `parsedBody` is what the + * transport reads, so `express.json()` and this agree on one parse. */ export async function handleMcpRequest( req: IncomingMessage & { body?: unknown }, @@ -28,7 +37,7 @@ export async function handleMcpRequest( context: RequestContext ): Promise { const server = createMcpServer(context); - const transport = new StreamableHTTPServerTransport({ + const transport = new WebStandardStreamableHTTPServerTransport({ sessionIdGenerator: undefined, enableJsonResponse: true, }); @@ -44,6 +53,80 @@ export async function handleMcpRequest( await requestContext.run(context, async () => { await server.connect(transport); - await transport.handleRequest(req, res, req.body); + const response = await transport.handleRequest(toWebRequest(req), { + parsedBody: req.body, + }); + await writeNodeResponse(res, response); + }); +} + +/** + * The node request as the web `Request` the transport takes. + * + * No body: the transport reads `parsedBody` instead, and attaching one would + * mean reading a stream `express.json()` has already consumed. `duplex` is + * therefore not needed either. + * + * The URL is absolute because `Request` requires one. Only its path and query + * are read — the transport matches the method and the headers — so the origin + * is reconstructed from the `host` header and a placeholder when a caller sent + * none, rather than being carried through to anything a client sees. + */ +function toWebRequest(req: IncomingMessage): Request { + const host = req.headers.host ?? 'mcp.invalid'; + const headers = new Headers(); + for (const [name, value] of Object.entries(req.headers)) { + if (value === undefined) { + continue; + } + // A header node parsed as a list arrives as an array; `set-cookie` is the + // only one it always does that for, and a request carries none. + for (const one of Array.isArray(value) ? value : [value]) { + headers.append(name, one); + } + } + return new Request(new URL(req.url ?? '/', `https://${host}`), { + method: req.method ?? 'POST', + headers, + }); +} + +/** + * The transport's web `Response`, written to the node response the host gave us. + * + * Read in full and handed to `end` in one call, rather than written chunk by + * chunk. `enableJsonResponse` makes every exchange a single JSON body, so there + * is no stream to preserve, and the two ways of pacing chunks against a + * consumer are each unavailable somewhere this runs: + * + * - `drain` is never emitted under `@vendia/serverless-express`, whose response + * carries a socket stand-in with `on` set to `Function.prototype`. + * - the `write` callback is dropped by `compression`, which replaces `write` + * with a two-parameter version that forwards to a zlib stream and returns. + * + * Either one silently never resolves, which on the api Lambda is a hung + * invocation and a 502 rather than an answer. Waiting on neither is what makes + * this behave the same under a socket, a compression middleware, and the + * Lambda's stand-in. + * + * If a later change turns `enableJsonResponse` off to stream notifications, + * this has to write through as chunks arrive — and `compression` on the same + * route has to learn about SSE at the same time, for the same reason. + */ +async function writeNodeResponse( + res: ServerResponse, + response: Response +): Promise { + res.statusCode = response.status; + response.headers.forEach((value, name) => { + res.setHeader(name, value); }); + + if (!response.body) { + res.end(); + return; + } + + const body = Buffer.from(await response.arrayBuffer()); + res.end(body.length > 0 ? body : undefined); } diff --git a/mcp-server/src/index.ts b/mcp-server/src/index.ts index e05556e..3e0260a 100644 --- a/mcp-server/src/index.ts +++ b/mcp-server/src/index.ts @@ -7,6 +7,9 @@ export { type RequestContext, type ToolCallReport, } from './lib/context'; +// Part of the same contract: the host marks a dispatched read its own deadline +// stopped, and the retry loop here reads the mark. +export { DEADLINE_EXCEEDED_HEADER } from './lib/request'; export { setLogger, type LogSink } from './lib/logger'; export { handleMcpRequest } from './http'; export { isToolGranted, type McpTool, type ToolScope } from './lib/tool'; diff --git a/mcp-server/src/lib/concurrency.test.ts b/mcp-server/src/lib/concurrency.test.ts new file mode 100644 index 0000000..649200a --- /dev/null +++ b/mcp-server/src/lib/concurrency.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest'; +import { mapWithConcurrency } from './concurrency'; + +/** Records how many calls overlapped, so a limit can be asserted on. */ +const tracking = () => { + const state = { running: 0, peak: 0, order: [] as number[] }; + const run = async (item: number) => { + state.running += 1; + state.peak = Math.max(state.peak, state.running); + state.order.push(item); + await new Promise((resolve) => setTimeout(resolve, 1)); + state.running -= 1; + return item * 2; + }; + return { state, run }; +}; + +describe('mapWithConcurrency', () => { + it('answers in the order the items were given', async () => { + const { run } = tracking(); + await expect(mapWithConcurrency([1, 2, 3, 4, 5], 2, run)).resolves.toEqual([ + 2, 4, 6, 8, 10, + ]); + }); + + it('runs no more than the limit at once', async () => { + const { state, run } = tracking(); + await mapWithConcurrency([1, 2, 3, 4, 5, 6, 7, 8], 3, run); + expect(state.peak).toBe(3); + }); + + it('starts every item', async () => { + const { state, run } = tracking(); + await mapWithConcurrency([1, 2, 3, 4, 5, 6, 7], 2, run); + expect(state.order.sort((a, b) => a - b)).toEqual([1, 2, 3, 4, 5, 6, 7]); + }); + + it('runs one at a time for a limit below one', async () => { + const { state, run } = tracking(); + await mapWithConcurrency([1, 2, 3], 0, run); + expect(state.peak).toBe(1); + }); + + it('answers nothing for no items', async () => { + const { state, run } = tracking(); + await expect(mapWithConcurrency([], 4, run)).resolves.toEqual([]); + expect(state.peak).toBe(0); + }); + + it('rejects with what an item threw', async () => { + await expect( + mapWithConcurrency([1, 2, 3], 2, async (item) => { + if (item === 2) { + throw new Error('boom'); + } + return item; + }) + ).rejects.toThrow('boom'); + }); + + // Anything still running once this has answered is outside the limit, which + // is the whole of what the caller asked for. + it('leaves nothing running once it has answered', async () => { + let running = 0; + let settled = false; + let ranAfterSettling = 0; + + const pending = mapWithConcurrency( + [1, 2, 3, 4, 5, 6, 7, 8], + 3, + async (item) => { + running += 1; + await new Promise((resolve) => setTimeout(resolve, 5)); + if (settled) { + ranAfterSettling += 1; + } + running -= 1; + if (item === 2) { + throw new Error('boom'); + } + return item; + } + ); + + await expect(pending).rejects.toThrow('boom'); + settled = true; + expect(running).toBe(0); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(ranAfterSettling).toBe(0); + }); + + it('stops claiming items once one has failed', async () => { + const started: number[] = []; + + await expect( + mapWithConcurrency([1, 2, 3, 4, 5, 6, 7, 8], 2, async (item) => { + started.push(item); + await new Promise((resolve) => setTimeout(resolve, 1)); + if (item === 1) { + throw new Error('boom'); + } + return item; + }) + ).rejects.toThrow('boom'); + + // The two in the first wave, and nothing claimed after the failure. + expect(started).toEqual([1, 2]); + }); + + it('throws the first rejection, not a later one', async () => { + await expect( + mapWithConcurrency([1, 2, 3, 4], 4, async (item) => { + await new Promise((resolve) => setTimeout(resolve, item)); + throw new Error(`boom ${item}`); + }) + ).rejects.toThrow('boom 1'); + }); +}); diff --git a/mcp-server/src/lib/concurrency.ts b/mcp-server/src/lib/concurrency.ts new file mode 100644 index 0000000..a22afb1 --- /dev/null +++ b/mcp-server/src/lib/concurrency.ts @@ -0,0 +1,49 @@ +/** + * `items.map(run)` with at most `limit` running at once, answering the results + * in the order the items were given. + * + * Its own few lines rather than a dependency: this package is published to + * npm, where every dependency is one a consumer installs, and the four it has + * today are the SDK, zod and two workspace packages. + * + * On a rejection it stops claiming items and then waits for the ones already + * running, rather than rejecting the moment the first one fails. A bare + * `Promise.all` answers the caller while its siblings are still going, so the + * limit stops meaning anything the moment one item fails — the calls it was + * holding back carry on outside it, against a host that has already been told + * the work is over. The first rejection is what it throws; a later one is + * dropped, the way `Promise.all` drops it. + */ +export async function mapWithConcurrency( + items: readonly T[], + limit: number, + run: (item: T, index: number) => Promise +): Promise { + const results = new Array(items.length); + // A container rather than a `let`: the workers assign it and the check below + // reads it after awaiting them, which narrowing on a local would not see. + const first: { failure?: { error: unknown } } = {}; + let next = 0; + + const worker = async () => { + for (let index = next++; index < items.length; index = next++) { + if (first.failure) { + return; + } + try { + results[index] = await run(items[index], index); + } catch (error) { + first.failure ??= { error }; + return; + } + } + }; + + await Promise.all( + Array.from({ length: Math.min(Math.max(1, limit), items.length) }, worker) + ); + if (first.failure) { + throw first.failure.error; + } + return results; +} diff --git a/mcp-server/src/lib/instructions.test.ts b/mcp-server/src/lib/instructions.test.ts index e6c21e5..088dbba 100644 --- a/mcp-server/src/lib/instructions.test.ts +++ b/mcp-server/src/lib/instructions.test.ts @@ -169,3 +169,25 @@ describe('scopes that reach no tool', () => { expect(SCOPES_WITHOUT_TOOLS).not.toContain('results:read'); }); }); + +describe('the skills line', () => { + it('names every skill and warns that a step may be out of reach', () => { + const text = buildServerInstructions({ apiKeyScope: 'write' }, [ + { name: 'collect-evidence' }, + { name: 'browser-evidence' }, + ]); + + expect(text).toContain('collect-evidence, browser-evidence'); + expect(text).toContain('does not reach'); + }); + + // A deployment whose skills did not ship serves every tool and no skill + // (`host/assets.ts`), and must not answer with a sentence naming none. + it('is left out when no skill shipped', () => { + const text = buildServerInstructions({ apiKeyScope: 'write' }, []); + + expect(text).not.toContain('prompts'); + expect(text).toBe(text.trimEnd()); + expect(text).toBe(buildServerInstructions({ apiKeyScope: 'write' })); + }); +}); diff --git a/mcp-server/src/lib/instructions.ts b/mcp-server/src/lib/instructions.ts index cffe9cd..125e8bd 100644 --- a/mcp-server/src/lib/instructions.ts +++ b/mcp-server/src/lib/instructions.ts @@ -1,4 +1,5 @@ import type { ApiKeyScope, OAuthApiScope } from '../host/scopes'; +import type { Skill } from '../skills'; import type { RequestContext } from './context'; /** @@ -88,6 +89,26 @@ const MISSING_TOOL_LINE = [ const API_KEY_LINE = 'This connection uses a Currents API key, which carries no scopes: it is read or write, and that decides every call at the REST API.'; +/** + * Names the skills, which `prompts/list` carries but nothing puts in front of + * the model before it has called anything. + * + * Named whatever the credential holds, unlike the tools above: a skill + * declares no scopes, so the alternative is withholding a workflow that is + * mostly readable from a connection missing one step. The caveat is stated + * instead, because a step refused halfway through is the confusing outcome — + * `currents-create-trace-link` is the live case, gated on a scope and an + * organization flag. + */ +const skillsLine = (skills: readonly Pick[]): string => + skills.length + ? `Multi-step workflows are published as prompts, one per skill: ${skills + .map((skill) => skill.name) + .join( + ', ' + )}. A prompt returns the whole workflow. Read the one that fits the task before calling tools for it, because the steps have an order. A workflow may name a tool this connection does not reach.` + : ''; + /** * The `instructions` a client gets back from `initialize` and a host puts in * front of the model — Claude Code renders it into the system prompt beside the @@ -98,6 +119,15 @@ const API_KEY_LINE = * called. */ export function buildServerInstructions( + context: Pick, + skills: readonly Pick[] = [] +): string { + return [credentialInstructions(context), skillsLine(skills)] + .filter(Boolean) + .join('\n\n'); +} + +function credentialInstructions( context: Pick ): string { if (context.oauthScopes !== undefined) { diff --git a/mcp-server/src/lib/request.test.ts b/mcp-server/src/lib/request.test.ts index a0a7911..6856d8c 100644 --- a/mcp-server/src/lib/request.test.ts +++ b/mcp-server/src/lib/request.test.ts @@ -8,6 +8,7 @@ import { fetchCursorBasedPaginatedApi, postApi, putApi, + DEADLINE_EXCEEDED_HEADER, REQUEST_TIMEOUT_MS, } from './request'; @@ -458,6 +459,121 @@ describe('retries', () => { expect(logged).toContain('HTTP 500'); expect(logged).not.toContain('author@example.com'); }); + + // The host ran this query for its whole deadline. A second attempt gets a + // fresh one and runs the same query again, which is the 51s against a 25s + // deadline ENG-1480 measured. + it('does not send a read again that the host stopped at its deadline', async () => { + global.fetch = vi.fn().mockResolvedValue( + respondWith(504, '{"status":"FAILED","error":"Timeout error."}', { + [DEADLINE_EXCEEDED_HEADER]: '25000', + }) + ); + + expect(await fetchApi('/tests/p1')).toMatchObject({ + ok: false, + status: 504, + deadlineMs: 25000, + }); + expect(global.fetch).toHaveBeenCalledOnce(); + expect(backoffs).toEqual([]); + }); + + // The refusal above is the marker's and not the status's: a 504 from a proxy + // in front of the API ran nothing for 25s, and a later attempt may get past + // it. + it('retries a 504 that carries no marker', async () => { + global.fetch = vi + .fn() + .mockResolvedValueOnce(respondWith(504, '')) + .mockResolvedValueOnce(okJson({ id: 1 })); + + expect(await fetchApi('/tests/p1')).toEqual({ ok: true, data: { id: 1 } }); + expect(backoffs).toEqual([300]); + }); + + // The broken stream says nothing a 401 or a 404 has not already settled, so + // the status is kept rather than weighed as no response at all. + it.each([401, 403, 404])( + 'does not send a read again whose %i had an unreadable body', + async (status) => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status, + headers: new Headers(), + text: async () => { + throw new Error('terminated'); + }, + }); + + expect(await fetchApi('/tests/p1')).toMatchObject({ + ok: false, + status, + error: 'terminated', + }); + expect(global.fetch).toHaveBeenCalledOnce(); + expect(backoffs).toEqual([]); + } + ); + + // The body breaking says nothing about why the API gave up, and the marker + // says the host already spent its whole deadline — so a second attempt buys + // the same 25s of query it just abandoned. + it('does not send a read again whose marked response broke mid-body', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 504, + headers: new Headers({ [DEADLINE_EXCEEDED_HEADER]: '25000' }), + text: async () => { + throw new Error('terminated'); + }, + }); + + expect(await fetchApi('/tests/p1')).toMatchObject({ + ok: false, + status: 504, + error: 'terminated', + deadlineMs: 25000, + }); + expect(global.fetch).toHaveBeenCalledOnce(); + expect(backoffs).toEqual([]); + }); + + // The marker and not the response is what makes it terminal: a stream that + // broke under a 200 is the likeliest transient failure on a large read, and + // still gets its attempts. + it('still sends a read again whose unmarked response broke mid-body', async () => { + const broken = { + ok: false, + status: 504, + headers: new Headers(), + text: async () => { + throw new Error('terminated'); + }, + }; + global.fetch = vi + .fn() + .mockResolvedValueOnce(broken) + .mockResolvedValueOnce(okJson({ id: 1 })); + + expect(await fetchApi('/tests/p1')).toEqual({ ok: true, data: { id: 1 } }); + expect(backoffs).toEqual([300]); + }); + + // The marker is what stops it, so a value that says nothing about how long + // the query got still stops it. + it('does not send one again whose marker carries no readable deadline', async () => { + global.fetch = vi + .fn() + .mockResolvedValue( + respondWith(504, '', { [DEADLINE_EXCEEDED_HEADER]: '' }) + ); + + const failure = await fetchApi('/tests/p1'); + + expect(global.fetch).toHaveBeenCalledOnce(); + expect(failure).not.toHaveProperty('deadlineMs'); + }); }); /** diff --git a/mcp-server/src/lib/request.ts b/mcp-server/src/lib/request.ts index e3f5565..5cdd454 100644 --- a/mcp-server/src/lib/request.ts +++ b/mcp-server/src/lib/request.ts @@ -62,6 +62,11 @@ export interface ApiFailure { * `DispatchTimeout` left a handler running with it. */ received?: 'no' | 'unknown'; + /** + * The deadline the host stopped this read at, in milliseconds. Set from + * `DEADLINE_EXCEEDED_HEADER`, and absent for every other failure. + */ + deadlineMs?: number; } export type ApiResult = { ok: true; data: T } | ApiFailure; @@ -176,14 +181,55 @@ export class DispatchTimeout extends Error { readonly path: string; } +/** + * Set by the host on a dispatched read its own deadline stopped, carrying that + * deadline in milliseconds. + * + * `DispatchTimeout` above refuses to re-send a read that never came back at + * all. This one did come back, as a 5xx like any other — which on a read + * `retryDelay` sends again, twice, each attempt getting a fresh deadline and + * running the query the first one already proved too slow. Measured on staging + * as 51s against a 25s deadline, and as three `TIMEOUT_EXCEEDED` rows in + * ClickHouse for one tool call (ENG-1480). + * + * A header rather than the 504 it comes with, because a 504 from a proxy in + * front of the API is a different failure: nothing ran for 25s behind it, and + * a second attempt may well get past it. + */ +export const DEADLINE_EXCEEDED_HEADER = 'x-currents-deadline-exceeded'; + +/** + * Whether the host marked this response as one its own deadline stopped. + * + * Separate from `deadlineMsOf` because the two questions have different + * answers: a marker carrying nothing readable still says the work was spent, + * and only the sentence the caller reads needs the number. + */ +function deadlineExceeded(response: Response): boolean { + return response.headers.has(DEADLINE_EXCEEDED_HEADER); +} + +/** The deadline off a response the host marked, or null for anything else. */ +function deadlineMsOf(response: Response): number | null { + const header = response.headers.get(DEADLINE_EXCEEDED_HEADER); + if (header === null) { + return null; + } + const ms = Number(header); + // Absent or unreadable, the refusal above still stands: `retryDelay` reads + // the header itself, and only the sentence the caller reads needs the number. + return Number.isFinite(ms) && ms > 0 ? ms : null; +} + /** * Answers the dispatch, or fails the call once the deadline passes. * * What this frees is the caller, not the process: nothing here can stop the - * handler, which goes on holding whatever it holds until it finishes on its - * own. Bounding the work itself is the query timeout in `connection.ts` - * (ENG-1427), and this logs so that a stall leaves a trace either way — the - * dispatched request is answered by nothing that writes an access log. + * handler. Bounding the work itself is the host's job — the hosted server puts + * a shorter deadline on the request that the mongo and ClickHouse clients read, + * so the queries stop before this fires. This logs so that a stall leaves a + * trace either way: the dispatched request is answered by nothing that writes + * an access log. */ function withDeadline( request: ApiRequest, @@ -233,9 +279,15 @@ export function failureFromResponse( status: response.status, body, ...challengeOf(response), + ...deadlineOf(response), }; } +function deadlineOf(response: Response): Pick { + const deadlineMs = deadlineMsOf(response); + return deadlineMs === null ? {} : { deadlineMs }; +} + function challengeOf(response: Response): Pick { const challenge = response.headers.get('www-authenticate'); return challenge ? { challenge } : {}; @@ -297,6 +349,10 @@ export function failureFromBrokenBody( body: null, error: describeError(error), ...challengeOf(response), + // Carried here as well as in `failureFromResponse`: the body breaking says + // nothing about why the API gave up, and without this the caller reads a + // 504 with no sentence telling it what to narrow. + ...deadlineOf(response), }; } @@ -437,10 +493,22 @@ export async function callApiWithRetries( try { return { response, text: await response.text() }; } catch (error: unknown) { - // Weighed as a call that produced no response at all, which is the - // failure it is: nothing usable arrived, and the status that did cannot - // say whether sending it again is safe. - const delay = retryDelay(method, attempt, null); + // A 2xx whose body broke is weighed as a call that produced no response + // at all: nothing usable arrived, and a 200 cannot say whether the write + // it acknowledged was carried out. On a large read that break is the + // commonest transient failure there is, and the status check cannot see + // it, so it keeps its attempts. + // + // A non-2xx reaching here was already weighed by the status check above + // and left unretried, so handing the response back keeps that answer: a + // 401 or a 404 says the same thing on the next attempt. `null` would + // discard it and turn the refusal into a retriable "no response". + // + // The marker is read first and separately from either, because whether + // the host spent its whole deadline does not depend on the body arriving. + const delay = deadlineExceeded(response) + ? null + : retryDelay(method, attempt, response.ok ? null : response); if (delay === null) { return { response, bodyError: error }; } @@ -472,6 +540,12 @@ function retryDelay( if (attempt >= MAX_RETRIES) { return null; } + // Before the rules below rather than as an exception to them: the host ran + // this query for its whole deadline, so there is no attempt left that could + // get past what stopped it. See `DEADLINE_EXCEEDED_HEADER`. + if (response && deadlineExceeded(response)) { + return null; + } const status = response?.status ?? null; const transient = status === 429 || @@ -492,7 +566,7 @@ function retryDelay( /** * `Retry-After` as delta-seconds, which is the form the API sends it in - * (`api/aiShare/rateLimit.ts`). An HTTP-date is left unread and the backoff + * (`api/share/rateLimit.ts`). An HTTP-date is left unread and the backoff * stands in for it. */ function retryAfterMs(response: Response): number | null { diff --git a/mcp-server/src/lib/toolResult.test.ts b/mcp-server/src/lib/toolResult.test.ts index dadf971..9efbd9c 100644 --- a/mcp-server/src/lib/toolResult.test.ts +++ b/mcp-server/src/lib/toolResult.test.ts @@ -40,6 +40,52 @@ describe('describeApiFailure', () => { ).toBe('GET /runs/run-1: HTTP 502 body unreadable (terminated)'); }); + // A 504 alone reads as an outage, and a caller that takes it for one calls + // the identical tool again for the identical answer. + it('says what a caller can narrow when the API stopped the query', () => { + expect( + describeApiFailure({ + ok: false, + method: 'GET', + path: '/tests/p1?date_start=2020-01-01', + status: 504, + body: { status: 'FAILED', error: 'Timeout error.' }, + deadlineMs: 25_000, + }) + ).toBe( + 'GET /tests/p1?date_start=2020-01-01: HTTP 504 {"status":"FAILED","error":"Timeout error."}' + + ' The API stopped this query after 25s. Narrow it - a shorter date range,' + + ' fewer branches or tags, a smaller limit - and call this tool again.' + ); + }); + + // A deadline set below a second, which only a test environment does, would + // otherwise report itself as no time at all. + it('never reports the query as having had no time', () => { + expect( + describeApiFailure({ + ok: false, + method: 'GET', + path: '/tests/p1', + status: 504, + body: null, + deadlineMs: 1, + }) + ).toContain('stopped this query after 1s'); + }); + + it('leaves a 504 the host did not mark to speak for itself', () => { + expect( + describeApiFailure({ + ok: false, + method: 'GET', + path: '/tests/p1', + status: 504, + body: null, + }) + ).toBe('GET /tests/p1: HTTP 504'); + }); + it('reports a request that never got a response', () => { expect( describeApiFailure({ diff --git a/mcp-server/src/lib/toolResult.ts b/mcp-server/src/lib/toolResult.ts index 66cfaa3..7a9e311 100644 --- a/mcp-server/src/lib/toolResult.ts +++ b/mcp-server/src/lib/toolResult.ts @@ -41,7 +41,29 @@ export function describeApiFailure( const described = detail ? `${call}: HTTP ${failure.status} ${detail}` : `${call}: HTTP ${failure.status}`; - return `${described}${remediation(failure)}`; + return `${described}${remediation(failure)}${deadlineAdvice(failure)}`; +} + +/** + * What the caller can change about a read the API stopped at its deadline. + * + * The status alone reads as an outage, and a caller that takes it for one calls + * the identical tool again — which gets the same deadline and the same answer. + * Nothing retries it on the caller's behalf: `retryDelay` refuses a read the + * host already spent its whole deadline on, so this sentence is the only place + * the caller is told that a narrower request is what makes it answerable. + * + * Never set alongside `remediation`: a deadline is not a refusal, so the + * response carries no `WWW-Authenticate` for that to read. + */ +function deadlineAdvice(failure: ApiFailure): string { + if (failure.deadlineMs === undefined) { + return ''; + } + // Seconds, with a floor, so a deadline set below a second in a test + // environment does not report itself as no time at all. + const seconds = Math.max(1, Math.round(failure.deadlineMs / 1000)); + return ` The API stopped this query after ${seconds}s. Narrow it - a shorter date range, fewer branches or tags, a smaller limit - and call this tool again.`; } /** diff --git a/mcp-server/src/server.test.ts b/mcp-server/src/server.test.ts index fd41bfb..5d7ecbc 100644 --- a/mcp-server/src/server.test.ts +++ b/mcp-server/src/server.test.ts @@ -13,11 +13,23 @@ const { registeredTools, registeredResources, serverOptions } = vi.hoisted( mimeType?: string; read: () => { contents: Array<{ uri: string; text: string }> }; }> = []; + const registeredPrompts: Array<{ + name: string; + description?: string; + get: () => { + messages: Array<{ content: { type: string; text: string } }>; + }; + }> = []; const serverOptions: Array<{ info: Record; options?: Record; }> = []; - return { registeredTools, registeredResources, serverOptions }; + return { + registeredTools, + registeredResources, + registeredPrompts, + serverOptions, + }; } ); @@ -58,6 +70,10 @@ vi.mock('@modelcontextprotocol/sdk/server/mcp.js', () => ({ ) { registeredResources.push({ name, uri, mimeType: opts.mimeType, read }); } + + // What the prompts are is `http.test.ts`, over the transport. Without the + // method the factory throws here and every test in this file fails. + registerPrompt() {} }, })); @@ -138,6 +154,8 @@ const EXPECTED_ANNOTATIONS: Record> = { // Each call mints another link to the same trace, and the ones already // handed out keep working. 'currents-create-trace-link': { r: false, d: false, i: false, o: false }, + // Each call records another run; the ones already recorded are untouched. + 'currents-create-session': { r: false, d: false, i: false, o: false }, 'currents-list-webhooks': { r: true, d: false, i: true, o: false }, 'currents-create-webhook': { r: false, d: false, i: false, o: true }, 'currents-get-webhook': { r: true, d: false, i: true, o: false }, @@ -276,27 +294,22 @@ describe('tools registered by scope', () => { return registeredTools.map((t) => t.name); }; - // `currents-create-trace-link` is the one tool behind an org feature flag; - // the rest of this suite passes no flags, which is the stdio case. - describe('a tool behind an org feature flag', () => { - const FLAGGED = 'currents-create-trace-link'; - - it('is listed to a results:read token when the flag is on', () => { + // The tools behind an org feature flag; the rest of this suite passes no + // flags, which is the stdio case. + describe.each([ + ['currents-create-trace-link', 'results:read' as const], + ['currents-create-session', 'runs:write' as const], + ])('%s, behind an org feature flag', (FLAGGED, SCOPE) => { + it('is listed to a token carrying its scope when the flag is on', () => { expect( toolsFor({ - oauthScopes: ['results:read'], + oauthScopes: [SCOPE], orgFeatures: { evidenceSharing: true }, }) ).toContain(FLAGGED); }); - it('is listed to an API key when the flag is on', () => { - expect( - toolsFor({ - apiKeyScope: 'read', - orgFeatures: { evidenceSharing: true }, - }) - ).toContain(FLAGGED); + it('is listed to a write API key when the flag is on', () => { expect( toolsFor({ apiKeyScope: 'write', @@ -305,17 +318,28 @@ describe('tools registered by scope', () => { ).toContain(FLAGGED); }); + // A read key reaches the flagged tools whose route takes one, and only + // those — the flag does not change which key a route asks for. + it('follows its route on a read API key', () => { + const names = toolsFor({ + apiKeyScope: 'read', + orgFeatures: { evidenceSharing: true }, + }); + + expect(names.includes(FLAGGED)).toBe(SCOPE === 'results:read'); + }); + it('is withheld when the flag is off, whatever the credential', () => { - expect( - toolsFor({ oauthScopes: ['results:read'], orgFeatures: {} }) - ).not.toContain(FLAGGED); + expect(toolsFor({ oauthScopes: [SCOPE], orgFeatures: {} })).not.toContain( + FLAGGED + ); expect(toolsFor({ apiKeyScope: 'write', orgFeatures: {} })).not.toContain( FLAGGED ); }); // The flag does not stand in for the scope its route names. - it('is withheld from a token without results:read, flag or not', () => { + it('is withheld from a token without that scope, flag or not', () => { expect( toolsFor({ oauthScopes: ['webhooks:read'], @@ -409,13 +433,28 @@ describe('instructions passed to the client', () => { it('describes the credential the server was built for', () => { expect(instructionsFor({ oauthScopes: ['results:read'] })).toBe( - buildServerInstructions({ oauthScopes: ['results:read'] }) + buildServerInstructions({ oauthScopes: ['results:read'] }, getSkills()) ); expect(instructionsFor({ apiKeyScope: 'read' })).toBe( - buildServerInstructions({ apiKeyScope: 'read' }) + buildServerInstructions({ apiKeyScope: 'read' }, getSkills()) ); }); + // A skill declares no scopes, so this names them for every credential — + // `lib/instructions.ts` says why, and the line carries the caveat. + it('names the skills whatever the credential', () => { + for (const context of [ + { oauthScopes: ['results:read'] as const }, + { apiKeyScope: 'read' as const }, + {}, + ]) { + const instructions = instructionsFor(context); + for (const skill of getSkills()) { + expect(instructions).toContain(skill.name); + } + } + }); + // The stdio server passes no context, and a client that gets an empty // `instructions` back learns nothing about the key it is calling with. it('is set for a caller with neither credential', () => { diff --git a/mcp-server/src/server.ts b/mcp-server/src/server.ts index adddb7b..947f54e 100644 --- a/mcp-server/src/server.ts +++ b/mcp-server/src/server.ts @@ -10,7 +10,7 @@ import { buildServerInstructions } from './lib/instructions'; import { isToolGranted, McpTool } from './lib/tool'; import { reportToolCall } from './lib/toolCallReport'; import { listTools, type CatalogTool } from './lib/toolList'; -import { registerSkills } from './skills'; +import { getSkills, registerSkills } from './skills'; // Actions tools import { createActionTool } from './tools/actions/create-action'; import { deleteActionTool } from './tools/actions/delete-action'; @@ -54,6 +54,8 @@ import { getTestSignatureTool } from './tools/tests/get-tests-signature'; import { getErrorsExplorerTool } from './tools/errors/get-errors-explorer'; // Evidence tools import { getTestEvidenceTool } from './tools/evidence/get-test-evidence'; +// Sessions tools +import { createSessionTool } from './tools/sessions/create-session'; // Traces tools import { createTraceLinkTool } from './tools/traces/create-trace-link'; // Webhooks tools @@ -460,6 +462,15 @@ export const TOOL_CATALOG: CatalogTool[] = [ }, createTraceLinkTool ), + catalogTool( + 'currents-create-session', + { + description: + "Record a browser session you drove as a Currents run, so its evidence can be read and shared like a CI run's. Use it when there is no test to run — a bug reproduced by hand, a fix demonstrated in a browser. Returns the run and an upload URL per file you declared; PUT the bytes to those, and the response says what to do next. A trace attached this way can then be turned into a link that needs no Currents credential.", + annotations: additiveWrite, + }, + createSessionTool + ), // Webhooks API tools catalogTool( 'currents-list-webhooks', @@ -558,7 +569,7 @@ export function createMcpServer( ] : undefined, }, - { instructions: buildServerInstructions(context) } + { instructions: buildServerInstructions(context, getSkills()) } ); const granted = TOOL_CATALOG.filter((entry) => @@ -588,6 +599,10 @@ export function createMcpServer( tools: listTools(granted), })); + // After any handler the factory sets by hand, and it has to stay there: the + // SDK throws when `registerPrompt` finds `prompts/get` already handled, and + // the server is built per request, so that would be a 500 on every one. The + // tools override above has the opposite constraint. registerSkills(server); return server; diff --git a/mcp-server/src/skills.test.ts b/mcp-server/src/skills.test.ts index 38d71cd..c3fe4f5 100644 --- a/mcp-server/src/skills.test.ts +++ b/mcp-server/src/skills.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { getSkills, skillFileUri } from './skills'; +import { + FILE_CLOSE_TAG, + getSkills, + skillDocument, + skillFileUri, + type Skill, +} from './skills'; describe('skills manifest', () => { it('inlines at least one skill', () => { @@ -21,6 +27,16 @@ describe('skills manifest', () => { } }); + // `skillDocument` writes each file into a `` block. A + // path holding a quote or a bracket, or content holding the closing tag, + // makes the boundary ambiguous to the model and fails nowhere else. + it('holds nothing that would break a file block', () => { + for (const file of skill.files) { + expect(file.path).not.toMatch(/["<>]/); + expect(file.content, `${file.path}`).not.toContain(FILE_CLOSE_TAG); + } + }); + it('references only files that ship with the skill', () => { const shipped = new Set(skill.files.map((f) => f.path)); const entryPoint = skill.files.find((f) => f.path === 'SKILL.md'); @@ -43,3 +59,47 @@ describe('skill resource URIs', () => { ); }); }); + +describe('skillDocument', () => { + const skill = (files: Skill['files']): Skill => ({ + name: 'test-skill', + description: 'a skill', + files, + }); + + // Built with the references first, which is the order `getSkills` returns + // on a machine whose collation sorts `references/` ahead of `SKILL.md`. + it('leads with the entry point and keeps the rest in order', () => { + const document = skillDocument( + skill([ + { path: 'references/a.md', content: 'A' }, + { path: 'references/b.md', content: 'B' }, + { path: 'SKILL.md', content: 'the workflow' }, + ]) + ); + + expect( + [...document.matchAll(//g)].map((m) => m[1]) + ).toEqual(['SKILL.md', 'references/a.md', 'references/b.md']); + }); + + it('carries the content of every file', () => { + const document = skillDocument( + skill([ + { path: 'SKILL.md', content: 'the workflow' }, + { path: 'references/a.md', content: 'the appendix' }, + ]) + ); + + expect(document).toContain('the workflow'); + expect(document).toContain('the appendix'); + }); + + it('serves a skill that has no references', () => { + const document = skillDocument( + skill([{ path: 'SKILL.md', content: 'the workflow' }]) + ); + + expect(document).toBe('\nthe workflow\n'); + }); +}); diff --git a/mcp-server/src/skills.ts b/mcp-server/src/skills.ts index 4296219..038a4f3 100644 --- a/mcp-server/src/skills.ts +++ b/mcp-server/src/skills.ts @@ -8,22 +8,52 @@ export type Skill = { name: string; description: string; files: SkillFile[] }; export const SKILL_MIME_TYPE = 'text/markdown'; +const ENTRY_POINT = 'SKILL.md'; + +/** The delimiter `skillDocument` writes, which no file content may hold. */ +export const FILE_CLOSE_TAG = ''; + export function skillFileUri(skillName: string, filePath: string): string { return `skill://currents/${skillName}/${filePath}`; } /** - * Publishes each skill's markdown as an MCP resource. + * The whole skill as one document. + * + * The entry point leads whatever order `getSkills` returned, because it is + * what links to the rest: a reader handed `references/x.md` first reaches the + * appendix before the workflow it belongs to. + * + * Each file is wrapped in a `` tag naming its path, so a + * `](references/x.md)` link in the entry point names something in the same + * message. A file whose own content held the closing tag would end its block + * early, which `skills.test.ts` rejects at the source. + */ +export function skillDocument(skill: Skill): string { + return [...skill.files] + .sort( + (a, b) => Number(b.path === ENTRY_POINT) - Number(a.path === ENTRY_POINT) + ) + .map((file) => `\n${file.content}\n`) + .join('\n\n'); +} + +/** + * Publishes each skill's markdown as an MCP resource, and each skill as a + * prompt. * - * The MCP SDK has no skill primitive, so resources are the only way an agent - * can read a skill off the server. Without this, the skills are reachable only - * by cloning the repo and copying the directory into the agent's skills folder. + * The MCP SDK has no skill primitive. A resource is read only by an agent + * that goes looking for it; a prompt is listed at the handshake, which is + * where a host can show one — Claude Code renders it as a slash command. + * + * The prompt carries every file rather than the entry point alone, because a + * reference left behind a second fetch is one a host has no reason to make. */ export function registerSkills(server: McpServer): void { for (const skill of getSkills()) { for (const file of skill.files) { const uri = skillFileUri(skill.name, file.path); - const isEntryPoint = file.path === 'SKILL.md'; + const isEntryPoint = file.path === ENTRY_POINT; server.registerResource( `${skill.name}/${file.path}`, uri, @@ -39,5 +69,18 @@ export function registerSkills(server: McpServer): void { }) ); } + + server.registerPrompt( + skill.name, + { description: skill.description }, + () => ({ + messages: [ + { + role: 'user', + content: { type: 'text', text: skillDocument(skill) }, + }, + ], + }) + ); } } diff --git a/mcp-server/src/tools/evidence/get-test-evidence.test.ts b/mcp-server/src/tools/evidence/get-test-evidence.test.ts index 055ccc0..f5932b4 100644 --- a/mcp-server/src/tools/evidence/get-test-evidence.test.ts +++ b/mcp-server/src/tools/evidence/get-test-evidence.test.ts @@ -270,3 +270,76 @@ describe('getTestEvidenceTool', () => { expect(manifest.specs[0].instanceId).toBe('inst-1'); }); }); + +/** + * The reads are bounded so one tool call cannot fan out to 25 concurrent `/v1` + * dispatches. Against a host that caps dispatches, an unbounded fan-out filled + * the cap and then refused its own reads, which land in the manifest as an + * `error` on the spec rather than as a failed tool call. + */ +describe('the instance read fan-out', () => { + const manySpecs = (count: number) => ({ + data: { + ...runPayload.data, + specs: Array.from({ length: count }, (_, i) => ({ + instanceId: `inst-${i}`, + spec: `e2e/spec-${i}.spec.ts`, + })), + }, + }); + + it('keeps at most five instance reads in flight', async () => { + let running = 0; + let peak = 0; + vi.mocked(request.fetchApi).mockImplementation(async (path: string) => { + if (path === '/runs/run-1') return ok(manySpecs(25)); + if (!path.startsWith('/instances/')) return failed(path, 404); + running += 1; + peak = Math.max(peak, running); + await new Promise((resolve) => setTimeout(resolve, 1)); + running -= 1; + return ok(instancePayload); + }); + + await getTestEvidenceTool.handler({ runId: 'run-1', maxInstances: 25 }); + + expect(peak).toBe(5); + }); + + it('still reads every selected spec', async () => { + vi.mocked(request.fetchApi).mockImplementation(async (path: string) => { + if (path === '/runs/run-1') return ok(manySpecs(12)); + if (path.startsWith('/instances/')) return ok(instancePayload); + return failed(path, 404); + }); + + const result = await getTestEvidenceTool.handler({ + runId: 'run-1', + maxInstances: 12, + }); + + const manifest = parseManifest(result); + expect(manifest.specs).toHaveLength(12); + expect(manifest.specs.every((s: any) => !s.error)).toBe(true); + }); + + it('keeps the manifest in the order the specs were selected', async () => { + vi.mocked(request.fetchApi).mockImplementation(async (path: string) => { + if (path === '/runs/run-1') return ok(manySpecs(8)); + if (!path.startsWith('/instances/')) return failed(path, 404); + // Later specs answer first, so the order cannot come from timing. + const index = Number(path.split('inst-')[1]); + await new Promise((resolve) => setTimeout(resolve, (8 - index) % 4)); + return ok(instancePayload); + }); + + const result = await getTestEvidenceTool.handler({ + runId: 'run-1', + maxInstances: 8, + }); + + expect(parseManifest(result).specs.map((s: any) => s.spec)).toEqual( + Array.from({ length: 8 }, (_, i) => `e2e/spec-${i}.spec.ts`) + ); + }); +}); diff --git a/mcp-server/src/tools/evidence/get-test-evidence.ts b/mcp-server/src/tools/evidence/get-test-evidence.ts index 5fdd37e..8c6b9ef 100644 --- a/mcp-server/src/tools/evidence/get-test-evidence.ts +++ b/mcp-server/src/tools/evidence/get-test-evidence.ts @@ -1,9 +1,21 @@ import { z } from 'zod'; +import { mapWithConcurrency } from '../../lib/concurrency'; import { fetchApi } from '../../lib/request'; import { apiFailureResult, describeApiFailure } from '../../lib/toolResult'; import { logger } from '../../lib/logger'; import type { McpTool } from '../../lib/tool'; +/** + * How many instance reads this tool has in flight at once. + * + * `maxInstances` allows 25, and issuing them together was 25 `/v1` calls from + * one tool call. Two concurrent calls of it saturated the staging task in the + * ENG-1385 load test, which is a load one caller can produce on its own. The + * manifest still covers every selected spec; only how many are read at once + * changes. + */ +const MAX_CONCURRENT_INSTANCE_READS = 5; + const zodSchema = z.object({ projectId: z .string() @@ -231,8 +243,10 @@ const handler = async ({ const titleFilter = testTitle?.toLowerCase(); const statusFilter = testStatus && testStatus.length > 0 ? testStatus : null; - const specs = await Promise.all( - selectedSpecs.map(async (specEntry) => { + const specs = await mapWithConcurrency( + selectedSpecs, + MAX_CONCURRENT_INSTANCE_READS, + async (specEntry) => { const instanceResponse = await fetchApi<{ data?: any }>( `/instances/${encodeURIComponent(specEntry.instanceId)}` ); @@ -296,7 +310,7 @@ const handler = async ({ tests, ...(hasEvidence(specLevel) ? { specLevelEvidence: specLevel } : {}), }; - }) + } ); const manifest = { diff --git a/mcp-server/src/tools/sessions/create-session.test.ts b/mcp-server/src/tools/sessions/create-session.test.ts new file mode 100644 index 0000000..98a946c --- /dev/null +++ b/mcp-server/src/tools/sessions/create-session.test.ts @@ -0,0 +1,206 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import * as request from '../../lib/request'; +import { createSessionTool } from './create-session'; + +vi.mock('../../lib/request'); + +const RUN = { + runId: 'a1b2c3d4e5f60718', + groupId: 'session', + instanceId: 'inst-1', + testId: 'test-1', + artifacts: [ + { + name: 'trace', + type: 'trace', + artifactId: 'art-1', + uploadUrl: 'https://fs/upload?sig', + }, + ], +}; + +const answer = (data: unknown) => + vi.spyOn(request, 'postApi').mockResolvedValue({ ok: true, data } as never); + +const parse = (result: { content: Array<{ text: string }> }) => + JSON.parse(result.content[0].text); + +const body = { + projectId: 'proj-1', + title: 'the submit button does nothing', + status: 'failed' as const, +}; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('createSessionTool', () => { + it('posts the body to the session route', async () => { + answer({ data: RUN }); + + await createSessionTool.handler({ ...body, durationMs: 12_000 }); + + expect(request.postApi).toHaveBeenCalledWith('/runs/session', { + ...body, + durationMs: 12_000, + }); + }); + + it('returns the run and the upload URLs', async () => { + answer({ data: RUN }); + + const result = parse(await createSessionTool.handler(body)); + + expect(result).toMatchObject({ + runId: RUN.runId, + instanceId: 'inst-1', + testId: 'test-1', + }); + expect(result.artifacts[0].uploadUrl).toBe('https://fs/upload?sig'); + }); + + // The URLs expire and a trace is unreadable until its bytes are there, so + // the order is the part the agent has to get right. + it('says to upload the files and then mint the link', async () => { + answer({ data: RUN }); + + const { nextSteps } = parse(await createSessionTool.handler(body)); + + expect(nextSteps[0]).toContain('uploadUrl'); + expect(nextSteps[1]).toContain('currents-create-trace-link'); + expect(nextSteps[1]).toContain('inst-1'); + expect(nextSteps[1]).toContain('test-1'); + }); + + it('mentions no trace link when no trace was attached', async () => { + answer({ + data: { + ...RUN, + artifacts: [ + { + name: 'shot', + type: 'screenshot', + artifactId: 'art-2', + uploadUrl: 'https://fs/png', + }, + ], + }, + }); + + const { nextSteps } = parse(await createSessionTool.handler(body)); + + expect(nextSteps).toHaveLength(1); + expect(nextSteps[0]).toContain('uploadUrl'); + }); + + it('says nothing to do when the session carried no files', async () => { + answer({ data: { ...RUN, artifacts: [] } }); + + expect(parse(await createSessionTool.handler(body)).nextSteps).toEqual([]); + }); + + it('reports the route failure', async () => { + vi.spyOn(request, 'postApi').mockResolvedValue({ + ok: false, + status: 403, + error: 'Evidence sharing is not enabled for this organization', + } as never); + + const result = await createSessionTool.handler(body); + + expect(result).toMatchObject({ isError: true }); + expect(result.content[0].text).toContain('Failed to record the session'); + }); + + // Every one of these is quoted back as something to call the next tool + // with, so a response missing any of them is an error, not a success. + it.each(['runId', 'instanceId', 'testId'])( + 'fails when the response omits %s', + async (field) => { + const { [field as keyof typeof RUN]: _omitted, ...rest } = RUN; + answer({ data: rest }); + + const result = await createSessionTool.handler(body); + + expect(result).toMatchObject({ isError: true }); + expect(result.content[0].text).toContain('did not identify the run'); + } + ); + + // The agent has to guess the content type of a file it produced; learning it + // was wrong from a 400 is a round trip it can avoid. + it('refuses a content type that does not match the artifact type', () => { + const parsed = createSessionTool.schema.safeParse({ + ...body, + artifacts: [{ name: 'trace', contentType: 'image/png', type: 'trace' }], + }); + + expect(parsed.success).toBe(false); + }); + + it.each([ + ['trace', 'application/zip'], + ['screenshot', 'image/png'], + ['video', 'video/webm'], + ['attachment', 'text/plain'], + ])('accepts a %s declared as %s', (type, contentType) => { + const parsed = createSessionTool.schema.safeParse({ + ...body, + artifacts: [{ name: 'a', contentType, type }], + }); + + expect(parsed.success).toBe(true); + }); + + // The selector is a name, so two traces sharing one leaves the link + // ambiguous — and the tool is what told the agent to select by name. + it('refuses two traces with the same name', () => { + const parsed = createSessionTool.schema.safeParse({ + ...body, + artifacts: [ + { name: 'trace', contentType: 'application/zip', type: 'trace' }, + { name: 'trace', contentType: 'application/zip', type: 'trace' }, + ], + }); + + expect(parsed.success).toBe(false); + }); + + // Only traces are selected by name; nothing picks a screenshot that way. + it('allows two screenshots with the same name', () => { + const parsed = createSessionTool.schema.safeParse({ + ...body, + artifacts: [ + { name: 'step', contentType: 'image/png', type: 'screenshot' }, + { name: 'step', contentType: 'image/png', type: 'screenshot' }, + ], + }); + + expect(parsed.success).toBe(true); + }); + + // With one trace the agent needs no selector; with two it does, or it links + // whichever was stored first. + it('names artifactName only when more than one trace was attached', async () => { + answer({ + data: { + ...RUN, + artifacts: [ + { name: 'before', type: 'trace', artifactId: 'a', uploadUrl: 'u1' }, + { name: 'after', type: 'trace', artifactId: 'b', uploadUrl: 'u2' }, + ], + }, + }); + + const { nextSteps } = parse(await createSessionTool.handler(body)); + + expect(nextSteps[1]).toContain('artifactName'); + expect(nextSteps[1]).toContain('before, after'); + }); + + it('is gated on the flag and the write scope its route names', () => { + expect(createSessionTool.scope).toBe('runs:write'); + expect(createSessionTool.feature).toBe('evidenceSharing'); + }); +}); diff --git a/mcp-server/src/tools/sessions/create-session.ts b/mcp-server/src/tools/sessions/create-session.ts new file mode 100644 index 0000000..672ba26 --- /dev/null +++ b/mcp-server/src/tools/sessions/create-session.ts @@ -0,0 +1,190 @@ +import { z } from 'zod'; +import { postApi } from '../../lib/request'; +import { apiFailureResult } from '../../lib/toolResult'; +import type { McpTool } from '../../lib/tool'; + +type ArtifactType = 'trace' | 'screenshot' | 'video' | 'attachment'; + +/** + * The route refuses a mismatch too. Repeated here because the agent has to + * guess the content type of a file it produced, and learning it was wrong + * from a 400 costs a round trip it can avoid. + * + * `packages/api/src/api/runs/session/session.validation.ts` is the one that + * decides; this only has to agree with it. + */ +const contentTypeMatches = (type: ArtifactType, contentType: string) => { + switch (type) { + case 'trace': + return contentType === 'application/zip'; + case 'screenshot': + return contentType.startsWith('image/'); + case 'video': + return contentType.startsWith('video/'); + case 'attachment': + return true; + } +}; + +const zodSchema = z.object({ + projectId: z + .string() + .min(1) + .describe('The project to record the session under.'), + title: z + .string() + .min(1) + .max(1024) + .describe( + 'What the session set out to show, as one line. Used as the run and test title.' + ), + status: z + .enum(['passed', 'failed']) + .describe('Whether the behaviour the session went looking for held.'), + error: z + .string() + .max(10_000) + .optional() + .describe('What went wrong. Shown as the test error on a failed session.'), + durationMs: z + .number() + .int() + .min(0) + .max(24 * 60 * 60 * 1000) + .optional() + .describe('How long the session took, in milliseconds.'), + tags: z + .array(z.string().min(1).max(255)) + .max(20) + .optional() + .describe('Tags to file the run under, as on a CI run.'), + artifacts: z + .array( + z + .object({ + name: z + .string() + .min(1) + .max(1024) + .describe( + 'How the file is labelled. Name a trace here to pick it out later when the session carries more than one.' + ), + contentType: z + .string() + .min(1) + .max(255) + .describe( + 'Must match the type: a trace is application/zip, a screenshot image/*, a video video/*. An attachment takes anything.' + ), + type: z.enum(['trace', 'screenshot', 'video', 'attachment']), + }) + .refine( + (artifact) => contentTypeMatches(artifact.type, artifact.contentType), + { + message: + 'contentType does not match the artifact type: a trace is application/zip, a screenshot is an image, a video is a video', + path: ['contentType'], + } + ) + ) + .max(50) + .optional() + .refine( + (artifacts) => { + const names = (artifacts ?? []) + .filter((artifact) => artifact.type === 'trace') + .map((artifact) => artifact.name); + return new Set(names).size === names.length; + }, + { + message: + 'two traces share a name, so artifactName cannot pick between them', + } + ) + .describe( + 'One entry per file to attach. Each comes back with a URL to PUT the bytes to.' + ), +}); + +type SessionArtifact = { + name: string; + type: string; + artifactId: string; + uploadUrl: string; +}; + +type SessionRun = { + runId: string; + groupId: string; + instanceId: string; + testId: string; + artifacts: SessionArtifact[]; +}; + +/** + * The upload URLs are short-lived, and a trace is unreadable until its bytes + * are there, so the order is what the agent has to get right. + */ +const nextSteps = (data: SessionRun) => { + const steps: string[] = []; + if (data.artifacts?.length) { + steps.push( + 'PUT each file to its uploadUrl. The URLs expire about 10 minutes after this call.' + ); + } + const traces = (data.artifacts ?? []).filter( + (artifact) => artifact.type === 'trace' + ); + if (traces.length) { + const pick = + traces.length > 1 + ? ` and artifactName set to one of ${traces.map((t) => t.name).join(', ')}` + : ''; + steps.push( + `Once the trace is uploaded, call currents-create-trace-link with instanceId ${data.instanceId}, testId ${data.testId}${pick} for a link that needs no Currents credential.` + ); + } + return steps; +}; + +const handler = async (body: z.infer) => { + const result = await postApi<{ data?: SessionRun }, typeof body>( + '/runs/session', + body + ); + + if (!result.ok) { + return apiFailureResult('Failed to record the session', result); + } + + const data = result.data?.data; + // All three are named in the guidance below; without them it would quote + // `undefined` back to the agent as something to call the next tool with. + if (!data?.runId || !data.instanceId || !data.testId) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: 'The session was recorded but the response did not identify the run.', + }, + ], + }; + } + + return { + content: [ + { + type: 'text' as const, + text: JSON.stringify({ ...data, nextSteps: nextSteps(data) }, null, 2), + }, + ], + }; +}; + +export const createSessionTool = { + scope: 'runs:write', + feature: 'evidenceSharing', + schema: zodSchema, + handler, +} satisfies McpTool;