refactor(core): remove unsafe type assertions in error utils (Phase 1.1)#19750
refactor(core): remove unsafe type assertions in error utils (Phase 1.1)#19750mattKorwel merged 2 commits intomainfrom
Conversation
Replaces 'as Type' assertions with runtime type guards (isGaxiosError, isResponseData, isErrorShape) and targeted suppressions where narrowing is required in errors.ts and googleErrors.ts. Fixes #19709
Summary of ChangesHello @mattKorwel, 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 improves the robustness of error handling within the core utilities by replacing potentially brittle Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid step towards improving type safety in the error handling utilities by replacing unsafe type assertions with type guards. The introduction of isGaxiosError, isResponseData, and isErrorShape is a good approach. However, I've identified a couple of critical issues in the new type guard implementations where they are either not fully safe or still rely on unsafe assertions, which undermines the goal of this refactoring. My review comments provide suggestions to make these guards more robust and truly type-safe.
| function isGaxiosError(error: unknown): error is GaxiosError { | ||
| return ( | ||
| typeof error === 'object' && | ||
| error !== null && | ||
| 'response' in error && | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| typeof (error as GaxiosError).response === 'object' | ||
| ); | ||
| } |
There was a problem hiding this comment.
The type guard isGaxiosError is not safe because typeof null is 'object'. If error.response is null, this check will incorrectly pass. The GaxiosError interface expects response to be an object with a data property or undefined, but not null. Additionally, the type assertion as GaxiosError is unsafe and can be avoided, which aligns with the goal of this PR.
| function isGaxiosError(error: unknown): error is GaxiosError { | |
| return ( | |
| typeof error === 'object' && | |
| error !== null && | |
| 'response' in error && | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | |
| typeof (error as GaxiosError).response === 'object' | |
| ); | |
| } | |
| function isGaxiosError(error: unknown): error is GaxiosError { | |
| return ( | |
| typeof error === 'object' && | |
| error !== null && | |
| 'response' in error && | |
| typeof (error as { response: unknown }).response === 'object' && | |
| (error as { response: unknown }).response !== null | |
| ); | |
| } |
| function isErrorShape(obj: unknown): obj is ErrorShape { | ||
| return ( | ||
| typeof obj === 'object' && | ||
| obj !== null && | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| (('message' in obj && typeof (obj as ErrorShape).message === 'string') || | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| ('code' in obj && typeof (obj as ErrorShape).code === 'number')) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The type guard isErrorShape uses an unsafe type assertion as ErrorShape which this pull request aims to remove. The assertions are suppressed with eslint comments, but they can be avoided altogether for a safer implementation.
You can make this type guard safer by using a more constrained type for the cast, based on what has already been verified, which also allows removing the eslint-disable comments.
function isErrorShape(obj: unknown): obj is ErrorShape {
return (
typeof obj === 'object' &&
obj !== null &&
(('message' in obj &&
typeof (obj as { message: unknown }).message === 'string') ||
('code' in obj && typeof (obj as { code: unknown }).code === 'number'))
);
}
|
Size Change: +1.2 kB (0%) Total Size: 25.2 MB
ℹ️ View Unchanged
|
gemini-cli
bot
added
the
area/core
|
Addressed critical code review feedback:
|
2 participants
Summary
Disallow and remove unsafe type assertions in core error utilities. This replaces brittle
as Typecasts with runtime type guards to improve system reliability and crash reporting.Details
isGaxiosError,isResponseData, andisErrorShapetype guards inerrors.tsandgoogleErrors.ts.toFriendlyErrorandparseGoogleApiErrorto use these guards before accessing properties.Related Issues
Fixes #19709
How to Validate
npm test -w @google/gemini-cli-core -- src/utils/errors.test.ts src/utils/googleErrors.test.tsnpm run lint -w @google/gemini-cli-corePre-Merge Checklist