Harden content exclusion failrue condition - #332368
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens Copilot content exclusion across repository discovery, ignore initialization, glob handling, and search filtering.
Changes:
- Improves exclusion-rule discovery, caching, retries, and enablement synchronization.
- Applies exclusions consistently to file and streamed text searches.
- Adds extensive tests and supporting mocks.
Show a summary per file
| File | Description |
|---|---|
extensions/copilot/src/util/common/glob.ts |
Flattens combined brace globs. |
extensions/copilot/src/util/common/test/glob.spec.ts |
Tests glob combinations. |
extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts |
Filters all search APIs. |
extensions/copilot/src/platform/search/vscode-node/test/searchServiceImpl.spec.ts |
Tests streamed-result filtering. |
extensions/copilot/src/platform/ignore/common/ignoreService.ts |
Concurrently filters resources. |
extensions/copilot/src/platform/ignore/node/ignoreServiceImpl.ts |
Synchronizes enablement and retries initialization. |
extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts |
Hardens repository and rule caching. |
extensions/copilot/src/platform/ignore/vscode-node/ignoreService.ts |
Correctly decodes ignore files. |
extensions/copilot/src/platform/ignore/node/test/ignoreServiceImpl.spec.ts |
Tests enablement and initialization. |
extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts |
Expands rule and repository tests. |
extensions/copilot/src/platform/ignore/node/test/mockAuthenticationService.ts |
Adds token-change events. |
extensions/copilot/src/platform/ignore/node/test/mockGitService.ts |
Adds repository-open events. |
extensions/copilot/src/platform/ignore/node/test/mockSearchService.ts |
Adds controllable search behavior. |
extensions/copilot/src/platform/git/vscode-node/gitServiceImpl.ts |
Waits for initial repository discovery. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
extensions/copilot/src/platform/search/vscode-node/searchServiceImpl.ts:32
- These results have already passed through this class's overridden
findFiles:_findFilesWithDefaultExcludesAndExcludescallsthis.findFiles, and line 45 filters there. Filtering again duplicates every ignore lookup (including content reads/hashes) for allfindFilesWithDefaultExcludescallers.
} else if (Array.isArray(results)) {
return await filterIngoredResources(this._ignoreService, results);
} else {
return await this._ignoreService.isCopilotIgnored(results) ? undefined : results;
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Balanced
- findFiles appends the exclusion as its own exclude entry instead of merging it into the caller's patterns, preserving RelativePattern baseUri semantics. - Remove combineGlob, now unused. VS Code's search glob engine cannot combine a character class alternative into a larger group, so a string merge is lossy for some inputs no matter how the braces are handled. - Apply maxResults to the filtered text search stream and cancel the search once the limit is met, so excluded hits cannot use up the caller's quota. - Bound the concurrency of exclusion checks when filtering search results. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26d0669e-e852-4f31-9aca-a0f58930e66c
| continue; | ||
| } | ||
| yield result; | ||
| if (maxResults !== undefined && ++yielded >= maxResults) { |
There was a problem hiding this comment.
AI Review: maxResults counts individual matches, but this increments once per TextSearchResult2. A match result can contain multiple ranges, while a context result contains none, so multi-range results can exceed the cap and surrounding context can exhaust it before enough matches are returned. Because the check follows yield, maxResults: 0 also returns one result. Count and trim match ranges, leave context out of the count, and handle a nonpositive limit before yielding.
| const response = await search; | ||
| let yielded = 0; | ||
| for await (const result of response.results) { | ||
| if (await ignoreService.isCopilotIgnored(result.uri)) { |
There was a problem hiding this comment.
AI Review: Cancellation reaches the provider search but not this exclusion check. A canceled search can remain blocked on ignore initialization, Git discovery, or rule loading because isCopilotIgnored receives no token and awaits shared initialization before its token-aware remote work. Pass the linked token here and race the complete exclusion decision and preflight against cancellation so both the iterator and complete settle promptly.
| startSearch: (token: vscode.CancellationToken) => Promise<vscode.FindTextInFilesResponse>, | ||
| token?: vscode.CancellationToken | ||
| ): vscode.FindTextInFilesResponse { | ||
| const source = new CancellationTokenSource(token); |
There was a problem hiding this comment.
AI Review: This linked source subscribes to the caller token but is disposed only by the lazy results generator. A caller can await only complete or never iterate, leaving that subscription retained after the eagerly started search settles. Dispose the source without cancellation when completion settles, while keeping dispose(true) for early iterator termination; if filtering also uses this token, preserve caller cancellation for buffered results with a separate lifetime.
|
|
||
| // 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(); |
There was a problem hiding this comment.
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.
No description provided.