feat(commands): Enable @file processing in TOML commands#6716
feat(commands): Enable @file processing in TOML commands#6716abhipatel12 merged 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @abhipatel12, 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 the ability to use @path/to/file syntax within the prompt of custom TOML commands. This allows users to inject file content directly into command prompts, similar to GEMINI.md files. A key aspect is the reordering of the prompt processing pipeline to ensure dynamic arguments and shell injections are handled before @file processing, enabling flexible use cases where file paths can be dynamically provided.
Highlights
@filesupport in TOML commands: Users can now include file content in their custom TOML command prompts using the@path/to/filesyntax, enabling dynamic context injection.- Refined Prompt Processor Order: The processing pipeline for TOML commands now executes shell and argument injections (
!{...},{{args}}) before@fileinjection, allowing for dynamic file paths to be used with@filesyntax. - Non-Recursive
@fileProcessing: UnlikeGEMINI.mdfiles,@fileprocessing in TOML commands is intentionally non-recursive. This design choice ensures predictable command behavior and prevents unexpected side effects from deeply nested imports. - "Silent Failure" for
@fileErrors: If an@filepath is invalid or a file is not found, the original@path/to/filestring is left in the prompt, and an error is logged to the console. This 'silent failure' allows the model to potentially reason about and correct the path.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a great new feature, enabling @file processing in TOML commands. The implementation is well-structured, reusing the core processImports function and adding a new AtFileProcessor. The reordering of the processor pipeline to support dynamic file paths is a thoughtful touch. The tests are comprehensive, covering the new processor and the updated pipeline logic. I found one issue in the core memoryImportProcessor where the import tree isn't correctly populated in non-recursive mode. Overall, this is a solid contribution.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
bf1fc2f to
d4bfecc
Compare
jacob314
left a comment
There was a problem hiding this comment.
How might someone create a TOML command that needs to use @ but doesn't want to have it trigger file processing?
For example
yarn workspace @test-workspace/app dev
where unfortunately test-workspace/app is a valid path so you can't just skip based on invalid paths.
I'm curious about a solution for this because I'm hoping what you do can also align with what we need to do for ImportProcessor in #5438
d4bfecc to
5a84bec
Compare
From our offline convo, changed this to use @{foo.txt} syntax. Let me know what you think! |
jacob314
left a comment
There was a problem hiding this comment.
Do we really need to support
prompt = "What is the purpose of this file: @{ !{find . -name 'FileCommandLoader.ts'} }"
my slight preference would be to omit that for now and keep the
@{} functionality simpler
We can always support more complex use cases in the future but it is harder to remove functionality once we have added it.
A user who really wants
@{ !{find . -name 'FileCommandLoader.ts'} } could accomplish it today by piping the results of the find into cat.
Counter point to this suggestion would be if
@{} was able to support loading image files correctly into the prompt at which point it would support something not currently feasible with shell scripts although I suppose the shell scripts could perhaps be tweaked to support scripts that output binary data.
| expect(result).toBe('Analyze content of path/with/{braces}/file.txt'); | ||
| }); | ||
|
|
||
| it('should leave the prompt unmodified if it contains an unclosed trigger', async () => { |
There was a problem hiding this comment.
for discussion: should this generate an error or warning of some sort rather than quietly not being parsed?
Agree that @world should just be ignored but @{world more likely indicates a typo by the author.
There was a problem hiding this comment.
I extracted the parsing logic to separate utility and added some mechanisms to let users know. Let me know what you think!
| ); | ||
| }); | ||
|
|
||
| it('should correctly parse paths that contain balanced braces', async () => { |
There was a problem hiding this comment.
what about if I need unbalanced braces in the prompt?
How might I write
'Analyze @{path/with/braces{example.txt}'
could I write
'Analyze @{{{path/with/braces{example.txt}}}'
?
There was a problem hiding this comment.
Made some changes to the parsing logic here. I'm not quite if it's worth supporting unbalanced braces if theres a brace within a file name.
But willing to change my mind!
| endIndex: number; | ||
| } | ||
|
|
||
| export class AtFileProcessor implements IPromptProcessor { |
There was a problem hiding this comment.
can you unify this parsing with parsing for !{ as I would expect there should be consistent handling of edge cases.
| const entries = await fs.readdir(absolutePath); | ||
| return `Directory listing for ${pathStr}:\n- ${entries.join('\n- ')}`; | ||
| } else { | ||
| return fs.readFile(absolutePath, 'utf-8'); |
There was a problem hiding this comment.
I would suggest we align this closely with read-many-files.ts which is what is called when @ are added to a prompt. If that is not feasible it should be at least aligned with
is there a reason we can't use ../utils/fileUtils.ts
That will keep the behavior consistent and handle cases such as reading binary files robustly. For example, fileUtils.ts has support for images and read-many-files and read-file have support for the correct logic to encode images in the format the Gemini API needs.
Without changes, referencing a binary image causes Gemini CLI to throw exceptions due to blowing out the context window by adding the bytes of the image into the prompt.
There was a problem hiding this comment.
To make the @ processing support multi-modal input, i had to make a lot of changes ... but it works! Let me know what you think!
a4fe0a5 to
15e3cf7
Compare
|
Thanks for the feedback @jacob314! Sorry about the big diff, but we have multi-modal support now! |
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; | ||
| const newPrompt = [...prompt]; // Create a mutable copy |
There was a problem hiding this comment.
nit: remove zero value comment.
| ]; | ||
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; |
There was a problem hiding this comment.
nit: use .at(-1) instead of prompt.length - 1
| context: CommandContext, | ||
| ): Promise<PromptPipelineContent> { | ||
| if (context.invocation?.args) { | ||
| if (prompt.length === 0) { |
There was a problem hiding this comment.
why are we still adding two trailing line breaks when the prompt is empty?
There was a problem hiding this comment.
We shouldnt. Fixed!
| ]; | ||
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; |
There was a problem hiding this comment.
optional nit: can you ask gemini CLI to linearize this flow? I think this is just a complexified version of adding to an existing text part if one exists, adding a new one if it doesn't. that would be a reasonable helper method to add to a util file.
There was a problem hiding this comment.
Done. Moved to a util file that deals with Part.
| ); | ||
| }); | ||
|
|
||
| it('should correctly parse paths that contain balanced braces', async () => { |
| }); | ||
| }); | ||
|
|
||
| // ⬇️ New test suite for flatMapTextParts |
There was a problem hiding this comment.
remove this comment that just indicates that new stuff was added.
| import type { FileDiscoveryService } from '../services/fileDiscoveryService.js'; | ||
|
|
||
| // Helper to conditionally run tests (useful for OS-specific behaviors like permissions) | ||
| const itif = (condition: boolean) => (condition ? it : it.skip); |
There was a problem hiding this comment.
instead of adding our own helper, use it.skipIf from vitest
| expect(result).toBe('Analyze content of path/with/{braces}/file.txt'); | ||
| }); | ||
|
|
||
| it('should leave the prompt unmodified if it contains an unclosed trigger', async () => { |
15e3cf7 to
4b244ad
Compare
abhipatel12
left a comment
There was a problem hiding this comment.
Thanks for the review!
| context: CommandContext, | ||
| ): Promise<PromptPipelineContent> { | ||
| if (context.invocation?.args) { | ||
| if (prompt.length === 0) { |
There was a problem hiding this comment.
We shouldnt. Fixed!
| ]; | ||
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; |
| ]; | ||
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; |
There was a problem hiding this comment.
Done. Moved to a util file that deals with Part.
| } | ||
|
|
||
| const lastPart = prompt[prompt.length - 1]; | ||
| const newPrompt = [...prompt]; // Create a mutable copy |
| }); | ||
| }); | ||
|
|
||
| // ⬇️ New test suite for flatMapTextParts |
| import type { FileDiscoveryService } from '../services/fileDiscoveryService.js'; | ||
|
|
||
| // Helper to conditionally run tests (useful for OS-specific behaviors like permissions) | ||
| const itif = (condition: boolean) => (condition ? it : it.skip); |
TLDR
This pull request introduces support for
@{path/to/file}syntax within thepromptof custom.tomlcommands. This feature allows users to create powerful, dynamic commands that can inline file or directory content directly into the prompt as context before it's sent to the model.The core change involves adding a new
AtFileProcessorto the prompt processing pipeline for file-based commands. The processor pipeline has been carefully ordered to enable dynamic use cases, such as generating a file path from a shell command.Key Changes & Design
New
@{...}Syntax@{path/to/file.txt}is replaced by the content offile.txt.@{path/to/dir}is traversed and each file present within the directory and all subdirectories are inserted into the prompt. This respects.gitignoreand.geminiignoreif enabled.@{...}is processed before shell commands (!{...}) and argument substitution ({{args}}).@{...}(the path) to have balanced braces ({and}).Processor Pipeline Order
The prompt processor pipeline for
.tomlcommands now executes in the following order:AtFileProcessor): Scans the prompt for@{...}placeholders and injects the corresponding file/directory content.ShellProcessor): Replaces!{...}and{{args}}placeholders.DefaultArgumentProcessor): If{{args}}was not used, the raw arguments are appended to the very end of the prompt.This order means that file injection is completed before shell and argument processing.
One thing to note: if an injected file contains placeholder they will be processed. Thus, an injected file that contains
!{rm -rf}would be parsed as such.Error Handling
If a path in an
@{...}placeholder cannot be found or read:@{path/to/nonexistent.file}) is left in the prompt.This approach empowers the model. Since the model has access to file-system tools, it can see the user's likely intent, reason about a potential typo in the path, and use its own tools to attempt to find the correct file.
Reviewer Test Plan
To validate these changes, reviewers can test the following scenarios:
Static Path: Create a
.tomlcommand with a hardcoded@{...}path.Run:
/staticVerify: The model receives the content of
README.md.Multi-modal: Ask it to describe an image.
Run:
/what-is-itVerify: The model receives the content of
image.pngand should tell you what the image contained.Invalid Path: Run a command with a typo in the file path.
Run:
/dynamic @{README.md-typo}@{README.md-typo}.Non-Matching Syntax: Use an
@symbol in a way that does not match the syntax.Run:
/at-test@the-teamand@ version1.0intact, as they do not match the@{...}syntax.Testing Matrix
Linked issues / bugs