refactor(core): remove eslint-disable and type-asserts from shellExecutionService - #28862
Conversation
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 'shellExecutionService' to improve type safety and resource management. By replacing loose type assertions with proper interface usage and implementing explicit cleanup for event listeners and terminal instances, the service becomes more resilient and memory-efficient. Additionally, the error handling logic was updated to better support fallback scenarios when PTY creation fails, ensuring more stable execution across different environments. 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
|
|
📊 PR Size: size/L
|
![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 introduces robust cleanup of PTY event listeners and headless terminals in ShellExecutionService to prevent memory and file descriptor leaks on process exit, abort, or execution failure. It also expands the fallback mechanism to child_process when PTY creation fails with ENXIO or "Device not configured" errors, accompanied by comprehensive unit tests. The review feedback suggests capturing spawnedPty as a constant to avoid unsafe non-null assertions and correcting the indentation of the onExit callback to align with formatting standards.
gemini-cli
Bot
added
the
status/need-issue
|
Size Change: +381 B (0%) Total Size: 35.2 MB
ℹ️ View Unchanged
|
7c5553d to
8e417ba
Compare
2 participants
Summary
This PR removes
eslint-disableand unsafe type-assertions fromshellExecutionService.tson the branchfix/mac-pty-resource-leak.Details
Previously, the
dataListenerandexitListenerwere registered by casting properties ofptyProcess(which was typed asanybecauseptyInfo.module.spawnreturnedany). By using the already type-assertedspawnedPtyof typeDestroyablePtyinstead,.onDataand.onExitare fully typed from@lydell/node-pty. This removes any need for theeslint-disableblock and custom assertions entirely. Additionally, unnecessary non-null assertionsspawnedPty!on lines 1244 and 1250 have been removed, as the TypeScript compiler control flow correctly infers that they are defined in that scope.Related Issues
Related to cleanup of eslint-disable comments on fix/mac-pty-resource-leak branch.
How to Validate
npm run lintto verify eslint rules pass.npm run typecheckto verify TypeScript types compile.npm test -w @google/gemini-cli-core -- src/services/shellExecutionService.test.tsto ensure PTY execution unit tests continue to pass.Pre-Merge Checklist