Smithery #159
Smithery #159
Conversation
commit: |
WalkthroughThis pull request introduces a major refactoring centred on Zod version 4 migration and Smithery build framework integration. The changes encompass build pipeline reorganisation (CI/CD workflows, package.json scripts), removal of the Dockerfile, new code-generation infrastructure for plugin loaders and resources, and comprehensive Zod 4 adoption across all schema definitions. The codebase transitions from named zod imports to namespace imports, replaces error message syntax with explicit error objects, restructures UUID and validation schemas, and updates schema construction patterns using Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools/macos/stop_mac_app.ts (1)
49-54: Potential command injection viaappName.The
appNameparameter is interpolated directly into a shell command without sanitisation. A malicious input likeCalculator"; rm -rf /; echo "could execute arbitrary commands.Consider validating/sanitising the input or using a safer approach that doesn't involve shell interpolation.
🔎 Proposed fix using input validation
+// Simple validation to prevent shell injection +const sanitisedAppName = params.appName!.replace(/[^a-zA-Z0-9._\- ]/g, ''); +if (sanitisedAppName !== params.appName) { + return { + content: [{ type: 'text', text: 'Invalid app name: contains disallowed characters.' }], + isError: true, + }; +} command = [ 'sh', '-c', - `pkill -f "${params.appName}" || osascript -e 'tell application "${params.appName}" to quit'`, + `pkill -f "${sanitisedAppName}" || osascript -e 'tell application "${sanitisedAppName}" to quit'`, ];
♻️ Duplicate comments (1)
src/mcp/tools/ui-testing/button.ts (1)
20-20: Verify error parameter syntax with Zod v4 documentation.Similar to the issue in
gesture.ts, this file passes error messages as direct strings (e.g.,{ error: 'Invalid Simulator UUID format' }) rather than functions. According to the Zod v4 documentation provided, theerrorparameter should accept a function returningstring | undefined.Please verify that the current syntax is supported by Zod v4, or update to use the function-based approach.
Also applies to: 22-22
🧹 Nitpick comments (27)
src/mcp/resources/__tests__/simulators.test.ts (1)
2-2: Remove unused zod import.The zod import on line 2 is not used anywhere in this test file. Consider removing it to keep the imports clean.
🔎 Proposed fix
-import * as z from 'zod'; -src/mcp/tools/simulator/__tests__/list_sims.test.ts (1)
12-19: Consider moving callHistory initialization to beforeEach.The
callHistoryarray is initialised at module scope (line 19) rather than within abeforeEachblock. Whilst this currently works because only one test uses it, best practice dictates resetting test state inbeforeEachto prevent potential test pollution if additional tests use this variable in future.🔎 Suggested refactor
describe('list_sims tool', () => { let callHistory: Array<{ command: string[]; logPrefix?: string; useShell?: boolean; env?: Record<string, string>; }>; - callHistory = []; + beforeEach(() => { + callHistory = []; + }); describe('Export Field Validation (Literal)', () => {src/mcp/tools/simulator-management/sim_statusbar.ts (1)
108-108: Double type assertion is acceptable for migration, but consider revisiting.The
as unknown as z.ZodType<SimStatusbarParams, unknown>double assertion is a common workaround during type system migrations. It's consistent with the pattern used across other files in this PR. Consider revisiting once the Zod v4 migration is fully stabilised to see if a cleaner type alignment is possible.src/mcp/tools/session-management/session_clear_defaults.ts (1)
19-22: Consider usingz.strictObject()for consistency with the migration pattern.Whilst the current
z.object()is valid in Zod v4, the broader PR migration adoptsz.strictObject()for public schema constructions to provide stricter validation by rejecting unknown properties. This would align with the pattern established across other tool modules.🔎 Optional refactor to use z.strictObject()
-const schemaObj = z.object({ +const schemaObj = z.strictObject({ keys: z.array(z.enum(keys)).optional(), all: z.boolean().optional(), });src/mcp/tools/session-management/session_set_defaults.ts (1)
7-21: Consider usingz.strictObject()for clearer parsing semantics.The AI summary indicates the codebase is transitioning to
z.strictObject()for schema construction. This provides clearer object parsing modes and prevents unexpected keys from being accepted.🔎 Proposed refactor
-const baseSchema = z.object({ +const baseSchema = z.strictObject({ projectPath: z.string().optional(), workspacePath: z.string().optional(), scheme: z.string().optional(), configuration: z.string().optional(), simulatorName: z.string().optional(), simulatorId: z.string().optional(), deviceId: z.string().optional(), useLatestOS: z.boolean().optional(), arch: z.enum(['arm64', 'x86_64']).optional(), suppressWarnings: z .boolean() .optional() .describe('When true, warning messages are filtered from build output to conserve context'), });src/mcp/tools/swift-package/swift_package_test.ts (1)
11-25: Consider usingz.strictObject()to complete the Zod 4 migration.Whilst
z.object()remains valid in Zod 4, the library introducedz.strictObject()andz.looseObject()for clearer object parsing modes. Based on the AI summary, the project is standardising onz.strictObject()as part of the Zod 4 migration. Consider updating this schema definition to align with that pattern.🔎 Proposed refactor to use z.strictObject()
-const swiftPackageTestSchema = z.object({ +const swiftPackageTestSchema = z.strictObject({ packagePath: z.string().describe('Path to the Swift package root (Required)'), testProduct: z.string().optional().describe('Optional specific test product to run'), filter: z.string().optional().describe('Filter tests by name (regex pattern)'), configuration: z .enum(['debug', 'release']) .optional() .describe('Swift package configuration (debug, release)'), parallel: z.boolean().optional().describe('Run tests in parallel (default: true)'), showCodecov: z.boolean().optional().describe('Show code coverage (default: false)'), parseAsLibrary: z .boolean() .optional() .describe('Add -parse-as-library flag for @main support (default: false)'), });src/mcp/tools/simulator-management/set_sim_location.ts (1)
118-120: Consider simplifying the schema chain using direct.omit()call.The
z.strictObject()pattern is valid in Zod v4, but the approach of extracting.shapeand re-wrapping adds unnecessary indirection. A more idiomatic approach in Zod v4 is to call.omit()directly on the schema, which returns a usable schema without requiring the intermediate.shapeextraction. If strict parsing is required on the final result, call.strict()afterwards:const publicSchemaObject = setSimulatorLocationSchema.omit({ simulatorId: true } as const).strict();or simply:
const publicSchemaObject = setSimulatorLocationSchema.omit({ simulatorId: true } as const);if the schema is already configured for strict parsing.
src/mcp/tools/macos/__tests__/stop_mac_app.test.ts (1)
12-12: Unused import.The
znamespace is imported but not used anywhere in this test file. Consider removing it to keep the imports clean.🔎 Proposed fix
import { describe, it, expect } from 'vitest'; -import * as z from 'zod'; import stopMacApp, { stop_mac_appLogic } from '../stop_mac_app.ts';src/mcp/tools/simulator/__tests__/build_sim.test.ts (1)
27-46: Public schema validation is thorough; consider optional test structure refinement.The public schema test correctly validates that
z.object(buildSim.schema)permits empty input and rejects invalid types. This is solid. Minor observation: you could optionally extract the repeated validation pattern into a helper function if this pattern appears in other test suites, but this is a nice-to-have that doesn't impact correctness.src/mcp/tools/device/launch_app_device.ts (1)
88-113: JSON parsing uses manual type guards; consider optional Zod validation for consistency.The type guard functions (lines 93-107) are comprehensive and safe. However, since the codebase is transitioning to Zod 4, you might optionally consider defining a Zod schema for the launch response structure (e.g.,
z.object({ result: z.object({ process: z.object({ processIdentifier: z.number() }) }) })) and using.safeParse(). This would reduce manual guard boilerplate and align with Zod patterns. This is a nice-to-have for consistency rather than a correctness issue.src/mcp/tools/device/list_devices.ts (1)
65-205: Type guard functions are comprehensive but deeply nested; optional refactoring for readability.The nested type guard functions on lines 82-205 are thorough and safe, validating the complex device structure from
devicectlJSON output. However, they are quite deeply nested and span 120 lines. For improved maintainability, consider optionally extracting these into separate, smaller guard functions with descriptive names (e.g.,isValidConnectionProperties(),isValidDeviceProperties(),isValidHardwareProperties()). Alternatively, consider defining a Zod schema for the entire device structure to replace these manual guards. This refactoring would improve readability without changing behaviour.src/mcp/tools/ui-testing/swipe.ts (1)
138-138: Consider verifying the necessity of the double cast.The
as unknown as z.ZodType<SwipeParams>pattern is a TypeScript escape hatch that bypasses type checking. Whilst this appears consistently across the codebase migration, it's worth verifying whether this double cast is required by Zod 4's type system or if there's a cleaner type-safe approach.src/mcp/tools/device/install_app_device.ts (1)
100-100: Consider verifying the necessity of the double cast.Similar to other files in this PR, the
as unknown as z.ZodType<InstallAppDeviceParams, unknown>pattern bypasses type checking. Verify whether Zod 4's type system requires this approach or if a cleaner solution exists.docs/ZOD_MIGRATION_GUIDE.md (2)
10-12: Add language specifier to fenced code block.The code block is missing a language specifier, which affects syntax highlighting.
🔎 Suggested fix
-``` +```bash npm install zod@^4.0.0</details> --- `1-879`: **Useful migration documentation.** This comprehensive Zod 4 migration guide provides valuable context for the codebase changes. Consider adding a section specifically covering the patterns used in this codebase, such as: - The `z.strictObject(...).shape` pattern for public schemas - The `as unknown as z.ZodType<T, unknown>` cast pattern for preprocessed schemas - The `error:` property usage in refinements </blockquote></details> <details> <summary>src/utils/axe-helpers.ts (1)</summary><blockquote> `24-26`: **Comment is now outdated.** The comment on line 24-25 describes the old `__dirname`-based approach. It should be updated to reflect the new `process.argv[1]`-based resolution. <details> <summary>🔎 Suggested fix</summary> ```diff -// In the npm package, build/index.js is at the same level as bundled/ -// So we go up one level from build/ to get to the package root +// Resolve bundled axe path relative to the package root +// Package root is determined from the entry point (process.argv[1]) const bundledAxePath = join(getPackageRoot(), 'bundled', 'axe');src/utils/sentry.ts (2)
12-82: Code duplication with doctor.deps.ts.These helper functions (
getXcodeInfo,getEnvironmentVariables,checkBinaryAvailability) are duplicated fromsrc/mcp/tools/doctor/lib/doctor.deps.ts. While the comment mentions this is to avoid circular dependencies, consider:
- Extracting these to a shared utility module that both files can import
- Creating a minimal
system-info.tsutility without dependencies on other modulesThis would reduce maintenance burden when these functions need updates.
118-120: Consider reducing trace sample rate for production.
tracesSampleRate: 1.0captures 100% of transactions, which may cause:
- Performance overhead in production
- Increased Sentry costs at scale
The comment on line 119 acknowledges this should be adjusted. Consider using a lower rate (e.g., 0.1-0.2) or making it configurable via environment variable.
🔎 Suggested approach
// Set tracesSampleRate to 1.0 to capture 100% of transactions for performance monitoring - // We recommend adjusting this value in production - tracesSampleRate: 1.0, + tracesSampleRate: parseFloat(process.env.SENTRY_TRACES_SAMPLE_RATE ?? '0.1'),.github/workflows/ci.yml (1)
29-33: Consider running tsup and Smithery builds in parallel.Both build steps are self-contained: each executes
npm run generate:version && npm run generate:loadersindependently, then runs its own build tool. Tsup outputs ESM artefacts to thebuild/directory whilst Smithery builds with its own esbuild configuration (CJS format) without consuming tsup's output. These builds have no explicit dependency on each other and could execute in parallel to reduce CI duration.docs/SMITHERY.md (1)
1-273: Consider minor documentation polishThe documentation is comprehensive and well-structured. Two optional stylistic improvements:
- Line 10: Consider rephrasing "one-click install it" to "one-click installation" for better flow
- Line 89: Consider adding a comma before "so" in "externalizes your SDKs during bundling so your runtime uses"
These are minor stylistic suggestions that don't impact clarity.
src/mcp/tools/ui-testing/long_press.ts (1)
32-35: Error messages are descriptions, not validation errors.The
errorproperty in Zod 4 is for customising validation failure messages. The current values appear to be field descriptions rather than error messages:
- Line 33:
'X coordinate for the long press'should describe the validation failure- Line 34:
'Y coordinate for the long press'should describe the validation failure- Line 35:
'Duration of the long press in milliseconds'should describe the validation failure🔎 Suggested fix
const longPressSchema = z.object({ simulatorId: z.uuid({ error: 'Invalid Simulator UUID format' }), - x: z.number().int({ error: 'X coordinate for the long press' }), - y: z.number().int({ error: 'Y coordinate for the long press' }), - duration: z.number().positive({ error: 'Duration of the long press in milliseconds' }), + x: z.number().int({ error: 'X coordinate must be an integer' }).describe('X coordinate for the long press'), + y: z.number().int({ error: 'Y coordinate must be an integer' }).describe('Y coordinate for the long press'), + duration: z.number().positive({ error: 'Duration must be a positive number' }).describe('Duration of the long press in milliseconds'), });src/mcp/tools/ui-testing/key_press.ts (1)
25-27: Error message on line 26 is a description, not a validation error.The
errorproperty should contain a message shown when validation fails.'HID keycode to press (0-255)'reads as a description of the field, not an error message for when.int()validation fails.🔎 Suggested fix
const keyPressSchema = z.object({ simulatorId: z.uuid({ error: 'Invalid Simulator UUID format' }), - keyCode: z.number().int({ error: 'HID keycode to press (0-255)' }).min(0).max(255), + keyCode: z.number().int({ error: 'Key code must be an integer' }).min(0).max(255).describe('HID keycode to press (0-255)'), duration: z.number().min(0, { error: 'Duration must be non-negative' }).optional(), });src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
20-70: Consider simplifying the helper function signature.The
executeSimctlCommandAndRespondfunction accepts 8 parameters, which increases cognitive complexity. Consider refactoring to accept a configuration object instead.💡 Proposed refactor using a configuration object
+interface SimctlCommandConfig { + params: SetSimAppearanceParams; + simctlSubCommand: string[]; + operationDescriptionForXcodeCommand: string; + successMessage: string; + failureMessagePrefix: string; + operationLogContext: string; + extraValidation?: () => ToolResponse | undefined; + executor?: CommandExecutor; +} + async function executeSimctlCommandAndRespond( - params: SetSimAppearanceParams, - simctlSubCommand: string[], - operationDescriptionForXcodeCommand: string, - successMessage: string, - failureMessagePrefix: string, - operationLogContext: string, - extraValidation?: () => ToolResponse | undefined, - executor: CommandExecutor = getDefaultCommandExecutor(), + config: SimctlCommandConfig, ): Promise<ToolResponse> { + const { + params, + simctlSubCommand, + operationDescriptionForXcodeCommand, + successMessage, + failureMessagePrefix, + operationLogContext, + extraValidation, + executor = getDefaultCommandExecutor(), + } = config;src/mcp/tools/ui-testing/screenshot.ts (1)
159-160: Consider simplifying the type cast.The double cast
as unknown as z.ZodType<ScreenshotParams, unknown>works but is verbose. Since this pattern appears across multiple tool files in this PR, it suggests a potential type compatibility issue between the schema definition and the expected type. This is acceptable for the migration but could be revisited once Zod v4 types stabilise.src/smithery.ts (2)
1-1: Inconsistent Zod import style.This file uses named import
import { z } from 'zod'whilst all other files in this PR use the namespace import patternimport * as z from 'zod'. For consistency across the codebase, consider aligning with the namespace import style.🔎 Proposed fix
-import { z } from 'zod'; +import * as z from 'zod';
41-49: Consider surfacing bootstrap errors more explicitly.The current error handling logs and rethrows, but if
connectis called after bootstrap fails, the rejected promise will propagate to the caller. This is acceptable behaviour, but you may want to consider storing the error state to provide a clearer error message ifconnectis called multiple times after a failed bootstrap.build-plugins/plugin-discovery.ts (1)
206-212: Consider addingindex.tsto the exclusion filter.For consistency with the workflow loader generation (lines 67-68), you may want to explicitly exclude
index.ts/index.jsfiles from resource discovery, even if they don't currently exist in the resources directory.🔎 Proposed fix
.filter( (name) => (name.endsWith('.ts') || name.endsWith('.js')) && !name.endsWith('.test.ts') && !name.endsWith('.test.js') && - !name.startsWith('__'), + !name.startsWith('__') && + name !== 'index.ts' && + name !== 'index.js', );
Changes: - Replaced 'error' keys with 'message' in Zod schema validations across multiple tools to standardize error handling and improve clarity. - Updated validation messages for projectPath, workspacePath, simulatorId, and simulatorName to enhance user feedback. This change ensures consistency in error messaging throughout the codebase, aligning with the latest Zod practices.
1 participant
Note
Introduces a dual-build flow and Smithery integration across CI/release, plus docs and packaging updates.
npm run build:tsupbefore the Smithery build (npm run build), clarifying steps as “Build (tsup)” and “Build (Smithery)`smithery.yaml,smithery.config.js) and README/docs for Smithery local server usagebuild:tsup/dev:tsupscripts, build pipeline runs version/loader generation,moduleset tosrc/smithery.ts, dependency updates (e.g.,zod@^4)Written by Cursor Bugbot for commit 960b1ca. This will update automatically on new commits. Configure here.