fix(core): remove dead code, fix cross-platform symlink detection, and add error logging#20162
fix(core): remove dead code, fix cross-platform symlink detection, and add error logging#20162lavjeetrai wants to merge 2 commits into
Conversation
…d add error logging
|
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 @lavjeetrai, 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 focuses on improving the robustness and reliability of the core codebase by addressing several identified bugs. It cleans up unreachable code, enhances cross-platform compatibility for file system operations, and prevents silent failures by introducing explicit logging for unexpected errors during process management. These changes contribute to a more stable and maintainable application. 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
The pull request addresses three bugs: dead code removal, cross-platform symlink detection, and improved error logging. The changes enhance code robustness and maintainability. The shellExecutionService.ts file now includes more explicit error handling for process.kill calls, preventing silent swallowing of unexpected errors. The editCorrector.ts file has unreachable code removed, improving clarity. Lastly, workspaceContext.ts now correctly handles Windows symlink paths. All changes are well-justified and improve the overall quality of the codebase.
| } catch (error: unknown) { | ||
| if (isNodeError(error) && error.code !== 'ESRCH') { | ||
| debugLogger.warn( | ||
| `Unexpected error checking child process ${pid}: ${error.code}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This change correctly addresses the issue of silently swallowed errors. By explicitly checking for ESRCH and logging other unexpected errors, the code becomes more robust and easier to debug. This is a critical improvement for production stability.
| } catch (error: unknown) { | ||
| if (isNodeError(error) && error.code !== 'ESRCH') { | ||
| debugLogger.warn( | ||
| `Unexpected error checking PTY process ${pid}: ${error.code}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, this change prevents silent error swallowing when checking PTY processes. Logging unexpected errors is crucial for identifying and resolving issues that might otherwise go unnoticed.
| import { debugLogger } from '../utils/debugLogger.js'; | ||
| import { isNodeError } from '../utils/errors.js'; |
There was a problem hiding this comment.
The addition of debugLogger and isNodeError imports is good for improving error visibility and handling. This aligns with the pull request's goal of adding error logging.
There was a problem hiding this comment.
Code Review
This pull request delivers several valuable bug fixes. It correctly removes dead code in editCorrector.ts and improves cross-platform symlink detection in workspaceContext.ts. The addition of error logging in shellExecutionService.ts is a good step towards more robust process management. However, I've identified a logical issue in the updated isPtyActive function where it incorrectly handles EPERM errors, potentially misreporting an active process as inactive. My review includes suggestions to address this.
| } catch (error: unknown) { | ||
| if (isNodeError(error) && error.code !== 'ESRCH') { | ||
| debugLogger.warn( | ||
| `Unexpected error checking child process ${pid}: ${error.code}`, | ||
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
While adding logging for unexpected errors is a great improvement, there's a subtle logic issue here. An EPERM error from process.kill(pid, 0) indicates that the process exists, but the current user lacks permission to send it a signal. In this case, isPtyActive should return true, not false. The current implementation incorrectly reports the process as inactive on EPERM errors.
| } catch (error: unknown) { | |
| if (isNodeError(error) && error.code !== 'ESRCH') { | |
| debugLogger.warn( | |
| `Unexpected error checking child process ${pid}: ${error.code}`, | |
| ); | |
| } | |
| return false; | |
| } | |
| } catch (error: unknown) { | |
| if (isNodeError(error)) { | |
| if (error.code === 'EPERM') { | |
| // We don't have permission to signal the process, but it does exist. | |
| return true; | |
| } | |
| if (error.code !== 'ESRCH') { | |
| debugLogger.warn( | |
| 'Unexpected error checking child process ' + pid + ': ' + error.code | |
| ); | |
| } | |
| } | |
| // For ESRCH or other errors, assume it's not active. | |
| return false; | |
| } |
| } catch (error: unknown) { | ||
| if (isNodeError(error) && error.code !== 'ESRCH') { | ||
| debugLogger.warn( | ||
| `Unexpected error checking PTY process ${pid}: ${error.code}`, | ||
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, this catch block should handle the EPERM error code. If process.kill(pid, 0) throws EPERM, it means the process exists, and therefore isPtyActive should return true.
} catch (error: unknown) {
if (isNodeError(error)) {
if (error.code === 'EPERM') {
// We don't have permission to signal the process, but it does exist.
return true;
}
if (error.code !== 'ESRCH') {
debugLogger.warn(
'Unexpected error checking PTY process ' + pid + ': ' + error.code
);
}
}
// For ESRCH or other errors, assume it's not active.
return false;
}
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
1 participant
…d add error logging
Summary
Fix three bugs found during a code quality audit: dead code caused by variable shadowing in editCorrector.ts, a Windows-specific symlink misclassification in workspaceContext.ts, and silently swallowed errors in shellExecutionService.ts.
Details
1. Dead code in editCorrector.ts
Inside ensureCorrectEdit, the
else if (occurrences > expectedReplacements)branch re-declaredconst expectedReplacements(shadowing the outer variable on line 188), then checkedif (occurrences === expectedReplacements)— which is logically impossible inside a>branch. Removed the 22 lines of unreachable dead code.2. Windows symlink path separator in workspaceContext.ts
isFileSymlink only checked for
/as a trailing separator, but on WindowsreadlinkSyncreturns paths with\. Now checks for both separators.3. Swallowed errors in shellExecutionService.ts
isPtyActive had two empty
catchblocks. WhileESRCHis expected when checking process existence, other errors likeEPERMwere silently ignored. AddeddebugLogger.warn()for unexpected error codes.Related Issues
Fixes #20159
How to Validate