fix(skills): respect .gitignore/.geminiignore in skill resource listing#28149
Conversation
When a skill is activated, its folder structure (shared with the model as "available resources") was built with `getFolderStructure()` without a file service, so .gitignore/.geminiignore were not applied. Ignored directories inside a skill — most notably a Python `.venv` created by tools like `uv` — were enumerated and sent to the model, wasting context and hitting the 200-item display limit. Pass `config.getFileService()` to `getFolderStructure()`, matching how the workspace directory structure is already built in `environmentContext.ts`, so the skill listing honors the same ignore rules. Fixes google-gemini#27205
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 addresses an issue where skill resource listing failed to honor ignore files, leading to the inclusion of unnecessary files in the model's context. By ensuring the file service is properly passed during the folder structure generation, the system now consistently filters files based on project-specific ignore configurations, improving both performance and model relevance. 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/S
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
![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 updates ActivateSkillTool to pass the FileDiscoveryService when retrieving a skill's folder structure, ensuring that ignored directories (like .venv) are respected. The review feedback recommends dynamically instantiating a new FileDiscoveryService when the skill is located outside the workspace root to correctly honor local ignore files, and mocking getProjectRoot in the tests to support this. Additionally, the reviewer suggests using the public shouldConfirmExecute method in tests instead of casting the invocation to access protected methods.
| if (this.cachedFolderStructure === undefined) { | ||
| this.cachedFolderStructure = await getFolderStructure( | ||
| path.dirname(skillLocation), | ||
| // Pass the file service so the skill's folder structure honors | ||
| // .gitignore/.geminiignore (matching how the workspace structure is | ||
| // built). Without it, ignored directories such as a Python `.venv` | ||
| // are shared with the model, wasting context. See issue #27205. | ||
| { fileService: this.config.getFileService() }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Using the workspace's FileDiscoveryService for skills located outside the workspace root (such as extension skills) will fail to respect their local .gitignore or .geminiignore files. This is because the workspace FileDiscoveryService is rooted at the project directory and only loads ignore files from there. For external skills, any .venv or other ignored directories will not be filtered out, causing them to be fully shared with the model and wasting context.
To fix this, we should check if the skill directory is inside the workspace. If it is not, we can dynamically instantiate a new FileDiscoveryService rooted at the skill's directory.
if (this.cachedFolderStructure === undefined) {
const skillDir = path.dirname(skillLocation);
const workspaceRoot = this.config.getProjectRoot();
const { isSubpath } = await import('../utils/paths.js');
const { FileDiscoveryService } = await import('../services/fileDiscoveryService.js');
const fileService = isSubpath(workspaceRoot, skillDir)
? this.config.getFileService()
: new FileDiscoveryService(skillDir);
this.cachedFolderStructure = await getFolderStructure(
skillDir,
{ fileService },
);
}| getFileService: vi.fn().mockReturnValue(mockFileService), | ||
| } as unknown as Config; |
There was a problem hiding this comment.
Since the updated implementation of ActivateSkillTool calls getProjectRoot() to determine if the skill is inside the workspace, we need to mock getProjectRoot in the test configuration to prevent runtime errors during test execution.
| getFileService: vi.fn().mockReturnValue(mockFileService), | |
| } as unknown as Config; | |
| getFileService: vi.fn().mockReturnValue(mockFileService), | |
| getProjectRoot: vi.fn().mockReturnValue('/path/to/test-skill'), | |
| } as unknown as Config; |
| it('builds the resource list with the file service so ignored paths (e.g. .venv) are honored', async () => { | ||
| const params = { name: 'test-skill' }; | ||
| const invocation = tool.build(params); | ||
| await ( | ||
| invocation as unknown as { | ||
| getConfirmationDetails: (signal: AbortSignal) => Promise<unknown>; | ||
| } | ||
| ).getConfirmationDetails(new AbortController().signal); | ||
|
|
||
| // The skill's folder structure must be built with the file service, so | ||
| // that .gitignore/.geminiignore are respected (issue #27205). Without it, | ||
| // ignored directories are shared with the model. | ||
| expect(getFolderStructure).toHaveBeenCalledWith( | ||
| '/path/to/test-skill', | ||
| expect.objectContaining({ fileService: mockFileService }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Instead of casting the invocation to unknown to call the protected getConfirmationDetails method, we can use the public shouldConfirmExecute method with the 'ask_user' decision. This is cleaner, safer, and avoids bypassing TypeScript's access controls.
it('builds the resource list with the file service so ignored paths (e.g. .venv) are honored', async () => {
const params = { name: 'test-skill' };
const invocation = tool.build(params);
await invocation.shouldConfirmExecute(
new AbortController().signal,
'ask_user',
);
// The skill's folder structure must be built with the file service, so
// that .gitignore/.geminiignore are respected (issue #27205). Without it,
// ignored directories are shared with the model.
expect(getFolderStructure).toHaveBeenCalledWith(
'/path/to/test-skill',
expect.objectContaining({ fileService: mockFileService }),
);
});References
- If using internal or undocumented SDK properties is unavoidable for critical functionality (e.g., security), define a local interface for those properties and add a detailed comment explaining the rationale and why a public API could not be used.
gemini-cli
Bot
added
priority/p2
1 participant
Summary
When a skill is activated, its folder structure is shared with the model as the skill's "available resources". That listing is built by
getFolderStructure()inactivate-skill.ts, but it was called without a file service:Without a
fileService,getFolderStructure()only skips a small hardcoded set of folders (node_modules,.git,dist,__pycache__) and does not apply.gitignore/.geminiignore. So an ignored directory inside a skill — most commonly a Python.venvcreated by tools likeuv— gets fully enumerated and sent to the model. This wastes context (thousands of irrelevant library files) and instantly hits the 200-item display limit, hiding the real source files (issue #27205).The workspace directory structure already does this correctly in
environmentContext.ts:Fix
Pass the file service when building the skill's folder structure, so it honors the same
.gitignore/.geminiignorerules as the rest of the CLI:getFolderStructuredefaultsfileFilteringOptionstoDEFAULT_FILE_FILTERING_OPTIONS(respectGitIgnore: true,respectGeminiIgnore: true), so passing the file service alone is sufficient.Testing
activate-skillcallsgetFolderStructurewith thefileService(the wiring that was missing). Combined with the existinggetFolderStructuregitignore/geminiignore tests (which prove{ fileService }causes ignored paths to be excluded), this verifies the full chain.scripts/.venv/...with.venv/in.geminiignore..venvis expanded, leakingpyvenv.cfgandlib/big.py..venv\...is collapsed and its contents are excluded.eslintandtsc --noEmitare clean for the package.Fixes #27205