refactor(mcp): project interaction family output schemas from their owning module - #3025
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/interaction/index.ts">
<violation number="1" location="src/commands/interaction/index.ts:300">
P3: `INTERACTION_COMMAND_OUTPUT_SCHEMAS` is exported as a mutable object, despite the promised frozen projection map. Wrap the map in `Object.freeze` so consumers cannot replace its command entries or mutate the canonical projection through the exported map.</violation>
</file>
<file name="src/mcp/__tests__/command-tools-interaction-schemas.test.ts">
<violation number="1" location="src/mcp/__tests__/command-tools-interaction-schemas.test.ts:13">
P3: The comment claims find is read-only, but the module's own find schema documents mutating actions (`x`/`y`: "Resolved x/y coordinate for mutating find actions", `message`: "Diagnostic message for mutating find actions") and the CLI accepts mutating find actions. Recount only what the test depends on: find carries no post-action observation trait.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| * validating. #1652: the opt-in `settle` observation is NOT listed here — the trait derivation | ||
| * pass in that file grafts it onto settle-capable entries. | ||
| */ | ||
| export const INTERACTION_COMMAND_OUTPUT_SCHEMAS = { |
There was a problem hiding this comment.
P3: INTERACTION_COMMAND_OUTPUT_SCHEMAS is exported as a mutable object, despite the promised frozen projection map. Wrap the map in Object.freeze so consumers cannot replace its command entries or mutate the canonical projection through the exported map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/interaction/index.ts, line 300:
<comment>`INTERACTION_COMMAND_OUTPUT_SCHEMAS` is exported as a mutable object, despite the promised frozen projection map. Wrap the map in `Object.freeze` so consumers cannot replace its command entries or mutate the canonical projection through the exported map.</comment>
<file context>
@@ -54,6 +63,288 @@ import {
+ * validating. #1652: the opt-in `settle` observation is NOT listed here — the trait derivation
+ * pass in that file grafts it onto settle-capable entries.
+ */
+export const INTERACTION_COMMAND_OUTPUT_SCHEMAS = {
+ press: tapInteractionResponseDataSchema,
+ click: tapInteractionResponseDataSchema,
</file context>
|
|
||
| // press, click, fill, longpress, and hover carry the post-action observation trait (#1652): the | ||
| // composed map grafts a `settle` property onto a COPY, so they are not reference-equal to the | ||
| // module's own object. find is read-only and carries no such trait. |
There was a problem hiding this comment.
P3: The comment claims find is read-only, but the module's own find schema documents mutating actions (x/y: "Resolved x/y coordinate for mutating find actions", message: "Diagnostic message for mutating find actions") and the CLI accepts mutating find actions. Recount only what the test depends on: find carries no post-action observation trait.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/__tests__/command-tools-interaction-schemas.test.ts, line 13:
<comment>The comment claims find is read-only, but the module's own find schema documents mutating actions (`x`/`y`: "Resolved x/y coordinate for mutating find actions", `message`: "Diagnostic message for mutating find actions") and the CLI accepts mutating find actions. Recount only what the test depends on: find carries no post-action observation trait.</comment>
<file context>
@@ -0,0 +1,58 @@
+
+// press, click, fill, longpress, and hover carry the post-action observation trait (#1652): the
+// composed map grafts a `settle` property onto a COPY, so they are not reference-equal to the
+// module's own object. find is read-only and carries no such trait.
+const SETTLE_DERIVED_COMMANDS = new Set(['press', 'click', 'fill', 'longpress', 'hover']);
+
</file context>
| // module's own object. find is read-only and carries no such trait. | |
| // module's own object. find, which carries no such trait, survives the spread untouched. |
|
Findings at 00d88dd. postActionSurfaceChangeSchema at https://github.com/callstack/agent-device/blob/00d88dd/src/commands/interaction/index.ts#L78 is a verbatim copy of the one at https://github.com/callstack/agent-device/blob/00d88dd/src/mcp/command-output-schemas.ts#L71, and each copy points a comment at the other as if that were a reason. It isn't: command-output-schemas.ts already imports interaction/index.ts (line 13), so interaction can export the schema and the MCP file can import it along that same edge, no back-import needed. Two sources of truth for one contract type means an edit to one copy lets --verify evidence and --settle observation report different surfaceChange shapes, and nothing tests that they still match. Each contract schema should have one declaration, with every other reader importing it. Can postActionSurfaceChangeSchema move to src/commands/interaction/index.ts (or src/commands/command-input.ts), get imported into command-output-schemas.ts, and have both mirror comments deleted? constSchema at https://github.com/callstack/agent-device/blob/00d88dd/src/commands/interaction/index.ts#L66 is now a third verbatim copy, alongside https://github.com/callstack/agent-device/blob/00d88dd/src/mcp/command-output-schemas.ts#L51 and https://github.com/callstack/agent-device/blob/00d88dd/src/commands/system/index.ts#L77. src/commands/command-input.ts already owns and exports the sibling primitives (objectSchema, enumSchema, stringSchema, booleanSchema...), and this module already imports from there, so the seam exists. The system family already copied constSchema once; repeating the pattern here means every remaining family migration under #2819 adds one more copy to keep in sync by hand. JSON-schema primitives should live only in src/commands/command-input.ts. Can constSchema (and nullableStringSchema) move there and be removed from all three local copies, including this one? The Coverage failure (eager-closure-budgets: src/cli.ts evaluates 296 modules vs 295) looks unrelated to this PR: it traces to the system/index.ts -> packages/contracts/src/fold-runtime.ts edge, which the stacked base commit 110cb57 (#3014) added, not this PR's diff. This PR's new imports in interaction/index.ts are type-only or already pulled from ../command-input.ts. Smoke Tests is still queued, and a schema move alone shouldn't touch the device route it exercises, but that run hasn't finished so it isn't confirmed either way. I didn't run the author's deep key-sorted JSON dump; the byte-identity claim rests on a textual diff of the moved hunks, and I didn't re-run the budget test on the head commit myself. Before this can merge, #3014 needs to fix the inherited eager-closure edge (system/index.ts -> fold-runtime.ts) so Coverage passes again, and this PR still needs the postActionSurfaceChangeSchema and constSchema duplicates resolved as described above. |
00d88dd to
9fa2dd5
Compare
|
At 9fa2dd5, both duplicate declarations from the earlier review (00d88dd, #3025 (comment)) are still there, so the findings from that round are still open.
The local The failing Coverage check looks unrelated to this diff. I didn't run the eager-closure test locally; the module-chain attribution above comes from the CI log and the commit topology. I also didn't check whether the interaction ownership test carries the two-way ownership proof that a595a82 added for the system family, since the prior review already covered that test and this delta doesn't touch it. There are no conflicts. The two duplicate declarations in src/commands/interaction/index.ts need to go before this can merge, and the fold-runtime eager-closure edge needs to be fixed separately on the #3014 base branch. |
9fa2dd5 to
311d4be
Compare
311d4be to
9b674a0
Compare
…wning module Moves the 6 hand-authored MCP outputSchema entries for press, click, fill, longpress, hover, and find out of command-output-schemas.ts into a frozen INTERACTION_COMMAND_OUTPUT_SCHEMAS map exported from src/commands/interaction/index.ts, following the #2810 projection seam. Registers the family in the PROJECTED_FAMILIES disjointness check and adds a colocated command-tools-interaction-schemas.test.ts proving reference equality with the module object, including the settle-observation copy for the five settle-capable entries (find has no such trait). postActionSurfaceChangeSchema is duplicated locally rather than imported back from command-output-schemas.ts (which still needs it for the generic --settle observation), matching the constSchema duplication precedent from the system family's move; responseCostSchema and the other interaction-only helpers had no other consumer and moved outright. Part of #2819.
9b674a0 to
dd27903
Compare
|
Rebased onto the updated base (system-index-schemas) and pushed; head is now dd27903.
pnpm check:affected passes locally. CI results are pending on the new head. |
|
The fixes since 9b674a0 look good, and I found no remaining problems in dd27903. The interaction family output schemas now come from their owning module. The removed declarations match the shared ones, and the builder bodies are identical. I compared the removed and shared declarations as text. I did not dump COMMAND_OUTPUT_SCHEMAS at base and head, and I did not run the unit tests or Two checks fail, and neither looks related to this diff. Smoke fails with "prepare ios-runner timed out" while it prepares the iOS runner. This diff only touches MCP output-schema data, so it does not reach that route. Coverage fails in Before merge, Smoke and Coverage need a green rerun or a fix on main, and the stacked base #3014 must merge first. I found no conflicts. |
Summary
Moves the 6 hand-authored MCP
outputSchemaentries forpress,click,fill,longpress,hover, andfindout ofsrc/mcp/command-output-schemas.tsinto a frozenINTERACTION_COMMAND_OUTPUT_SCHEMASmap exported fromsrc/commands/interaction/index.ts, following the same #2810 projection seam the system family used. Registers the family in thePROJECTED_FAMILIESdisjointness check and adds a colocatedcommand-tools-interaction-schemas.test.tsproving reference equality with the module object (including the settle-observation copy for the 5 settle-capable entries;findcarries no such trait).postActionSurfaceChangeSchemais duplicated locally in the interaction module rather than imported back fromcommand-output-schemas.ts(which still needs it for the generic--settleobservation shared across families) — same duplication precedent the system family's move established forconstSchema.responseCostSchemaand the other interaction-only helpers had no other consumer and moved outright, no duplication.Behavior is unchanged: every advertised MCP
outputSchemafor these 6 commands is byte-identical before and after (verified below).Part of #2819. Stacked on
refactor/mcp-output-schemas-2819-system-index-schemas(#3014).Touched files: 4 (
src/commands/interaction/index.ts,src/mcp/command-output-schemas.ts,src/mcp/__tests__/command-tools-replay-schemas.test.ts, newsrc/mcp/__tests__/command-tools-interaction-schemas.test.ts).Validation
Tested commit:
00d88dd7f8.pnpm check:affected --run: 1343/1344 passed. The one failure,test/integration/provider-scenarios/ios-lifecycle.test.ts(5s timeout, unrelated iOS provider-scenario test), passed cleanly in isolation on retry — a contention flake, not caused by this change.No device is needed for this change (pure MCP schema projection, no runtime behavior touched). Instead, byte-identical proof was gathered by dumping
COMMAND_OUTPUT_SCHEMAS.{press,click,fill,longpress,hover,find}(deep, key-sorted JSON) from a worktree checked out at the parent commit and from this branch's head, then diffing:No unresolved risk: the change is a pure move with a compiler-enforced (
satisfies) totality check and a reference-equality test proving the composed map still serves the family module's own schema objects.