fix(mcp): admit a batch step's input against the nested command's own schema - #2076
Merged
Conversation
… 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.
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
`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.
Member
Author
|
P1: recursive admission exact-matches the nested command, while batch later trims and lowercases it. A step command like |
…xact 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.
Member
Author
|
P1 confirmed and fixed in Reproduced first: The cause was that admission had its own notion of "which command is this" ( Tests: a red case over the casing/whitespace corpus (
|
Member
Author
|
Re-reviewed at |
thymikee
added
the
ready-for-human
|
thymikee
added a commit
that referenced
this pull request
Aug 27, 2026
#2076 landed the same fix on main from a parallel session, and its design is the one to keep: the seam is declared on the SCHEMA (`JsonSchema.commandInputFor` names the sibling holding the nested command's name), so the boundary walks the tool's own schema and re-runs the flat admission per nested input. That states the invariant as a parity -- a step admits exactly what the nested command's own tool admits -- rather than as a key list, and it needs no per-command metadata. This branch's version is removed entirely: `CommandMetadata.nestedCommandInputs`, batch's step extractor, its step-command resolver, and the duplicate regression tests. What remains here is the #2027 refactor, with main's `findInadmissibleNestedCommandInput` recursing through this branch's audience-derived `findInadmissibleInput` -- main's nested-admission suite passes against it unchanged, which is the integration proof that the two compose.
thymikee
added a commit
that referenced
this pull request
Aug 27, 2026
"Who may write this input field, on which surface" was expressed three times,
each a separate name-keyed mechanism: `retiredField()` in the command field
maps, `ALWAYS_HIDDEN_FIELDS` in the AI SDK adapter, and
`OPERATOR_INPUT_GUIDANCE` / `CONFIG_LOADER_GUIDANCE` at the MCP admission
boundary -- twelve hand-written refusal sentences keyed by name, far from the
fields they govern.
The root cause was that the ~19 shared common fields existed only as parallel
enumerations by name -- `commonProperties()`, `readCommonInput()`,
`commonToClientOptions()`, and the `CommonCommandInput` type -- carrying no
metadata, so any policy about a field forced a new name-keyed map elsewhere.
Declare each common field once, in `commands/common-input-fields.ts`, keyed by
its input key and carrying `{ schema?, read?, clientKey?, audience? }`. The JSON
schema, the readers, the client-options projection, and the model-facing
audience boundary all derive from that one table, and `satisfies Record<keyof
CommonCommandInput | 'target', ...>` makes a row without a field, or a field
without a row, a type error in both directions.
`audience` is the unified vocabulary (`commands/input-audience.ts`): `operator`
keys stay in the CLI and Node schemas but are hidden from and refused by every
model-facing tool schema; `retired` keys are absent from every schema yet still
recognized, so they answer with migration guidance. `retiredField()` now sets
`audience: 'retired'`, metro's `bearerToken`/`proxyBaseUrl` declare
`audience: 'operator'` at the field, and `stateDir` declares it in the new
`mcp/tool-control-fields.ts` beside the other MCP-only tool arguments. Refusal
guidance is rendered from each declaration's operator path -- env var names via
`buildPrimaryEnvVarName`, the operator config file, or an explicit sentence --
rather than hand-written per key, and `OperatorInputSource` is shaped so a
declaration naming no path at all does not typecheck.
`#2076`'s nested-step admission recurses through the same derived
`findInadmissibleInput`, so a batch step's refusals come from this audience map
rather than a second filter; its suite passes against this unchanged.
A field-level audience only reaches the boundaries through its command's
metadata, so that wiring is closed structurally rather than by convention:
`inputAudience` is required on `CommandMetadata`, and
`defineFieldCommandMetadata` -- which now takes an optional custom reader, so
`batch` and `gesture` go through it too -- is the only construction path for a
field-map command. At the boundary, a command's own audiences merge before the
global operator classifications, so an `operator` key outranks a colliding
per-command `retired` one and a name collision fails closed.
`command-input.ts` was 705 lines and over the 300-line target; the record
readers move to `commands/input-readers.ts` so the table can use them without an
import cycle. `click`/`press`/`fill` move onto `defineFieldCommandMetadata` --
they were that helper inlined.
`COMMON_COMMAND_SUPPORTED_FLAG_KEYS` stays hand-maintained: it is the CLI
parser's axis, and 25 of its 42 keys never become structured command input while
the table's `cwd` and `debug` are not flags. The reasoning is recorded above the
constant.
Purely internal: `listCommandTools()`, the CLI command schemas, and every
command `inputSchema` are byte-identical, verified by diffing the serialized
surfaces before and after.
Refs #2027
thymikee
added a commit
that referenced
this pull request
Aug 27, 2026
"Who may write this input field, on which surface" was expressed three times,
each a separate name-keyed mechanism: `retiredField()` in the command field
maps, `ALWAYS_HIDDEN_FIELDS` in the AI SDK adapter, and
`OPERATOR_INPUT_GUIDANCE` / `CONFIG_LOADER_GUIDANCE` at the MCP admission
boundary -- twelve hand-written refusal sentences keyed by name, far from the
fields they govern.
The root cause was that the ~19 shared common fields existed only as parallel
enumerations by name -- `commonProperties()`, `readCommonInput()`,
`commonToClientOptions()`, and the `CommonCommandInput` type -- carrying no
metadata, so any policy about a field forced a new name-keyed map elsewhere.
Declare each common field once, in `commands/common-input-fields.ts`, keyed by
its input key and carrying `{ schema?, read?, clientKey?, audience? }`. The JSON
schema, the readers, the client-options projection, and the model-facing
audience boundary all derive from that one table, and `satisfies Record<keyof
CommonCommandInput | 'target', ...>` makes a row without a field, or a field
without a row, a type error in both directions.
`audience` is the unified vocabulary (`commands/input-audience.ts`): `operator`
keys stay in the CLI and Node schemas but are hidden from and refused by every
model-facing tool schema; `retired` keys are absent from every schema yet still
recognized, so they answer with migration guidance. `retiredField()` now sets
`audience: 'retired'`, metro's `bearerToken`/`proxyBaseUrl` declare
`audience: 'operator'` at the field, and `stateDir` declares it in the new
`mcp/tool-control-fields.ts` beside the other MCP-only tool arguments. Refusal
guidance is rendered from each declaration's operator path -- env var names via
`buildPrimaryEnvVarName`, the operator config file, or an explicit sentence --
rather than hand-written per key, and `OperatorInputSource` is shaped so a
declaration naming no path at all does not typecheck.
`#2076`'s nested-step admission recurses through the same derived
`findInadmissibleInput`, so a batch step's refusals come from this audience map
rather than a second filter; its suite passes against this unchanged.
A field-level audience only reaches the boundaries through its command's
metadata, so that wiring is closed structurally rather than by convention:
`inputAudience` is required on `CommandMetadata`, and
`defineFieldCommandMetadata` -- which now takes an optional custom reader, so
`batch` and `gesture` go through it too -- is the only construction path for a
field-map command. At the boundary, a command's own audiences merge before the
global operator classifications, so an `operator` key outranks a colliding
per-command `retired` one and a name collision fails closed.
`command-input.ts` was 705 lines and over the 300-line target; the record
readers move to `commands/input-readers.ts` so the table can use them without an
import cycle. `click`/`press`/`fill` move onto `defineFieldCommandMetadata` --
they were that helper inlined.
`COMMON_COMMAND_SUPPORTED_FLAG_KEYS` stays hand-maintained: it is the CLI
parser's axis, and 25 of its 42 keys never become structured command input while
the table's `cwd` and `debug` are not flags. The reasoning is recorded above the
constant.
Purely internal: `listCommandTools()`, the CLI command schemas, and every
command `inputSchema` are byte-identical, verified by diffing the serialized
surfaces before and after.
Refs #2027
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An MCP/AI-SDK tool call could write operator-owned infrastructure paths into a
batchstep, which the admission boundary refuses on a flat call. The daemon then obeyed them per step.findInadmissibleInputreads the flat keys of a tool call. Every tool schema isadditionalProperties:false, so that is the whole boundary — for the keys a schema can name. A batch step's accepted keys depend on its siblingcommand, which JSON Schema cannot express, sosteps[].inputis declared free-form and the flat scan looked straight past it.Everything the flat scan refuses was therefore admitted one level in:
That is not inert.
readBatchDaemonStepprojects a step's input into per-step daemon request flags, and the daemon obeys them:iosSimulatorDeviceSetresolveTargetDevicesearches (core/dispatch-resolve.ts,daemon/session-selector.ts)iosXctestrunFile.xctestrunthe Apple runner launches (runner-artifact.ts:resolveExternalXctestrunArtifact)iosXctestDerivedDataPath,iosXctestEnvDirdaemon/apple-runner-options.ts,daemon/context.ts)This is the operator-infrastructure write
OPERATOR_INPUT_GUIDANCEexists to refuse, reached through the one input the model is invited to nest. The threat model is the one already written there: the model both reads untrusted app UI text and picks tool arguments, so a screen must find no parameter to write an infrastructure path into.cwd/stateDir/daemonBaseUrl/daemonAuthTokenwere admitted too but are inert on this route —cwdridesmeta(parent-only),stateDiris absent fromreadCommonInput, and the daemon base URL and token are read only by the client-side lifecycle, which has already run. They are refused now anyway, because the fix restores parity rather than enumerating the reachable subset.The fix: declare the seam, don't detect it
JsonSchema.commandInputFormarks an object as another command's input and names the sibling property holding that command's name.batchStepSchemasetscommandInputFor: 'command'onsteps[].input, next to theadditionalProperties: truethat creates the opacity.findInadmissibleNestedCommandInputwalks the tool's own schema against the raw arguments and re-runsfindInadmissibleInputper nested input, against that command's advertised schema.resolveStructuredBatchCommandName, extracted fromreadStructuredBatchCommandNameso the readers and the boundary share one implementation. A step is normalized before it runs, so a boundary matching the raw value exactly would guard a different command than the one that executes —SNAPSHOTchecked as nothing, run assnapshot.No
if (name === 'batch')anywhere — the walker only honors what a schema declares. Both model-facing surfaces are covered at once, since the MCP router and the AI SDK adapter share this executor.Refusals keep the flat guidance and gain a location:
A step whose command cannot be resolved falls through untouched: it has no schema to check against and needs none, because the batch reader refuses that step before anything it carries reaches a flag. Answering here would bury the real error under a key complaint.
Validation
Reachability was confirmed end-to-end before any fix was written, at four points: MCP admission admits the nested key;
prepareDaemonCommandRequest('batch', …)emits it as a per-step flag;runBatchhands that flag to the per-stepinvoke; andbuildDeviceInventoryRequestFromFlags/buildAppleRunnerRequestOptionsturn it into a device-set path and runner paths. Re-running that same probe against this branch flips it: all eight keys refused nested, none admitted.command-tools-nested-admission.test.tsstates a parity, not a key list — for every batchable command, nested admission returns the same verdict as flat admission over a probe set spanning operator keys, config-loader keys, and an unknown key. A key added toOPERATOR_INPUT_GUIDANCElater is covered the day it lands, with nothing in the test to update. Revertingcommand-tools.tsfails it on the first operator key of the first batchable command. Plus: refusal at a non-zero step index, the unresolvable-command fall-through, and a legitimate five-step batch (open/snapshot/tap/type/wait, carryingnoRecord,session, and a stepruntime) proving valid input is untouched.Local gates green at
54ab0b0222:pnpm test:unit(1063 files / 8102 tests), typecheck, lint,fallow audit,check:layering,check:daemon-wire-compat(protocol unchanged), format. All 20 CI checks passed on that commit, Integration Tests and Coverage included.b8ec89ef35then trimmed thecommandInputForJSDoc, whichJsonSchemawas publishing intoclient-types.d.tsat ~500 B — half the package's unpacked growth — to restate what the walker already documents. It changes no behavior.7dd8aa3f33fixes review P1: admission matched the rawsteps[].commandwhile the reader normalizes it, so casing or surrounding whitespace skipped the check and still ran the step. It adds the casing/whitespace corpus and a drift guard stated against the reader — admission refuses a spelling iff the reader resolves it. Both new tests were confirmed to fail on an exact-match revert. Gates re-run green at that commit:pnpm test:unit(1063 files / 8104 tests), typecheck, lint,fallow audit,check:layering,check:daemon-wire-compat.No device evidence is owed: the change is confined to the model-facing admission boundary and adds no platform behavior. Its effect on a device path is refusing input that previously reached one, which the parity test covers at the boundary. No
agent-devicesessions were opened.Notes for review
command-tools.tsadmission and touchescommand-contract.tsandbatch/metadata.ts; #2067 touchesbatch/metadata.ts. This bug predates refactor(commands): one audience table for common input fields #2074 and reproduces identically on it — itsfindInadmissibleInputis still flat, and it leaves the batch daemon-writer path untouched. Rebasing onto refactor(commands): one audience table for common input fields #2074 makes the fix smaller, not different: the per-step call becomestoolInputAudience(metadata)-driven like the flat one. Happy to sequence behind either.commandInputForis emitted in the advertised MCP schema rather than stripped. It costs one key on one tool, JSON Schema ignores unknown keywords, and it documents why that object is free-form. Say the word if you would rather the published surface carry no internal markers, and I will filter it inlistCommandTools.oneOfbranches as well asproperties/items. No marker lives inside aoneOftoday; the guard that makes this safe is that a branch only contributes a refusal when the value genuinely carries a known command name at the marked sibling, which means it is a nested command input.