fix(mcpl): an MCPL server's boolean settings are true or false, and anything else is refused (#379)

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.

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<string, string>(); // 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,15 +17394,19 @@ 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;
   }
 
   /**
    * 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<void> {
+    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

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;
       }
 

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);
       }
     });

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<McplServerConfig, 'env' | 'inheritEnv'>,
   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<McplServerConfig[K]> 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<BooleanSetting, string> = {
+  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

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;
 

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;