feat(cli): Add 'list' subcommand to '/commands'#22324
feat(cli): Add 'list' subcommand to '/commands'#22324spencer426 merged 26 commits intogoogle-gemini:mainfrom
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 introduces a new 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a list subcommand to /commands, allowing users to see all loaded custom command files. The implementation is straightforward and includes corresponding documentation and tests. My main feedback is to improve error handling when scanning for command files to provide better diagnostics for users, ensuring detailed logging for unexpected errors as per best practices.
- Refactor FileCommandLoader to expose getCommandDirectories - Improve error handling with isNodeError and add internal debugLogger - Fix strict typing issues in tests - Add mock cleanup (restoreAllMocks) in afterEach
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful list subcommand to the /commands utility, allowing users to see all loaded .toml command files. The implementation is well-structured and includes corresponding documentation and unit tests. I've identified one area for improvement regarding code duplication in commandsCommand.ts to enhance maintainability.
|
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! |
|
@Jwhyee, apologies for the bot closing this PR! We have reopened it. Please sync your branch to the latest |
|
@cocosheng-g Fixed test failures! A type error introduced by the upstream merge. |
|
@cocosheng-g Thanks for enabling auto-merge! The docs-pr-check has been queued for over 3 hours without running. is there anything I need to do on my end, or does this need a re-trigger? |
|
Requesting review from gemini-cli-docs |
Head branch was pushed to by a user without write access
jkcinouye
left a comment
There was a problem hiding this comment.
LGTM - docs.
One minor. note.
|
@jkcinouye Thank you for the suggestion. I've applied the changes to make it read more naturally. |
|
@jkcinouye The docs-pr-check has been stuck in the queue for about 2 weeks. could you help re-trigger it when you get a chance? |
|
@cocosheng-g Once the check passes, could you re-add this to the merge queue?! |
spencer426
left a comment
There was a problem hiding this comment.
Thanks for adding the /commands list feature! This is a great addition for visibility into custom commands.
The changes look mostly solid, but there are a few areas that need to be aligned with the project's strict development rules.
Architectural Boundaries
FileCommandLoader.ts is currently located in packages/cli/src/services/ but performs filesystem operations (like glob). Our architecture rules dictate that non-UI logic and filesystem operations MUST reside in packages/core. While this file already existed here, introducing more filesystem logic compounds the architectural debt. We may want to consider moving this service to core in a future PR.
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| if ((e as { code?: string }).code === 'ENOENT') { |
There was a problem hiding this comment.
Our strict development rules require handling filesystem errors gracefully using isNodeError from packages/core/src/utils/errors.ts and explicitly forbid eslint-disable-next-line for unsafe type assertions.
Please export isNodeError from packages/core/src/index.ts (if it isn't already exported) and update this block to use it: if (isNodeError(e) && e.code === 'ENOENT').
| // Snapshot the text content | ||
| const addItemCall = vi.mocked(context.ui.addItem).mock.calls[0][0]; | ||
|
|
||
| expect((addItemCall as { text: string }).text).toMatchSnapshot(); |
There was a problem hiding this comment.
The rules state to avoid any and unsafe type casting in tests.
Please cast this to the proper type HistoryItemInfo (which can be imported from packages/cli/src/ui/types.ts) or narrow the type properly instead of using a partial shape cast.
| }, | ||
| ]); | ||
|
|
||
| const listCmd = commandsCommand.subCommands!.find( |
There was a problem hiding this comment.
The use of the non-null assertion operator (!) should be avoided unless absolutely necessary.
Consider restructuring this slightly to use standard type narrowing (e.g., asserting the command is found and throwing a descriptive error if not) rather than chaining !.
Co-authored-by: Coco Sheng <cocosheng@google.com> Co-authored-by: Spencer <spencertang@google.com>
Co-authored-by: Coco Sheng <cocosheng@google.com> Co-authored-by: Spencer <spencertang@google.com>
Summary
Adds a
listsubcommand to the/commandsutility to display all currently loaded.tomlcommand files. This improves the developer experience by allowing users to quickly verify which custom commands and prompts are active without manually inspecting the file system.Details
Currently, the
/commandscommand only supports thereloadsubcommand. This PR introduces thelistsubcommand, which reads and outputs the filenames from the directories evaluated by the CLI (user-level~/.gemini/commands/, project-level<project>/.gemini/commands/, MCP prompts, and extensions).commandsCommand.test.tsto ensure the correct paths are scanned and listed.docs/reference/commands.md.Related Issues
Fixes #22321
How to Validate
npm run buildfollowed bynpm start..tomlfile to~/.gemini/commands/or<project>/.gemini/commands/./commands list..tomlfiles present in those directories.npm run test.npm run preflightto confirm formatting and linting pass.Pre-Merge Checklist