fix(cli): throw FatalConfigError instead of process.exit in parseArguments#27987
fix(cli): throw FatalConfigError instead of process.exit in parseArguments#27987rmedranollamas wants to merge 4 commits into
Conversation
|
📊 PR Size: size/S
|
Summary of ChangesHello, 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 argument parsing logic in the CLI to enhance robustness and testability. By transitioning from immediate process termination to throwing a 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 the 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 counterproductive. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors argument parsing in packages/cli/src/config/config.ts to throw a FatalConfigError on failure instead of exiting directly. However, it also removes the runExitCleanup() call and its import when handling --help and --version flags. The feedback correctly points out that bypassing this cleanup before calling process.exit(0) will prevent essential cleanup tasks from running, and recommends restoring both the cleanup call and the import.
| import { promptForSetting } from './extensions/extensionSettings.js'; | ||
| import type { EventEmitter } from 'node:stream'; | ||
| import { runExitCleanup } from '../utils/cleanup.js'; | ||
|
|
Refactored argument parsing to throw FatalConfigError instead of process.exit(1). Moved help/version exit logic to main() with test guards to avoid Vitest hangs while ensuring E2E tests pass by allowing normal return from main().
This resolves the issue where E2E tests for --help/--version would fail with exit code 42 (FATAL_INPUT_ERROR) because they continued into the main loop after the exit was skipped due to VITEST=true environment variable propagation.