Skip to content

Harden content exclusion failrue condition #332368

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

Merged
Logan Ramos (lramos15) merged 2 commits into from
Aug 24, 2026
Merged

Harden content exclusion failrue condition #332368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88f7521
Harden content exclusion failrue condition
lramos15 Aug 24, 2026
1617893
Address review feedback on content exclusion hardening
lramos15 Aug 24, 2026
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
32 changes: 31 additions & 1 deletion extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -191,6 +197,10 @@ export class GitServiceImpl extends Disposable implements IGitService {
async getRepositoryFetchUrls(uri: URI): Promise<Pick<RepoContext, 'rootUri' | 'remoteFetchUrls'> | 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();

@dmitrivMS Dmitriy Vasyura (dmitrivMS) Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AI Review: This waits up to 30 seconds before checking whether a Git API exists. When vscode.git is absent, or activation fails without publishing a terminal state, _isInitialized remains false and all first-window callers stall even though no repository can be discovered. Return immediately for known unavailable or disabled states, publish activation failure as terminal, and reserve this wait for genuinely pending activation.


const gitAPI = this.gitExtensionService.getExtensionApi();
if (!gitAPI) {
return undefined;
Expand Down Expand Up @@ -432,6 +442,26 @@ export class GitServiceImpl extends Disposable implements IGitService {
}
}

private _initialDiscoverySettled: Promise<void> | 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<void> {
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<void> {
this.logService.trace(`[GitServiceImpl][doOpenRepository] Repository: ${repository.rootUri.toString()}`);

Expand Down
11 changes: 4 additions & 7 deletions extensions/copilot/src/platform/ignore/common/ignoreService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,8 @@ export class NullIgnoreService implements IIgnoreService {
}

export async function filterIngoredResources(ignoreService: IIgnoreService, resources: URI[]): Promise<URI[]> {
const result: URI[] = [];
for (const resource of resources) {
if (!await ignoreService.isCopilotIgnored(resource)) {
result.push(resource);
}
}
return result;
// Checked concurrently because this now runs over every search result, not just the rare
// case where content based rules are configured.
const ignored = await Promise.all(resources.map(resource => ignoreService.isCopilotIgnored(resource)));
return resources.filter((_, index) => !ignored[index]);
Comment thread
lramos15 marked this conversation as resolved.
Outdated
}
100 changes: 71 additions & 29 deletions extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,17 @@ 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;

private readonly _copilotIgnoreFiles = new IgnoreFile();
private _remoteContentExclusions: RemoteContentExclusion | undefined;
private _copilotIgnoreEnabled = false;
private _disposed = false;
private readonly _onDidChangeCopilotIgnoreEnablement = new Emitter<boolean>();

protected _disposables: IDisposable[] = [];
Expand All @@ -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();
Expand All @@ -89,19 +108,24 @@ export class BaseIgnoreService implements IIgnoreService {
}

public async isCopilotIgnored(file: URI, token?: CancellationToken): Promise<boolean> {
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<string | undefined> {
this.syncEnablement();
if (!this._copilotIgnoreEnabled) {
return;
}
await this.init();
const all: string[][] = [];

const gitRepoRoots = (await this.searchService.findFiles('**/.git/HEAD', {
Expand All @@ -124,12 +148,30 @@ export class BaseIgnoreService implements IIgnoreService {
}

private _init: Promise<void> | undefined;
private _initFailedAt: number | undefined;

public init(): Promise<void> {
// 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;
}
Expand Down Expand Up @@ -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);
}
}
Expand Down
Loading
Loading