-
Notifications
You must be signed in to change notification settings - Fork 3
refactor: flatten client structure and add Zod validation to RPC client #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: flatten client structure and add Zod validation to RPC client #168
Conversation
Remove the external TypeScript client SDK dependency and implement a custom RPC client for the StackOne API. This reduces dependencies and gives full control over the API client implementation. Changes: - Add src/client/rpc-client.ts with RpcClient class - Update toolsets/base.ts to use new RpcClient instead of StackOne - Update tests to mock RpcClient instead of StackOne - Add comprehensive tests for the new RPC client using MSW - Add MSW handler for /actions/rpc endpoint - Remove @stackone/stackone-client-ts from dependencies BREAKING CHANGE: BaseToolSetConfig.stackOneClient is now rpcClient
- Merge rpc-client.ts implementation into client/index.ts - Rename tests/rpc-client.spec.ts to index.spec.ts - Remove empty client/tests/ directory - Update import paths in test file
- Move src/client/index.ts to src/rpc-client.ts - Move src/client/index.spec.ts to src/rpc-client.spec.ts - Rename src/mcp.ts to src/mcp-client.ts - Update import paths in base.ts and test files - Remove src/client/ directory This simplifies the directory structure by placing small client modules at the src/ root level rather than in nested directories.
- Add Zod schemas for RpcActionRequest, RpcActionResponse, and RpcClientConfig - Validate config in constructor and request in rpcAction - Validate response body before returning - Use catalog:dev for zod dependency - Remove duplicate catalog entry from pnpm-workspace.yaml
…ient tests - Use toMatchObject for structural validation instead of 'as' type assertions - Use expect.any(String) for dynamic values - Use rejects.toMatchObject for error property validation - Remove all type casting from test file
- Use safeParse instead of parse for response validation - Throw StackOneAPIError with context on validation failure - Add 'as const satisfies' for requestBody type narrowing - Remove type assertion from return value
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 9 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/rpc-client.spec.ts">
<violation number="1" location="src/rpc-client.spec.ts:86">
P2: The test assertion doesn't verify that the `body` field is included in `requestBody`. Based on the test name "should include request body in error for debugging" and the request including `body: { debug: 'data' }`, the assertion should verify this data is captured.</violation>
</file>
<file name="src/rpc-client.ts">
<violation number="1" location="src/rpc-client.ts:37">
P1: Rule violated: **Flag Security Vulnerabilities**
The `serverURL` config parameter accepts any string without validating HTTPS. Since authentication credentials (Basic auth header with username/password) are sent to this URL, a misconfigured HTTP URL would transmit credentials in plaintext. Add HTTPS validation to the schema to prevent credential exposure.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }) | ||
| ).rejects.toMatchObject({ | ||
| statusCode: 500, | ||
| requestBody: { action: 'test_error_action' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The test assertion doesn't verify that the body field is included in requestBody. Based on the test name "should include request body in error for debugging" and the request including body: { debug: 'data' }, the assertion should verify this data is captured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/rpc-client.spec.ts, line 86:
<comment>The test assertion doesn't verify that the `body` field is included in `requestBody`. Based on the test name "should include request body in error for debugging" and the request including `body: { debug: 'data' }`, the assertion should verify this data is captured.</comment>
<file context>
@@ -0,0 +1,100 @@
+ })
+ ).rejects.toMatchObject({
+ statusCode: 500,
+ requestBody: { action: 'test_error_action' },
+ });
+});
</file context>
| * Zod schema for RPC client configuration validation | ||
| */ | ||
| const rpcClientConfigSchema = z.object({ | ||
| serverURL: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Rule violated: Flag Security Vulnerabilities
The serverURL config parameter accepts any string without validating HTTPS. Since authentication credentials (Basic auth header with username/password) are sent to this URL, a misconfigured HTTP URL would transmit credentials in plaintext. Add HTTPS validation to the schema to prevent credential exposure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/rpc-client.ts, line 37:
<comment>The `serverURL` config parameter accepts any string without validating HTTPS. Since authentication credentials (Basic auth header with username/password) are sent to this URL, a misconfigured HTTP URL would transmit credentials in plaintext. Add HTTPS validation to the schema to prevent credential exposure.</comment>
<file context>
@@ -0,0 +1,128 @@
+ * Zod schema for RPC client configuration validation
+ */
+const rpcClientConfigSchema = z.object({
+ serverURL: z.string().optional(),
+ security: z.object({
+ username: z.string(),
</file context>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the client structure by replacing the external @stackone/stackone-client-ts dependency with a custom RpcClient implementation that includes Zod validation. The directory structure is flattened by moving client files to the root src directory.
Key Changes:
- Custom
RpcClientreplaces the StackOne TypeScript SDK dependency, adding Zod validation for requests/responses - Directory structure simplified:
src/client/→src/rpc-client.ts,src/mcp.ts→src/mcp-client.ts - Test improvements using
toMatchObjectinstead of type assertions
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rpc-client.ts | New custom RPC client implementation with Zod validation schemas and basic authentication |
| src/rpc-client.spec.ts | Comprehensive test suite for the new RPC client |
| src/mcp-client.ts | Renamed from src/mcp.ts, functionality unchanged |
| src/toolsets/base.ts | Updated to use RpcClient instead of StackOne SDK client |
| src/toolsets/tests/stackone.mcp-fetch.spec.ts | Updated test mocks to use RpcClient type |
| package.json | Removed @stackone/stackone-client-ts dependency |
| pnpm-workspace.yaml | Removed stackone-client-ts from catalog and release age exclusions |
| pnpm-lock.yaml | Removed lockfile entries for stackone-client-ts |
| mocks/handlers.ts | Added mock handler for StackOne Actions RPC endpoint |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new StackOneAPIError( | ||
| `Invalid RPC action response for ${url}`, | ||
| response.status, | ||
| responseBody, | ||
| requestBody | ||
| ); | ||
| } |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling for response validation uses response.status which would be 200 (OK) when validation fails. This is misleading since the issue is with the response format, not an HTTP error. Consider using a different status code (e.g., 500) or creating a separate error type for validation failures.
Additionally, response.json() on line 99 can throw if the response body is not valid JSON, but this is not caught. Consider wrapping it in a try-catch to handle malformed responses gracefully.
| import { StackOneAPIError } from './utils/errors'; | ||
|
|
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import JsonDict from './types' is unused in this file. Consider removing it to keep the imports clean.
| import { StackOneAPIError } from './utils/errors'; |
| "dependencies": { | ||
| "@modelcontextprotocol/sdk": "catalog:prod", | ||
| "@orama/orama": "catalog:prod", | ||
| "@stackone/stackone-client-ts": "catalog:prod", | ||
| "json-schema": "catalog:prod" |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rpc-client.ts file imports and uses Zod for runtime validation, but zod is listed in devDependencies instead of dependencies. This will cause runtime errors when the package is installed in production environments.
Move zod from devDependencies to dependencies to ensure it's available at runtime.
| authentication?: AuthenticationConfig; | ||
| headers?: Record<string, string>; | ||
| stackOneClient?: StackOne; | ||
| rpcClient?: RpcClient; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BaseToolSetConfig interface exposes RpcClient as a public type (line 69), but RpcClient is not exported from the main package entry point. This will cause TypeScript compilation errors for users who want to provide a custom rpcClient in their configuration.
Ensure that RpcClient is exported from src/index.ts so users can import and use it:
export { RpcClient } from './rpc-client';| prod: | ||
| '@modelcontextprotocol/sdk': ^1.19.1 | ||
| '@orama/orama': ^3.1.11 | ||
| '@stackone/stackone-client-ts': 4.32.2 | ||
| json-schema: ^0.4.0 |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zod is currently listed in the dev catalog (line 28) but it's used at runtime in src/rpc-client.ts. It should be moved to the prod catalog to ensure it's available in production environments.
Add zod to the prod catalog:
prod:
'@modelcontextprotocol/sdk': ^1.19.1
'@orama/orama': ^3.1.11
json-schema: ^0.4.0
zod: ^3.23.8And remove it from the dev catalog, or keep it in both if it's also used in dev dependencies.
| }) | ||
| ).rejects.toMatchObject({ | ||
| statusCode: 500, | ||
| requestBody: { action: 'test_error_action' }, |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requestBody assertion is incomplete. The test expects { action: 'test_error_action' } but the actual request body should also include the body field with { debug: 'data' }. This assertion won't properly validate the error debugging information.
Consider updating the assertion to:
requestBody: {
action: 'test_error_action',
body: { debug: 'data' }
}| requestBody: { action: 'test_error_action' }, | |
| requestBody: { action: 'test_error_action', body: { debug: 'data' } }, |
…seApiModel The previous implementation wrapped the API response in an artificial `actionsRpcResponse` property which didn't match the actual server response structure from unified-cloud-api. Changes: - Update RpcActionResponseSchema to match server's ActionsRpcResponseApiModel: - `data`: object, array of objects, or null - `next`: optional pagination cursor - Use passthrough() to allow additional connector-specific fields - Fix headers type from Record<string, string> to Record<string, unknown> to match server's ActionsRpcRequestDto - Remove artificial wrapping of response in `actionsRpcResponse` property - Add explicit `unknown` type annotation to response.json() for type safety - Export RpcActionResponseData type for consumers needing the data shape This enables proper type inference without type assertions when consuming the RPC client response. Refs: unified-cloud-api/src/modules/actions/models/actions-rpc-response.api.model.ts
Replace `as JsonDict` type assertion with explicit conversion function `rpcResponseToJsonDict()` that iterates over response properties. The RpcActionResponse type uses z.passthrough() which preserves additional fields beyond `data` and `next`. This makes it structurally compatible with Record<string, unknown>, but TypeScript's type system requires explicit conversion to maintain type safety. This change ensures no implicit `any` or unsafe type assertions are used when handling RPC responses in the toolset.
Update test expectations to match the actual server response structure where `data` is a direct property of the response, not nested under `actionsRpcResponse`. Changes: - Access response.data directly instead of response.actionsRpcResponse - Add explicit assertions for response data content - Rename test to clarify array data handling - Add comments explaining response structure alignment with server
0edd790 to
034b905
Compare
Summary
src/client/tosrc/rpc-client.tsand renamesrc/mcp.tstosrc/mcp-client.tstoMatchObjectChanges
Directory Structure
src/client/index.ts→src/rpc-client.tssrc/client/index.spec.ts→src/rpc-client.spec.tssrc/mcp.ts→src/mcp-client.tsRPC Client Improvements
RpcActionRequest,RpcActionResponse, andRpcClientConfigrpcActionsafeParsewith proper error handling for response validation@seelinks to StackOne API documentationTest Improvements
describeblocks and useless constructor testsit()withtest()for flat test structureas) withtoMatchObjectfor proper validationexpect.any(String)for dynamic value matchingTest Plan
Summary by cubic
Replaced the StackOne TS SDK with a custom RPC client and flattened the client structure. This reduces dependencies and adds strict Zod validation for RPC requests and responses. Implements Linear issue 152 by moving to a safer, simpler actions client.
Refactors
Migration
Written for commit 32bf1ff. Summary will update automatically on new commits.