diff --git a/changelog.d/473-code-execution-enabled.fixed.md b/changelog.d/473-code-execution-enabled.fixed.md new file mode 100644 index 000000000..fa4728cbb --- /dev/null +++ b/changelog.d/473-code-execution-enabled.fixed.md @@ -0,0 +1 @@ +- `codeExecution.enabled` takes `true` or `false`, or can be left out, and `null` counts as left out (#473). `create()` refuses any other value, and a `codeExecution` that isn't an object, naming the value's kind, not the value. The switch was read by truthiness, so `"enabled": "false"`, as a JSON config or a `${VAR}` substitution can write it, gave agents the `code_execution` tool and a python interpreter on the host, and a bare `"codeExecution": true` was read as off. Only `true` turns it on now. As before, a configuration that isn't on has its other settings left unchecked. A host that passes `codeExecution` on from its own configuration without checking it now gets this refusal from `create()` where such a value used to start. diff --git a/src/code-execution/tool-definition.ts b/src/code-execution/tool-definition.ts index cab62de22..7a0fea14e 100644 --- a/src/code-execution/tool-definition.ts +++ b/src/code-execution/tool-definition.ts @@ -9,6 +9,7 @@ */ import type { ToolDefinition } from '../types/index.js'; +import { kindOf } from '../mcpl/transport.js'; import { formatLimit, MAX_TIMER_MS } from './py-runner.js'; export const CODE_EXECUTION_TOOL_NAME = 'code_execution'; @@ -106,15 +107,41 @@ export function scriptTimeLimits(opts?: { const defaultMs = Math.min(MAX_TIMER_MS, opts?.scriptTimeoutMs ?? 600_000); return { defaultMs, - // A ceiling below the default is refused when the framework is created - // (validateCodeExecutionConfig); never below the default here either. + // A ceiling below the default is refused when a framework with code + // execution on is created (validateCodeExecutionConfig); never below the + // default here either. maxMs: Math.min(MAX_TIMER_MS, Math.max(defaultMs, opts?.maxScriptTimeoutMs ?? defaultMs)), backgroundMaxMs: Math.min(MAX_TIMER_MS, opts?.backgroundMaxLifetimeMs ?? 86_400_000), }; } -/** Refuse a configuration whose time limits contradict each other. */ -export function validateCodeExecutionConfig(cfg: { scriptTimeoutMs?: number; maxScriptTimeoutMs?: number }): void { +/** + * Refuse a codeExecution that isn't an object, or whose switch is set to + * anything but true or false, and, once it's on, one whose time limits + * contradict each other. A JSON config, or a `${VAR}` substituted as a string, + * can carry a quoted "false" where the type says boolean, and only `true` turns + * code execution on, so the quotes would hide what the value does; a bare + * `codeExecution: true` would be read as off. A null switch counts as not set, + * as a left-out one does. The refusal names the value's kind, not the value. + * A configuration that isn't on has nothing else read. + */ +export function validateCodeExecutionConfig(cfg: { + enabled?: unknown; + scriptTimeoutMs?: number; + maxScriptTimeoutMs?: number; +}): void { + if (cfg === null || typeof cfg !== 'object' || Array.isArray(cfg)) { + throw new Error(`codeExecution must be an object, such as { enabled: true }, not ${cfg === null ? 'null' : kindOf(cfg)}`); + } + if (cfg.enabled !== undefined && cfg.enabled !== null && typeof cfg.enabled !== 'boolean') { + const wanted = typeof cfg.enabled === 'string' ? 'true or false, unquoted,' : 'true or false,'; + throw new Error( + `codeExecution.enabled must be ${wanted} not ${kindOf(cfg.enabled)}: ` + + 'true gives agents the code_execution tool, which runs their scripts in a python interpreter on the host, ' + + 'and false or leaving it out doesn\'t', + ); + } + if (cfg.enabled !== true) return; const defaultMs = cfg.scriptTimeoutMs ?? 600_000; if (cfg.maxScriptTimeoutMs !== undefined && !(cfg.maxScriptTimeoutMs >= defaultMs)) { throw new Error( diff --git a/src/framework.ts b/src/framework.ts index 7fe1bdda8..8b0b34b16 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2300,7 +2300,7 @@ export class AgentFramework { */ static async create(config: FrameworkConfig): Promise { // Before anything opens or starts: a refused config must leave nothing behind. - if (config.codeExecution?.enabled) validateCodeExecutionConfig(config.codeExecution); + if (config.codeExecution != null) validateCodeExecutionConfig(config.codeExecution); // Create or use existing store let store: JsStore; @@ -2636,7 +2636,7 @@ export class AgentFramework { // Client-side programmatic tool calling (code_execution). Config is only // retained when enabled — everything downstream gates on the field. - framework.codeExecutionConfig = config.codeExecution?.enabled ? config.codeExecution : null; + framework.codeExecutionConfig = config.codeExecution?.enabled === true ? config.codeExecution : null; framework.toolClassOverrides = normalizeClassTable(config.toolClassOverrides, 'toolClassOverrides'); framework.hostToolClasses = [ diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 6109f3276..8be8747ed 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -520,7 +520,7 @@ export function assertServerConfigBooleans(config: McplServerConfig): void { /** 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 { +export 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; diff --git a/src/types/framework.ts b/src/types/framework.ts index 003ed8d92..a58cdf41d 100644 --- a/src/types/framework.ts +++ b/src/types/framework.ts @@ -41,7 +41,9 @@ export interface ProcessLoggingConfig { * the agent already has broader host access through its tools). */ export interface CodeExecutionConfig { - /** Master switch — the tool is only synthesized when true. */ + /** Master switch — only `true` synthesizes the tool. `create()` refuses a + * value that isn't a boolean, a quoted "false" included; `null` or leaving + * it out counts as off. */ enabled: boolean; /** Python interpreter to spawn (default: 'python3' from PATH). */ pythonPath?: string; @@ -210,7 +212,8 @@ export interface FrameworkConfig { /** * Client-side programmatic tool calling (`code_execution` tool). Omitted or - * `enabled: false` → tool absent, no python is ever spawned. + * `enabled: false` → tool absent, no python is ever spawned. A value that + * isn't an object is refused at `create()`. */ codeExecution?: CodeExecutionConfig; diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 0b0093d75..408fcaf15 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -1302,6 +1302,77 @@ describe('code_execution per-call time limit (time_limit_ms)', () => { } }); + const createWith = (storePath: string, codeExecution: unknown) => + AgentFramework.create({ + storePath, + membrane: new MockMembrane().asMembrane(), + agents: [], + modules: [], + syncIntervalMs: 0, + codeExecution: codeExecution as never, + }); + + /** create() refuses each config with exactly its message, before anything + * opens: no store is left at storePath. */ + async function assertRefused(cases: ReadonlyArray): Promise { + const { tempDir, storePath } = tempStorePath('pytc-refused-'); + try { + for (const [codeExecution, message] of cases) { + await assert.rejects(createWith(storePath, codeExecution), (error: Error) => { + assert.equal(error.message, message); + return true; + }); + assert.equal(existsSync(storePath), false, 'the refused config opened no store'); + } + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + } + + it('create() refuses a codeExecution.enabled that is not true or false, a quoted "false" included', async () => { + // A quoted "false" is truthy, so it turned code execution on, and 0 is + // falsy, so it was read as off without a word. Each message names the + // value's kind, never the value, since a substituted value can be a + // credential. + const whatTrueDoes = 'true gives agents the code_execution tool, which runs their scripts in a python interpreter on the host, ' + + 'and false or leaving it out doesn\'t'; + await assertRefused([ + [{ enabled: 'false' }, `codeExecution.enabled must be true or false, unquoted, not a string: ${whatTrueDoes}`], + [{ enabled: 'sk-secret-token' }, `codeExecution.enabled must be true or false, unquoted, not a string: ${whatTrueDoes}`], + [{ enabled: 0 }, `codeExecution.enabled must be true or false, not a number: ${whatTrueDoes}`], + [{ enabled: { on: true } }, `codeExecution.enabled must be true or false, not an object: ${whatTrueDoes}`], + ]); + }); + + it('create() refuses a codeExecution that is not an object, which would read as off', async () => { + await assertRefused([ + [true, 'codeExecution must be an object, such as { enabled: true }, not a boolean'], + ['true', 'codeExecution must be an object, such as { enabled: true }, not a string'], + [[], 'codeExecution must be an object, such as { enabled: true }, not an array'], + ]); + }); + + it('a null or left-out codeExecution.enabled counts as off, and a configuration that is off has nothing else checked', async () => { + // Contradictory limits are refused only once code execution is on. + const contradictory = { scriptTimeoutMs: 600_000, maxScriptTimeoutMs: 60_000 }; + for (const codeExecution of [ + null, + { enabled: null, ...contradictory }, + { enabled: false, ...contradictory }, + { ...contradictory }, + ]) { + const { tempDir, storePath } = tempStorePath('pytc-enabled-off-'); + let framework: AgentFramework | undefined; + try { + framework = await createWith(storePath, codeExecution); + assert.equal(framework.getAllTools().some((tool) => tool.name === 'code_execution'), false); + } finally { + await framework?.stop(); + rmSync(tempDir, { recursive: true, force: true }); + } + } + }); + async function withFramework( codeExecution: Record, body: (framework: AgentFramework) => Promise,