diff --git a/changelog.d/379-server-config-booleans.fixed.md b/changelog.d/379-server-config-booleans.fixed.md new file mode 100644 index 000000000..5f20a60d9 --- /dev/null +++ b/changelog.d/379-server-config-booleans.fixed.md @@ -0,0 +1 @@ +- An MCPL server's `inheritEnv`, `reconnect` and `allowHostCommands` each take `true` or `false`, or can be left out, and `null` counts as left out (#379). `create()`, `connectMcplServer` and `restartMcplServer` refuse any other value, naming the server, the setting and the value's kind, and `restartMcplServer` refuses before it disconnects the running server, which stays up. `inheritEnv` is checked only for a stdio server, since a WebSocket server never reads it. `inheritEnv` and `reconnect` were read by truthiness, so `"inheritEnv": "false"` handed a stdio server the host's whole environment, API keys and bot tokens included, and `"reconnect": "false"` turned retrying on. `allowHostCommands` was read as on only when `true`, so `"allowHostCommands": "true"` granted nothing. Every reader of the three now takes only `true` as on, and `McplServerRegistry` asks the connection whether it will reconnect instead of reading `reconnect` again. So a server with `reconnect: true` that the host closes for good, as a policy refusal's `"close"` fallback does, no longer stays registered as a closed connection. A quoted `"true"` is refused wherever the setting is checked, including where the truthy reading made it work, so write `true` or `false` without quotes. A host that passes these settings on from its own configuration without checking them now gets this refusal from `create()` where a quoted value used to start. diff --git a/src/framework.ts b/src/framework.ts index 5b4f3785f..482645b32 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -404,7 +404,7 @@ import { EventGate, formatShadowWarning } from './gate/event-gate.js'; import { UsageTracker, type PersistedUsageState } from './usage/usage-tracker.js'; import type { SessionUsageSnapshot, UsageUpdatedEvent } from './usage/types.js'; import { stderrTailOf, withStderr, type McplConnectionCloseInfo, type McplServerConnection } from './mcpl/server-connection.js'; -import { isWebSocketTransport } from './mcpl/transport.js'; +import { assertServerConfigBooleans, isWebSocketTransport } from './mcpl/transport.js'; import type { McplServerConfig, McplHostCapabilities, @@ -2487,10 +2487,13 @@ export class AgentFramework { // Initialize MCPL subsystems if configured if (config.mcplServers && config.mcplServers.length > 0) { - // Validate tool prefixes: no collisions with module names or between servers + // Check every server's config before any is brought up: its boolean + // settings, and its tool prefix, which mustn't collide with a module + // name or another server's prefix. const moduleNames = new Set(config.modules.map(m => m.name)); const prefixesSeen = new Map(); // prefix → serverId for (const serverConfig of config.mcplServers) { + assertServerConfigBooleans(serverConfig); const prefix = serverConfig.toolPrefix ?? `mcpl--${serverConfig.id}`; if (moduleNames.has(prefix)) { throw new Error( @@ -17391,7 +17394,9 @@ export class AgentFramework { }; const declared = valueIn(config.env); if (declared !== undefined) return { value: declared, source: 'env' }; - const inherited = config.inheritEnv ? valueIn(process.env) : undefined; + // Only true inherits, as in buildChildEnv: a value that reached here + // unchecked keeps the protective default. + const inherited = config.inheritEnv === true ? valueIn(process.env) : undefined; return inherited !== undefined ? { value: inherited, source: 'inherited' } : undefined; } @@ -17399,7 +17404,9 @@ export class AgentFramework { * Connect a new MCPL server at runtime. Refreshes the tool list and * notifies the agent of newly available tools. Throws if the MCPL * subsystem is not initialized, the id is already connected, or the - * connection/handshake fails. + * connection/handshake fails. A config whose `inheritEnv` (on a stdio + * server), `reconnect` or `allowHostCommands` holds anything but true, + * false or null is refused, as `create()` refuses one. */ async connectMcplServer( config: import('./mcpl/types.js').McplServerConfig, @@ -17411,6 +17418,7 @@ export class AgentFramework { private async runConnectMcplServer( config: import('./mcpl/types.js').McplServerConfig, ): Promise { + assertServerConfigBooleans(config); // Lazily bring up the MCPL subsystem — a framework that started with zero // configured servers can still deploy its first one at runtime. if (!this.mcplServerRegistry || !this.mcplHostCapabilities) { @@ -17480,6 +17488,8 @@ export class AgentFramework { * same (or an updated) config. This actually respawns a stdio child — * unlike `reconnect: true`, which only retries after transport-level * failures. Throws if the server has no stored config and none is given. + * A config `connectMcplServer` would refuse for its boolean settings is + * refused before the running server is disconnected, so it stays up. */ async restartMcplServer( id: string, @@ -17497,6 +17507,10 @@ export class AgentFramework { if (!config) { throw new Error(`MCPL server "${id}" is not configured`); } + // Checked before the running server is disconnected, so a config this + // refuses leaves it up. (A colliding toolPrefix is refused only by the + // connect below, once the server is down.) + assertServerConfigBooleans(config); // Every connect reads the awareness ledger: its awareness barrier // reconciles it, and a stdio child taking the protective default derives // that from it. Read it before disconnecting, so an unreadable ledger diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 6c378cdae..4a88eed27 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -533,8 +533,9 @@ export class McplServerConnection extends EventEmitter { connection.allowHostCommands = config.allowHostCommands === true; connection.requestTimeoutMs = config.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS; - // Store config for reconnection - if (config.reconnect) { + // Store config for reconnection. Only `true` turns reconnect on, as at + // every other reader of it. + if (config.reconnect === true) { connection.config = config; connection.hostCapabilities = hostCapabilities; connection.reconnectEnabled = true; @@ -683,7 +684,7 @@ export class McplServerConnection extends EventEmitter { try { return await McplServerConnection.connect(config, hostCapabilities); } catch (error) { - if (!config.reconnect) { + if (config.reconnect !== true) { throw error; } diff --git a/src/mcpl/server-registry.ts b/src/mcpl/server-registry.ts index 7cb5a1281..7aed41e47 100644 --- a/src/mcpl/server-registry.ts +++ b/src/mcpl/server-registry.ts @@ -42,14 +42,16 @@ export class McplServerRegistry { throw new Error(`MCPL server "${config.id}" is already registered`); } - const connection = config.reconnect + const connection = config.reconnect === true ? await McplServerConnection.connectWithReconnect(config, hostCapabilities) : await McplServerConnection.connect(config, hostCapabilities); this.servers.set(config.id, connection); - // Auto-remove on unexpected close (unless reconnect will re-add) + // Auto-remove on close unless the connection will reconnect. That was + // decided once, from `reconnect === true` at connect, and a close() the + // host asks for disarms it first, so a server closed for good goes too. connection.on('close', () => { - if (!config.reconnect) { + if (!connection.willReconnect) { this.servers.delete(config.id); } }); diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 65fc538cb..6109f3276 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -141,15 +141,17 @@ export const CHILD_ENV_ALLOWLIST = [ /** Environment for a stdio child: allowlisted host vars (+ LC_*), then the * server's declared `env` on top. Windows names merge case-insensitively; - * POSIX names remain case-sensitive. `inheritEnv: true` restores full inheritance. */ + * POSIX names remain case-sensitive. `inheritEnv: true` restores full inheritance, + * and only `true` does: a direct caller can pass any value, unchecked. */ export function buildChildEnv( config: Pick, hostEnv: NodeJS.ProcessEnv = process.env, platform: NodeJS.Platform = process.platform, ): NodeJS.ProcessEnv { const win = platform === 'win32'; - const env: NodeJS.ProcessEnv = config.inheritEnv ? { ...hostEnv } : {}; - if (!config.inheritEnv) { + const inherit = config.inheritEnv === true; + const env: NodeJS.ProcessEnv = inherit ? { ...hostEnv } : {}; + if (!inherit) { for (const [key, value] of Object.entries(hostEnv)) { if (value === undefined) continue; // Windows commonly enumerates Path/SystemRoot rather than PATH/SYSTEMROOT. @@ -481,6 +483,53 @@ export function isWebSocketTransport(config: McplServerConfig): boolean { return Boolean(config.url) && !config.command; } +/** The settings of McplServerConfig whose type is boolean. */ +type BooleanSetting = { + [K in keyof McplServerConfig]-?: NonNullable extends boolean ? K : never; +}[keyof McplServerConfig]; + +/** What `true` does for each boolean setting, as its refusal says it. A + * boolean setting added to McplServerConfig doesn't compile until it has + * an entry here, so the check below covers it too. */ +const SERVER_CONFIG_BOOLEANS: Record = { + inheritEnv: 'true passes the server the host\'s whole environment, and false or leaving it out passes only the allowlisted and LC_* host variables', + reconnect: 'true retries in the background when the server can\'t be reached or its connection drops, and false or leaving it out doesn\'t', + allowHostCommands: 'true lets the server run the host\'s admin commands, such as undo, hide, unstick and quiesce, and false or leaving it out refuses them', +}; + +/** + * Refuse a server config whose `inheritEnv`, `reconnect` or `allowHostCommands` + * holds anything but true or false. A config read from JSON can carry a quoted + * "false" where the type says boolean, and every reader of these settings takes + * only `true` as on, so the quotes would hide what the value does. null counts + * as not set, so a generated config that writes an unset setting as null + * keeps working. `inheritEnv` is checked only for a stdio server, since a + * WebSocket server never reads it. The error names the server, the setting + * and the value's kind, not the value, which could be a secret pasted into + * the wrong field. + */ +export function assertServerConfigBooleans(config: McplServerConfig): void { + for (const [setting, whatTrueDoes] of Object.entries(SERVER_CONFIG_BOOLEANS) as Array<[BooleanSetting, string]>) { + if (setting === 'inheritEnv' && isWebSocketTransport(config)) continue; + const value: unknown = config[setting]; + if (value === undefined || value === null || typeof value === 'boolean') continue; + const wanted = typeof value === 'string' ? 'true or false, unquoted,' : 'true or false,'; + throw new Error(`MCPL server "${config.id}" ${setting} must be ${wanted} not ${kindOf(value)}: ${whatTrueDoes}`); + } +} + +/** The kind of a value that isn't undefined, null or a boolean, as a refusal + * names it in place of the value. */ +function kindOf(value: unknown): string { + if (Array.isArray(value)) return 'an array'; + if (typeof value !== 'object') return `a ${typeof value}`; + const prototype = Object.getPrototypeOf(value) as { constructor?: { name?: string } } | null; + if (prototype === null || prototype === Object.prototype) return 'an object'; + const name = prototype.constructor?.name; + if (!name) return 'an object that isn\'t plain'; + return `${/^[aeiou]/i.test(name) ? 'an' : 'a'} ${name}`; +} + /** * Open the transport selected by `config` (WebSocket when `transport` is * 'websocket' or a bare `url` is given; stdio otherwise). The returned transport diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index 8c2cef42f..78d876e08 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -238,7 +238,10 @@ export interface McplServerConfig { /** * Escape hatch: pass the host's entire environment (including secrets) to * the stdio child, as older versions did. Only for servers that genuinely - * need it. Default false. + * need it. Default false. Only `true` turns it on, and null counts as not + * set. For a stdio server, `create()`, `connectMcplServer` and + * `restartMcplServer` refuse any other value, such as a quoted "false". A + * WebSocket server never reads it. */ inheritEnv?: boolean; @@ -278,7 +281,9 @@ export interface McplServerConfig { * unstick). DEFAULT FALSE: no §6.2 capability path exists for it, and the * grant cannot confer it — only this config, decided by the operator, can * (PR #79 review blocker 9). The one legitimate fleet user is - * discord-mcpl's slash-command relay. + * discord-mcpl's slash-command relay. Only `true` turns it on, and null + * counts as not set. `create()`, `connectMcplServer` and + * `restartMcplServer` refuse any other value, such as a quoted "true". */ allowHostCommands?: boolean; @@ -351,6 +356,9 @@ export interface McplServerConfig { * When true, connect() resolves immediately (with null capabilities) if the * server is unavailable, and retries in the background. * Adapted from Anarchid/agent-framework@mcpl-module-proto. + * Only `true` turns it on, and null counts as not set. `create()`, + * `connectMcplServer` and `restartMcplServer` refuse any other value, such + * as a quoted "false". */ reconnect?: boolean; diff --git a/test/mcpl-child-env.test.ts b/test/mcpl-child-env.test.ts index 2a12deb90..cfc824dba 100644 --- a/test/mcpl-child-env.test.ts +++ b/test/mcpl-child-env.test.ts @@ -81,6 +81,14 @@ test('inheritEnv: true passes the whole host env, declared env on top', () => { assert.equal(env.EXTRA, '1'); }); +test('only inheritEnv: true passes the whole host env; a quoted "false" or any other value keeps the allowlist', () => { + const allowlistOnly = buildChildEnv({}, HOST_ENV); + for (const inheritEnv of ['false', 'true', 'no', 1, 0, {}, [], null]) { + assert.deepEqual(buildChildEnv({ inheritEnv: inheritEnv as unknown as boolean }, HOST_ENV), allowlistOnly, + JSON.stringify(inheritEnv)); + } +}); + test('a real spawned stdio child sees only allowlist + declared env', async () => { const secretKey = 'MCPL_ENV_TEST_HOST_SECRET'; const previousCaBundle = process.env.REQUESTS_CA_BUNDLE; diff --git a/test/mcpl-config-booleans.test.ts b/test/mcpl-config-booleans.test.ts new file mode 100644 index 000000000..42fccef1a --- /dev/null +++ b/test/mcpl-config-booleans.test.ts @@ -0,0 +1,215 @@ +/** + * The boolean settings of an MCPL server's config (#379): `inheritEnv`, + * `reconnect` and `allowHostCommands` each take true or false, or are left + * out, and null counts as left out. The framework's entry points refuse any + * other value (test/mcpl-runtime-lifecycle.test.ts), and every reader takes + * only `true` as on. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { McplServerConnection } from '../src/mcpl/server-connection.js'; +import { McplServerRegistry } from '../src/mcpl/server-registry.js'; +import { assertServerConfigBooleans } from '../src/mcpl/transport.js'; +import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.js'; + +const WHAT_TRUE_DOES = { + inheritEnv: "true passes the server the host's whole environment, and false or leaving it out passes only the allowlisted and LC_* host variables", + reconnect: "true retries in the background when the server can't be reached or its connection drops, and false or leaving it out doesn't", + allowHostCommands: "true lets the server run the host's admin commands, such as undo, hide, unstick and quiesce, and false or leaving it out refuses them", +}; + +const STDIO = { id: 'discord', command: 'discord-mcpl' }; +const WEBSOCKET = { id: 'remote', url: 'ws://127.0.0.1:1' }; + +const check = (config: Record) => assertServerConfigBooleans(config as unknown as McplServerConfig); + +test('each boolean setting takes true, false, null or nothing, on either transport', () => { + for (const setting of Object.keys(WHAT_TRUE_DOES)) { + for (const value of [undefined, null, true, false]) { + check({ ...STDIO, [setting]: value }); + check({ ...WEBSOCKET, [setting]: value }); + } + } +}); + +test('any other value is refused, naming the server, the setting and the kind of value but not the value', () => { + const refused: Array<[unknown, string]> = [ + ['true', 'true or false, unquoted, not a string'], + ['false', 'true or false, unquoted, not a string'], + ['', 'true or false, unquoted, not a string'], + ['xoxb-pasted-token', 'true or false, unquoted, not a string'], + [0, 'true or false, not a number'], + [1, 'true or false, not a number'], + [[], 'true or false, not an array'], + [{}, 'true or false, not an object'], + [Object.create(null), 'true or false, not an object'], + [new Map(), 'true or false, not a Map'], + [new (class Item {})(), 'true or false, not an Item'], + [Object.create({}), 'true or false, not an Object'], + [new (class {})(), "true or false, not an object that isn't plain"], + ]; + for (const [setting, whatTrueDoes] of Object.entries(WHAT_TRUE_DOES)) { + for (const [value, wanted] of refused) { + assert.throws(() => check({ ...STDIO, [setting]: value }), (err: Error) => { + assert.equal(err.message, `MCPL server "discord" ${setting} must be ${wanted}: ${whatTrueDoes}`); + return true; + }, `${setting}: ${JSON.stringify(value)}`); + } + } +}); + +test('inheritEnv is checked only for a stdio server, since a WebSocket server never reads it', () => { + // A url alone or transport: 'websocket' dials, as openTransport chooses. + check({ ...WEBSOCKET, inheritEnv: 'false' }); + check({ id: 'remote', url: 'ws://127.0.0.1:1', command: 'unused', transport: 'websocket', inheritEnv: 'no' }); + // A url beside a command, with no transport, spawns the command. + assert.throws(() => check({ id: 'both', url: 'ws://127.0.0.1:1', command: 'discord-mcpl', inheritEnv: 'false' }), + /MCPL server "both" inheritEnv must be true or false, unquoted, not a string: /); + // The other two apply to both transports. + assert.throws(() => check({ ...WEBSOCKET, reconnect: 'false' }), + /MCPL server "remote" reconnect must be true or false, unquoted, not a string: /); + assert.throws(() => check({ ...WEBSOCKET, allowHostCommands: 'true' }), + /MCPL server "remote" allowHostCommands must be true or false, unquoted, not a string: /); +}); + +// The .mjs fixture is not compiled/copied by tsc, so when this test runs from +// dist/test it must reach back into the source tree for it. +const FIXTURE = [ + join(import.meta.dirname, 'fixtures/flaky-mcpl-server.mjs'), + join(import.meta.dirname, '../../test/fixtures/flaky-mcpl-server.mjs'), +].find((p) => existsSync(p))!; + +const HOST_CAPS: McplHostCapabilities = { + version: '0.5', + pushEvents: true, + contextHooks: { beforeInference: true }, + featureSets: true, +}; + +/** The flaky fixture exits before its handshake unless `flagFile` exists, + * and exits on tools/list once up. */ +function flaky(flagFile: string, reconnect: unknown): McplServerConfig { + return { + id: 'flaky', + command: process.execPath, + args: [FIXTURE, flagFile], + reconnect: reconnect as boolean, + reconnectIntervalMs: 50, + }; +} + +test('a reconnect that is not true never retries a failed first connect: the failure is thrown', async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpl-booleans-')); + const flag = join(dir, 'up'); // never written: the server exits before its handshake + const registry = new McplServerRegistry(); + try { + for (const reconnect of ['false', 'true', 1]) { + // A connection returned anyway is closed, so the failure is this + // assertion rather than a stub retrying in the background. + await assert.rejects( + McplServerConnection.connectWithReconnect(flaky(flag, reconnect), HOST_CAPS) + .then(async (connection) => { await connection.close(); return connection; }), + /before handshake/, + `connectWithReconnect, reconnect ${JSON.stringify(reconnect)}`, + ); + await assert.rejects(registry.addServer(flaky(flag, reconnect), HOST_CAPS), /before handshake/, + `addServer, reconnect ${JSON.stringify(reconnect)}`); + } + } finally { + await registry.closeAll(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a reconnect of "false" leaves a dropped server down: nothing retries, and the registry lets it go', { timeout: 30_000 }, async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpl-booleans-')); + const flag = join(dir, 'up'); + writeFileSync(flag, ''); + const registry = new McplServerRegistry(); + try { + const connection = await registry.addServer(flaky(flag, 'false'), HOST_CAPS); + const attempts: string[] = []; + connection.on('reconnect', () => attempts.push('reconnect')); + connection.on('reconnect-failed', () => attempts.push('reconnect-failed')); + const closed = new Promise((resolve) => connection.on('close', () => resolve())); + connection.ready(); + connection.sendToolsList().catch(() => {}); // the fixture exits on tools/list + await closed; + // A retry would start reconnectIntervalMs (50 ms, give or take a quarter) + // after the drop and respawn the fixture, whose flag file still exists, + // ending in 'reconnect' or 'reconnect-failed'. Neither comes within two + // seconds, forty intervals. + await new Promise((resolve) => setTimeout(resolve, 2000)); + assert.deepEqual(attempts, [], 'nothing retried after the drop'); + assert.equal(connection.willReconnect, false, 'no retry is armed'); + assert.equal(registry.getServer('flaky'), null, 'the registry dropped the closed server'); + } finally { + await registry.closeAll(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a server with reconnect: true that drops stays in the registry while it retries, and after it is back', { timeout: 30_000 }, async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpl-booleans-')); + const flag = join(dir, 'up'); + writeFileSync(flag, ''); + const registry = new McplServerRegistry(); + try { + const connection = await registry.addServer(flaky(flag, true), HOST_CAPS); + // Listeners run in the order they were added, so the registry's own + // 'close' handler has run by the time this one resolves. + const closed = new Promise((resolve) => connection.once('close', () => resolve())); + const reconnected = new Promise((resolve) => connection.once('reconnect', () => resolve())); + connection.ready(); + connection.sendToolsList().catch(() => {}); // the fixture exits on tools/list, and its flag brings it back + await closed; + assert.equal(registry.getServer('flaky'), connection, 'kept while its retry is armed'); + await reconnected; + assert.equal(registry.getServer('flaky'), connection, 'still the registered server once it is back'); + } finally { + await registry.closeAll(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a server the host closes leaves the registry, with reconnect: true too, since nothing will bring it back', { timeout: 30_000 }, async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpl-booleans-')); + const flag = join(dir, 'up'); + writeFileSync(flag, ''); + const registry = new McplServerRegistry(); + try { + const connection = await registry.addServer(flaky(flag, true), HOST_CAPS); + assert.equal(connection.willReconnect, true, 'a retry is armed while it runs'); + await connection.close(); // as a policy refusal's "close" fallback does + assert.equal(connection.willReconnect, false); + assert.equal(registry.getServer('flaky'), null, 'the registry dropped the server the host closed'); + } finally { + await registry.closeAll(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('an allowHostCommands that is not true reads as off, at connect and again at a reconnect', { timeout: 30_000 }, async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpl-booleans-')); + const flag = join(dir, 'up'); + writeFileSync(flag, ''); + let connection: McplServerConnection | undefined; + try { + connection = await McplServerConnection.connect( + { ...flaky(flag, true), allowHostCommands: 'true' as unknown as boolean }, HOST_CAPS); + assert.equal(connection.allowHostCommands, false, 'off at connect'); + const live = connection; + const reconnected = new Promise((resolve) => live.once('reconnect', () => resolve())); + live.ready(); + live.sendToolsList().catch(() => {}); // the fixture exits on tools/list, and reconnect: true brings it back + await reconnected; + assert.equal(live.allowHostCommands, false, 'off again after the reconnect'); + } finally { + await connection?.close(); + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/mcpl-runtime-lifecycle.test.ts b/test/mcpl-runtime-lifecycle.test.ts index 82c48137a..f3ad6c9c4 100644 --- a/test/mcpl-runtime-lifecycle.test.ts +++ b/test/mcpl-runtime-lifecycle.test.ts @@ -7,7 +7,7 @@ import { test } from 'node:test'; import assert from 'node:assert'; import { tmpdir } from 'node:os'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { WebSocketServer } from 'ws'; @@ -130,6 +130,87 @@ test('connectMcplServer rejects a duplicate id and a colliding prefix', async () }); }); +test('create() refuses a boolean setting that holds anything but true or false, before it spawns any server', async () => { + const tempDir = mkdtempSync(join(tmpdir(), 'mcpl-lifecycle-')); + const marker = join(tempDir, 'spawned'); + // A valid server listed first writes a marker when it starts, so a refusal + // that came after anything was spawned leaves the marker. Both servers + // exit as soon as they start, so a spawn that shouldn't have happened + // leaves no child holding the test open. + const early = { id: 'early', command: process.execPath, args: ['-e', `require('node:fs').writeFileSync(${JSON.stringify(marker)}, '')`] }; + const refused = (extra: Record) => ({ id: 'quoted', command: process.execPath, args: ['-e', ''], ...extra }); + const cases: Array<[string, Record, string]> = [ + ['inheritEnv', { inheritEnv: 'false' }, 'true or false, unquoted, not a string'], + ['reconnect', { reconnect: 'true' }, 'true or false, unquoted, not a string'], + ['allowHostCommands', { allowHostCommands: 1 }, 'true or false, not a number'], + ]; + try { + for (const [setting, extra, wanted] of cases) { + const created = AgentFramework.create({ + storePath: join(tempDir, `test-${setting}.chronicle`), + membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'assistant', model: 'test-model', systemPrompt: 'test' }], + modules: [], + mcplServers: [early, refused(extra)] as never, + }); + // A framework created anyway is stopped, so the failure is this + // assertion rather than a file held open by its server. + await assert.rejects( + created.then(async (framework) => { await framework.stop(); return framework; }), + new RegExp(`MCPL server "quoted" ${setting} must be ${wanted}: `), + ); + assert.equal(existsSync(marker), false, `no server was spawned before ${setting} was refused`); + } + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } +}); + +test('connectMcplServer refuses a boolean setting that holds anything but true or false, and registers nothing', async () => { + await withFramework(true, async (framework) => { + for (const [setting, value] of [['inheritEnv', 'false'], ['reconnect', 'false'], ['allowHostCommands', 'true']]) { + await assert.rejects( + framework.connectMcplServer(serverConfig('second', { [setting]: value }) as never), + new RegExp(`MCPL server "second" ${setting} must be true or false, unquoted, not a string: `), + ); + } + assert.deepEqual(framework.listMcplServers().map(s => s.id), ['first']); + }); +}); + +test('restartMcplServer refuses such a setting before it disconnects the running server, which stays up', async () => { + await withFramework(true, async (framework) => { + for (const [setting, value] of [['inheritEnv', 'true'], ['reconnect', 'true'], ['allowHostCommands', 'false']]) { + await assert.rejects( + framework.restartMcplServer('first', serverConfig('first', { [setting]: value }) as never), + new RegExp(`MCPL server "first" ${setting} must be true or false, unquoted, not a string: `), + ); + const servers = framework.listMcplServers(); + assert.equal(servers.length, 1); + assert.equal(servers[0].connected, true, `the running server stays connected after ${setting} was refused`); + } + }); +}); + +test('restartMcplServer given no config checks the stored one, before it disconnects the running server', async () => { + await withFramework(false, async (framework) => { + const config: Record = serverConfig('stored'); + await framework.connectMcplServer(config as never); + // The framework keeps the caller's own config object, so a host that + // changes it in place changes what a restart with no new config uses. + config.reconnect = 'true'; + await assert.rejects( + framework.restartMcplServer('stored'), + /MCPL server "stored" reconnect must be true or false, unquoted, not a string: /, + ); + assert.deepEqual( + framework.listMcplServers().map((s) => [s.id, s.connected]), + [['stored', true]], + 'the running server stays connected', + ); + }); +}); + test('connectMcplServer hot-connects a network-only (WebSocket) MCPL server', async () => { // Minimal in-process MCPL server over WebSocket (same shape as // mcpl-ws-transport.test.ts) — no child process involved. diff --git a/test/reaction-baseline-platform.test.ts b/test/reaction-baseline-platform.test.ts index 83b9e1163..85639b922 100644 --- a/test/reaction-baseline-platform.test.ts +++ b/test/reaction-baseline-platform.test.ts @@ -137,3 +137,14 @@ test('POSIX reaction baseline keeps differently cased env names distinct', async assert.ok(composed[KEY]!.split(',').includes('💤')); assert.ok(composed[KEY]!.split(',').includes('🔕')); }); + +test('an inheritEnv that is not true never reads the host baseline: the protective default stands', async () => { + // create() and connectMcplServer refuse such a value; this is the read a + // config reaching connectMcplServerInternal unchecked would meet. + for (const inheritEnv of ['false', 'true', 1]) { + const composed = await connectionEnv('linux', {}, inheritEnv as unknown as boolean, { [KEY]: '🟦' }); + assert.equal(composed[KEY], DEFAULT, JSON.stringify(inheritEnv)); + } + const inherited = await connectionEnv('linux', {}, true, { [KEY]: '🟦' }); + assert.equal(inherited[KEY], '🟦', 'true still inherits it'); +});