fix(harness): harden MCP config values and send servers clean arguments

- Refuse env and header values with a line break or NUL without quoting them.
- Run a !command with stdin closed, and reuse its output for the life of the process.
- Report auth/oauth entries as unsupported instead of ignoring the keys.
- Drop the nulls strict sampling writes for omitted optional arguments before the call.
- Validate JSON Schema patterns instead of skipping the whole schema.
- Refresh a changed tool list without holding the server lock.
- Retry a server whose connect failed at the next exchange, in the background.
- Stop removed or changed servers without making the prompt wait.
- Keep hashed tool names within 64 characters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: sanchitrk

packages/harness/src/mcp/config.ts

@@ -55,6 +55,9 @@ export const validate = (name: string, value: unknown): ServerConfig | string =>
 		return `server "${name}": description must be a string`;
 	if (timeout !== undefined && !(Predicate.isNumber(timeout) && timeout > 0))
 		return `server "${name}": timeout must be a positive number of seconds`;
+	// Until sign-in exists, an entry relying on it could only fail; say so instead of ignoring the key.
+	for (const key of ["auth", "oauth"])
+		if (value[key] !== undefined) return `server "${name}": "${key}" is not supported yet`;
 	if (type === "sse") return `server "${name}": legacy SSE transport is not supported; use the streamable HTTP URL`;
 
 	if (Predicate.isString(value.url) && (type === undefined || type === "http" || type === "streamable-http")) {

packages/harness/src/mcp/mcp.ts

@@ -194,21 +194,22 @@ export const layer = (options: { readonly hostCwd: string; readonly home: string
 				);
 				// Calls replace an expired session themselves; this covers a background request seeing it first.
 				live.connection.onSessionExpired(current(retire(server, live)));
+				// Listed without the server's lock, which calls and closing need: the listing can take a while.
 				live.connection.onToolsChanged(
-					current(
-						live.connection.tools().pipe(
-							Effect.tap((tools) =>
-								Effect.sync(() => {
-									server.tools = tools;
-								}),
-							),
-							Effect.catch((error) =>
-								Effect.sync(() => {
-									server.error = `failed to refresh tools: ${error.message}`;
+					Effect.sync(() => {
+						fork(
+							live.connection.tools().pipe(
+								Effect.match({
+									onSuccess: (tools) => {
+										if (server.live === live) server.tools = tools;
+									},
+									onFailure: (error) => {
+										if (server.live === live) server.error = `failed to refresh tools: ${error.message}`;
+									},
 								}),
 							),
-						),
-					),
+						);
+					}),
 				);
 				live.connection.onLog((entry) => log.write(server.name, entry));
 			};
@@ -392,10 +393,14 @@ export const layer = (options: { readonly hostCwd: string; readonly home: string
 						// Exposure and description apply to the next exchange without reconnecting.
 						if (config !== undefined && connectionKey(config, cwd, roots) === server.key) {
 							server.config = config;
+							// A server that could not connect declares no tools, so no call would retry it.
+							// Retry in the background; waiting for it would hold up every prompt while it is down.
+							if (server.state === "failed" && server.attempt === undefined) start(server);
 							continue;
 						}
 						session.servers.delete(name);
-						yield* close(server);
+						// Stopping can take seconds; the prompt does not wait for it.
+						fork(close(server));
 					}
 					for (const [name, config] of wanted) {
 						if (session.servers.has(name)) continue;

packages/harness/src/mcp/tool.ts

@@ -3,6 +3,7 @@
  * reaches it.
  */
 
+import { normalizeOptionalNulls } from "@codeworksh/aikit";
 import type { Exposure, ServerConfig } from "@codeworksh/plugin/plugin/mcp";
 import type { CallToolResult, Tool as McpTool } from "@modelcontextprotocol/client";
 import { Effect, Schema } from "effect";
@@ -26,7 +27,8 @@ export const name = (server: string, tool: string, taken: (candidate: string) =>
 	if (plain.length <= MAX_NAME_LENGTH && !taken(plain)) return plain;
 	const hash = Hash.sha256(`${server}\0${tool}`);
 	let candidate = plain;
-	for (let length = 8; length <= hash.length; length += 8) {
+	// The executor refuses two tools of one name, so the hash grows, while `mcp__` still fits before it.
+	for (let length = 8; length <= MAX_NAME_LENGTH - "mcp__".length - 1; length += 8) {
 		candidate = `${plain.slice(0, MAX_NAME_LENGTH - length - 1)}_${hash.slice(0, length)}`;
 		if (!taken(candidate)) return candidate;
 	}
@@ -99,6 +101,12 @@ const parameters = (schema: McpTool["inputSchema"]): Record<string, unknown> =>
 	...(schema.properties === undefined ? { properties: {} } : {}),
 });
 
+const withoutOptionalNulls = (args: Record<string, unknown>, schema: Record<string, unknown>) => {
+	const normalized = structuredClone(args);
+	normalizeOptionalNulls(normalized, schema);
+	return normalized;
+};
+
 export interface Call {
 	readonly args: Record<string, unknown>;
 	readonly onProgress: (progress: Progress) => void;
@@ -122,26 +130,28 @@ export const make = (input: {
 		server,
 		tool: tool.name,
 	});
+	const schema = parameters(tool.inputSchema);
 	return Tool.register(
 		Tool.make({
 			name: input.name,
 			label: `${server}/${tool.name}`,
 			description: tool.description?.trim() || title || `MCP tool ${tool.name} from server ${server}`,
-			parameters: Tool.fromJsonSchema(parameters(tool.inputSchema)),
+			parameters: Tool.fromJsonSchema(schema),
 			success: Outcome,
 			failure: Outcome,
 			encodeContent: (outcome) => outcome.content as McpContent.Content[],
 			encodeFailureContent: (outcome) => outcome.content as McpContent.Content[],
 			encodeDetails: ({ content: _content, ...details }) => details,
-			// The arguments as the model wrote them: validation only checks them, and the server, not
-			// a translation of its schema, decides what they mean.
+			// The arguments as the model wrote them, minus the `null`s strict sampling writes for omitted
+			// optional ones: validation only checks them, and the server, not a translation of its schema,
+			// decides what they mean.
 			handler: (_params, ctx) =>
 				Effect.gen(function* () {
 					const progress = yield* ToolProgress;
 					const report = Effect.runForkWith(yield* Effect.context<never>());
 					const result = yield* input
 						.call({
-							args: ctx.rawArgs,
+							args: withoutOptionalNulls(ctx.rawArgs, schema),
 							onProgress: (update) => {
 								const total = update.total === undefined ? "" : `/${update.total}`;
 								const text = update.message ?? `Progress ${update.progress}${total}`;

packages/harness/src/mcp/value.ts

@@ -1,9 +1,13 @@
 /**
  * Values of a server's `env` and `headers`, resolved when the server connects:
  *
- * - `!command` runs the rest through `/bin/sh` and uses its trimmed stdout.
+ * - `!command` runs the rest through `/bin/sh` and uses its trimmed stdout, once per process: later
+ *   connects reuse it. A command that fails runs again next time, since the host outlives sessions.
  * - Otherwise `$NAME` and `${NAME}` are replaced by host environment variables; `$$` is a literal
  *   `$` and `$!` a literal `!`. Anything else is literal.
+ *
+ * A value holding a line break or NUL is refused without being shown: it cannot be sent as a header
+ * or set as a variable, and the error the runtime would raise quotes the value, often a secret.
  */
 
 import { Config, Duration, Effect, Option, Stream } from "effect";
@@ -12,6 +16,8 @@ import { ChildProcess, ChildProcessSpawner } from "effect/process";
 const COMMAND_TIMEOUT = Duration.seconds(10);
 const NAME = /^[A-Za-z_][A-Za-z0-9_]*$/;
 const NAME_PREFIX = /^[A-Za-z_][A-Za-z0-9_]*/;
+const UNSAFE = /[\r\n\0]/;
+const commands = new Map<string, string>();
 
 type Part = { readonly type: "literal"; readonly value: string } | { readonly type: "env"; readonly name: string };
 
@@ -66,7 +72,9 @@ const template = (value: string): ReadonlyArray<Part> => {
 const run = (command: string) =>
 	Effect.gen(function* () {
 		const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
-		const handle = yield* spawner.spawn(ChildProcess.make("/bin/sh", ["-c", command], { stderr: "ignore" }));
+		const handle = yield* spawner.spawn(
+			ChildProcess.make("/bin/sh", ["-c", command], { stdin: "ignore", stderr: "ignore" }),
+		);
 		const [output, code] = yield* Effect.all([Stream.mkString(Stream.decodeText(handle.stdout)), handle.exitCode], {
 			concurrency: 2,
 		});
@@ -79,14 +87,16 @@ const run = (command: string) =>
 		Effect.orElseSucceed(() => undefined),
 	);
 
-/** One value. `what` names it in the failure, for example `MCP server "docs" header "Authorization"`. */
-export const resolve = Effect.fnUntraced(function* (value: string, what: string) {
-	if (value.startsWith("!")) {
-		const output = yield* run(value.slice(1));
-		if (output === undefined)
-			return yield* Effect.fail(`Failed to resolve ${what} from shell command: ${value.slice(1)}`);
-		return output;
-	}
+const command = Effect.fnUntraced(function* (line: string, what: string) {
+	const cached = commands.get(line);
+	if (cached !== undefined) return cached;
+	const output = yield* run(line);
+	if (output === undefined) return yield* Effect.fail(`Failed to resolve ${what} from shell command: ${line}`);
+	commands.set(line, output);
+	return output;
+});
+
+const variables = Effect.fnUntraced(function* (value: string, what: string) {
 	const parts = template(value);
 	const names = [...new Set(parts.flatMap((part) => (part.type === "env" ? [part.name] : [])))];
 	// An empty variable counts as unset.
@@ -103,6 +113,13 @@ export const resolve = Effect.fnUntraced(function* (value: string, what: string)
 	return parts.map((part) => (part.type === "literal" ? part.value : env.get(part.name))).join("");
 });
 
+/** One value. `what` names it in the failure, for example `MCP server "docs" header "Authorization"`. */
+export const resolve = Effect.fnUntraced(function* (value: string, what: string) {
+	const resolved = yield* value.startsWith("!") ? command(value.slice(1), what) : variables(value, what);
+	if (UNSAFE.test(resolved)) return yield* Effect.fail(`${what} contains a line break or NUL character`);
+	return resolved;
+});
+
 /** Every value of a record, failing on the first that does not resolve. */
 export const resolveAll = Effect.fnUntraced(function* (values: Readonly<Record<string, string>>, what: string) {
 	const resolved: Record<string, string> = {};

packages/harness/test/__artifacts__/mcp.session.json

@@ -2,6 +2,7 @@
 	"requests": 6,
 	"tools": [
 		"mcp__e2e__echo",
+		"mcp__e2e__args",
 		"mcp__e2e__fail",
 		"mcp__e2e__pid",
 		"mcp__e2e__crash",
@@ -38,6 +39,10 @@
 				[
 					"my-srv",
 					false
+				],
+				[
+					"badheader",
+					true
 				]
 			],
 			[
@@ -52,6 +57,10 @@
 				[
 					"my-srv",
 					false
+				],
+				[
+					"badheader",
+					true
 				]
 			]
 		],
@@ -82,6 +91,13 @@
 				"{\n  \"version\": 2\n}"
 			]
 		},
+		"nulls": {
+			"status": "completed",
+			"isError": false,
+			"content": [
+				"{\"word\":\"abc\"}"
+			]
+		},
 		"pid-before": {
 			"status": "completed",
 			"isError": false,
@@ -109,13 +125,20 @@
 			"content": [
 				"Invalid arguments for mcp__e2e__echo: Missing key\n  at [\"text\"]"
 			]
+		},
+		"pattern": {
+			"status": "error",
+			"isError": true,
+			"content": [
+				"Invalid arguments for mcp__e2e__args: Expected a string matching the RegExp ^[a-z]+$\n  at [\"word\"]"
+			]
 		}
 	},
 	"status": [
 		{
 			"server": "e2e",
 			"state": "connected",
-			"tools": 7
+			"tools": 8
 		},
 		{
 			"server": "off",
@@ -126,11 +149,18 @@
 			"server": "my-srv",
 			"state": "disabled",
 			"tools": 0
+		},
+		{
+			"server": "badheader",
+			"state": "failed",
+			"error": "MCP server \"badheader\" header \"Authorization\" contains a line break or NUL character",
+			"tools": 0
 		}
 	],
 	"configErrors": [
 		"<tmp>/home/mcp.json: server \"my_srv\" conflicts with \"my-srv\"",
 		"<tmp>/home/mcp.json: server \"broken\": url must be an http or https URL",
+		"<tmp>/home/mcp.json: server \"signin\": \"oauth\" is not supported yet",
 		"<tmp>/project/.codework/mcp.json: server \"ghost\" needs \"command\" or \"url\", or a user server to override"
 	]
 }

packages/harness/test/__artifacts__/mcp.session.lifecycle.json

@@ -58,6 +58,7 @@
 	"declared": [
 		[
 			"mcp__e2e__echo",
+			"mcp__e2e__args",
 			"mcp__e2e__fail",
 			"mcp__e2e__pid",
 			"mcp__e2e__crash",
@@ -68,6 +69,7 @@
 		],
 		[
 			"mcp__e2e__echo",
+			"mcp__e2e__args",
 			"mcp__e2e__fail",
 			"mcp__e2e__pid",
 			"mcp__e2e__crash",
@@ -78,6 +80,7 @@
 		],
 		[
 			"mcp__e2e__echo",
+			"mcp__e2e__args",
 			"mcp__e2e__fail",
 			"mcp__e2e__pid",
 			"mcp__e2e__crash",
@@ -88,6 +91,7 @@
 		],
 		[
 			"mcp__e2e__echo",
+			"mcp__e2e__args",
 			"mcp__e2e__fail",
 			"mcp__e2e__pid",
 			"mcp__e2e__crash",
@@ -98,6 +102,7 @@
 		[
 			"mcp__remote__echo",
 			"mcp__e2e__echo",
+			"mcp__e2e__args",
 			"mcp__e2e__fail",
 			"mcp__e2e__pid",
 			"mcp__e2e__crash",
@@ -130,7 +135,7 @@
 		{
 			"server": "e2e",
 			"state": "connected",
-			"tools": 7
+			"tools": 8
 		}
 	]
 }

packages/harness/test/fixtures/mcp/session.mjs

@@ -13,6 +13,11 @@ server.setRequestHandler("tools/list", () => ({
 			description: "Echo text back, prefixed with the configured greeting.",
 			inputSchema: object({ text: { type: "string", description: "What to echo." } }, ["text"]),
 		},
+		{
+			name: "args",
+			description: "The arguments as received.",
+			inputSchema: object({ word: { type: "string", pattern: "^[a-z]+$" }, limit: { type: "integer" } }, ["word"]),
+		},
 		{ name: "fail", description: "Answer with an error result.", inputSchema: object() },
 		{ name: "pid", description: "The server's process id.", inputSchema: object() },
 		{ name: "crash", description: "Exit while the call is running.", inputSchema: object() },
@@ -26,6 +31,8 @@ server.setRequestHandler("tools/call", (request) => {
 	switch (request.params.name) {
 		case "echo":
 			return text(`${process.env.E2E_GREETING}: ${String(args.text)}`);
+		case "args":
+			return text(JSON.stringify(args));
 		case "fail":
 			return { isError: true, content: [{ type: "text", text: "the fixture refused" }] };
 		case "pid":