diff --git a/extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts b/extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts index 1b8c077eb9112f..7d68b09c5dd8b5 100644 --- a/extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts +++ b/extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts @@ -10,7 +10,7 @@ import { promisify } from 'util'; import { Uri } from 'vscode'; import { BatchedProcessor } from '../../../util/common/async'; import { coalesce } from '../../../util/vs/base/common/arrays'; -import { Sequencer } from '../../../util/vs/base/common/async'; +import { raceTimeout, Sequencer } from '../../../util/vs/base/common/async'; import { CachedFunction } from '../../../util/vs/base/common/cache'; import { CancellationToken, cancelOnDispose } from '../../../util/vs/base/common/cancellation'; import { Emitter, Event } from '../../../util/vs/base/common/event'; @@ -27,6 +27,12 @@ import { API, APIState, Branch, Change, CommitOptions, CommitShortStat, DiffChan const execFileAsync = promisify(execFile); +/** + * How long {@link GitServiceImpl.getRepositoryFetchUrls} waits for initial repository discovery. + * Bounded so a missing or disabled Git extension cannot hang every caller. + */ +const INITIAL_DISCOVERY_TIMEOUT_MS = 30_000; + export class GitServiceImpl extends Disposable implements IGitService { declare readonly _serviceBrand: undefined; @@ -191,6 +197,10 @@ export class GitServiceImpl extends Disposable implements IGitService { async getRepositoryFetchUrls(uri: URI): Promise | undefined> { this.logService.trace(`[GitServiceImpl][getRepositoryFetchUrls] URI: ${uri.toString()}`); + // Answering before discovery settles reports the file as belonging to no repository, which + // content exclusion reads as "no repository rules apply to this file". + await this.waitForInitialDiscovery(); + const gitAPI = this.gitExtensionService.getExtensionApi(); if (!gitAPI) { return undefined; @@ -432,6 +442,26 @@ export class GitServiceImpl extends Disposable implements IGitService { } } + private _initialDiscoverySettled: Promise | undefined; + + /** + * Resolves once initial repository discovery has settled, or once {@link INITIAL_DISCOVERY_TIMEOUT_MS} + * elapses. Unlike {@link initialize} this never rejects, and the promise is shared between callers. + */ + private waitForInitialDiscovery(): Promise { + if (this._isInitialized.get()) { + return Promise.resolve(); + } + this._initialDiscoverySettled ??= raceTimeout( + // Rejects when the service is disposed, which is not worth propagating to a caller that + // only wants to know discovery is no longer pending. + waitForState(this._isInitialized, state => state, undefined, cancelOnDispose(this._store)).catch(() => undefined), + INITIAL_DISCOVERY_TIMEOUT_MS, + () => this.logService.warn(`[GitServiceImpl][waitForInitialDiscovery] Timed out after ${INITIAL_DISCOVERY_TIMEOUT_MS}ms.`) + ).then(() => undefined); + return this._initialDiscoverySettled; + } + private async doOpenRepository(repository: Repository): Promise { this.logService.trace(`[GitServiceImpl][doOpenRepository] Repository: ${repository.rootUri.toString()}`); diff --git a/extensions/copilot/src/platform/ignore/common/ignoreService.ts b/extensions/copilot/src/platform/ignore/common/ignoreService.ts index c0ba6af73e2149..0f5e15ba3427b3 100644 --- a/extensions/copilot/src/platform/ignore/common/ignoreService.ts +++ b/extensions/copilot/src/platform/ignore/common/ignoreService.ts @@ -5,6 +5,7 @@ import * as l10n from '@vscode/l10n'; import { createServiceIdentifier } from '../../../util/common/services'; +import { Limiter } from '../../../util/vs/base/common/async'; import { CancellationToken } from '../../../util/vs/base/common/cancellation'; import { URI } from '../../../util/vs/base/common/uri'; @@ -12,6 +13,9 @@ export const HAS_IGNORED_FILES_MESSAGE = l10n.t('\n\n**Note:** Some files were e export const IIgnoreService = createServiceIdentifier('IIgnoreService'); +/** How many exclusion checks may run at once when filtering a batch of search results. */ +const IGNORE_CHECK_CONCURRENCY = 20; + export interface IIgnoreService { _serviceBrand: undefined; @@ -62,11 +66,13 @@ export class NullIgnoreService implements IIgnoreService { } export async function filterIngoredResources(ignoreService: IIgnoreService, resources: URI[]): Promise { - const result: URI[] = []; - for (const resource of resources) { - if (!await ignoreService.isCopilotIgnored(resource)) { - result.push(resource); - } + // Bounded because this runs over every search result, and an unresolved repository turns each + // check into a git extension lookup, plus a file read when content rules are configured. + const limiter = new Limiter(IGNORE_CHECK_CONCURRENCY); + try { + const ignored = await Promise.all(resources.map(resource => limiter.queue(() => ignoreService.isCopilotIgnored(resource)))); + return resources.filter((_, index) => !ignored[index]); + } finally { + limiter.dispose(); } - return result; } diff --git a/extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts b/extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts index b5502c87ca809a..a7c00b29acdc6b 100644 --- a/extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts +++ b/extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts @@ -23,6 +23,9 @@ import { RemoteContentExclusion } from './remoteContentExclusion'; export const COPILOT_IGNORE_FILE_NAME = '.copilotignore'; +/** How long a failed workspace scan is remembered before the next enforcement retries it. */ +const INIT_RETRY_INTERVAL_MS = 5_000; + export class BaseIgnoreService implements IIgnoreService { declare readonly _serviceBrand: undefined; @@ -30,6 +33,7 @@ export class BaseIgnoreService implements IIgnoreService { private readonly _copilotIgnoreFiles = new IgnoreFile(); private _remoteContentExclusions: RemoteContentExclusion | undefined; private _copilotIgnoreEnabled = false; + private _disposed = false; private readonly _onDidChangeCopilotIgnoreEnablement = new Emitter(); protected _disposables: IDisposable[] = []; @@ -45,33 +49,48 @@ export class BaseIgnoreService implements IIgnoreService { private readonly searchService: ISearchService, private readonly fs: IFileSystemService, private readonly _requestLogger: IRequestLogger, + // Injectable so tests can exercise the failed scan retry without waiting on the wall clock. + private readonly _now: () => number = Date.now, ) { this._disposables.push(this._onDidChangeCopilotIgnoreEnablement); - this._disposables.push(this._authService.onDidCopilotTokenChange(() => { - const copilotIgnoreEnabled = this._authService.copilotToken?.isCopilotIgnoreEnabled() ?? false; - if (this._copilotIgnoreEnabled !== copilotIgnoreEnabled) { - this._onDidChangeCopilotIgnoreEnablement.fire(copilotIgnoreEnabled); - } - this._copilotIgnoreEnabled = copilotIgnoreEnabled; - if (this._copilotIgnoreEnabled === false && this._remoteContentExclusions) { - this._remoteContentExclusions.dispose(); - this._remoteContentExclusions = undefined; - } - if (this._copilotIgnoreEnabled === true && !this._remoteContentExclusions) { - this._remoteContentExclusions = new RemoteContentExclusion( - this._gitService, - this._logService, - this._authService, - this._capiClientService, - this.fs, - this._workspaceService, - this._requestLogger - ); - } - })); + this._disposables.push(this._authService.onDidCopilotTokenChange(() => this.syncEnablement())); + // The token can already be present when this service is created, and no further change event + // is due until the next refresh. + this.syncEnablement(); + } + + /** + * Brings enablement, and the remote rule fetcher it owns, in line with the current Copilot token. + * Called on every decision because an earlier token listener could otherwise ask about a file first. + */ + private syncEnablement(): void { + if (this._disposed) { + return; + } + const copilotIgnoreEnabled = this._authService.copilotToken?.isCopilotIgnoreEnabled() ?? false; + if (this._copilotIgnoreEnabled !== copilotIgnoreEnabled) { + this._onDidChangeCopilotIgnoreEnablement.fire(copilotIgnoreEnabled); + } + this._copilotIgnoreEnabled = copilotIgnoreEnabled; + if (this._copilotIgnoreEnabled === false && this._remoteContentExclusions) { + this._remoteContentExclusions.dispose(); + this._remoteContentExclusions = undefined; + } + if (this._copilotIgnoreEnabled === true && !this._remoteContentExclusions) { + this._remoteContentExclusions = new RemoteContentExclusion( + this._gitService, + this._logService, + this._authService, + this._capiClientService, + this.fs, + this._workspaceService, + this._requestLogger + ); + } } dispose(): void { + this._disposed = true; this._disposables.forEach(d => d.dispose()); if (this._remoteContentExclusions) { this._remoteContentExclusions.dispose(); @@ -89,19 +108,24 @@ export class BaseIgnoreService implements IIgnoreService { } public async isCopilotIgnored(file: URI, token?: CancellationToken): Promise { - let copilotIgnored = false; - if (this._copilotIgnoreEnabled) { - const localCopilotIgnored = this._copilotIgnoreFiles.isIgnored(file); - copilotIgnored = localCopilotIgnored || await (this._remoteContentExclusions?.isIgnored(file, token) ?? false); + this.syncEnablement(); + if (!this._copilotIgnoreEnabled) { + return false; } - return copilotIgnored; + // Local ignore files are read asynchronously, and answering before that read finishes would + // report every file as allowed for the whole of extension startup. + await this.init(); + const localCopilotIgnored = this._copilotIgnoreFiles.isIgnored(file); + return localCopilotIgnored || await (this._remoteContentExclusions?.isIgnored(file, token) ?? false); } async asMinimatchPattern(): Promise { + this.syncEnablement(); if (!this._copilotIgnoreEnabled) { return; } + await this.init(); const all: string[][] = []; const gitRepoRoots = (await this.searchService.findFiles('**/.git/HEAD', { @@ -124,12 +148,30 @@ export class BaseIgnoreService implements IIgnoreService { } private _init: Promise | undefined; + private _initFailedAt: number | undefined; public init(): Promise { + // Enforcement decisions arrive once per search result, so retrying a failed scan on each one + // would turn a failing workspace into a stall. + if (this._initFailedAt !== undefined && this._now() - this._initFailedAt >= INIT_RETRY_INTERVAL_MS) { + this._initFailedAt = undefined; + this._init = undefined; + } this._init ??= (async () => { + let failed = false; for (const folder of this._workspaceService.getWorkspaceFolders()) { - await this.addWorkspace(folder); + try { + await this.addWorkspace(folder); + } catch (err) { + // Enforcement awaits this promise, so one unreadable folder must not reject it + // and take every later ignore check down with it. + failed = true; + this._logService.error(`Failed to read ignore files in ${folder.toString()}: ${err}`); + } } + // Remembering a failed scan forever would leave the local ignore rules empty, and so + // unenforced, for the rest of the session. + this._initFailedAt = failed ? this._now() : undefined; })(); return this._init; } @@ -169,7 +211,7 @@ export class BaseIgnoreService implements IIgnoreService { const files: URI[] = await this.searchService.findFilesWithDefaultExcludes(new RelativePattern(workspaceUri, `${COPILOT_IGNORE_FILE_NAME}`), undefined, CancellationToken.None); for (const file of files) { - const contents = (await this.fs.readFile(file)).toString(); + const contents = new TextDecoder().decode(await this.fs.readFile(file)); this.trackIgnoreFile(workspaceUri, file, contents); } } diff --git a/extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts b/extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts index 93a210110ab736..1bc4c8c63baaa5 100644 --- a/extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts +++ b/extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts @@ -40,6 +40,13 @@ type ContentExclusionResponse = { type RepoMetadata = { repoRootPath: string; fetchUrls: string[] }; +/** + * A repository that was actually resolved, and so can be cached and matched against future files. + * `rootUri` is kept alongside the path because paths alone are ambiguous across schemes. + */ +type CachedRepoMetadata = RepoMetadata & { rootUri: URI }; + + /** Rules for a single repo, along with when they were fetched so they can expire individually. */ type CachedRules = { patterns: string[]; @@ -107,9 +114,9 @@ export class RemoteContentExclusion implements IDisposable { private readonly _fetchExclusionRules: HttpFetchFn; private _disposables: IDisposable[] = []; private readonly _fileReadLimiter: Limiter; - // Cache of repository root paths to their metadata to avoid calling getRepositoryFetchUrls for every file + // Cache of repository roots to their metadata to avoid calling getRepositoryFetchUrls for every file // This is critical for performance when there are many files in a workspace - private readonly _repoRootCache: Map = new Map(); + private readonly _repoRootCache: Map = new Map(); constructor( private readonly _gitService: IGitService, @@ -122,13 +129,24 @@ export class RemoteContentExclusion implements IDisposable { // Injectable so tests can exercise rule expiry and backoff without waiting on the wall clock. private readonly _now: () => number = Date.now ) { + this._disposables.push(this._gitService.onDidOpenRepository((r) => { + const repoInfo = this.getRepositoryInfo(r); + if (!repoInfo) { + return; + } + this.cacheRepoMetadata(repoInfo); + // Files under this root may already have been evaluated as belonging to no repository, + // and so judged without this repo's rules. Those verdicts have to be recomputed. + this.invalidateVerdicts(); + })); + this._disposables.push(this._gitService.onDidCloseRepository((r) => { const repoInfo = this.getRepositoryInfo(r); if (!repoInfo) { return; } // Remove from repo root cache - this._repoRootCache.delete(repoInfo.repoRootPath); + this._repoRootCache.delete(repoRootCacheKey(repoInfo.rootUri)); for (const url of repoInfo.fetchUrls) { this._contentExclusionCache.delete(url); } @@ -163,23 +181,25 @@ export class RemoteContentExclusion implements IDisposable { // Try to find the repository from the cache first to avoid expensive git extension calls // This is critical for performance when there are many files in a workspace - let repoMetadata = this.findCachedRepoMetadataForFile(file); + let resolvedRepo = this.findCachedRepoMetadataForFile(file); // If not in cache, query the git extension (this is expensive for many files) - if (!repoMetadata) { + if (!resolvedRepo) { const repo = await raceCancellationError(this._gitService.getRepositoryFetchUrls(file), token); - repoMetadata = this.getRepositoryInfo(repo); + resolvedRepo = this.getRepositoryInfo(repo); // Cache the result for future lookups - if (repoMetadata) { - this._repoRootCache.set(repoMetadata.repoRootPath, repoMetadata); + if (resolvedRepo) { + this.cacheRepoMetadata(resolvedRepo); } } + // A negative verdict is only safe to trust for the whole rule TTL once the repository is + // settled. A repo with no usable remote is unsettled because remotes can still arrive. + const repoSettled = resolvedRepo ? resolvedRepo.fetchUrls.length > 0 : this._gitService.isInitialized; + // No repository is associated with this file, so we set it to the 'virtual' non-git file repo / key // This way when we go to lookup rules for this file it will pull the non git file rules - if (!repoMetadata) { - repoMetadata = { repoRootPath: '', fetchUrls: [NON_GIT_FILE_KEY] }; - } + const repoMetadata: RepoMetadata = resolvedRepo ?? { repoRootPath: '', fetchUrls: [NON_GIT_FILE_KEY] }; const fileName = file.path.toLowerCase().replace(repoMetadata.repoRootPath.toLowerCase(), ''); @@ -199,7 +219,16 @@ export class RemoteContentExclusion implements IDisposable { } let fileContents: string = ''; let fileContentHash: string = ''; - for (const fetchUrl of repoMetadata.fetchUrls) { + // Unscoped organization rules are keyed under the non-git pseudo repo and apply to any file. + // Their globs already reach every file, so content rules must be evaluated against them too. + const regexRuleSources = repoMetadata.fetchUrls.includes(NON_GIT_FILE_KEY) + ? repoMetadata.fetchUrls + : [...repoMetadata.fetchUrls, NON_GIT_FILE_KEY]; + // Regex rules are per repository, so the rule set is part of the key. Otherwise a permitted + // file would hand its verdict to a same-content file whose own repository excludes it. + const regexScope = regexRuleSources.join(' '); + let regexCacheKey: string = ''; + for (const fetchUrl of regexRuleSources) { const { ifAnyMatch, ifNoneMatch } = this._contentExclusionCache.get(fetchUrl) ?? { ifAnyMatch: [], ifNoneMatch: [] }; // We only want to read the file if we absolutely must as it can be expensive if (ifAnyMatch.length > 0 || ifNoneMatch.length > 0) { @@ -210,8 +239,9 @@ export class RemoteContentExclusion implements IDisposable { const fileContentOrBuffer = await this._fileReadLimiter.queue(() => readFileFromTextBufferOrFS(this._fileSystemService, this._workspaceService, file, 1024)); fileContents = typeof fileContentOrBuffer === 'string' ? fileContentOrBuffer : new TextDecoder().decode(fileContentOrBuffer); fileContentHash = await createSha256Hash(fileContents); + regexCacheKey = `${regexScope}\n${fileContentHash}`; // Cache hit for these file contents, no need to run the regex patterns - const cachedRegexVerdict = this._ignoreRegexResultCache.get(fileContentHash); + const cachedRegexVerdict = this._ignoreRegexResultCache.get(regexCacheKey); if (cachedRegexVerdict && cachedRegexVerdict.generation === generation) { return cachedRegexVerdict.verdict; } @@ -223,23 +253,23 @@ export class RemoteContentExclusion implements IDisposable { } if (ifAnyMatch.length > 0 && fileContents && ifAnyMatch.some(pattern => pattern.test(fileContents))) { this._logService.debug(`File ${file.path} is ignored by content exclusion rule ifAnyMatch`); - this._ignoreRegexResultCache.set(fileContentHash, { verdict: true, generation }); + this._ignoreRegexResultCache.set(regexCacheKey, { verdict: true, generation }); return true; } if (ifNoneMatch.length > 0 && fileContents && !ifNoneMatch.some(pattern => pattern.test(fileContents))) { this._logService.debug(`File ${file.path} is ignored by content exclusion rule ifNoneMatch`); - this._ignoreRegexResultCache.set(fileContentHash, { verdict: true, generation }); + this._ignoreRegexResultCache.set(regexCacheKey, { verdict: true, generation }); return true; } } - // Only memoise a negative verdict once every relevant rule set has actually loaded. Caching it - // after a failed fetch would leave the file permanently allowed. - if (rulesLoaded) { + // Memoise a negative verdict only once the rules have loaded and the repository is settled. + // Doing it earlier would keep the file allowed long after its real rules become known. + if (rulesLoaded && repoSettled) { this._ignoreGlobResultCache.set(file, { verdict: false, generation }); // Only meaningful when regex rules forced us to read (and hash) the file. - if (fileContentHash) { - this._ignoreRegexResultCache.set(fileContentHash, { verdict: false, generation }); + if (regexCacheKey) { + this._ignoreRegexResultCache.set(regexCacheKey, { verdict: false, generation }); } } return false; @@ -281,13 +311,24 @@ export class RemoteContentExclusion implements IDisposable { const repoInfo = this.getRepositoryInfo(repo); // Populate the repo root cache for future lookups if (repoInfo) { - this._repoRootCache.set(repoInfo.repoRootPath, repoInfo); + this.cacheRepoMetadata(repoInfo); fetchUrls.push(...repoInfo.fetchUrls); } } await this.ensureRulesLoaded(fetchUrls); } + /** + * Records a resolved repo so that later files under it skip the git extension lookup. + * A repo with no usable remote is not cached, so its files retry once remotes are known. + */ + private cacheRepoMetadata(metadata: CachedRepoMetadata): void { + if (metadata.fetchUrls.length === 0) { + return; + } + this._repoRootCache.set(repoRootCacheKey(metadata.rootUri), metadata); + } + public async asMinimatchPatterns() { // Anything already queued must land first so callers see a complete pattern set. await Promise.all([...this._pendingRepos.values()].map(pending => pending.deferred.p)); @@ -521,7 +562,7 @@ export class RemoteContentExclusion implements IDisposable { } - private getRepositoryInfo(repo: Pick | undefined): RepoMetadata | undefined { + private getRepositoryInfo(repo: Pick | undefined): CachedRepoMetadata | undefined { if (!repo || !repo.remoteFetchUrls) { return undefined; } @@ -536,7 +577,7 @@ export class RemoteContentExclusion implements IDisposable { return undefined; } })); - return { repoRootPath: repo.rootUri.path, fetchUrls: fetchUrls }; + return { rootUri: repo.rootUri, repoRootPath: repo.rootUri.path, fetchUrls: fetchUrls }; } /** @@ -545,13 +586,18 @@ export class RemoteContentExclusion implements IDisposable { * Returns the most specific (longest) matching repository to handle nested repos/submodules correctly. * This avoids expensive calls to the git extension API for every file. */ - private findCachedRepoMetadataForFile(file: URI): RepoMetadata | undefined { + private findCachedRepoMetadataForFile(file: URI): CachedRepoMetadata | undefined { const filePath = file.path.toLowerCase(); - let bestMatch: RepoMetadata | undefined; + let bestMatch: CachedRepoMetadata | undefined; let bestMatchLength = 0; - for (const [repoRootPath, metadata] of this._repoRootCache.entries()) { - const normalizedRepoRoot = repoRootPath.toLowerCase(); + for (const metadata of this._repoRootCache.values()) { + // Paths alone are ambiguous: the same path can exist under file:// and under a virtual + // file system that points at an entirely different repository. + if (metadata.rootUri.scheme !== file.scheme || metadata.rootUri.authority !== file.authority) { + continue; + } + const normalizedRepoRoot = metadata.repoRootPath.toLowerCase(); if ((filePath.startsWith(normalizedRepoRoot + '/') || filePath === normalizedRepoRoot) && normalizedRepoRoot.length > bestMatchLength) { bestMatch = metadata; @@ -562,6 +608,11 @@ export class RemoteContentExclusion implements IDisposable { } } +/** Keys a repo root by identity rather than path, so schemes cannot collide with one another. */ +function repoRootCacheKey(rootUri: URI): string { + return `${rootUri.scheme}://${rootUri.authority}${rootUri.path.toLowerCase()}`; +} + /** Compares two rule sets by content, so an unchanged refresh does not retire memoised verdicts. */ function isSameRuleSet(a: Omit, b: Omit): boolean { return equalStrings(a.patterns, b.patterns) diff --git a/extensions/copilot/src/platform/ignore/node/test/ignoreServiceImpl.spec.ts b/extensions/copilot/src/platform/ignore/node/test/ignoreServiceImpl.spec.ts new file mode 100644 index 00000000000000..4b806aa61566b8 --- /dev/null +++ b/extensions/copilot/src/platform/ignore/node/test/ignoreServiceImpl.spec.ts @@ -0,0 +1,196 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { beforeEach, describe, expect, suite, test } from 'vitest'; +import { URI } from '../../../../util/vs/base/common/uri'; +import { IAuthenticationService } from '../../../authentication/common/authentication'; +import { CopilotToken, createTestExtendedTokenInfo } from '../../../authentication/common/copilotToken'; +import { ICAPIClientService } from '../../../endpoint/common/capiClient'; +import { MockFileSystemService } from '../../../filesystem/node/test/mockFileSystemService'; +import { NullRequestLogger } from '../../../requestLogger/node/nullRequestLogger'; +import { TestLogService } from '../../../testing/common/testLogService'; +import { COPILOT_IGNORE_FILE_NAME, BaseIgnoreService } from '../ignoreServiceImpl'; +import { MockAuthenticationService } from './mockAuthenticationService'; +import { MockCAPIClientService } from './mockCAPIClientService'; +import { MockGitService } from './mockGitService'; +import { MockSearchService } from './mockSearchService'; +import { MockWorkspaceService } from './mockWorkspaceService'; + +suite('BaseIgnoreService', () => { + let ignoreService: BaseIgnoreService; + let mockGitService: MockGitService; + let mockAuthService: MockAuthenticationService; + let mockCAPIClientService: MockCAPIClientService; + let mockFileSystemService: MockFileSystemService; + let mockWorkspaceService: MockWorkspaceService; + let mockSearchService: MockSearchService; + let now: number; + + const workspaceRoot = URI.file('/workspace'); + const ignoreFile = URI.file(`/workspace/${COPILOT_IGNORE_FILE_NAME}`); + const secretFile = URI.file('/workspace/secrets/keys.ts'); + const ordinaryFile = URI.file('/workspace/src/index.ts'); + + /** A token shaped like the one an organization with content exclusion enabled receives. */ + function tokenWithContentExclusion(enabled: boolean): Omit { + return new CopilotToken(createTestExtendedTokenInfo({ token: 'test-token', copilotignore_enabled: enabled })); + } + + function createService(): BaseIgnoreService { + return new BaseIgnoreService( + mockGitService, + new TestLogService(), + // These mocks implement all the methods used by BaseIgnoreService, but don't satisfy the + // full interface signatures (e.g., overloaded methods). + mockAuthService as unknown as IAuthenticationService, + mockWorkspaceService, + mockCAPIClientService as unknown as ICAPIClientService, + mockSearchService, + mockFileSystemService, + new NullRequestLogger(), + () => now + ); + } + + beforeEach(() => { + now = Date.UTC(2026, 0, 1); + mockGitService = new MockGitService(); + mockAuthService = new MockAuthenticationService(); + mockCAPIClientService = new MockCAPIClientService(); + mockFileSystemService = new MockFileSystemService(); + mockWorkspaceService = new MockWorkspaceService(); + mockSearchService = new MockSearchService(); + + mockWorkspaceService.setWorkspaceFolders([workspaceRoot]); + mockSearchService.setResults([ignoreFile]); + mockFileSystemService.mockFile(ignoreFile, 'secrets/\n'); + // No repository, so only the local ignore file is in play unless a test says otherwise. + mockGitService.setRepositoryFetchUrls(undefined); + }); + + describe('enablement', () => { + test('adopts a token that was already present when the service was created', () => { + mockAuthService.copilotToken = tokenWithContentExclusion(true); + + // No token change event fires: the token predates this service. + expect(createService().isEnabled).toBe(true); + }); + + test('excludes a file for a token that was already present when the service was created', async () => { + mockAuthService.copilotToken = tokenWithContentExclusion(true); + ignoreService = createService(); + + expect(await ignoreService.isCopilotIgnored(secretFile)).toBe(true); + }); + + test('picks up a token that arrives after the service was created', async () => { + ignoreService = createService(); + const beforeToken = await ignoreService.isCopilotIgnored(secretFile); + + mockAuthService.setCopilotToken(tokenWithContentExclusion(true)); + + expect({ beforeToken, afterToken: await ignoreService.isCopilotIgnored(secretFile) }) + .toEqual({ beforeToken: false, afterToken: true }); + }); + + test('enforces a token that arrived without this service having handled the event yet', async () => { + ignoreService = createService(); + + // Models a listener registered before this service reacting to the same token arrival: + // the token is readable, but no change event has reached the ignore service. + mockAuthService.copilotToken = tokenWithContentExclusion(true); + + expect(await ignoreService.isCopilotIgnored(secretFile)).toBe(true); + }); + + test('allows every file while content exclusion is disabled for the token', async () => { + mockAuthService.copilotToken = tokenWithContentExclusion(false); + ignoreService = createService(); + + expect(await ignoreService.isCopilotIgnored(secretFile)).toBe(false); + }); + + test('stops enforcing once a token without content exclusion replaces one that had it', async () => { + mockAuthService.copilotToken = tokenWithContentExclusion(true); + ignoreService = createService(); + const whileEnabled = await ignoreService.isCopilotIgnored(secretFile); + + mockAuthService.setCopilotToken(tokenWithContentExclusion(false)); + + expect({ whileEnabled, afterDisabled: await ignoreService.isCopilotIgnored(secretFile) }) + .toEqual({ whileEnabled: true, afterDisabled: false }); + }); + }); + + describe('local ignore files', () => { + beforeEach(() => { + mockAuthService.copilotToken = tokenWithContentExclusion(true); + }); + + test('excludes a file without init having been called explicitly', async () => { + ignoreService = createService(); + + // Nothing awaited init(), which is how the extension starts the service up. + expect(await ignoreService.isCopilotIgnored(secretFile)).toBe(true); + }); + + test('waits for an in-progress workspace scan before answering', async () => { + mockSearchService.blockSearches(); + ignoreService = createService(); + + const verdict = ignoreService.isCopilotIgnored(secretFile); + const settledEarly = await Promise.race([verdict, Promise.resolve('pending' as const)]); + + mockSearchService.releaseSearches(); + + expect({ settledEarly, verdict: await verdict }).toEqual({ settledEarly: 'pending', verdict: true }); + }); + + test('scans the workspace once no matter how many files are checked', async () => { + ignoreService = createService(); + + await Promise.all([ + ignoreService.isCopilotIgnored(secretFile), + ignoreService.isCopilotIgnored(ordinaryFile), + ignoreService.isCopilotIgnored(URI.file('/workspace/src/other.ts')) + ]); + + expect(mockSearchService.findFilesCallCount).toBe(1); + }); + + test('allows files the ignore file does not cover', async () => { + ignoreService = createService(); + + expect(await ignoreService.isCopilotIgnored(ordinaryFile)).toBe(false); + }); + + test('retries the workspace scan after a failure rather than remembering it', async () => { + mockSearchService.failWith(new Error('workspace is not trusted')); + ignoreService = createService(); + + // A rejected scan must not poison the shared init promise, and must not be cached as if + // it had found no ignore files, which would leave the session unenforced. + const whileFailing = await ignoreService.isCopilotIgnored(secretFile); + mockSearchService.failWith(undefined); + now += 5_000; + + expect({ whileFailing, afterRecovery: await ignoreService.isCopilotIgnored(secretFile) }) + .toEqual({ whileFailing: false, afterRecovery: true }); + }); + + test('does not rescan the workspace for every check while the scan keeps failing', async () => { + mockSearchService.failWith(new Error('workspace is not trusted')); + ignoreService = createService(); + + // Enforcement runs once per search result, so an unbounded retry would turn a failing + // workspace into a stall rather than an answer. + for (let i = 0; i < 20; i++) { + await ignoreService.isCopilotIgnored(URI.file(`/workspace/src/file${i}.ts`)); + } + + expect(mockSearchService.findFilesCallCount).toBe(1); + }); + }); +}); diff --git a/extensions/copilot/src/platform/ignore/node/test/mockAuthenticationService.ts b/extensions/copilot/src/platform/ignore/node/test/mockAuthenticationService.ts index 6340ac31e854b4..619364c0806a46 100644 --- a/extensions/copilot/src/platform/ignore/node/test/mockAuthenticationService.ts +++ b/extensions/copilot/src/platform/ignore/node/test/mockAuthenticationService.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import type { AuthenticationGetSessionOptions, AuthenticationSession } from 'vscode'; -import { Event } from '../../../../util/vs/base/common/event'; +import { Emitter, Event } from '../../../../util/vs/base/common/event'; import { IAuthenticationService } from '../../../authentication/common/authentication'; import { CopilotToken } from '../../../authentication/common/copilotToken'; @@ -18,7 +18,8 @@ export class MockAuthenticationService implements IAuthenticationService { readonly isMinimalMode = false; readonly onDidAuthenticationChange: Event = Event.None; readonly onDidAccessTokenChange: Event = Event.None; - readonly onDidCopilotTokenChange: Event = Event.None; + private readonly _onDidCopilotTokenChange = new Emitter(); + readonly onDidCopilotTokenChange: Event = this._onDidCopilotTokenChange.event; readonly onDidAdoAuthenticationChange: Event = Event.None; readonly anyGitHubSession: AuthenticationSession | undefined = undefined; readonly permissiveGitHubSession: AuthenticationSession | undefined = undefined; @@ -27,6 +28,12 @@ export class MockAuthenticationService implements IAuthenticationService { copilotToken: Omit | undefined = undefined; speculativeDecodingEndpointToken: string | undefined = undefined; + /** Replaces the current token and notifies listeners, as a real token refresh would. */ + setCopilotToken(token: Omit | undefined): void { + this.copilotToken = token; + this._onDidCopilotTokenChange.fire(); + } + getGitHubSession(_kind: 'permissive' | 'any', _options?: AuthenticationGetSessionOptions): Promise; getGitHubSession(_kind: 'permissive' | 'any', _options?: AuthenticationGetSessionOptions): Promise; getGitHubSession(_kind: 'permissive' | 'any', _options?: AuthenticationGetSessionOptions): Promise { @@ -43,5 +50,7 @@ export class MockAuthenticationService implements IAuthenticationService { return Promise.resolve(undefined); } - dispose(): void { } + dispose(): void { + this._onDidCopilotTokenChange.dispose(); + } } diff --git a/extensions/copilot/src/platform/ignore/node/test/mockGitService.ts b/extensions/copilot/src/platform/ignore/node/test/mockGitService.ts index 3f9beaf603b9d8..c076166b6a28e1 100644 --- a/extensions/copilot/src/platform/ignore/node/test/mockGitService.ts +++ b/extensions/copilot/src/platform/ignore/node/test/mockGitService.ts @@ -25,7 +25,9 @@ export class MockGitService implements IGitService { private readonly _onDidCloseRepository = new Emitter(); public readonly onDidCloseRepository: Event = this._onDidCloseRepository.event; - public readonly onDidOpenRepository: Event = Event.None; + private readonly _onDidOpenRepository = new Emitter(); + public readonly onDidOpenRepository: Event = this._onDidOpenRepository.event; + public readonly onDidFinishInitialization: Event = Event.None; public readonly activeRepository: IObservable = observableValue('test-git-activeRepo', undefined); public repositories: RepoContext[] = []; @@ -58,6 +60,13 @@ export class MockGitService implements IGitService { this._onDidCloseRepository.fire(repo); } + /** + * Fires the onDidOpenRepository event with the given repository context. + */ + fireDidOpenRepository(repo: Pick): void { + this._onDidOpenRepository.fire(repo as RepoContext); + } + getRepository(_uri: URI, _forceOpen?: boolean): Promise { return Promise.resolve(undefined); } @@ -180,5 +189,6 @@ export class MockGitService implements IGitService { dispose(): void { this._onDidCloseRepository.dispose(); + this._onDidOpenRepository.dispose(); } } diff --git a/extensions/copilot/src/platform/ignore/node/test/mockSearchService.ts b/extensions/copilot/src/platform/ignore/node/test/mockSearchService.ts new file mode 100644 index 00000000000000..70065710e4b210 --- /dev/null +++ b/extensions/copilot/src/platform/ignore/node/test/mockSearchService.ts @@ -0,0 +1,59 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import type * as vscode from 'vscode'; +import { DeferredPromise } from '../../../../util/vs/base/common/async'; +import { URI } from '../../../../util/vs/base/common/uri'; +import { AbstractSearchService } from '../../../search/common/searchService'; + +/** + * A minimal mock implementation of ISearchService for testing. + * Searches can be held open so callers racing an in-progress workspace scan can be exercised. + */ +export class MockSearchService extends AbstractSearchService { + + private _results: URI[] = []; + private _gate: DeferredPromise | undefined; + private _error: Error | undefined; + + public findFilesCallCount = 0; + + /** Sets the files every search resolves with. */ + setResults(results: URI[]): void { + this._results = results; + } + + /** Makes every search reject, as an unreadable or untrusted workspace would. */ + failWith(error: Error | undefined): void { + this._error = error; + } + + /** Holds every subsequent search open until {@link releaseSearches}. */ + blockSearches(): void { + this._gate = new DeferredPromise(); + } + + releaseSearches(): void { + this._gate?.complete(undefined); + this._gate = undefined; + } + + async findFiles(): Promise { + this.findFilesCallCount++; + await this._gate?.p; + if (this._error) { + throw this._error; + } + return this._results as vscode.Uri[]; + } + + findTextInFiles(): Promise { + return Promise.reject(new Error('Not implemented')); + } + + findTextInFiles2(): vscode.FindTextInFilesResponse { + throw new Error('Not implemented'); + } +} diff --git a/extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts b/extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts index 5dfa9902deaea6..4f468a6025a823 100644 --- a/extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts +++ b/extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts @@ -493,4 +493,232 @@ suite('RemoteContentExclusion', () => { mockCAPIClientService.releaseRequests(); }); }); + + describe('deferring verdicts until repositories resolve', () => { + const repoRoot = '/workspace/my-repo'; + const file = URI.file('/workspace/my-repo/src/secret.ts'); + + test('applies a repository rule to a file first seen before discovery resolved it', async () => { + respondWithRules({ [repoRoot]: { paths: ['*'] } }); + // The Git extension has not finished discovering repositories yet. + mockGitService.isInitialized = false; + mockGitService.setRepositoryFetchUrls(undefined); + + const beforeDiscovery = await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + mockGitService.isInitialized = true; + mockGitService.setRepositoryFetchUrls({ rootUri: URI.file(repoRoot), remoteFetchUrls: [remoteFor(repoRoot)] }); + const afterDiscovery = await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + expect({ beforeDiscovery, afterDiscovery }).toEqual({ beforeDiscovery: false, afterDiscovery: true }); + }); + + test('does not memoise a verdict reached while repository discovery is still running', async () => { + mockGitService.isInitialized = false; + mockGitService.setRepositoryFetchUrls(undefined); + + await remoteContentExclusion.isIgnored(file, CancellationToken.None); + await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + // Memoising here would pin the file open for the whole rule TTL. + expect(mockGitService.getRepositoryFetchUrlsCallCount).toBe(2); + }); + + test('memoises a verdict for a file that genuinely belongs to no repository', async () => { + mockGitService.setRepositoryFetchUrls(undefined); + + const nonGitFile = URI.file('/some/random/file.txt'); + await remoteContentExclusion.isIgnored(nonGitFile, CancellationToken.None); + await remoteContentExclusion.isIgnored(nonGitFile, CancellationToken.None); + + expect(mockGitService.getRepositoryFetchUrlsCallCount).toBe(1); + }); + + test('applies a repository rule to files evaluated before that repository opened', async () => { + respondWithRules({ [repoRoot]: { paths: ['*'] } }); + mockGitService.setRepositoryFetchUrls(undefined); + + const beforeOpen = await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + mockGitService.fireDidOpenRepository({ rootUri: URI.file(repoRoot), remoteFetchUrls: [remoteFor(repoRoot)] }); + const afterOpen = await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + expect({ beforeOpen, afterOpen }).toEqual({ beforeOpen: false, afterOpen: true }); + }); + + test('does not cache a repository that resolved without a usable remote', async () => { + // Remotes can arrive after the repository itself does, so an empty remote list is not + // an answer worth reusing for every other file under that root. + mockGitService.setRepositoryFetchUrls({ rootUri: URI.file('/workspace/local-only'), remoteFetchUrls: [] }); + + await remoteContentExclusion.isIgnored(URI.file('/workspace/local-only/a.ts'), CancellationToken.None); + await remoteContentExclusion.isIgnored(URI.file('/workspace/local-only/b.ts'), CancellationToken.None); + + expect(mockGitService.getRepositoryFetchUrlsCallCount).toBe(2); + }); + + test('applies a rule once a remote appears on a repository that had none', async () => { + respondWithRules({ '/workspace/local-only': { paths: ['*'] } }); + const rootUri = URI.file('/workspace/local-only'); + mockGitService.setRepositoryFetchUrls({ rootUri, remoteFetchUrls: [] }); + + const localOnlyFile = URI.file('/workspace/local-only/a.ts'); + const beforeRemote = await remoteContentExclusion.isIgnored(localOnlyFile, CancellationToken.None); + + // The user publishes the repository, so it becomes subject to server side rules. + mockGitService.setRepositoryFetchUrls({ rootUri, remoteFetchUrls: [remoteFor('/workspace/local-only')] }); + + expect({ beforeRemote, afterRemote: await remoteContentExclusion.isIgnored(localOnlyFile, CancellationToken.None) }) + .toEqual({ beforeRemote: false, afterRemote: true }); + }); + + test('does not match a cached repository root against a file from another scheme', async () => { + routeToRepos([repoRoot]); + await remoteContentExclusion.isIgnored(file, CancellationToken.None); + const afterLocalFile = mockGitService.getRepositoryFetchUrlsCallCount; + + // Same path, different file system: this is not the repository that was cached. + const virtual = URI.from({ scheme: 'vscode-vfs', authority: 'github', path: file.path }); + await remoteContentExclusion.isIgnored(virtual, CancellationToken.None); + + expect({ afterLocalFile, afterVirtualFile: mockGitService.getRepositoryFetchUrlsCallCount }) + .toEqual({ afterLocalFile: 1, afterVirtualFile: 2 }); + }); + }); + + describe('rule scoping', () => { + test('matches fetched globs against every file rather than only their own repository', async () => { + routeToRepos(['/workspace/repo-a', '/workspace/repo-b']); + respondWithRules({ '/workspace/repo-a': { paths: ['**/secret.ts'] } }); + + // Rules compile into one flattened matcher list, so a sibling repo is over-blocked rather + // than under-blocked. Pinned because that safe direction is what makes it acceptable. + expect({ + excludedRepo: await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-a/secret.ts'), CancellationToken.None), + siblingRepo: await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-b/secret.ts'), CancellationToken.None) + }).toEqual({ excludedRepo: true, siblingRepo: true }); + }); + + test('applies organization rules to files inside and outside a repository', async () => { + routeToRepos(['/workspace/repo-a']); + // Rules that are not scoped to a repository come back under the non-git pseudo repo. + mockCAPIClientService.setResponder(repos => rulesResponse(new Map([[NON_GIT_FILE_KEY, { paths: ['**/*.pem'] }]]), repos)); + + expect({ + inRepo: await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-a/key.pem'), CancellationToken.None), + outsideRepo: await remoteContentExclusion.isIgnored(URI.file('/elsewhere/key.pem'), CancellationToken.None) + }).toEqual({ inRepo: true, outsideRepo: true }); + }); + + test('applies organization content rules to files inside a repository', async () => { + routeToRepos(['/workspace/repo-a']); + // The repository itself has no rules, so only the unscoped organization rule can + // exclude this file. Content rules must reach in-repo files just as globs do. + mockCAPIClientService.setResponder(repos => rulesResponse(new Map([[NON_GIT_FILE_KEY, { ifAnyMatch: ['BEGIN RSA PRIVATE KEY'] }]]), repos)); + const inRepo = URI.file('/workspace/repo-a/id_rsa'); + const outsideRepo = URI.file('/elsewhere/id_rsa'); + mockFileSystemService.mockFile(inRepo, '-----BEGIN RSA PRIVATE KEY-----'); + mockFileSystemService.mockFile(outsideRepo, '-----BEGIN RSA PRIVATE KEY-----'); + + expect({ + inRepo: await remoteContentExclusion.isIgnored(inRepo, CancellationToken.None), + outsideRepo: await remoteContentExclusion.isIgnored(outsideRepo, CancellationToken.None) + }).toEqual({ inRepo: true, outsideRepo: true }); + }); + + test('excludes every file in a repository that is fully excluded', async () => { + // The `paths: ["*"]` shape an administrator uses to exclude a whole repository. + routeToRepos(['/workspace/repo-a']); + respondWithRules({ '/workspace/repo-a': { paths: ['*'] } }); + + expect({ + nested: await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-a/src/deeply/nested.ts'), CancellationToken.None), + atRoot: await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-a/top.ts'), CancellationToken.None) + }).toEqual({ nested: true, atRoot: true }); + }); + + test('resolves a file in a submodule against the innermost repository', async () => { + routeToRepos(['/workspace/repo-a', '/workspace/repo-a/vendor/sub']); + + await remoteContentExclusion.isIgnored(URI.file('/workspace/repo-a/vendor/sub/index.ts'), CancellationToken.None); + + expect([...mockCAPIClientService.requestedRepos].sort()) + .toEqual([NON_GIT_FILE_KEY, remoteFor('/workspace/repo-a/vendor/sub')].sort()); + }); + }); + + describe('content based rules', () => { + const file = URI.file('/workspace/repo-a/notes.ts'); + const publicFile = URI.file('/workspace/repo-a/public.ts'); + + test('excludes a file whose contents match ifAnyMatch', async () => { + routeToRepos(['/workspace/repo-a']); + respondWithRules({ '/workspace/repo-a': { ifAnyMatch: ['CONFIDENTIAL'] } }); + mockFileSystemService.mockFile(file, '// CONFIDENTIAL\nexport const a = 1;'); + mockFileSystemService.mockFile(publicFile, 'export const b = 2;'); + + expect({ + confidential: await remoteContentExclusion.isIgnored(file, CancellationToken.None), + unmarked: await remoteContentExclusion.isIgnored(publicFile, CancellationToken.None) + }).toEqual({ confidential: true, unmarked: false }); + }); + + test('excludes a file that lacks a required ifNoneMatch marker', async () => { + routeToRepos(['/workspace/repo-a']); + respondWithRules({ '/workspace/repo-a': { ifNoneMatch: ['PUBLIC'] } }); + mockFileSystemService.mockFile(file, 'export const a = 1;'); + mockFileSystemService.mockFile(publicFile, '// PUBLIC\nexport const b = 2;'); + + expect({ + unmarked: await remoteContentExclusion.isIgnored(file, CancellationToken.None), + marked: await remoteContentExclusion.isIgnored(publicFile, CancellationToken.None) + }).toEqual({ unmarked: true, marked: false }); + }); + + test('excludes a file that cannot be read while content rules are in force', async () => { + routeToRepos(['/workspace/repo-a']); + respondWithRules({ '/workspace/repo-a': { ifAnyMatch: ['CONFIDENTIAL'] } }); + + // Nothing is mocked for this path, so the read fails and the contents are unknown. + expect(await remoteContentExclusion.isIgnored(file, CancellationToken.None)).toBe(true); + }); + + test('reports regex exclusions only once a regex rule has been fetched', async () => { + routeToRepos(['/workspace/repo-a']); + respondWithRules({ '/workspace/repo-a': { ifAnyMatch: ['CONFIDENTIAL'] } }); + const beforeAnyFetch = remoteContentExclusion.isRegexContextExclusionsEnabled; + + mockFileSystemService.mockFile(file, 'export const a = 1;'); + await remoteContentExclusion.isIgnored(file, CancellationToken.None); + + expect({ beforeAnyFetch, afterFetch: remoteContentExclusion.isRegexContextExclusionsEnabled }) + .toEqual({ beforeAnyFetch: false, afterFetch: true }); + }); + + test('does not reuse one repository content verdict for the same file in another', async () => { + routeToRepos(['/workspace/repo-a', '/workspace/repo-b']); + // Both repos exclude on content, but on different markers, so identical files must + // reach different verdicts. + respondWithRules({ + '/workspace/repo-a': { ifAnyMatch: ['SECRET-A'] }, + '/workspace/repo-b': { ifAnyMatch: ['SECRET-B'] } + }); + + const shared = '// SECRET-B\nexport const shared = 1;'; + const inRepoA = URI.file('/workspace/repo-a/shared.ts'); + const inRepoB = URI.file('/workspace/repo-b/shared.ts'); + mockFileSystemService.mockFile(inRepoA, shared); + mockFileSystemService.mockFile(inRepoB, shared); + + // Both rule sets are loaded up front, so evaluating the second file does not trigger a + // fetch that would incidentally retire the first file's cached verdict. + await remoteContentExclusion.loadRepos([URI.file('/workspace/repo-a'), URI.file('/workspace/repo-b')]); + + // repo-a is evaluated first, so its permissive verdict for these contents is cached. + const repoA = await remoteContentExclusion.isIgnored(inRepoA, CancellationToken.None); + + expect({ repoA, repoB: await remoteContentExclusion.isIgnored(inRepoB, CancellationToken.None) }) + .toEqual({ repoA: false, repoB: true }); + }); + }); }); diff --git a/extensions/copilot/src/platform/ignore/vscode-node/ignoreService.ts b/extensions/copilot/src/platform/ignore/vscode-node/ignoreService.ts index 1f92fa75e9b969..d3a66b41b5078a 100644 --- a/extensions/copilot/src/platform/ignore/vscode-node/ignoreService.ts +++ b/extensions/copilot/src/platform/ignore/vscode-node/ignoreService.ts @@ -54,7 +54,7 @@ export class VsCodeIgnoreService extends BaseIgnoreService { this._disposables.push( workspace.onDidSaveTextDocument(async doc => { if (this.isIgnoreFile(doc.uri)) { - const contents = (await workspace.fs.readFile(doc.uri)).toString(); + const contents = new TextDecoder().decode(await workspace.fs.readFile(doc.uri)); const folder = workspace.getWorkspaceFolder(doc.uri); this.trackIgnoreFile(folder?.uri, doc.uri, contents); } @@ -67,7 +67,7 @@ export class VsCodeIgnoreService extends BaseIgnoreService { workspace.onDidRenameFiles(async e => { for (const f of e.files) { if (this.isIgnoreFile(f.newUri)) { - const contents = (await workspace.fs.readFile(f.newUri)).toString(); + const contents = new TextDecoder().decode(await workspace.fs.readFile(f.newUri)); this.removeIgnoreFile(f.oldUri); const folder = workspace.getWorkspaceFolder(f.newUri); this.trackIgnoreFile(folder?.uri, f.newUri, contents); diff --git a/extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts b/extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts index 9a1142f10b0eac..b109b860ca1525 100644 --- a/extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts +++ b/extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import type * as vscode from 'vscode'; -import { combineGlob } from '../../../util/common/glob'; +import { CancellationTokenSource } from '../../../util/vs/base/common/cancellation'; import { filterIngoredResources, IIgnoreService } from '../../ignore/common/ignoreService'; import { LogExecTime } from '../../log/common/logExecTime'; import { ILogService } from '../../log/common/logService'; @@ -24,7 +24,7 @@ export class SearchServiceImpl extends BaseSearchServiceImpl { override async findFilesWithDefaultExcludes(include: vscode.GlobPattern, maxResults: 1 | number | undefined, token: vscode.CancellationToken): Promise { const copilotIgnoreExclude = await this._ignoreService.asMinimatchPattern(); const results = await this._findFilesWithDefaultExcludesAndExcludes(include, copilotIgnoreExclude, maxResults, token); - if (!this._ignoreService.isRegexExclusionsEnabled || !results) { + if (!results) { return results; } else if (Array.isArray(results)) { return await filterIngoredResources(this._ignoreService, results); @@ -36,17 +36,23 @@ export class SearchServiceImpl extends BaseSearchServiceImpl { @LogExecTime(self => self._logService, 'SearchServiceImpl::findFiles') override async findFiles(filePattern: vscode.GlobPattern | vscode.GlobPattern[], options?: vscode.FindFiles2Options | undefined, token?: vscode.CancellationToken | undefined): Promise { const copilotIgnoreExclude = await this._ignoreService.asMinimatchPattern(); - if (options?.exclude) { - options = { ...options, exclude: copilotIgnoreExclude ? options.exclude.map(e => combineGlob(e, copilotIgnoreExclude)) : options.exclude }; - } else { - options = { ...options, exclude: copilotIgnoreExclude ? [copilotIgnoreExclude] : options?.exclude }; - } - const results = await super.findFiles(filePattern, options, token); - if (!this._ignoreService.isRegexExclusionsEnabled) { - return results; - } else { - return await filterIngoredResources(this._ignoreService, results); - } + // Exclude patterns are combined with a logical AND, so appending an entry only narrows the + // results. Appending also keeps any RelativePattern the caller passed scoped to its baseUri. + const exclude = copilotIgnoreExclude ? [...options?.exclude ?? [], copilotIgnoreExclude] : options?.exclude; + const results = await super.findFiles(filePattern, { ...options, exclude }, token); + return await filterIngoredResources(this._ignoreService, results); + } + + override findTextInFiles2(query: vscode.TextSearchQuery2, options?: vscode.FindTextInFilesOptions2, token?: vscode.CancellationToken): vscode.FindTextInFilesResponse { + // The search cannot start until the exclusion pattern is known, so it is kicked off as a + // promise and both members of the response are derived from that one search. + return excludeIgnoredTextSearchResults(this._ignoreService, options?.maxResults, async searchToken => { + const copilotIgnoreExclude = await this._ignoreService.asMinimatchPattern(); + const exclude = copilotIgnoreExclude ? [...options?.exclude ?? [], copilotIgnoreExclude] : options?.exclude; + // The limit is re-applied to the filtered stream, so it must not also cap the search: + // excluded hits would otherwise use up the caller's quota before allowed ones arrive. + return super.findTextInFiles2(query, { ...options, exclude, maxResults: undefined }, searchToken); + }, token); } override async findTextInFiles(query: vscode.TextSearchQuery, options: vscode.FindTextInFilesOptions, progress: vscode.Progress, token: vscode.CancellationToken): Promise { @@ -67,3 +73,45 @@ export class SearchServiceImpl extends BaseSearchServiceImpl { return result; } } + +/** + * Filters content excluded files out of a streamed text search response. + * Results carry matching lines, and content rules are not expressible as a glob, so each hit is checked. + * + * `maxResults` is applied to the filtered stream rather than the search, so excluded hits cannot use + * up the caller's quota. The search is cancelled as soon as enough allowed results have been seen. + */ +export function excludeIgnoredTextSearchResults( + ignoreService: IIgnoreService, + maxResults: number | undefined, + startSearch: (token: vscode.CancellationToken) => Promise, + token?: vscode.CancellationToken +): vscode.FindTextInFilesResponse { + const source = new CancellationTokenSource(token); + const search = startSearch(source.token); + const complete = search.then(response => response.complete); + // A caller that abandons the iteration may never await `complete`, so make sure a failed search + // is not reported as an unhandled rejection. Anyone who does await it still observes the error. + complete.catch(() => { }); + + return { + results: (async function* () { + try { + const response = await search; + let yielded = 0; + for await (const result of response.results) { + if (await ignoreService.isCopilotIgnored(result.uri)) { + continue; + } + yield result; + if (maxResults !== undefined && ++yielded >= maxResults) { + return; + } + } + } finally { + source.dispose(true); + } + })(), + complete + }; +} diff --git a/extensions/copilot/src/platform/search/vscode-node/test/searchServiceImpl.spec.ts b/extensions/copilot/src/platform/search/vscode-node/test/searchServiceImpl.spec.ts new file mode 100644 index 00000000000000..984230edba78a9 --- /dev/null +++ b/extensions/copilot/src/platform/search/vscode-node/test/searchServiceImpl.spec.ts @@ -0,0 +1,155 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { expect, suite, test } from 'vitest'; +import type * as vscode from 'vscode'; +import { URI } from '../../../../util/vs/base/common/uri'; +import { IIgnoreService } from '../../../ignore/common/ignoreService'; +import { excludeIgnoredTextSearchResults } from '../searchServiceImpl'; + +/** An ignore service that excludes an explicit set of files, as a content exclusion rule would. */ +function ignoreServiceExcluding(...excluded: URI[]): IIgnoreService { + const excludedFiles = new Set(excluded.map(uri => uri.toString())); + return { + _serviceBrand: undefined, + isEnabled: true, + isRegexExclusionsEnabled: false, + dispose: () => { }, + init: () => Promise.resolve(), + isCopilotIgnored: (file: URI) => Promise.resolve(excludedFiles.has(file.toString())), + asMinimatchPattern: () => Promise.resolve(undefined) + }; +} + +function textSearchResponse(results: vscode.TextSearchResult2[], complete: Promise = Promise.resolve({})): vscode.FindTextInFilesResponse { + return { + results: (async function* () { + for (const result of results) { + yield result; + } + })(), + complete + }; +} + +/** A text search hit carrying the matching line, which is what an exclusion rule must protect. */ +function match(uri: URI, text: string): vscode.TextSearchResult2 { + return { uri, ranges: [], previewText: text } as unknown as vscode.TextSearchResult2; +} + +suite('excludeIgnoredTextSearchResults', () => { + const excludedFile = URI.file('/workspace/repo/secrets.ts'); + const allowedFile = URI.file('/workspace/repo/index.ts'); + + async function collect(response: vscode.FindTextInFilesResponse): Promise { + const seen: string[] = []; + for await (const result of response.results) { + seen.push(result.uri.toString()); + } + return seen; + } + + /** Wraps a ready made response, capturing the token the search was started with. */ + function fromResponse(response: vscode.FindTextInFilesResponse | Promise) { + const tokens: vscode.CancellationToken[] = []; + return { + tokens, + start: (token: vscode.CancellationToken) => { + tokens.push(token); + return Promise.resolve(response); + } + }; + } + + test('drops matches from a content excluded file', async () => { + const response = excludeIgnoredTextSearchResults( + ignoreServiceExcluding(excludedFile), + undefined, + fromResponse(textSearchResponse([ + match(excludedFile, 'const apiKey = "sk-live-1234";'), + match(allowedFile, 'export const a = 1;') + ])).start + ); + + expect(await collect(response)).toEqual([allowedFile.toString()]); + }); + + test('keeps every match when nothing is excluded', async () => { + const response = excludeIgnoredTextSearchResults( + ignoreServiceExcluding(), + undefined, + fromResponse(textSearchResponse([match(excludedFile, 'a'), match(allowedFile, 'b')])).start + ); + + expect(await collect(response)).toEqual([excludedFile.toString(), allowedFile.toString()]); + }); + + test('does not let excluded matches consume the caller limit', async () => { + // The excluded hits arrive first, so a limit applied before filtering would return nothing. + const allowed = [URI.file('/workspace/repo/a.ts'), URI.file('/workspace/repo/b.ts')]; + const response = excludeIgnoredTextSearchResults( + ignoreServiceExcluding(excludedFile), + 2, + fromResponse(textSearchResponse([ + match(excludedFile, 'secret one'), + match(excludedFile, 'secret two'), + match(allowed[0], 'a'), + match(allowed[1], 'b') + ])).start + ); + + expect(await collect(response)).toEqual(allowed.map(uri => uri.toString())); + }); + + test('stops the search once the caller limit is met', async () => { + const search = fromResponse(textSearchResponse([ + match(allowedFile, 'a'), + match(allowedFile, 'b'), + match(allowedFile, 'c') + ])); + const response = excludeIgnoredTextSearchResults(ignoreServiceExcluding(), 2, search.start); + + const seen = await collect(response); + + expect({ seen: seen.length, searchCancelled: search.tokens[0].isCancellationRequested }) + .toEqual({ seen: 2, searchCancelled: true }); + }); + + test('surfaces the underlying completion result', async () => { + const response = excludeIgnoredTextSearchResults( + ignoreServiceExcluding(excludedFile), + undefined, + fromResponse(textSearchResponse([], Promise.resolve({ limitHit: true }))).start + ); + + expect(await response.complete).toEqual({ limitHit: true }); + }); + + test('reports a failed search to a caller that awaits completion', async () => { + const response = excludeIgnoredTextSearchResults( + ignoreServiceExcluding(), + undefined, + () => Promise.reject(new Error('search provider failed')) + ); + + await expect(response.complete).rejects.toThrow('search provider failed'); + }); + + test('does not raise an unhandled rejection when a failed search is abandoned', async () => { + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => unhandled.push(reason); + process.on('unhandledRejection', onUnhandled); + try { + // Neither member of the response is ever consumed, which is what an aborted tool call + // leaves behind. + excludeIgnoredTextSearchResults(ignoreServiceExcluding(), undefined, () => Promise.reject(new Error('search provider failed'))); + await new Promise(resolve => setTimeout(resolve, 10)); + } finally { + process.off('unhandledRejection', onUnhandled); + } + + expect(unhandled).toEqual([]); + }); +}); diff --git a/extensions/copilot/src/util/common/glob.ts b/extensions/copilot/src/util/common/glob.ts index cded7c3dc7879e..7d2fa638c70394 100644 --- a/extensions/copilot/src/util/common/glob.ts +++ b/extensions/copilot/src/util/common/glob.ts @@ -57,13 +57,3 @@ export function shouldInclude(uri: URI, options: GlobIncludeOptions | undefined) return true; } - -export function combineGlob(glob1: string | vscode.RelativePattern, glob2: string | vscode.RelativePattern): string { - let stringGlob1 = typeof glob1 === 'string' ? glob1 : glob1.baseUri.toString() + glob1.pattern; - let stringGlob2 = typeof glob2 === 'string' ? glob2 : glob2.baseUri.toString() + glob2.pattern; - // Remove any bracket expansion from the globs - stringGlob1 = stringGlob1.replace(/\{.*\}/g, ''); - stringGlob2 = stringGlob2.replace(/\{.*\}/g, ''); - // Combine them into one bracket expanded glob pattern - return `{${stringGlob1},${stringGlob2}}`; -}