diff --git a/docs/agent-environments.md b/docs/agent-environments.md index e393e0697c..6b11bd92a7 100644 --- a/docs/agent-environments.md +++ b/docs/agent-environments.md @@ -63,6 +63,7 @@ where the answer came from: "exact_match": true, "recorded_path": "/api/users/2", "recorded_query": null, + "body_recorded": true, "truncated": false } } @@ -75,7 +76,9 @@ record and reports `exact_match: false`. `body` is parsed JSON when the recorded content type is JSON-like (`application/json`, `application/*+json`, `text/plain`, `text/markdown`) and the text parses; otherwise it is the recorded text. Bodies over 256 KiB are cut at a UTF-8 boundary and returned as -text with `truncated: true`. Request bodies passed to `POST`, `PUT` and `PATCH` +text with `truncated: true`. `body_recorded` is false when the capture kept the +response's status and content type but no body; `body` is then `null`, which +is not a recorded JSON `null`. Request bodies passed to `POST`, `PUT` and `PATCH` tools are accepted and ignored: nothing is written, and no call changes what a later call returns. Arguments the tool's input schema does not name are refused by the MCP server as invalid arguments, so a misspelled parameter never @@ -164,7 +167,10 @@ connecting to `/mcp`: `tools/list` advertises over MCP. Path parameters are required strings whose description names the segment index, the templating rule and the observed values; query parameter names are optional strings; write methods accept an -optional `body` object described by the recorded top-level key types. Every +optional `body` object described by the recorded top-level key types. Each +input keeps its own argument: a query name equal to a path parameter's name is +advertised as `query_`, and the write body as `request_body` when a query +parameter is named `body`. Every schema closes with `additionalProperties: false`, a key the echo example's schema does not carry. `extensions`, `operations` and `children_environments` are fixed to `null`, diff --git a/docs/specs/agent-environments/environment-serve/spec.md b/docs/specs/agent-environments/environment-serve/spec.md index 2a9d457e56..415e3047e5 100644 --- a/docs/specs/agent-environments/environment-serve/spec.md +++ b/docs/specs/agent-environments/environment-serve/spec.md @@ -27,7 +27,9 @@ later increment. - Build an `McpServer` from the manifest alone: each tool's input schema lists every path parameter as a required string described by its segment index, rule and observed values, every observed query name as an optional string, - and an optional `body` object for `POST`, `PUT` and `PATCH`, closed with + and an optional `body` object for `POST`, `PUT` and `PATCH` (a query name + that collides with a path parameter becomes `query_`, and the body + becomes `request_body` when a query parameter is named `body`), closed with `additionalProperties: false` so unknown arguments are refused; the description reads `METHOD template; replays retained records [ordinals]; evidence ev_…`. The served catalog stays separate from REA's own tool @@ -38,7 +40,9 @@ later increment. `content_type`, `body` (parsed JSON for JSON-like content types that parse, else text; bodies over 256 KiB cut at a UTF-8 boundary with `truncated: true`) and `replay` with `record_ordinal`, `evidence_id`, - `exact_match`, `recorded_path`, `recorded_query` and `truncated`, as both + `exact_match`, `recorded_path`, `recorded_query`, `body_recorded` (false + when the capture kept no response body, so a `null` body is not mistaken for + a recorded one) and `truncated`, as both structured content and text, with REA's existing target-content provenance marker in `_meta`. - Serve the card with exactly the echo example's keys and nesting: `name` diff --git a/src/application/environment/environmentReplay.test.ts b/src/application/environment/environmentReplay.test.ts index ec69803176..6b1ab00bc0 100644 --- a/src/application/environment/environmentReplay.test.ts +++ b/src/application/environment/environmentReplay.test.ts @@ -68,6 +68,7 @@ describe("replayEnvironmentTool selection", () => { exact_match: true, recorded_path: "/api/users/2", recorded_query: null, + body_recorded: true, truncated: false, }, }); @@ -163,7 +164,56 @@ describe("replayEnvironmentTool selection", () => { }); }); +describe("replayEnvironmentTool argument names", () => { + it("matches a query value whose name collides with a path parameter under its renamed argument", () => { + const users = tool("get_users_by_id"); + const exchange = (ordinal: number, query: string): RecordedExchange => ({ + ordinal, + path: "/api/users/2", + query, + status: 200, + contentType: "application/json", + bodyText: JSON.stringify({ query }), + }); + const colliding = { + ...users, + query_parameter_names: ["id"], + record_ordinals: [901, 902], + }; + const from: EnvironmentSource = { + ...source, + exchanges: new Map([ + [901, exchange(901, "id=3")], + [902, exchange(902, "id=4")], + ]), + }; + const result = replayEnvironmentTool(from, colliding, { + id: "2", + query_id: "4", + }); + if (!result.ok) throw result.error; + expect(result.value.replay).toMatchObject({ + record_ordinal: 902, + recorded_query: "id=4", + exact_match: true, + }); + }); +}); + describe("replayEnvironmentTool bodies", () => { + it("reports a response whose body the capture did not retain instead of passing off null as recorded", () => { + const result = replay( + "get_users_by_id", + { id: "2" }, + singleExchangeSource(null, "application/json"), + ); + expect(result.body).toBeNull(); + expect(result.replay).toMatchObject({ + body_recorded: false, + truncated: false, + }); + }); + it("returns text for non-JSON content types and for JSON-like content that does not parse", () => { expect( replay( diff --git a/src/application/environment/environmentReplay.ts b/src/application/environment/environmentReplay.ts index d035faa481..4454722bf1 100644 --- a/src/application/environment/environmentReplay.ts +++ b/src/application/environment/environmentReplay.ts @@ -5,6 +5,7 @@ import { AnalysisInputError } from "../../domain/analysisErrorCore.js"; import { jsonValueSchema, type JsonValue } from "../../domain/jsonValue.js"; import { err, ok, type Result } from "../../domain/result.js"; import { safeParseJson } from "../../domain/safeJson.js"; +import { environmentToolArguments } from "./environmentTools.js"; import { ENVIRONMENT_SERVE_OPERATION, type EnvironmentManifestTool, @@ -29,6 +30,7 @@ export interface ReplayResult { readonly exact_match: boolean; readonly recorded_path: string; readonly recorded_query: string | null; + readonly body_recorded: boolean; readonly truncated: boolean; }; } @@ -50,6 +52,7 @@ const decoded = (segment: string): string => { /** Count how many supplied path and query values the recorded request line satisfies. */ const scoreExchange = ( tool: EnvironmentManifestTool, + queryArguments: ReadonlyMap, exchange: RecordedExchange, args: ReplayArguments, ): MatchScore => { @@ -70,8 +73,8 @@ const scoreExchange = ( const query = new URLSearchParams(exchange.query ?? ""); let queryProvided = 0; let queryMatches = 0; - for (const name of tool.query_parameter_names) { - const value = args[name]; + for (const [name, argument] of queryArguments) { + const value = args[argument]; if (typeof value !== "string") continue; queryProvided += 1; if (query.getAll(name).includes(value)) queryMatches += 1; @@ -92,10 +95,11 @@ export const selectReplayExchange = ( | { readonly exchange: RecordedExchange; readonly exact: boolean } | undefined => { let best: { exchange: RecordedExchange; score: MatchScore } | undefined; + const queryArguments = environmentToolArguments(tool).query; for (const ordinal of tool.record_ordinals) { const exchange = exchanges.get(ordinal); if (exchange === undefined) continue; - const score = scoreExchange(tool, exchange, args); + const score = scoreExchange(tool, queryArguments, exchange, args); if ( best === undefined || score.pathMatches > best.score.pathMatches || @@ -109,12 +113,11 @@ export const selectReplayExchange = ( : { exchange: best.exchange, exact: best.score.exact }; }; -/** Cut text at the last UTF-8 character boundary within the byte limit. */ -const truncateUtf8 = (text: string, maxBytes: number): string => { - const bytes = Buffer.from(text, "utf8"); - let end = maxBytes; - while (end > 0 && (bytes[end]! & 0xc0) === 0x80) end -= 1; - return bytes.toString("utf8", 0, end); +/** The longest prefix of text whose UTF-8 encoding fits the byte limit, encoding only that prefix. */ +const utf8Prefix = (text: string, maxBytes: number): string => { + // encodeInto stops before the first character that does not fit, so `read` ends on a boundary. + const { read } = new TextEncoder().encodeInto(text, new Uint8Array(maxBytes)); + return text.slice(0, read); }; /** Parse a retained body as JSON when its content type is JSON-like; otherwise return the text. */ @@ -123,11 +126,8 @@ const replayBody = ( ): { readonly body: JsonValue; readonly truncated: boolean } => { const text = exchange.bodyText; if (text === null) return { body: null, truncated: false }; - if (Buffer.byteLength(text, "utf8") > REPLAY_BODY_LIMIT_BYTES) - return { - body: truncateUtf8(text, REPLAY_BODY_LIMIT_BYTES), - truncated: true, - }; + const prefix = utf8Prefix(text, REPLAY_BODY_LIMIT_BYTES); + if (prefix.length < text.length) return { body: prefix, truncated: true }; if ( exchange.contentType !== null && isJsonLike(mediaTypeEssence(exchange.contentType)) @@ -170,6 +170,7 @@ export const replayEnvironmentTool = ( exact_match: exact, recorded_path: exchange.path, recorded_query: exchange.query, + body_recorded: exchange.bodyText !== null, truncated, }, }); diff --git a/src/application/environment/environmentTools.test.ts b/src/application/environment/environmentTools.test.ts index df67efa7ad..a048db1ec5 100644 --- a/src/application/environment/environmentTools.test.ts +++ b/src/application/environment/environmentTools.test.ts @@ -92,6 +92,30 @@ describe("environmentToolDefinitions", () => { }); }); +describe("environmentToolInputSchema argument names", () => { + it("keeps every input addressable when query names collide with a path parameter, the body or inherited names", () => { + const schema = environmentToolInputSchema({ + ...tool("post_folders"), + params: tool("get_users_by_id").params, + query_parameter_names: ["id", "body", "constructor"], + }); + expect(Object.keys(schema.properties)).toEqual([ + "id", + "query_id", + "body", + "constructor", + "request_body", + ]); + expect(Object.hasOwn(schema.properties, "constructor")).toBe(true); + expect(schema.properties.query_id?.description).toBe( + "Query parameter id; optional, matched against the recorded query on replay.", + ); + expect(schema.properties.body?.type).toBe("string"); + expect(schema.properties.request_body?.type).toBe("object"); + expect(schema.required).toEqual(["id"]); + }); +}); + describe("environmentCard", () => { it("shapes the card with fixed protocol fields, the tools and null extension slots in contract key order", () => { const card = environmentCard("grafana-example", definitions); diff --git a/src/application/environment/environmentTools.ts b/src/application/environment/environmentTools.ts index e2bf8012fc..18eea3af9c 100644 --- a/src/application/environment/environmentTools.ts +++ b/src/application/environment/environmentTools.ts @@ -40,27 +40,70 @@ const bodyDescription = (tool: EnvironmentManifestTool): string => .map(([key, type]) => `${key} (${type})`) .join(", ")}.`; +/** Where each input of one tool lives in the flat argument object the schema advertises. */ +export interface EnvironmentToolArguments { + /** Query parameter name to argument name. */ + readonly query: ReadonlyMap; + /** Argument name of the write body, for write methods. */ + readonly body: string | undefined; +} + +const freeName = ( + taken: ReadonlySet, + preferred: string, + prefix: string, +): string => { + if (!taken.has(preferred)) return preferred; + let candidate = `${prefix}${preferred}`; + for (let suffix = 2; taken.has(candidate); suffix += 1) + candidate = `${prefix}${preferred}_${String(suffix)}`; + return candidate; +}; + +/** Path parameters keep their names; a query name or the write body that would collide is renamed, so every input stays addressable. */ +export const environmentToolArguments = ( + tool: EnvironmentManifestTool, +): EnvironmentToolArguments => { + const taken = new Set(tool.params.map((param) => param.name)); + const query = new Map(); + for (const name of tool.query_parameter_names) { + const argument = freeName(taken, name, "query_"); + taken.add(argument); + query.set(name, argument); + } + return { + query, + body: WRITE_METHODS.has(tool.method) + ? freeName(taken, "body", "request_") + : undefined, + }; +}; + /** Map one manifest tool to its advertised MCP input schema. */ export const environmentToolInputSchema = ( tool: EnvironmentManifestTool, ): EnvironmentToolInputSchema => { - const properties: Record = {}; + // A Map, not an object literal, so names such as `constructor` are never mistaken for taken. + const properties = new Map(); for (const param of tool.params) - properties[param.name] = { + properties.set(param.name, { type: "string", description: `Path segment ${String(param.index)} (${param.rule}); observed: ${observed(param.observed_values)}`, - }; - for (const name of tool.query_parameter_names) - if (!(name in properties)) - properties[name] = { - type: "string", - description: `Query parameter ${name}; optional, matched against the recorded query on replay.`, - }; - if (WRITE_METHODS.has(tool.method) && !("body" in properties)) - properties.body = { type: "object", description: bodyDescription(tool) }; + }); + const args = environmentToolArguments(tool); + for (const [name, argument] of args.query) + properties.set(argument, { + type: "string", + description: `Query parameter ${name}; optional, matched against the recorded query on replay.`, + }); + if (args.body !== undefined) + properties.set(args.body, { + type: "object", + description: bodyDescription(tool), + }); return { type: "object", - properties, + properties: Object.fromEntries(properties), required: tool.params.map((param) => param.name), additionalProperties: false, }; diff --git a/src/server/http/serveStreamableHttp.test.ts b/src/server/http/serveStreamableHttp.test.ts index accba0c190..9c5b43b22a 100644 --- a/src/server/http/serveStreamableHttp.test.ts +++ b/src/server/http/serveStreamableHttp.test.ts @@ -519,6 +519,30 @@ it("requires exact Accept media ranges before a sessionless POST may replace the await client.close(); }); +it("keeps the live session when the new client left before its initialize was probed", async () => { + const slot = new McpSessionSlot( + createEchoServer(() => undefined), + undefined, + ); + const close = vi.spyOn( + WebStandardStreamableHTTPServerTransport.prototype, + "close", + ); + try { + expect((await slot.handle(initializeRequest())).status).toBe(200); + const left = new AbortController(); + left.abort(); + const abandoned = new Request(initializeRequest(), { signal: left.signal }); + expect((await slot.handle(abandoned)).status).toBe(400); + expect(close).not.toHaveBeenCalled(); + expect((await slot.handle(initializeRequest())).status).toBe(200); + expect(close).toHaveBeenCalledTimes(1); + } finally { + close.mockRestore(); + await slot.close(); + } +}); + it("keeps a session whose transport shutdown failed so the next close retries it", async () => { let shutdowns = 0; const slot = new McpSessionSlot( diff --git a/src/server/http/serveStreamableHttp.ts b/src/server/http/serveStreamableHttp.ts index 463914b427..5c0d878e21 100644 --- a/src/server/http/serveStreamableHttp.ts +++ b/src/server/http/serveStreamableHttp.ts @@ -238,7 +238,9 @@ export class McpSessionSlot { const open = this.#live; if ( open?.transport.sessionId !== undefined && - (await isInitializePost(request)) + (await isInitializePost(request)) && + // A client that left while its initialize was probed would evict the session for nobody. + !request.signal.aborted ) { if (this.#closed) return SHUTTING_DOWN(); this.logger?.info("MCP HTTP session replaced by a new client"); diff --git a/tests/boundary/process/environmentServeShutdown.test.ts b/tests/boundary/process/environmentServeShutdown.test.ts index 039f7dee55..95299fb558 100644 --- a/tests/boundary/process/environmentServeShutdown.test.ts +++ b/tests/boundary/process/environmentServeShutdown.test.ts @@ -10,9 +10,13 @@ import { processTest } from "../../support/process/processFixture.js"; const SHUTDOWN_TIMEOUT_MS = 90_000; +// Windows has no SIGTERM handler to run: Node's kill("SIGTERM") ends the child outright there. +const [SIGNAL, STATUS]: readonly [NodeJS.Signals, number] = + process.platform === "win32" ? ["SIGINT", 130] : ["SIGTERM", 143]; + describe("rea environment-serve shutdown", () => { processTest( - "closes the listener on SIGTERM, prints one stopped document and exits 143", + `closes the listener on ${SIGNAL}, prints one stopped document and exits ${String(STATUS)}`, async ({ processes }) => { const root = await createTestTempDirectory("rea-environment-serve-stop-"); const files = await writeEnvironmentServeFiles(root); @@ -33,9 +37,9 @@ describe("rea environment-serve shutdown", () => { new URL("/.well-known/agent-env.json", started.url.origin), ); expect(card.status).toBe(200); - started.child.kill("SIGTERM"); + started.child.kill(SIGNAL); const [code, signalCode] = await once(started.child, "close"); - expect(code).toBe(143); + expect(code).toBe(STATUS); expect(signalCode).toBeNull(); expect(JSON.parse(stdout)).toMatchObject({ status: "stopped",