refactor(commands): one audience table for common input fields - #2074
Conversation
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
|
P1: MCP/AI batch bypasses operator-input admission. |
|
Re-reviewed exact head |
|
P1 fixed at Admission now recurses, with the nesting declared by the command that nests rather than discovered by the boundary — A refused nested key answers with the same guidance as the top-level argument, prefixed by its step, and deny-by-default holds one level down too: Planted red, both directions: reverting admission to the shallow boundary, or dropping batch's No schema change — One residual I did not close: 🤖 Addressed by Claude Code |
|
Correction to my previous comment: #2076 landed the same fix on #2076's design is better and is what's kept — declaring the seam on the schema ( The useful part is that the two compose: #2076's Surface re-diffed against My apologies for the duplicated work — I spawned that investigation as a background task before you asked for the fix here, and didn't check for a sibling PR before implementing it myself. 🤖 Addressed by Claude Code |
|
[P1] Reconcile nested admission with the #2076 contract now on main The nested MCP/AI admission bypass is closed in this implementation, but the PR's merge CI fails all three nested-admission regressions merged through #2076. Main's contract reports the stable location as |
thymikee
added
the
ready-for-human
|
Re-reviewed the replacement head. The #2076 contract conflict is resolved: this branch drops the parallel metadata walker, keeps the stable |
"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
9b2a4ab to
738bc1c
Compare
|
Re-reviewed exact head 738bc1c after the main reconciliation. The branch-owned audience-table refactor remains clean and now composes with #2076’s nested commandInputFor admission seam and the #2070–#2072 platform-boundary changes; it does not restore a parallel metadata walker. The field declaration → metadata/descriptor projection → MCP/AI admission route is coherent, regression tests exercise the owning mappings, and all substantive CI including both coverage shards, integration, smokes, guards, CodeQL, and size is green (only the expected cache matrix skip). Merge-ready. |
|
1 participant
Summary
Closes #2027.
"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_FIELDSin the AISDK adapter, and
OPERATOR_INPUT_GUIDANCE/CONFIG_LOADER_GUIDANCEat 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 theCommonCommandInputtype), carrying no metadata. Any policy about a field therefore had to be stated in a new
name-keyed map somewhere else.
Now there is one table.
src/commands/common-input-fields.tsdeclares each common field once:The JSON schema, the readers, the client-options projection, and the model-facing audience boundary
all derive from it, and
satisfies Record<keyof CommonCommandInput | 'target', …>makes a rowwithout a type field — or a type field without a row — a compile error in both directions.
audienceis the unified vocabulary (src/commands/input-audience.ts):operatorretiredretiredField()now setsaudience: 'retired'. Metro'sbearerTokenandproxyBaseUrldeclareaudience: 'operator'at the field.stateDirdeclares it in the newsrc/mcp/tool-control-fields.ts, next to the other MCP-only tool arguments, which also stopped beingenumerated three times (advertised schema / stripped-before-dispatch / AI SDK hidden set).
Refusal guidance is rendered from each declaration's operator path rather than written per key —
env var names through the existing
buildPrimaryEnvVarName, the operator config file, or an explicitsentence for the three keys neither describes. Every message is byte-identical to today's.
OperatorInputSourceis shaped so a declaration that names no path at all does not typecheck.What this buys
Reclassifying
tenantas operator-only is now this, and nothing else:tenant: { schema: { type: 'string', description: 'Remote tenant identifier.' }, read: (record) => optionalString(record, 'tenant'), + audience: ENV_OR_OPERATOR_CONFIG, },The schema, the readers, the projection, the MCP admission allowlist, its guidance string, and the AI
SDK schema all follow. In #2023 the operator/endpoint/path removal would have been a handful of row
edits instead of ~250 lines across three mechanisms.
The MCP deny side also became genuinely derived rather than hand-kept:
bearerTokenis refused onevery tool because the
metrocommand declares it, not because someone remembered to add it to amap in
command-tools.ts. Deleting that oneoperatorField(...)now turns three existing assertionsred.
Making the wiring unforgettable
A field-level
audienceonly reaches the boundaries if its command's metadata carries it, so twothings close that gap structurally rather than by convention:
inputAudienceis required onCommandMetadata. A hand-rolleddefineCommandMetadata({…})that omits it is a type error, not a silently model-writable credential.
defineFieldCommandMetadatais the only construction path for a field-map command — it nowtakes an optional custom reader, so
batchandgesture(which read their fields themselves) gothrough it too instead of assembling
fieldsInputSchema+readFieldInputby hand.click,press, andfillwere that helper inlined and now call it directly.At the boundary, a command's own audiences are merged before the global operator classifications,
so an
operatorkey always outranks a colliding per-commandretiredone — a name collision failsclosed on the refusal instead of open on the migration guidance.
Batch step admission (P1 from review) — superseded by #2076
Review found that admission read only the outer keys of a tool call, so every operator-owned key
refused on the
snapshottool reached the command route by riding in asbatch.steps[].input. Ireproduced it and fixed it here — and #2076 landed the same fix on
mainfrom a parallel sessionwhile this was in flight.
#2076's design is the one kept, and this branch's version is removed entirely. It declares the
seam on the schema (
JsonSchema.commandInputFornames the sibling holding the nested command'sname) rather than on each command's metadata, so the boundary walks the tool's own schema and
re-runs flat admission per nested input. That states the invariant as a parity — a step admits
exactly what the nested command's own tool admits — instead of a key list, and it needs no
per-command declaration to be kept in step.
What this leaves for review here is the #2027 refactor alone, with #2076's
findInadmissibleNestedCommandInputrecursing through this branch's audience-derivedfindInadmissibleInput. #2076's nested-admission suite passes against it unchanged, which is theintegration proof that the two compose: nested refusals are produced by the same audience map that
produces the flat ones.
Not done, deliberately
The issue also lists
COMMON_COMMAND_SUPPORTED_FLAG_KEYSas something to derive from the table.That is a different axis — which flags the CLI parser accepts — and neither list contains the
other: 25 of its 42 keys (
remoteConfig,stateDir,daemonTransport,sessionIsolation,leaseBackend,sessionLock, everyprovider*andaws*) never become structured command input,while the table's
cwdanddebugare not flags and itsdeviceTargetrow is spelledtargetthere. Deriving it would mean 25 rows carrying no schema, reader, or projection. The reasoning is
recorded above the constant so the next reader does not have to re-derive it; the table does declare
envFlagKeys, which is the part the issue needed a flag key for.Validation
No wire or behavior change intended, and that is checked rather than asserted: I serialized
listCommandTools(), every CLI command schema, and every commandinputSchemabefore and after anddiffed them — byte-identical, property order included. All twelve operator and config-loader
refusal messages were captured from the running executor and match the previous strings exactly.
Three independent adversarial reviews ran against the change — behavior parity, security boundary,
and issue fit. Parity found no non-equivalence. The security review confirmed the boundary is sound
and module init order is safe, and raised the merge-order and prototype-key hardening now folded in
above. The design review drove the one-construction-path change, the
flag-groups.tsnote, and thetest rebalance below.
Local gates green:
pnpm check:affected --run, the full unit bundle (1063 files / 8105 tests),check:layering,check:fallow,check:daemon-wire-compat,check:mcp-metadata,check:gate-manifest,check:agent-guidance,pnpm build,check:bundle-owner-files,check:package.check:production-exportsreports 83 findings on this branch and the identical 83on
origin/mainin a clean worktree — pre-existing, none in the touched files.One of four full-bundle runs tripped the wall-clock slow-test gate on
gesture-admission-parity.test.ts(5.01s against a 2.5s budget). That test's eager import closure is82 modules and contains none of the changed or added ones — not even
command-input.ts— so thischange cannot affect its import cost; it passes in isolation and the other three bundle runs were
clean. Host variance, per the
AGENTS.mdrule for timeout-shaped failures.Every new assertion was observed red before green, against planted defects rather than cited:
clientKey: 'target'from thedeviceTargetrow → "deviceTarget is dropped between readerand projection"; remove a row's
read→ the same for that key.CommonCommandInputfield →tscfails in bothdirections.
operatorField(...)from metro'sbearerToken→ "metro advertises bearerToken", plus therefusal on a non-metro tool losing its guidance.
inputAudiencefrom a hand-rolled command → "Property 'inputAudience' is missing".The new test file pins the four guidance-rendering shapes and the claim that an operator
classification narrows only the model-facing surfaces —
commonProperties()must keep advertisingthose keys, or the CLI and Node client lose the operator's own way to pass them, which no MCP test
would catch. Which keys are operator-owned stays pinned by name in
command-tools-operator-inputs.test.ts, at the boundary that enforces it, and that file gains thecomplementary assertion that the model-writable common keys are still advertised — so a
misclassification now fails in both directions.
No device evidence: the change is internal to input parsing and schema derivation, and every
observable surface is pinned identical above.
Scope and residuals
26 files, +754/−471 production lines, but move-dominated.
command-input.tswas 705 lines againstthe 300-line target; its record readers move to
src/commands/input-readers.tsso the table can usethem without an import cycle. It is now 476 — under the 500-line hard rule but still over target. The
remaining split (JSON-schema builders vs. field constructors) would separate two things every field
constructor pairs, and cost another eager-closure module, so I left it; the
src/cli.tsclosure pinmoves 365 → 368 for the three new leaf modules, with the reason recorded in the row.
AGENTS.mdgains the new declaration site, anddocs/agents/cli-flags.mdstep 4 now says which flagshave to declare an audience — worth having, since that is the step where a new credential-shaped flag
would otherwise reach a model-facing schema unclassified.