From 4f26bddbdc891b85ba5bdf898913476d8bc39e62 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 02:38:45 -0400 Subject: [PATCH 1/3] fix(mcpl): an MCPL server's boolean settings are true or false, and anything else is refused (#379) Nothing checked the type of an MCPL server's boolean settings before it was spawned or dialed, and a config read from JSON can carry anything there: - inheritEnv: buildChildEnv and declaredReactionBaseline read it by truthiness, so `"inheritEnv": "false"`, or "no", 1, {} or [], handed a stdio child the host's whole environment, API keys and bot tokens included (#379, with the probe there). - reconnect: McplServerConnection.connect and connectWithReconnect and McplServerRegistry.addServer read it by truthiness, so "false" turned retrying on, while the startup connect's failure trace read it as `=== true`. - allowHostCommands: every reader takes only `true`, so "true" granted nothing, though the config read as a grant. - assertServerConfigBooleans (src/mcpl/transport.ts) checks the three: true and false stand, null counts as not set, and anything else is refused, naming the server, the setting and the value's kind, never the value (it could be a secret pasted into the wrong field), and saying what true does there. "unquoted" is said only for a string. inheritEnv is checked only for a stdio server, by isWebSocketTransport, since a WebSocket server never reads it. The table it reads is typed over McplServerConfig's boolean settings, so a boolean setting added later doesn't compile without an entry. - create() checks every configured server before any is brought up, beside the toolPrefix checks; runConnectMcplServer checks first; and runRestartMcplServer checks before it disconnects the running server, so a refused restart leaves it up. A colliding toolPrefix on a restart is still refused only by the connect, after the disconnect, as on main. - reconnect's four truthy readers take only true, as inheritEnv's two do (buildChildEnv, declaredReactionBaseline), so a value that reaches a reader without passing an entry, such as a direct McplServerConnection.connect caller's, reads as off everywhere. - McplServerConfig's docs of the three, and connectMcplServer's and restartMcplServer's, say what's refused. Tests: - mcpl-config-booleans (new): each setting takes true, false, null or nothing on either transport; anything else is refused with the exact message, which names the kind and not the value; inheritEnv is skipped for a WebSocket server (a url alone, or transport: 'websocket') and checked for a url beside a command; connectWithReconnect and addServer throw a failed first connect when reconnect isn't true; and a connected server with reconnect "false" arms no retry and leaves the registry when it drops. - mcpl-runtime-lifecycle: create() refuses each setting, with a valid server listed first that writes a marker when spawned, and no marker is written; connectMcplServer refuses each and registers nothing; restartMcplServer refuses each and the server stays connected. - mcpl-child-env: buildChildEnv gives the allowlist for "false", "true", "no", 1, 0, {}, [] and null; reaction-baseline-platform: "false", "true" and 1 keep the protective default. The fragment names the quoted cases and what to write instead, and what connectome-host's readers pass on. Co-Authored-By: Claude Opus 5.5 --- .../379-server-config-booleans.fixed.md | 1 + src/framework.ts | 22 ++- src/mcpl/server-connection.ts | 7 +- src/mcpl/server-registry.ts | 4 +- src/mcpl/transport.ts | 53 ++++++- src/mcpl/types.ts | 12 +- test/mcpl-child-env.test.ts | 8 + test/mcpl-config-booleans.test.ts | 142 ++++++++++++++++++ test/mcpl-runtime-lifecycle.test.ts | 64 +++++++- test/reaction-baseline-platform.test.ts | 11 ++ 10 files changed, 309 insertions(+), 15 deletions(-) create mode 100644 changelog.d/379-server-config-booleans.fixed.md create mode 100644 test/mcpl-config-booleans.test.ts 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..36825485b --- /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. A quoted `"true"` is refused for all three, including where the truthy reading made it work: write `true` or `false` without quotes. connectome-host refuses a non-boolean `inheritEnv` in its own readers but passes the other two on as written, so a quoted `reconnect` or `allowHostCommands` in its configuration now stops `create()` with this refusal. 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..5c62ec7c3 100644 --- a/src/mcpl/server-registry.ts +++ b/src/mcpl/server-registry.ts @@ -42,14 +42,14 @@ 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) connection.on('close', () => { - if (!config.reconnect) { + if (config.reconnect !== true) { this.servers.delete(config.id); } }); diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 65fc538cb..93fa646bc 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,51 @@ 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 send host/command requests, 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, as JSON writes that. `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; + return name ? `a ${name}` : 'an object that isn\'t plain'; +} + /** * 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..6b835aa6d --- /dev/null +++ b/test/mcpl-config-booleans.test.ts @@ -0,0 +1,142 @@ +/** + * 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 send host/command requests, 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'], + [new Map(), 'true or false, not a Map'], + ]; + 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 that is not true leaves a dropped server down: nothing retries, and the registry lets it go', 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); + assert.equal(connection.willReconnect, false, 'no retry is armed'); + const closed = new Promise((resolve) => connection.on('close', () => resolve())); + connection.ready(); + connection.sendToolsList().catch(() => {}); // the fixture exits on tools/list + await closed; + assert.equal(registry.getServer('flaky'), null, 'the registry dropped the closed server'); + assert.equal(connection.willReconnect, false); + } finally { + await registry.closeAll(); + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/mcpl-runtime-lifecycle.test.ts b/test/mcpl-runtime-lifecycle.test.ts index 82c48137a..8ab3769ac 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,68 @@ 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('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'); +}); From c8405821b0d1b03ad6b3a6705c5f7d69d19ff25f Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 09:39:32 -0400 Subject: [PATCH 2/3] fix(mcpl): answer the round's review: the registry asks the connection, kinds named rightly, and tests that observe Our internal review of the previous commit found that the registry's close handler read `reconnect` from the config a second time, that kindOf wrote "a Object" and "a Item", that a test's title promised more than it ran, and that several pieces had no test that would fail without them. - McplServerRegistry's close handler drops a server unless the connection will reconnect (`connection.willReconnect`), instead of reading `config.reconnect` again. The connection decided that once, from `reconnect === true` at connect, and a close() the host asks for disarms it first. 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, and a config changed in place after connect can't make the two disagree. - kindOf writes "an" before a constructor name that starts with a vowel ("an Item", and "an Object" for an object whose prototype is another plain object). - The allowHostCommands refusal says what `true` grants in the terms an operator or agent reads, the host's admin commands such as undo, hide, unstick and quiesce, rather than the method name host/command. - The fragment says a quoted "true" is refused wherever the setting is checked, since inheritEnv isn't checked for a WebSocket server, and it no longer describes connectome-host's readers. Those are in the pull request's description, read at connectome-host 4fbc924: its mcpl-servers.json reader and its recipe validator refuse a non-boolean inheritEnv and its agent overlay drops the field, so its overlay doesn't refuse one, and its mcpl-servers.json reader drops allowHostCommands. Tests: - restartMcplServer given no config checks the stored one before it disconnects the running server (the framework keeps the caller's object, so a host can change it in place); - a server the host closes leaves the registry with reconnect: true too; - a non-true allowHostCommands reads as off at connect and again at a reconnect, which pins both connection readers; - the dropped-server test now observes that no retry comes within forty reconnect intervals before it reads the connection's own state, is named for the "false" it runs, and has a timeout, so a close that never comes fails it rather than hanging the file; - the refusal's kinds include a null-prototype object, a class named with a vowel, an object whose prototype is plain, and an anonymous class. Co-Authored-By: Claude Opus 5.5 --- .../379-server-config-booleans.fixed.md | 2 +- src/mcpl/server-registry.ts | 6 +- src/mcpl/transport.ts | 5 +- test/mcpl-config-booleans.test.ts | 56 ++++++++++++++++++- test/mcpl-runtime-lifecycle.test.ts | 19 +++++++ 5 files changed, 80 insertions(+), 8 deletions(-) diff --git a/changelog.d/379-server-config-booleans.fixed.md b/changelog.d/379-server-config-booleans.fixed.md index 36825485b..5f20a60d9 100644 --- a/changelog.d/379-server-config-booleans.fixed.md +++ b/changelog.d/379-server-config-booleans.fixed.md @@ -1 +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. A quoted `"true"` is refused for all three, including where the truthy reading made it work: write `true` or `false` without quotes. connectome-host refuses a non-boolean `inheritEnv` in its own readers but passes the other two on as written, so a quoted `reconnect` or `allowHostCommands` in its configuration now stops `create()` with this refusal. +- 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/mcpl/server-registry.ts b/src/mcpl/server-registry.ts index 5c62ec7c3..7aed41e47 100644 --- a/src/mcpl/server-registry.ts +++ b/src/mcpl/server-registry.ts @@ -47,9 +47,11 @@ export class McplServerRegistry { : 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 !== true) { + if (!connection.willReconnect) { this.servers.delete(config.id); } }); diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 93fa646bc..9b740c0ae 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -494,7 +494,7 @@ type BooleanSetting = { 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 send host/command requests, and false or leaving it out refuses them', + 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', }; /** @@ -525,7 +525,8 @@ function kindOf(value: unknown): string { const prototype = Object.getPrototypeOf(value) as { constructor?: { name?: string } } | null; if (prototype === null || prototype === Object.prototype) return 'an object'; const name = prototype.constructor?.name; - return name ? `a ${name}` : 'an object that isn\'t plain'; + if (!name) return 'an object that isn\'t plain'; + return `${/^[aeiou]/i.test(name) ? 'an' : 'a'} ${name}`; } /** diff --git a/test/mcpl-config-booleans.test.ts b/test/mcpl-config-booleans.test.ts index 6b835aa6d..d9040a13c 100644 --- a/test/mcpl-config-booleans.test.ts +++ b/test/mcpl-config-booleans.test.ts @@ -19,7 +19,7 @@ import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.j 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 send host/command requests, and false or leaving it out refuses them', + 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' }; @@ -46,7 +46,11 @@ test('any other value is refused, naming the server, the setting and the kind of [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) { @@ -121,22 +125,68 @@ test('a reconnect that is not true never retries a failed first connect: the fai } }); -test('a reconnect that is not true leaves a dropped server down: nothing retries, and the registry lets it go', async () => { +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); - assert.equal(connection.willReconnect, false, 'no retry is armed'); + 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 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 8ab3769ac..f3ad6c9c4 100644 --- a/test/mcpl-runtime-lifecycle.test.ts +++ b/test/mcpl-runtime-lifecycle.test.ts @@ -192,6 +192,25 @@ test('restartMcplServer refuses such a setting before it disconnects the running }); }); +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. From c1538b111ee4c5d8b7440564ac6deff69a7c6516 Mon Sep 17 00:00:00 2001 From: Effie-2756 Date: Sat, 17 Oct 2026 09:54:26 -0400 Subject: [PATCH 3/3] test(mcpl): a retrying server stays in the registry; null is the config's choice The registry's close handler now asks the connection whether it will reconnect. The tests pinned the side that drops a server (reconnect off, or a server the host closed for good), but nothing pinned the side that keeps one: a handler that dropped every closed server would leave a reconnecting server unregistered while it came back, and no test would fail. A server with reconnect: true that drops now stays registered while its retry is armed, and is still the registered server once it is back. assertServerConfigBooleans' comment said null counts as not set "as JSON writes that". JSON can also leave a setting out; counting null as not set is the config's choice, which keeps a generated config that writes an unset setting as null working. Co-Authored-By: Claude Opus 5.5 --- src/mcpl/transport.ts | 9 +++++---- test/mcpl-config-booleans.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 9b740c0ae..6109f3276 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -502,10 +502,11 @@ const SERVER_CONFIG_BOOLEANS: Record = { * 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, as JSON writes that. `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. + * 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]>) { diff --git a/test/mcpl-config-booleans.test.ts b/test/mcpl-config-booleans.test.ts index d9040a13c..42fccef1a 100644 --- a/test/mcpl-config-booleans.test.ts +++ b/test/mcpl-config-booleans.test.ts @@ -153,6 +153,29 @@ test('a reconnect of "false" leaves a dropped server down: nothing retries, and } }); +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');