fix: resolve infinite loop when using 'Modify with external editor'#17453
Conversation
…oogle-gemini#7669) This fix addresses the infinite loop issue reported in google-gemini#7669 where selecting "Modify with external editor" would loop indefinitely when no editor was configured or available. Root cause: When getPreferredEditor() returned undefined, the code silently returned without changing the outcome, causing the while loop to repeat. Changes: - Add detectFirstAvailableEditor() to auto-detect available editors - Add resolveEditor() to handle editor resolution with proper error messages - Update confirmation.ts to break the loop and show error when editor unavailable - Update coreToolScheduler.ts to cancel operation with feedback when editor unavailable - Add 11 new tests for the new editor resolution functions The fix: 1. Properly validates editor availability before attempting to use it 2. Auto-detects an available editor if none is configured 3. Provides clear error messages explaining why the editor cannot be used 4. Breaks the loop gracefully instead of looping infinitely
Summary of ChangesHello @ppgranger, 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 addresses a critical user experience issue where attempting to modify content with an external editor, without a valid editor configured or available, would lead to an infinite loop. The changes introduce a comprehensive editor detection and resolution system, ensuring that users receive clear feedback and the operation is gracefully cancelled instead of getting stuck. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a critical infinite loop bug that occurred when attempting to use an external editor without one being configured or available. The introduction of the resolveEditor utility is a robust solution that properly detects available editors, provides clear user feedback on failure, and gracefully cancels the operation. The changes are well-implemented across coreToolScheduler.ts and confirmation.ts, and the new logic is thoroughly covered by unit tests.
I have one suggestion regarding the use of synchronous process execution, which could be refactored to be asynchronous to better align with project conventions for non-blocking operations.
Replace synchronous execSync calls with async alternatives in editor detection functions to prevent blocking the Node.js event loop. Changes: - Add commandExistsAsync using promisified exec - Add checkHasEditorTypeAsync, isEditorAvailableAsync, detectFirstAvailableEditorAsync, and resolveEditorAsync - Update confirmation.ts and coreToolScheduler.ts to use resolveEditorAsync - Mark synchronous resolveEditor as deprecated - Add comprehensive tests for all async functions The synchronous versions are kept for UI components that require synchronous execution (useEditorSettings, editorSettingsManager).
Extract the platform-specific command construction into getCommandExistsCmd() to avoid duplication between commandExists and commandExistsAsync.
|
Small commit : extracted the platform-specific command construction (where.exe on Windows, command -v elsewhere) into a shared getCommandExistsCmd() helper function to remove duplication between commandExists and commandExistsAsync. |
|
Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request. To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues. If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding! |
|
Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request. To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues. If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding! |
|
Hi @jackwotherspoon , I'm a bit confused regarding the status of the PR, gemini-cli still closing it every day ... |
|
Hi @ppgranger, could you please fix the current build errors? |
…r-infinite-loop-7669
Replace direct property access with 'in' operator to properly narrow the union type, fixing TypeScript compilation error.
|
Hi @ehedlund, done :) |
- Fixes an infinite loop when using 'Modify with Editor' without a configured editor. - Implements interactive editor selection via a UI dialog. - Returns to the previous confirmation prompt if selection is cancelled or fails. - Simplifies editor availability logic and removes deprecated sync functions. Fixes google-gemini#7669
The test was timing out because resolveEditorAsync waits for an EditorSelected event when no editor is available. Mock the function to return 'vscode' directly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request effectively resolves the infinite loop issue when using 'Modify with external editor' without a configured or available editor. The introduction of resolveEditorAsync() and the associated event-driven editor selection mechanism significantly improves the user experience by allowing interactive editor selection and providing clear feedback when an editor is unavailable. The changes are well-implemented, include appropriate error handling, and are supported by comprehensive tests for the new asynchronous editor resolution logic.
|
Hi @ehedlund , any plan to merge this PR :)? Would like me to add something? |
|
@ppgranger yes we plan to merge this, no action needed from you |
- Remove redundant `success` field from ExternalModificationResult, check for error presence instead - Rename checkHasEditorType to hasValidEditorCommand for clarity - Run command existence checks in parallel in hasValidEditorCommandAsync - Simplify JSDoc comment for isEditorAvailableAsync
- Remove redundant `if (modResult.error)` check - Update parallel implementation of `hasValidEditorCommandAsync` to short-circuit return true as soon as the first command is found to exist
|
@scidomino @ehedlund |
…oogle-gemini#17453) Co-authored-by: Jack Wotherspoon <jackwoth@google.com> Co-authored-by: ehedlund <ehedlund@google.com>
…oogle-gemini#17453) Co-authored-by: Jack Wotherspoon <jackwoth@google.com> Co-authored-by: ehedlund <ehedlund@google.com>
|
Did this issue finally get resolved? |
Summary
Fix infinite loop when selecting "Modify with external editor" without a configured or available editor. Also improve the user experience by prompting the user to select an editor interactively.
Details
Root cause: In both
confirmation.tsandcoreToolScheduler.ts, whengetPreferredEditor()returnedundefined, the code silently returned without changing the loop outcome variable, causing the confirmation loop to repeat indefinitely.Solution:
resolveEditorAsync()- validates availability of preferred editor and prompts the user to select an editorconfirmation.tsto check editor resolution result and return to previous screen when unavailableRelated Issues
Fixes #7669
How to Validate
npm run startin a workspace wherepreferredEditoris NOT set insettings.json.Modify with Editor.Escor choose "none" in the editor dialog.accept once/accept alwaysprompt with an error message, instead of the turn being cancelled.vim).vimopens with the diff.Pre-Merge Checklist