Skip to content

refactor(mcp): project recording family output schemas from their owning module - #3031

Merged
thymikee merged 2 commits into
refactor/mcp-output-schemas-2819-management-push-schemasfrom
refactor/mcp-output-schemas-2819-recording-index-schemas
Sep 29, 2026
Merged

thymikee merged 2 commits into
refactor/mcp-output-schemas-2819-management-push-schemasfrom
refactor/mcp-output-schemas-2819-recording-index-schemas

Conversation

@thymikee

@thymikee thymikee commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

Projects the recording and replay MCP output schemas (record, trace) out of the shared command-output-schemas.ts map and into their owning module, src/commands/recording/index.ts, following the pattern already used for the system, interaction, device-management, and push-management families in this stack.

Behavior is unchanged: the published outputSchema for these MCP tools is byte-identical before and after. Part of #2819. Touches 4 files. Stacked on refactor/mcp-output-schemas-2819-management-push-schemas.

Validation

Tested commit: a04c376fb18068caada4dd6df9d616a61683ecd8

  • pnpm format: no changes.
  • pnpm check:affected --run: all runnable checks passed (160 test files, 1350 tests).
  • No device needed (pure MCP schema move). Proved the published outputSchema is unchanged: added a temporary vitest test dumping COMMAND_OUTPUT_SCHEMAS.record and .trace to JSON, ran it against origin/main (d3c9d052b5, via a throwaway git worktree add .tmp-main-check refs/remotes/origin/main --detach with node_modules symlinked from this worktree, removed after) and against this branch's HEAD; diff main-schemas.json head-schemas.json reported no differences (IDENTICAL).
  • Mutation check: temporarily made command-output-schemas.ts spread copies of record/trace instead of referencing RECORDING_COMMAND_OUTPUT_SCHEMAS directly, confirmed the new reference-equality test in command-tools-recording-schemas.test.ts fails, then reverted.

No unresolved risk.

Review in cubic

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.88 MB 4.88 MB +11 B
Package (unpacked) 4.88 MB 4.88 MB +11 B
Package (download) 1.46 MB 1.46 MB +16 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.5 ms 28.2 ms -1.3 ms
CLI --help 84.1 ms 81.6 ms -2.5 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Re-trigger cubic

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at a04c376. The code works, but the extraction copies constSchema instead of moving it, so this ships a fifth private copy of the same helper.

recording/index.ts:37 declares function constSchema(value: string): JsonSchema { return { type: 'string', const: value }; }. The same function still lives at src/mcp/command-output-schemas.ts:52 (https://github.com/callstack/agent-device/blob/a04c376/src/mcp/command-output-schemas.ts#L52), and also in interaction/index.ts:66, management/push.ts:31, and system/index.ts:77. Every family extraction so far (00d88dd, c7d92a6, 110cb57) has repeated this pattern, so the recording family isn't the first offender, but it's the fifth copy now. src/commands/command-input.ts already exports the other shared JSON-Schema primitives (stringSchema, enumSchema); why not constSchema? There's no runtime defect today since all five copies produce identical output, but if the const-schema shape ever changes, all five call sites need to change together, and nothing enumerates them for you. Can constSchema be exported from src/commands/command-input.ts and imported in recording/index.ts, with the other four copies replaced either in this PR or a small follow-up?

CI is green: Smoke Tests (both shards reported), Repo Guards, Lint, Typecheck, Integration Tests, Bundle Size, and Compatibility all pass; only Coverage and one Smoke Tests shard are still pending, with no failures so far. The diff is a same-file schema relocation exercised by the existing recording and replay schema test files, which pass. I did not rerun the full affected-package suite locally, and I'm relying on the PR's reported run plus these CI results. Nothing in the diff touches a coverage-gated or smoke-exercised runtime path beyond the schema move, so there's no reason to expect the remaining jobs to fail, but they haven't finished yet.

@thymikee
thymikee added this pull request to stack #3043 September 28, 2026 13:44
…ing module

Moves the 2 hand-authored MCP outputSchema entries for record and trace out of
command-output-schemas.ts into a frozen RECORDING_COMMAND_OUTPUT_SCHEMAS map
exported from src/commands/recording/index.ts, following the #2810 projection
seam already used by the replay, system, interaction, device-management, and
push-management families on this stack. Registers the family in the
PROJECTED_FAMILIES disjointness check and extends the pre-existing
command-tools-recording-schemas.test.ts with a reference-equality proof
against the module object and an owner-file check, matching the pattern from
the other projected families. Neither command carries the post-action
observation trait, so no settle-graft copy applies here.

The moved code is byte-identical to what command-output-schemas.ts held: same
objectSchema/stringSchema/enumSchema calls and field order, with a locally
duplicated constSchema helper and artifactSchema object matching the
precedent from the other families' moves (artifactSchema had no other
consumer in command-output-schemas.ts once record/trace moved, so it moved
outright rather than staying duplicated).

Part of #2819.
@thymikee
thymikee force-pushed the refactor/mcp-output-schemas-2819-recording-index-schemas branch from a04c376 to ed6ce2e Compare September 29, 2026 05:54
@thymikee

Copy link
Copy Markdown
Member Author

Pushed ed6ce2e. The branch now sits on the updated push management base.

  • Duplicate constSchema: the recording family no longer declares its own copy. It imports the shared one from src/commands/command-input.ts (1c5ae11). A grep of src/ and packages/ shows that file is now the only declaration of it.
  • Eager closure: the recording schemas pulled two contract modules into the CLI startup closure, which would have failed the eager-closure budget check. I moved them into src/commands/recording/output-schemas.ts, which only the MCP schema map imports, and listed it as an owner file for record and trace (ed6ce2e).

pnpm check:affected --run passes locally on this head. CI is running on the new commit.

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready at ed6ce2e. The duplicate constSchema finding from the earlier review is fixed, and nothing from that review remains open. I reviewed the logical patch as c6c6834..ed6ce2e (two commits, five files), because the delta since a04c376 mostly holds the upstream base you rebased onto, and I did not review those upstream changes. I did not run the eager-closure budget test locally, so the closure claim rests on the importer grep and the green run. All 19 checks pass, including the eager-closure budget test and the MCP schema tests that exercise this change, and there are no conflicts. Not blocking, take it or leave it: the test 'declares exactly the commands its module owns' in command-tools-recording-schemas.test.ts only checks one direction, so a registry command that lists output-schemas.ts but has no schema entry would still pass, and you could rename the test to match what it checks or add the reverse check over the registry commands that list that file. Nothing else stands in the way, and the branch can merge once its stacked base lands.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 29, 2026
@thymikee
thymikee merged commit 99e4860 into main Sep 29, 2026
19 checks passed
@thymikee
thymikee deleted the refactor/mcp-output-schemas-2819-recording-index-schemas branch September 29, 2026 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant