Skip to content

fix(skills): respect .gitignore/.geminiignore in skill resource listing #28149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhay-codes07 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(skills): respect .gitignore/.geminiignore in skill resource listing #28149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/core/src/tools/activate-skill.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { ActivateSkillTool } from './activate-skill.js';
import type { Config } from '../config/config.js';
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { createMockMessageBus } from '../test-utils/mock-message-bus.js';
import { getFolderStructure } from '../utils/getFolderStructure.js';
import type { FileDiscoveryService } from '../services/fileDiscoveryService.js';

vi.mock('../utils/getFolderStructure.js', () => ({
getFolderStructure: vi.fn().mockResolvedValue('Mock folder structure'),
Expand All @@ -18,9 +20,14 @@ describe('ActivateSkillTool', () => {
let mockConfig: Config;
let tool: ActivateSkillTool;
let mockMessageBus: MessageBus;
let mockFileService: FileDiscoveryService;

beforeEach(() => {
mockMessageBus = createMockMessageBus();
mockFileService = {
shouldIgnoreFile: vi.fn(),
shouldIgnoreDirectory: vi.fn(),
} as unknown as FileDiscoveryService;
const skills = [
{
name: 'test-skill',
Expand Down Expand Up @@ -48,8 +55,10 @@ describe('ActivateSkillTool', () => {
}),
activateSkill: vi.fn(),
}),
getFileService: vi.fn().mockReturnValue(mockFileService),
} as unknown as Config;
Comment on lines +58 to 59

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;

tool = new ActivateSkillTool(mockConfig, mockMessageBus);
vi.mocked(getFolderStructure).mockClear();
});

it('should return enhanced description', () => {
Expand All @@ -76,6 +85,24 @@ describe('ActivateSkillTool', () => {
expect(details.prompt).toContain('Mock folder structure');
});

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 }),
);
});
Comment on lines +88 to +104

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. 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.


it('should skip confirmation for built-in skills', async () => {
const builtinSkill = {
name: 'builtin-skill',
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/tools/activate-skill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ class ActivateSkillToolInvocation extends BaseToolInvocation<
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() },
);
}
Comment on lines 64 to 73

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 },
      );
    }

return this.cachedFolderStructure;
Expand Down
Loading