fix(core): remove unsafe type assertions in mcp and code_assist (Phase 7)#19860
fix(core): remove unsafe type assertions in mcp and code_assist (Phase 7)#19860ktk-07 wants to merge 10 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @ktk-07, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the type safety and robustness of the core MCP (Managed Control Plane) and code assist modules by eliminating unsafe type assertions. It achieves this by integrating Zod for runtime validation of JSON data, implementing custom type guards for error handling and network address information, and refining existing type definitions. The changes aim to align the codebase with stricter ESLint rules and improve overall code quality and maintainability. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request significantly improves type safety by removing many unsafe type assertions and introducing Zod validation for OAuth-related data structures. This aligns well with the project's goal of moving towards stricter TypeScript enforcement. I have identified a few areas for improvement, including a redundant type assertion in a new type guard, a naming convention violation, code duplication for error handling, and a schema weakening that deviates from RFC 8414, which also goes against the principle of 'fail-closed' security checks.
|
As mentioned in my initial pull request under "Important Note section", i initially took the approach of relaxing issuer in TypeScript interface and the Zod schema so that the unit tests passed. Because keeping it both required in TypeScript interface and the Zod schema, causes a few unit tests to fail in oauth-provider.test.ts as their mocked auth server metadata omitted issuer. Since gemini-code-assist bot reinforces the need for RFC 8414 requires issuer, I’m keeping it required in both the TypeScript interface and the Zod schema. I will be updating those mocks in packages/core/src/mcp/oauth-provider.test.ts to include a valid issuer string so the tests match the spec and the stricter runtime validation. I will also be making the necessary changes like removing redundant assertion, fixing camelCase naming and removing code duplication |
|
I addressed code review feedback (recent changes):
|
e9c0948 to
dcc5837
Compare
…on in oauth-utils.ts
…onse with zod and replace unsafe AddressInfo type assertions
…od, the schema was imported. Also replace unsafe type assignment
…uard for ErrnoException
…aligned the test mocks
dcc5837 to
df6da1a
Compare
Summary
This PR Removes @typescript-eslint/no-unsafe-type-assertion suppressions in:
Changes include:
This improves type safety and aligns with the ESLint @typescript-eslint/no-unsafe-type-assertion rules enforced by CI.
Details
Removal of Unsafe Type Assertions
Replaced unsafe assertions such as:
with:
Important Note: OAuth Metadata Strictness Adjustment
While adding Zod validation for OAuthAuthorizationServerMetadata, I discovered that three existing unit tests use minimal metadata mocks that omit the issuer field.
Per RFC 8414, issuer is required in Authorization Server Metadata. However, enforcing this strictly in the schema caused existing tests to fail.
To keep this PR focused on removing unsafe type assertions and avoid expanding scope:
If strict RFC 8414 compliance is preferred, we could follow up by:
This implementation could be in this pr or another pr.
Happy to adjust based on maintainer preference.
Related Issues
Fixes #19735
Parent issuse #19708
How to Validate
From repository root:
Expected results:
Manually verify:
Pre-Merge Checklist