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..b12157c30e 100644 --- a/src/commands/command-contract.ts +++ b/src/commands/command-contract.ts @@ -14,6 +14,12 @@ 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. Model-facing admission recurses into it + * with that command's own advertised schema (`mcp/command-tools.ts`). + */ + commandInputFor?: string; }; export type CommandMetadata = { 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 new file mode 100644 index 0000000000..03041530ff --- /dev/null +++ b/src/mcp/__tests__/command-tools-nested-admission.test.ts @@ -0,0 +1,184 @@ +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 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 +// 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..1158773a54 100644 --- a/src/mcp/command-tools.ts +++ b/src/mcp/command-tools.ts @@ -11,12 +11,14 @@ 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, } 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 +136,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 +353,91 @@ 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 nested = parent[key]; + // 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}`; +} + +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