Feat/git remote name utility #14269
Feat/git remote name utility #14269
Conversation
Co-authored-by: riddhi <[email protected]> ## Summary <!-- Concisely describe what this PR changes and why. Focus on impact and urgency. --> ## Details <!-- Add any extra context and design decisions. Keep it brief but complete. --> ## Related Issues <!-- Use keywords to auto-close issues (Closes google-gemini#123, Fixes google-gemini#456). If this PR is only related to an issue or is a partial fix, simply reference the issue number without a keyword (Related to google-gemini#123). --> ## How to Validate <!-- List exact steps for reviewers to validate the change. Include commands, expected results, and edge cases. --> ## Pre-Merge Checklist <!-- Check all that apply before requesting review or merging. --> - [ ] Updated relevant documentation and README (if needed) - [ ] Added/updated tests (if needed) - [ ] Noted breaking changes (if any) - [ ] Validated on required platforms/methods: - [ ] MacOS - [ ] npm run - [ ] npx - [ ] Docker - [ ] Podman - [ ] Seatbelt - [ ] Windows - [ ] npm run - [ ] npx - [ ] Docker - [ ] Linux - [ ] npm run - [ ] npx - [ ] Docker
This commit introduces a new setting in `.vscode/settings.json` to store the Gemini API key. This key will be used for authentication when interacting with the Gemini API.
…configuration file. Here's a cleaned up version that resolves the issues:
```yaml
have_fun: false
code_review:
disable: false
comment_severity_threshold: 'MEDIUM' # Changed from HIGH to MEDIUM
max_review_comments: 20 # Changed from -1 to 20
pull_request_opened:
help: false
summary: true
code_review: true
ignore_patterns:
- "**/*.md"
- "**/package-lock.json"
- "**/dist/"
```
1. **Removed duplicate `ignore_patterns`** - You had two separate `ignore_patterns` sections
2. **Resolved conflicting values** - The later settings were overriding the earlier ones
3. **Applied your intended changes** - Now using MEDIUM severity and 20 comment limit
- **More detailed reviews** (MEDIUM severity catches more issues than HIGH)
- **Prevents comment overload** (max 20 comments instead of unlimited)
- **Ignores common noise files** (docs, lock files, build outputs)
- **Still provides summaries and code reviews** on PR open
This should give you balanced, actionable code reviews without being overwhelmed by too many comments!
…onfigurations for code formatting and linting. These changes aim to improve code consistency and quality across the project.
This commit updates the `.github/CODEOWNERS` file to reflect recent team changes and ensure proper ownership of code. These changes aim to improve maintainability and streamline the code review process.
This commit introduces a new utility function getRemoteName and its corresponding tests. The getRemoteName function is designed to determine the remote name associated with a given Git URL by parsing the output of git remote -v. This utility can be crucial for various Git-related operations within the CLI, especially when dealing with multiple remotes or needing to programmatically identify the remote linked to a specific repository URL. This feature is separated from other changes to adhere to the single responsibility principle, allowing for independent review, testing, and potential future reverts without impacting unrelated functionalities.
Summary of ChangesHello @aka1976mb, 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 primarily focuses on improving the configuration and utility aspects of the project. It introduces a new helper function to identify Git remote names, which can streamline operations involving remote repositories. A significant enhancement is made to how the Gemini API key is handled, allowing for more direct configuration and clearer prioritization. Additionally, the PR includes routine dependency updates and minor adjustments to development environment settings and code ownership definitions. Highlights
Ignored Files
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 introduces a new utility for determining the git remote name and adds support for a geminiApiKey in the configuration. The changes are a good addition, but I've found a few critical issues that need to be addressed. The API key precedence logic is incorrect, prioritizing stored keys over environment variables, which is contrary to the new tests' intent. Additionally, the new getRemoteName utility uses a global function without declaring it, which will lead to a runtime error. Finally, the new tests for API key handling could be improved for clarity and correctness in how they mock dependencies.
| config?.geminiApiKey || | ||
| (await loadApiKey()) || | ||
| process.env['GEMINI_API_KEY'] || | ||
| undefined; |
There was a problem hiding this comment.
The order of precedence for resolving the Gemini API key is incorrect. The environment variable GEMINI_API_KEY should have a higher priority than the key loaded from storage via loadApiKey(). The current implementation checks the stored key first. The typical and expected order is: direct configuration, environment variable, then stored value.
| config?.geminiApiKey || | |
| (await loadApiKey()) || | |
| process.env['GEMINI_API_KEY'] || | |
| undefined; | |
| process.env['GEMINI_API_KEY'] || | |
| (await loadApiKey()) || | |
| undefined; |
| } | ||
|
|
||
| const defaultShellRunner: ShellRunner = async (command, description) => | ||
| run_shell_command({ command, description }); |
There was a problem hiding this comment.
The function run_shell_command is used here but it is not defined or imported. This will cause a ReferenceError at runtime. Based on conventions in other parts of the codebase (e.g., packages/core/src/tools/shell.ts), it seems this is a globally available function that needs to be declared for TypeScript to recognize it. Please add the following declaration at the top of the file (e.g., after the license header):
declare function run_shell_command(args: {
command: string;
description: string;
}): Promise<ShellCommandOutput>;| const loadApiKey = vi.mocked( | ||
| (await vi.importActual( | ||
| '@google/gemini-cli-core/core/apiKeyCredentialStorage', | ||
| )) as { loadApiKey: () => Promise<string | undefined> }, | ||
| ).loadApiKey; | ||
| loadApiKey.mockResolvedValueOnce('LOADED_API_KEY'); |
There was a problem hiding this comment.
The way loadApiKey is being mocked in this test is confusing and potentially incorrect. Using vi.importActual and then vi.mocked on a non-mocked function can lead to unexpected behavior and makes the test harder to understand. A clearer approach would be to use vi.importMock to get a handle to the mocked function and then configure its behavior for the test.
For example:
import { createContentGeneratorConfig } from '@google/gemini-cli-core/core/contentGenerator';
// ... other imports
// Keep the module mock at the top
vi.mock('@google/gemini-cli-core/core/apiKeyCredentialStorage', () => ({
loadApiKey: vi.fn(() => Promise.resolve(undefined)),
}));
// ... inside describe block
it('should prioritize...', async () => {
// ...
const { loadApiKey } = await vi.importMock<typeof import('@google/gemini-cli-core/core/apiKeyCredentialStorage')>('@google/gemini-cli-core/core/apiKeyCredentialStorage');
vi.mocked(loadApiKey).mockResolvedValueOnce('LOADED_API_KEY');
// ... rest of the test
});This approach is more explicit about interacting with the mocked module and improves the test's readability and maintainability.
|
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ Do you have an alternative command to check out pull request 14269, or can you provide the branch name so I can use a |
|
I have completed the inspection of both pull requests. PR #14269 (
PR (
|
1 participant
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist