fix(patch): cherry-pick bdf80ea to release/v0.18.0-pr-13600 to patch version v0.18.0 and create version 0.18.1#13861
Conversation
Co-authored-by: jacob314 <jacob314@gmail.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @gemini-cli-robot, 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 refactors the exit mechanism for the CLI's Highlights
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 refactors the exit logic for extensions and mcp subcommands to use a centralized exitCli utility, ensuring cleanup actions are always performed. This is a good improvement for robustness. However, the refactoring is incomplete. Several command implementation files still contain direct calls to process.exit(1) in their error handling paths. These calls bypass the new exitCli utility and its cleanup logic, partially defeating the purpose of this change. It is critical to replace all process.exit(1) calls with await exitCli(1) to ensure consistent and correct exit behavior across all scenarios. I've added specific comments pointing out these inconsistencies.
| name: argv['name'] as string, | ||
| scope: argv['scope'] as string, | ||
| }); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the handleDisable function in this file. On an error (line 45), handleDisable calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleDisable should be updated to use await exitCli(1) for error exits.
| allowPreRelease: argv['pre-release'] as boolean | undefined, | ||
| consent: argv['consent'] as boolean | undefined, | ||
| }); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the handleInstall function in this file. On an error (line 88), handleInstall calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleInstall should be updated to use await exitCli(1) for error exits.
| await handleLink({ | ||
| path: argv['path'] as string, | ||
| }); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the handleLink function in this file. On an error (line 45), handleLink calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleLink should be updated to use await exitCli(1) for error exits.
| builder: (yargs) => yargs, | ||
| handler: async () => { | ||
| await handleList(); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the handleList function in this file. On an error (line 39), handleList calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleList should be updated to use await exitCli(1) for error exits.
| await handleUninstall({ | ||
| names: argv['names'] as string[], | ||
| }); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there are inconsistent exit paths within the handleUninstall function in this file. On an error (lines 45 and 49), handleUninstall calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleUninstall should be updated to use await exitCli(1) for error exits.
| await handleValidate({ | ||
| path: args['path'] as string, | ||
| }); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the handleValidate function in this file. On an error (line 30), handleValidate calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, handleValidate should be updated to use await exitCli(1) for error exits.
| excludeTools: argv['excludeTools'] as string[] | undefined, | ||
| }, | ||
| ); | ||
| await exitCli(); |
There was a problem hiding this comment.
While this correctly uses the new exitCli utility for successful command execution, there's an inconsistent exit path within the addMcpServer function in this file. On an error (line 48), addMcpServer calls process.exit(1) directly, which will bypass the cleanup logic in exitCli. To ensure consistent behavior and proper cleanup on both success and failure, addMcpServer should be updated to use await exitCli(1) for error exits.
|
Size Change: +388 B (0%) Total Size: 21.1 MB ℹ️ View Unchanged
|
SandyTao520
deleted the
hotfix/v0.18.0/0.18.1/stable/cherry-pick-bdf80ea/pr-13600
branch
3 participants
This PR automatically cherry-picks commit bdf80ea to patch version v0.18.0 in the stable release to create version 0.18.1.