From 54ab0b0222fb3c81de55493a700960c5afb1020a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 27 Aug 2026 10:21:56 +0200 Subject: [PATCH 1/3] fix(mcp): admit a batch step's input against the nested command's own schema The MCP admission boundary reads the FLAT keys of a tool call. Every tool schema is `additionalProperties:false`, so that is the whole boundary -- except for the keys a schema cannot NAME. A batch step's accepted keys depend on its sibling `command`, which JSON Schema cannot express, so `steps[].input` is declared free-form and the flat scan looked straight past it. Everything the flat scan refuses was therefore admitted one level in. `readBatchDaemonStep` projects a step's input into per-step daemon request FLAGS, and the daemon obeys them: `iosSimulatorDeviceSet` selects the simulator device set `resolveTargetDevice` searches, and `iosXctestrunFile` selects the `.xctestrun` the Apple runner launches. That is the operator-infrastructure write `OPERATOR_INPUT_GUIDANCE` exists to refuse, reached through the one input the model is invited to nest. Declare the seam instead of detecting it. `JsonSchema.commandInputFor` marks an object as another command's input and names the sibling holding that command's name; `batchStepSchema` sets it on `steps[].input`; and `findInadmissibleNestedCommandInput` walks the tool's own schema against the raw arguments, re-running `findInadmissibleInput` per nested input against that command's advertised schema. Nested admission is the same function, and returns the same answer, as flat admission -- a step accepts exactly what the nested command's own tool accepts. Both model-facing surfaces are covered at once: the MCP router and the AI SDK adapter share this executor. The regression test states that parity rather than a key list, so a key added to `OPERATOR_INPUT_GUIDANCE` later is covered the day it lands. --- src/commands/batch/metadata.ts | 6 + src/commands/command-contract.ts | 9 ++ .../command-tools-nested-admission.test.ts | 127 ++++++++++++++++++ src/mcp/command-tools.ts | 93 ++++++++++++- 4 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 src/mcp/__tests__/command-tools-nested-admission.test.ts diff --git a/src/commands/batch/metadata.ts b/src/commands/batch/metadata.ts index d997a9039b..2b2f425e42 100644 --- a/src/commands/batch/metadata.ts +++ b/src/commands/batch/metadata.ts @@ -89,6 +89,12 @@ function batchStepSchema(nestedCommands: readonly string[]): JsonSchema { input: { type: 'object', additionalProperties: true, + // The accepted keys depend on `command`, so this object cannot be typed + // here. `commandInputFor` names the sibling that resolves them, which is + // what lets the model-facing admission boundary check a step's input + // against that command's own advertised schema instead of waving through + // an opaque object (`mcp/command-tools.ts`). + commandInputFor: 'command', description: 'Structured command input for the nested command. Use the matching MCP tool schema for this object.', }, diff --git a/src/commands/command-contract.ts b/src/commands/command-contract.ts index e06e4a89b2..7259117897 100644 --- a/src/commands/command-contract.ts +++ b/src/commands/command-contract.ts @@ -14,6 +14,15 @@ export type JsonSchema = { const?: unknown; minimum?: number; maximum?: number; + /** + * Marks this object as ANOTHER command's input, naming the sibling property + * that holds that command's name. Such an object is free-form by necessity — + * its accepted keys depend on a value, which JSON Schema cannot express — so + * without the marker it is opaque to the model-facing admission boundary, + * which then checks nothing inside it. Declaring it here is what lets + * admission recurse with the named command's own advertised schema. + */ + commandInputFor?: string; }; export type CommandMetadata = { diff --git a/src/mcp/__tests__/command-tools-nested-admission.test.ts b/src/mcp/__tests__/command-tools-nested-admission.test.ts new file mode 100644 index 0000000000..447d75b9ce --- /dev/null +++ b/src/mcp/__tests__/command-tools-nested-admission.test.ts @@ -0,0 +1,127 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import type { AgentDeviceClient } from '../../client/client-types.ts'; +import { STRUCTURED_BATCH_COMMAND_NAMES } from '../../core/batch-policy.ts'; +import { findCommandMetadata } from '../../commands/command-metadata.ts'; +import { createCommandToolExecutor } from '../command-tools.ts'; + +// `batch` is the one tool whose input nests another command's input, and its +// step object is free-form by necessity (the accepted keys depend on the sibling +// `command`, which JSON Schema cannot express). The flat admission scan looked +// straight past it, so every key refused on a flat call was admitted one level +// in — and `readBatchDaemonStep` projects a step's input into per-step daemon +// request FLAGS, where `iosSimulatorDeviceSet` selects the simulator device set +// `resolveTargetDevice` searches and `iosXctestrunFile` selects the `.xctestrun` +// the Apple runner launches. +// +// The invariant these tests hold is a PARITY, not a key list: a batch step's +// input admits exactly what the nested command's own tool admits. Stated that +// way, a key added to `OPERATOR_INPUT_GUIDANCE` — or a whole new operator +// classification — is covered the day it lands, with nothing here to update. +const OPERATOR_PROBES: Readonly> = { + daemonAuthToken: 'stolen-token', + daemonBaseUrl: 'http://attacker.example:9000', + stateDir: '/attacker/state-dir', + cwd: '/attacker/cwd', + iosSimulatorDeviceSet: '/attacker/device-set', + iosXctestrunFile: '/attacker/run.xctestrun', + iosXctestDerivedDataPath: '/attacker/derived', + iosXctestEnvDir: '/attacker/env', + config: '/attacker/config.json', + remoteConfig: '/attacker/remote.json', + totallyUnknownKey: 'x', +}; + +function createProbeExecutor() { + const calls: Array<{ name: string; input: Record }> = []; + const executor = createCommandToolExecutor({ + createClient: () => ({}) as AgentDeviceClient, + runCommand: async (_client, name, input) => { + calls.push({ name, input: input as Record }); + return { total: 0, executed: 0, totalDurationMs: 0, results: [] }; + }, + }); + return { calls, executor }; +} + +test('a batch step admits exactly what the nested command tool admits', async () => { + const { calls, executor } = createProbeExecutor(); + + for (const command of STRUCTURED_BATCH_COMMAND_NAMES) { + for (const [key, probe] of Object.entries(OPERATOR_PROBES)) { + const flat = await executor.execute(command, { [key]: probe }); + calls.length = 0; + const nested = await executor.execute('batch', { + steps: [{ command, input: { [key]: probe } }], + }); + assert.equal( + nested.isError, + flat.isError, + `${command}: nested ${key} must be admitted iff flat ${key} is`, + ); + if (!flat.isError) continue; + assert.deepEqual(calls, [], `${command}: a refused nested ${key} must not be dispatched`); + assert.match( + nested.content[0]?.text ?? '', + new RegExp(`batch\\.steps\\[0\\]\\.input: ${key} is not`), + `${command}: nested ${key} must be refused with the flat guidance, located`, + ); + } + } +}); + +test('a refused step is refused wherever it sits in the batch', async () => { + const { calls, executor } = createProbeExecutor(); + + const result = await executor.execute('batch', { + steps: [ + { command: 'snapshot', input: {} }, + { command: 'tap', input: { target: { kind: 'selector', selector: 'text=OK' } } }, + { command: 'snapshot', input: { iosXctestrunFile: '/attacker/run.xctestrun' } }, + ], + }); + + assert.equal(result.isError, true); + assert.match(result.content[0]?.text ?? '', /batch\.steps\[2\]\.input: iosXctestrunFile is not/); + assert.match(result.content[0]?.text ?? '', /AGENT_DEVICE_IOS_XCTESTRUN_FILE/); + assert.deepEqual(calls, [], 'no step runs when one is refused'); +}); + +test('nested admission leaves legitimate batch input untouched', async () => { + const { calls, executor } = createProbeExecutor(); + + const steps = [ + { command: 'open', input: { app: 'settings' } }, + { command: 'snapshot', input: { interactiveOnly: true, noRecord: true } }, + { command: 'tap', input: { target: { kind: 'selector', selector: 'text=OK' } } }, + { command: 'type', input: { text: 'hello' }, runtime: { platform: 'ios' } }, + { command: 'wait', input: { kind: 'duration', durationMs: 100, session: 'sim' } }, + ]; + const result = await executor.execute('batch', { steps }); + + assert.equal(result.isError, false, result.content[0]?.text); + assert.deepEqual(calls[0]?.input.steps, steps); +}); + +// A step whose command cannot be resolved has no schema to check its input +// against — and needs none, because that step cannot run. Admission must fall +// through to the reader that owns the error instead of answering with a key +// complaint that would bury it. +test('an unresolvable step command is left to the batch reader', async () => { + const { calls, executor } = createProbeExecutor(); + const steps = [ + { command: 'not-a-command', input: { iosXctestrunFile: '/attacker/run.xctestrun' } }, + ]; + + const result = await executor.execute('batch', { steps }); + + assert.equal(result.isError, false, 'admission has no verdict to give here'); + assert.deepEqual(calls[0]?.input.steps, steps); + + // And the reader that does own it still refuses the step, so the unchecked + // input never reaches a daemon request flag. + assert.throws( + () => findCommandMetadata('batch').readInput({ steps }), + /not available through command batch/, + ); +}); diff --git a/src/mcp/command-tools.ts b/src/mcp/command-tools.ts index 3324ab7ec1..83f8487af1 100644 --- a/src/mcp/command-tools.ts +++ b/src/mcp/command-tools.ts @@ -17,6 +17,7 @@ import { } from '../core/command-descriptor/registry.ts'; import { MCP_COMMAND_OUTPUT_SCHEMAS } from './mcp-output-schemas.ts'; import { AppError } from '@agent-device/kernel/errors'; +import { isRecord } from '../utils/parsing.ts'; import { formatToolErrorText, normalizeToolError } from './tool-error.ts'; import { resolveMcpConfigDefaults } from './tool-input-config.ts'; import { projectStructuredContent } from './tool-result.ts'; @@ -134,8 +135,16 @@ export function createCommandToolExecutor(deps: CommandToolExecutorDeps = {}): C // with the operator's token. Reject every raw key the advertised schema // does not list, BEFORE config/env defaults merge (operator env/config // values still resolve below — they never arrive as tool input). + // + // "Every tool schema is additionalProperties:false" holds for the keys a + // schema can NAME. A nested command's input keys depend on a sibling + // value, so `batch` cannot name them and declares that object free-form; + // the flat scan below then looked straight past it. The second check + // recurses there — see `findInadmissibleNestedCommandInput`. const metadata = findCommandMetadata(name); - const rejection = findInadmissibleInput(name, metadata, input); + const rejection = + findInadmissibleInput(name, metadata, input) ?? + findInadmissibleNestedCommandInput(metadata.inputSchema, input, name); if (rejection) { return buildErrorToolResult( new AppError('INVALID_ARGS', rejection), @@ -343,6 +352,88 @@ function findInadmissibleInput( return undefined; } +/** + * The first inadmissible key inside a NESTED command input, with guidance, or + * undefined. + * + * {@link findInadmissibleInput} reads the FLAT keys of a tool call, which is the + * whole boundary for a tool whose schema is `additionalProperties:false` all the + * way down. `batch` is not: a step's accepted keys depend on its sibling + * `command`, which JSON Schema cannot express, so `steps[].input` is declared + * free-form and the flat scan looked straight past it. Everything the flat scan + * refuses was therefore admitted one level in — and `readBatchDaemonStep` + * projects a step's input into per-step daemon request FLAGS, where + * `iosSimulatorDeviceSet` picks the simulator device set `resolveTargetDevice` + * searches and `iosXctestrunFile` picks the `.xctestrun` the Apple runner + * launches. That is the operator-infrastructure write + * {@link OPERATOR_INPUT_GUIDANCE} exists to refuse, reached through the one + * input the model is invited to nest. + * + * So the schema declares where those objects are (`commandInputFor` names the + * sibling holding the command name) and this walks the tool's own schema against + * the raw arguments, checking each one with {@link findInadmissibleInput} + * against that command's advertised schema. Nested admission is the same + * function, and returns the same answer, as flat admission: a step's input + * accepts exactly what the nested command's own tool accepts, no more. + */ +function findInadmissibleNestedCommandInput( + schema: JsonSchema | undefined, + value: unknown, + path: string, +): string | undefined { + if (!schema || value === undefined) return undefined; + if (Array.isArray(value)) { + return firstRejection(value, (item, index) => + findInadmissibleNestedCommandInput(schema.items, item, `${path}[${index}]`), + ); + } + if (!isRecord(value)) return undefined; + return ( + firstRejection(schema.oneOf ?? [], (branch) => + findInadmissibleNestedCommandInput(branch, value, path), + ) ?? + firstRejection(Object.entries(schema.properties ?? {}), ([key, property]) => + Object.hasOwn(value, key) + ? findInadmissibleNestedProperty(property, value, key, `${path}.${key}`) + : undefined, + ) + ); +} + +function findInadmissibleNestedProperty( + property: JsonSchema, + parent: Record, + key: string, + path: string, +): string | undefined { + const commandKey = property.commandInputFor; + if (commandKey === undefined) { + return findInadmissibleNestedCommandInput(property, parent[key], path); + } + const command = parent[commandKey]; + const nested = parent[key]; + // A missing or unrecognized command name leaves nothing to check against, and + // nothing to protect: the nested reader rejects that step, so nothing it + // carries reaches a daemon request flag. Reporting it is the reader's job — + // answering here would bury the real error under a key complaint. + if (typeof command !== 'string' || !isCommandName(command) || !isRecord(nested)) { + return undefined; + } + const rejection = findInadmissibleInput(command, findCommandMetadata(command), nested); + return rejection === undefined ? undefined : `${path}: ${rejection}`; +} + +function firstRejection( + items: readonly TItem[], + check: (item: TItem, index: number) => string | undefined, +): string | undefined { + for (const [index, item] of items.entries()) { + const rejection = check(item, index); + if (rejection) return rejection; + } + return undefined; +} + /** * AS-011 answer: the client request envelope from the descriptor registry * (timeout-policy, ADR 0008), declared on the tool description so the surface From b8ec89ef35048025f86c81d6d71c3e30e46a9f83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 27 Aug 2026 10:49:01 +0200 Subject: [PATCH 2/3] refactor(commands): keep the admission rationale off the published type `JsonSchema` ships in `client-types.d.ts`, so the JSDoc on `commandInputFor` was ~500 B of published surface restating what `findInadmissibleNestedCommandInput` already documents at length. The type now says what the field is and points at the boundary that enforces it; the reasoning stays where the enforcement lives. --- src/commands/command-contract.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/commands/command-contract.ts b/src/commands/command-contract.ts index 7259117897..b12157c30e 100644 --- a/src/commands/command-contract.ts +++ b/src/commands/command-contract.ts @@ -16,11 +16,8 @@ export type JsonSchema = { maximum?: number; /** * Marks this object as ANOTHER command's input, naming the sibling property - * that holds that command's name. Such an object is free-form by necessity — - * its accepted keys depend on a value, which JSON Schema cannot express — so - * without the marker it is opaque to the model-facing admission boundary, - * which then checks nothing inside it. Declaring it here is what lets - * admission recurse with the named command's own advertised schema. + * that holds that command's name. Model-facing admission recurses into it + * with that command's own advertised schema (`mcp/command-tools.ts`). */ commandInputFor?: string; }; From 7dd8aa3f33f50a5aaf7a59b2aadab665135851fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 27 Aug 2026 11:15:16 +0200 Subject: [PATCH 3/3] fix(mcp): resolve a batch step's command as the reader will, not by exact match Nested admission matched the raw `steps[].command` with `isCommandName`, but a step is normalized before it runs. ` SNAPSHOT ` was therefore no command to admission, which checked nothing inside its input, and `snapshot` to the reader, which ran it -- with the operator paths the flat boundary refuses still aboard. Casing and surrounding whitespace reopened the whole bypass. Admission now resolves through `resolveStructuredBatchCommandName`, extracted from `readStructuredBatchCommandName` so the readers and the boundary share one implementation of "what command will this step run as" rather than two that can disagree. The read is that function plus an error. Red tests cover the casing/whitespace corpus directly, and a drift guard states the invariant against the reader itself: admission refuses a spelling iff the reader resolves it. Both fail if the resolution is reverted to an exact match. --- src/core/batch-policy.ts | 22 ++++++- .../command-tools-nested-admission.test.ts | 57 +++++++++++++++++++ src/mcp/command-tools.ts | 20 ++++--- 3 files changed, 89 insertions(+), 10 deletions(-) diff --git a/src/core/batch-policy.ts b/src/core/batch-policy.ts index 9d78a1ed8a..534fc00b5c 100644 --- a/src/core/batch-policy.ts +++ b/src/core/batch-policy.ts @@ -59,12 +59,30 @@ export function normalizeBatchCommandName(command: unknown): string { return typeof command === 'string' ? command.trim().toLowerCase() : ''; } +/** + * The command a raw step value will RUN as, or undefined. + * + * Every caller that decides something about a step — the readers below, and the + * model-facing admission boundary in `mcp/command-tools.ts` — must resolve the + * name through THIS function, because a step is normalized before it runs: + * a caller matching the raw value exactly would see ` SNAPSHOT ` as no command + * at all, while the reader resolves it to `snapshot` and runs it. Admission + * checking one command while the daemon runs another is the whole failure, so + * the read below is this function plus an error rather than a second copy. + */ +export function resolveStructuredBatchCommandName( + command: unknown, +): StructuredBatchCommandName | undefined { + const normalized = normalizeBatchCommandName(command); + return isStructuredBatchCommandName(normalized) ? normalized : undefined; +} + export function readStructuredBatchCommandName( command: unknown, stepNumber: number, ): StructuredBatchCommandName { - const normalized = normalizeBatchCommandName(command); - if (isStructuredBatchCommandName(normalized)) return normalized; + const resolved = resolveStructuredBatchCommandName(command); + if (resolved !== undefined) return resolved; throw new AppError( 'INVALID_ARGS', `Batch step ${stepNumber} command is not available through command batch: ${String(command)}`, diff --git a/src/mcp/__tests__/command-tools-nested-admission.test.ts b/src/mcp/__tests__/command-tools-nested-admission.test.ts index 447d75b9ce..03041530ff 100644 --- a/src/mcp/__tests__/command-tools-nested-admission.test.ts +++ b/src/mcp/__tests__/command-tools-nested-admission.test.ts @@ -103,6 +103,63 @@ test('nested admission leaves legitimate batch input untouched', async () => { assert.deepEqual(calls[0]?.input.steps, steps); }); +// A step is NORMALIZED before it runs (`resolveStructuredBatchCommandName` +// trims and lowercases), so an admission boundary that matched the raw value +// exactly would see ` SNAPSHOT ` as no command at all, check nothing, and let +// the daemon run `snapshot` with the operator paths still aboard. Admission and +// the reader must resolve a name identically or the boundary guards a different +// command than the one that runs. +const COMMAND_SPELLINGS = [ + 'snapshot', + ' snapshot', + 'snapshot ', + ' SNAPSHOT ', + 'SnApShOt', + '\tsnapshot\n', +] as const; + +test('a step is admitted as the command it will run as, however it is spelled', async () => { + const { calls, executor } = createProbeExecutor(); + + for (const command of COMMAND_SPELLINGS) { + const steps = [{ command, input: { iosXctestrunFile: '/attacker/run.xctestrun' } }]; + calls.length = 0; + const result = await executor.execute('batch', { steps }); + + assert.equal(result.isError, true, `${JSON.stringify(command)} must be refused`); + assert.match( + result.content[0]?.text ?? '', + /batch\.steps\[0\]\.input: iosXctestrunFile is not/, + `${JSON.stringify(command)} must be refused as the command it resolves to`, + ); + assert.deepEqual(calls, [], `${JSON.stringify(command)} must not be dispatched`); + } +}); + +// The drift guard behind the case above: whatever spelling the reader accepts, +// admission must have checked. Stated against the reader itself, so a change to +// how a step name is normalized cannot quietly reopen the gap. +test('admission refuses every spelling the batch reader resolves', async () => { + const { executor } = createProbeExecutor(); + const readBatch = findCommandMetadata('batch').readInput; + + for (const command of [...COMMAND_SPELLINGS, 'not-a-command', 'batch', '', ' ']) { + const steps = [{ command, input: { iosXctestrunFile: '/attacker/run.xctestrun' } }]; + let resolves = true; + try { + readBatch({ steps }); + } catch { + resolves = false; + } + const result = await executor.execute('batch', { steps }); + assert.equal( + result.isError, + resolves, + `${JSON.stringify(command)}: admission must refuse it iff the reader runs it`, + ); + } +}); + // A step whose command cannot be resolved has no schema to check its input // against — and needs none, because that step cannot run. Admission must fall // through to the reader that owns the error instead of answering with a key diff --git a/src/mcp/command-tools.ts b/src/mcp/command-tools.ts index 83f8487af1..1158773a54 100644 --- a/src/mcp/command-tools.ts +++ b/src/mcp/command-tools.ts @@ -11,6 +11,7 @@ import { type CommandName, } from '../commands/command-metadata.ts'; import { mcpBody } from '../commands/command-text.ts'; +import { resolveStructuredBatchCommandName } from '../core/batch-policy.ts'; import { resolveCommandRecordsSessionAction, resolveCommandTimeoutPolicy, @@ -410,15 +411,18 @@ function findInadmissibleNestedProperty( if (commandKey === undefined) { return findInadmissibleNestedCommandInput(property, parent[key], path); } - const command = parent[commandKey]; const nested = parent[key]; - // A missing or unrecognized command name leaves nothing to check against, and - // nothing to protect: the nested reader rejects that step, so nothing it - // carries reaches a daemon request flag. Reporting it is the reader's job — - // answering here would bury the real error under a key complaint. - if (typeof command !== 'string' || !isCommandName(command) || !isRecord(nested)) { - return undefined; - } + // Resolve exactly as the nested reader does, never by matching the raw value: + // a step is normalized before it runs, so ` SNAPSHOT ` is no command here and + // `snapshot` there — admission would check nothing and the daemon would run it + // with the operator paths still aboard. `resolveStructuredBatchCommandName` is + // the function the readers themselves call, so the two cannot drift apart. + const command = resolveStructuredBatchCommandName(parent[commandKey]); + // A name that resolves to nothing leaves nothing to check against, and nothing + // to protect: the reader refuses that step, so nothing it carries reaches a + // daemon request flag. Reporting it is the reader's job — answering here would + // bury the real error under a key complaint. + if (command === undefined || !isRecord(nested)) return undefined; const rejection = findInadmissibleInput(command, findCommandMetadata(command), nested); return rejection === undefined ? undefined : `${path}: ${rejection}`; }