diff --git a/extensions/copilot/src/extension/review/node/doReview.ts b/extensions/copilot/src/extension/review/node/doReview.ts index 219057205246..2c1b6eb3e7e8 100644 --- a/extensions/copilot/src/extension/review/node/doReview.ts +++ b/extensions/copilot/src/extension/review/node/doReview.ts @@ -81,7 +81,7 @@ export class ReviewSession { } } -function combineCancellationTokens(token1: CancellationToken, token2: CancellationToken): CancellationToken { +export function combineCancellationTokens(token1: CancellationToken, token2: CancellationToken): CancellationToken { const combinedSource = new CancellationTokenSource(); const subscription1 = token1.onCancellationRequested(() => { diff --git a/extensions/copilot/src/extension/review/node/githubReviewAgent.ts b/extensions/copilot/src/extension/review/node/githubReviewAgent.ts index 17b3bb69c02a..672d7fece7cb 100644 --- a/extensions/copilot/src/extension/review/node/githubReviewAgent.ts +++ b/extensions/copilot/src/extension/review/node/githubReviewAgent.ts @@ -208,7 +208,7 @@ export async function githubReview( return { type: 'success', comments, excludedComments, reason: unsupportedLanguages.length ? l10n.t('Some of the submitted languages are currently not supported: {0}', unsupportedLanguages.join(', ')) : undefined }; } -function createReviewComment(ghComment: ResponseComment | ExcludedComment, request: ReviewRequest, document: TextDocument, index: number) { +export function createReviewComment(ghComment: ResponseComment | ExcludedComment, request: ReviewRequest, document: TextDocument, index: number) { const fromLine = document.lineAt(ghComment.data.line - 1); const lastNonWhitespaceCharacterIndex = fromLine.text.trimEnd().length; const range = new Range(fromLine.lineNumber, fromLine.firstNonWhitespaceCharacterIndex, fromLine.lineNumber, lastNonWhitespaceCharacterIndex); @@ -245,7 +245,7 @@ function createReviewComment(ghComment: ResponseComment | ExcludedComment, reque } const SUGGESTION_EXPRESSION = /```suggestion(\u0020*(\r\n|\n))((?[\s\S]*?)(\r\n|\n))?```/g; -function removeSuggestion(body: string) { +export function removeSuggestion(body: string) { const suggestions: string[] = []; const content = body.replaceAll(SUGGESTION_EXPRESSION, (_match, _ws, _nl, suggestion) => { if (suggestion) { @@ -291,9 +291,9 @@ interface FileState { // } // } -type ResponseReference = ResponseComment | ExcludedComment | ExcludedFile | { type: 'unknown' }; +export type ResponseReference = ResponseComment | ExcludedComment | ExcludedFile | { type: 'unknown' }; -interface ResponseComment { +export interface ResponseComment { type: 'github.generated-pull-request-comment'; data: { // The path of the file @@ -306,7 +306,7 @@ interface ResponseComment { }; } -interface ExcludedComment { +export interface ExcludedComment { type: 'github.excluded-pull-request-comment'; data: { path: string; @@ -317,7 +317,7 @@ interface ExcludedComment { }; } -interface ExcludedFile { +export interface ExcludedFile { type: 'github.excluded-file'; data: { file_path: string; @@ -326,7 +326,7 @@ interface ExcludedFile { }; } -function parseLine(line: string): ResponseReference[] { +export function parseLine(line: string): ResponseReference[] { if (line === 'data: [DONE]') { return []; } if (line === '') { return []; } @@ -428,19 +428,19 @@ async function fetchComments(logService: ILogService, authService: IAuthenticati }; } -function reversePatch(after: string, diff: string) { +export function reversePatch(after: string, diff: string) { const patch = parsePatch(diff.split(/\r?\n/)); const patchedLines = reverseParsedPatch(after.split(/\r?\n/), patch); return patchedLines.join('\n'); } -interface LineChange { +export interface LineChange { beforeLineNumber: number; content: string; type: 'add' | 'remove'; } -function parsePatch(patchLines: string[]): LineChange[] { +export function parsePatch(patchLines: string[]): LineChange[] { const changes: LineChange[] = []; let beforeLineNumber = -1; @@ -465,7 +465,7 @@ function parsePatch(patchLines: string[]): LineChange[] { return changes; } -function reverseParsedPatch(fileLines: string[], patch: LineChange[]): string[] { +export function reverseParsedPatch(fileLines: string[], patch: LineChange[]): string[] { for (const change of patch) { if (change.type === 'add') { fileLines.splice(change.beforeLineNumber - 1, 1); @@ -477,7 +477,7 @@ function reverseParsedPatch(fileLines: string[], patch: LineChange[]): string[] return fileLines; } -interface CodingGuideline { +export interface CodingGuideline { type: string; id: string; data: { @@ -489,7 +489,7 @@ interface CodingGuideline { }; } -async function loadCustomInstructions(customInstructionsService: ICustomInstructionsService, workspaceService: IWorkspaceService, kind: 'selection' | 'diff', languageIdToFilePatterns: Map>, firstId: number): Promise { +export async function loadCustomInstructions(customInstructionsService: ICustomInstructionsService, workspaceService: IWorkspaceService, kind: 'selection' | 'diff', languageIdToFilePatterns: Map>, firstId: number): Promise { const customInstructionRefs = []; let nextId = firstId; diff --git a/extensions/copilot/src/extension/review/node/test/doReview.spec.ts b/extensions/copilot/src/extension/review/node/test/doReview.spec.ts new file mode 100644 index 000000000000..e2949ce3c37d --- /dev/null +++ b/extensions/copilot/src/extension/review/node/test/doReview.spec.ts @@ -0,0 +1,73 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { describe, suite, test } from 'vitest'; +import { CancellationTokenSource } from '../../../../util/vs/base/common/cancellation'; +import { combineCancellationTokens } from '../doReview'; + +suite('doReview', () => { + + describe('combineCancellationTokens', () => { + + test('returns token that is not cancelled when both inputs are not cancelled', () => { + const source1 = new CancellationTokenSource(); + const source2 = new CancellationTokenSource(); + try { + const combined = combineCancellationTokens(source1.token, source2.token); + assert.strictEqual(combined.isCancellationRequested, false); + } finally { + source1.dispose(); + source2.dispose(); + } + }); + + test('cancels combined token when first token is cancelled after creation', () => { + const source1 = new CancellationTokenSource(); + const source2 = new CancellationTokenSource(); + try { + const combined = combineCancellationTokens(source1.token, source2.token); + assert.strictEqual(combined.isCancellationRequested, false); + source1.cancel(); + assert.strictEqual(combined.isCancellationRequested, true); + } finally { + source1.dispose(); + source2.dispose(); + } + }); + + test('cancels combined token when second token is cancelled after creation', () => { + const source1 = new CancellationTokenSource(); + const source2 = new CancellationTokenSource(); + try { + const combined = combineCancellationTokens(source1.token, source2.token); + assert.strictEqual(combined.isCancellationRequested, false); + source2.cancel(); + assert.strictEqual(combined.isCancellationRequested, true); + } finally { + source1.dispose(); + source2.dispose(); + } + }); + + test('only cancels combined token once when both tokens are cancelled', () => { + const source1 = new CancellationTokenSource(); + const source2 = new CancellationTokenSource(); + try { + const combined = combineCancellationTokens(source1.token, source2.token); + let cancelCount = 0; + combined.onCancellationRequested(() => cancelCount++); + + source1.cancel(); + source2.cancel(); + // The combined token should only fire once despite both being cancelled + assert.strictEqual(cancelCount, 1); + } finally { + source1.dispose(); + source2.dispose(); + } + }); + }); +}); diff --git a/extensions/copilot/src/extension/review/node/test/githubReviewAgent.spec.ts b/extensions/copilot/src/extension/review/node/test/githubReviewAgent.spec.ts new file mode 100644 index 000000000000..7278012c53df --- /dev/null +++ b/extensions/copilot/src/extension/review/node/test/githubReviewAgent.spec.ts @@ -0,0 +1,1187 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { describe, suite, test } from 'vitest'; +import type { TextDocument } from 'vscode'; +import { IAuthenticationService } from '../../../../platform/authentication/common/authentication'; +import { CopilotToken, createTestExtendedTokenInfo } from '../../../../platform/authentication/common/copilotToken'; +import { ICustomInstructionsService } from '../../../../platform/customInstructions/common/customInstructionsService'; +import { ICAPIClientService } from '../../../../platform/endpoint/common/capiClient'; +import { IDomainService } from '../../../../platform/endpoint/common/domainService'; +import { IEnvService } from '../../../../platform/env/common/envService'; +import { IGitExtensionService } from '../../../../platform/git/common/gitExtensionService'; +import { NullGitExtensionService } from '../../../../platform/git/common/nullGitExtensionService'; +import { IIgnoreService, NullIgnoreService } from '../../../../platform/ignore/common/ignoreService'; +import { MockAuthenticationService } from '../../../../platform/ignore/node/test/mockAuthenticationService'; +import { MockCAPIClientService } from '../../../../platform/ignore/node/test/mockCAPIClientService'; +import { MockWorkspaceService } from '../../../../platform/ignore/node/test/mockWorkspaceService'; +import { IFetcherService } from '../../../../platform/networking/common/fetcherService'; +import { ReviewComment, ReviewRequest } from '../../../../platform/review/common/reviewService'; +import { MockCustomInstructionsService } from '../../../../platform/test/common/testCustomInstructionsService'; +import { createFakeStreamResponse } from '../../../../platform/test/node/fetcher'; +import { TestLogService } from '../../../../platform/testing/common/testLogService'; +import { IWorkspaceService } from '../../../../platform/workspace/common/workspaceService'; +import { createTextDocumentData } from '../../../../util/common/test/shims/textDocument'; +import { CancellationToken } from '../../../../util/vs/base/common/cancellation'; +import { Event } from '../../../../util/vs/base/common/event'; +import { URI } from '../../../../util/vs/base/common/uri'; +import { + createReviewComment, + ExcludedComment, + LineChange, + loadCustomInstructions, + parseLine, + parsePatch, + removeSuggestion, + ResponseComment, + reverseParsedPatch, + reversePatch +} from '../githubReviewAgent'; + +suite('githubReviewAgent', () => { + + describe('parseLine', () => { + + test('returns empty array for empty line', () => { + const result = parseLine(''); + assert.deepStrictEqual(result, []); + }); + + test('returns empty array for DONE marker', () => { + const result = parseLine('data: [DONE]'); + assert.deepStrictEqual(result, []); + }); + + test('returns empty array when no copilot_references', () => { + const result = parseLine('data: {"choices":[]}'); + assert.deepStrictEqual(result, []); + }); + + test('returns empty array when copilot_references is empty', () => { + const result = parseLine('data: {"copilot_references":[]}'); + assert.deepStrictEqual(result, []); + }); + + test('parses generated pull request comment', () => { + const data = { + copilot_references: [{ + type: 'github.generated-pull-request-comment', + data: { + path: 'src/file.ts', + line: 10, + body: 'This is a bug' + } + }] + }; + const result = parseLine(`data: ${JSON.stringify(data)}`); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'github.generated-pull-request-comment'); + if (result[0].type === 'github.generated-pull-request-comment') { + assert.strictEqual(result[0].data.path, 'src/file.ts'); + assert.strictEqual(result[0].data.line, 10); + assert.strictEqual(result[0].data.body, 'This is a bug'); + } + }); + + test('parses excluded pull request comment', () => { + const data = { + copilot_references: [{ + type: 'github.excluded-pull-request-comment', + data: { + path: 'src/file.ts', + line: 5, + body: 'Low confidence comment', + exclusion_reason: 'denylisted_type' + } + }] + }; + const result = parseLine(`data: ${JSON.stringify(data)}`); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'github.excluded-pull-request-comment'); + }); + + test('parses excluded file reference', () => { + const data = { + copilot_references: [{ + type: 'github.excluded-file', + data: { + file_path: 'src/file.txt', + language: 'plaintext', + reason: 'file_type_not_supported' + } + }] + }; + const result = parseLine(`data: ${JSON.stringify(data)}`); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'github.excluded-file'); + }); + + test('parses multiple references in single line', () => { + const data = { + copilot_references: [ + { + type: 'github.generated-pull-request-comment', + data: { path: 'a.ts', line: 1, body: 'Comment 1' } + }, + { + type: 'github.generated-pull-request-comment', + data: { path: 'b.ts', line: 2, body: 'Comment 2' } + } + ] + }; + const result = parseLine(`data: ${JSON.stringify(data)}`); + + assert.strictEqual(result.length, 2); + }); + + test('filters out references without type', () => { + const data = { + copilot_references: [ + { type: 'github.generated-pull-request-comment', data: { path: 'a.ts', line: 1, body: 'Valid' } }, + { data: { path: 'b.ts', line: 2, body: 'No type field' } } + ] + }; + const result = parseLine(`data: ${JSON.stringify(data)}`); + + assert.strictEqual(result.length, 1); + }); + }); + + describe('removeSuggestion', () => { + + test('returns original content when no suggestion block', () => { + const body = 'This is a regular comment without suggestions.'; + const result = removeSuggestion(body); + + assert.strictEqual(result.content, body); + assert.deepStrictEqual(result.suggestions, []); + }); + + test('extracts single suggestion and removes block', () => { + const body = 'Fix the typo.\n```suggestion\nconst fixed = true;\n```'; + const result = removeSuggestion(body); + + assert.strictEqual(result.content, 'Fix the typo.\n'); + // The regex captures content including the trailing newline before ``` + assert.deepStrictEqual(result.suggestions, ['const fixed = true;\n']); + }); + + test('extracts multiple suggestions', () => { + const body = 'First issue.\n```suggestion\nfix1\n```\nSecond issue.\n```suggestion\nfix2\n```'; + const result = removeSuggestion(body); + + assert.strictEqual(result.suggestions.length, 2); + // The regex captures content including the trailing newline before ``` + assert.strictEqual(result.suggestions[0], 'fix1\n'); + assert.strictEqual(result.suggestions[1], 'fix2\n'); + }); + + test('handles suggestion with CRLF line endings', () => { + const body = 'Fix.\r\n```suggestion\r\nconst x = 1;\r\n```'; + const result = removeSuggestion(body); + + // The regex captures content including the trailing CRLF before ``` + assert.deepStrictEqual(result.suggestions, ['const x = 1;\r\n']); + }); + + test('handles empty suggestion block', () => { + const body = 'Remove this line.\n```suggestion\n```'; + const result = removeSuggestion(body); + + assert.strictEqual(result.content, 'Remove this line.\n'); + assert.deepStrictEqual(result.suggestions, []); + }); + + test('handles suggestion with trailing spaces after keyword', () => { + const body = 'Fix.\n```suggestion \ncode here\n```'; + const result = removeSuggestion(body); + + // The regex captures content including the trailing newline before ``` + assert.deepStrictEqual(result.suggestions, ['code here\n']); + }); + + test('preserves non-suggestion code blocks', () => { + const body = 'Example:\n```typescript\nconst x = 1;\n```\nDone.'; + const result = removeSuggestion(body); + + assert.strictEqual(result.content, body); + assert.deepStrictEqual(result.suggestions, []); + }); + }); + + describe('parsePatch', () => { + + test('returns empty array for empty input', () => { + const result = parsePatch([]); + assert.deepStrictEqual(result, []); + }); + + test('parses single addition', () => { + const patchLines = [ + '@@ -1,3 +1,4 @@', + ' line1', + '+added line', + ' line2', + ' line3' + ]; + const result = parsePatch(patchLines); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'add'); + assert.strictEqual(result[0].content, 'added line'); + assert.strictEqual(result[0].beforeLineNumber, 2); + }); + + test('parses single deletion', () => { + const patchLines = [ + '@@ -1,4 +1,3 @@', + ' line1', + '-deleted line', + ' line2', + ' line3' + ]; + const result = parsePatch(patchLines); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'remove'); + assert.strictEqual(result[0].content, 'deleted line'); + assert.strictEqual(result[0].beforeLineNumber, 2); + }); + + test('parses mixed additions and deletions', () => { + const patchLines = [ + '@@ -1,3 +1,3 @@', + ' line1', + '-old line', + '+new line', + ' line3' + ]; + const result = parsePatch(patchLines); + + assert.strictEqual(result.length, 2); + assert.strictEqual(result[0].type, 'remove'); + assert.strictEqual(result[0].content, 'old line'); + assert.strictEqual(result[1].type, 'add'); + assert.strictEqual(result[1].content, 'new line'); + }); + + test('parses multiple hunks', () => { + const patchLines = [ + '@@ -1,2 +1,3 @@', + ' line1', + '+added1', + '@@ -10,2 +11,3 @@', + ' line10', + '+added2' + ]; + const result = parsePatch(patchLines); + + assert.strictEqual(result.length, 2); + assert.strictEqual(result[0].beforeLineNumber, 2); + assert.strictEqual(result[1].beforeLineNumber, 11); + }); + + test('ignores lines before first hunk header', () => { + const patchLines = [ + 'diff --git a/file.ts b/file.ts', + 'index abc..def 100644', + '--- a/file.ts', + '+++ b/file.ts', + '@@ -1,2 +1,3 @@', + ' context', + '+added' + ]; + const result = parsePatch(patchLines); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].content, 'added'); + }); + }); + + describe('reverseParsedPatch', () => { + + test('returns original lines when patch is empty', () => { + const lines = ['line1', 'line2', 'line3']; + const result = reverseParsedPatch([...lines], []); + + assert.deepStrictEqual(result, lines); + }); + + test('reverses an addition by removing the line', () => { + const afterLines = ['line1', 'added', 'line2']; + const patch: LineChange[] = [ + { beforeLineNumber: 2, content: 'added', type: 'add' } + ]; + const result = reverseParsedPatch([...afterLines], patch); + + assert.deepStrictEqual(result, ['line1', 'line2']); + }); + + test('reverses a deletion by re-adding the line', () => { + const afterLines = ['line1', 'line3']; + const patch: LineChange[] = [ + { beforeLineNumber: 2, content: 'line2', type: 'remove' } + ]; + const result = reverseParsedPatch([...afterLines], patch); + + assert.deepStrictEqual(result, ['line1', 'line2', 'line3']); + }); + + test('reverses a replacement (delete then add)', () => { + // After state: ['line1', 'new', 'line3'] where 'old' was replaced with 'new' + // parsePatch would produce a delete at line 2 and an add at line 3: + // -old => { beforeLineNumber: 2, content: 'old', type: 'remove' } + // +new => { beforeLineNumber: 3, content: 'new', type: 'add' } + // reverseParsedPatch should reconstruct the original ['line1', 'old', 'line3'] + const afterLines = ['line1', 'new', 'line3']; + const patch: LineChange[] = [ + { beforeLineNumber: 2, content: 'old', type: 'remove' }, + { beforeLineNumber: 3, content: 'new', type: 'add' } + ]; + const result = reverseParsedPatch([...afterLines], patch); + + assert.deepStrictEqual(result, ['line1', 'old', 'line3']); + }); + }); + + describe('reversePatch', () => { + + test('reverses simple addition', () => { + const after = 'line1\nadded\nline2'; + const diff = '@@ -1,2 +1,3 @@\n line1\n+added\n line2'; + + const result = reversePatch(after, diff); + + assert.strictEqual(result, 'line1\nline2'); + }); + + test('reverses simple deletion', () => { + const after = 'line1\nline3'; + const diff = '@@ -1,3 +1,2 @@\n line1\n-line2\n line3'; + + const result = reversePatch(after, diff); + + assert.strictEqual(result, 'line1\nline2\nline3'); + }); + + test('reverses replacement', () => { + const after = 'line1\nnew\nline3'; + const diff = '@@ -1,3 +1,3 @@\n line1\n-old\n+new\n line3'; + + const result = reversePatch(after, diff); + + assert.strictEqual(result, 'line1\nold\nline3'); + }); + + test('handles CRLF in after content', () => { + const after = 'line1\r\nadded\r\nline2'; + const diff = '@@ -1,2 +1,3 @@\n line1\n+added\n line2'; + + const result = reversePatch(after, diff); + + assert.strictEqual(result, 'line1\nline2'); + }); + + test('handles empty diff', () => { + const after = 'line1\nline2'; + const diff = ''; + + const result = reversePatch(after, diff); + + assert.strictEqual(result, 'line1\nline2'); + }); + }); + + describe('createReviewComment', () => { + + function createTestRequest(overrides?: Partial): ReviewRequest { + return { + source: 'githubReviewAgent', + promptCount: 1, + messageId: 'test-message-id', + inputType: 'change', + inputRanges: [], + ...overrides, + }; + } + + test('creates comment with correct range from line number', () => { + const docData = createTextDocumentData( + URI.file('/test/file.ts'), + 'line1\n indented line\nline3', + 'typescript' + ); + const ghComment: ResponseComment = { + type: 'github.generated-pull-request-comment', + data: { + path: 'file.ts', + line: 2, + body: 'This line has an issue.' + } + }; + const request = createTestRequest(); + + const comment = createReviewComment(ghComment, request, docData.document, 0); + + assert.strictEqual(comment.range.start.line, 1); // 0-indexed + assert.strictEqual(comment.range.start.character, 4); // firstNonWhitespaceCharacterIndex + assert.strictEqual(comment.range.end.line, 1); + assert.strictEqual(comment.languageId, 'typescript'); + assert.strictEqual(comment.originalIndex, 0); + assert.strictEqual(comment.kind, 'bug'); + assert.strictEqual(comment.severity, 'medium'); + }); + + test('extracts suggestion from body and creates edit', () => { + const docData = createTextDocumentData( + URI.file('/test/file.ts'), + 'const x = 1;\nconst y = 2;\nconst z = 3;', + 'typescript' + ); + const ghComment: ResponseComment = { + type: 'github.generated-pull-request-comment', + data: { + path: 'file.ts', + line: 2, + body: 'Fix the variable name.\n```suggestion\nconst fixedY = 2;\n```' + } + }; + const request = createTestRequest(); + + const comment = createReviewComment(ghComment, request, docData.document, 0); + + // Body should have suggestion removed - body is MarkdownString in this case + const bodyValue = typeof comment.body === 'string' ? comment.body : comment.body.value; + assert.strictEqual(bodyValue, 'Fix the variable name.\n'); + // Should have one edit suggestion + assert.ok(comment.suggestion); + assert.ok(!('then' in comment.suggestion)); // Not a promise + const suggestion = comment.suggestion as { edits: { newText: string }[] }; + assert.strictEqual(suggestion.edits.length, 1); + assert.strictEqual(suggestion.edits[0].newText, 'const fixedY = 2;\n'); + }); + + test('handles comment with start_line for multi-line range', () => { + const docData = createTextDocumentData( + URI.file('/test/file.ts'), + 'line1\nline2\nline3\nline4', + 'typescript' + ); + const ghComment: ResponseComment = { + type: 'github.generated-pull-request-comment', + data: { + path: 'file.ts', + line: 3, + start_line: 2, + body: 'Multi-line issue.\n```suggestion\nreplacement\n```' + } + }; + const request = createTestRequest(); + + const comment = createReviewComment(ghComment, request, docData.document, 1); + + // Suggestion range should span from start_line to line + assert.ok(comment.suggestion); + assert.ok(!('then' in comment.suggestion)); // Not a promise + const suggestion = comment.suggestion as { edits: { range: { start: { line: number }; end: { line: number } } }[] }; + assert.strictEqual(suggestion.edits[0].range.start.line, 1); // start_line - 1 + assert.strictEqual(suggestion.edits[0].range.end.line, 3); // line + assert.strictEqual(comment.originalIndex, 1); + }); + + test('handles excluded comment', () => { + const docData = createTextDocumentData( + URI.file('/test/file.ts'), + 'line1\nline2\nline3', + 'typescript' + ); + const ghComment: ExcludedComment = { + type: 'github.excluded-pull-request-comment', + data: { + path: 'file.ts', + line: 2, + body: 'Low confidence comment.', + exclusion_reason: 'denylisted_type' + } + }; + const request = createTestRequest(); + + const comment = createReviewComment(ghComment, request, docData.document, 0); + + const bodyValue = typeof comment.body === 'string' ? comment.body : comment.body.value; + assert.strictEqual(bodyValue, 'Low confidence comment.'); + assert.strictEqual(comment.range.start.line, 1); + }); + + test('handles comment without suggestion', () => { + const docData = createTextDocumentData( + URI.file('/test/file.ts'), + 'const x = 1;', + 'typescript' + ); + const ghComment: ResponseComment = { + type: 'github.generated-pull-request-comment', + data: { + path: 'file.ts', + line: 1, + body: 'Consider renaming this variable.' + } + }; + const request = createTestRequest(); + + const comment = createReviewComment(ghComment, request, docData.document, 0); + + const bodyValue = typeof comment.body === 'string' ? comment.body : comment.body.value; + assert.strictEqual(bodyValue, 'Consider renaming this variable.'); + assert.ok(comment.suggestion); + assert.ok(!('then' in comment.suggestion)); // Not a promise + const suggestion = comment.suggestion as { edits: unknown[] }; + assert.strictEqual(suggestion.edits.length, 0); + }); + }); + + describe('loadCustomInstructions', () => { + + function createMockWorkspaceService(): IWorkspaceService { + return { + asRelativePath: (uri: URI) => uri.path.split('/').pop() || uri.path + } as IWorkspaceService; + } + + test('returns empty array when no instructions configured', async () => { + const customInstructionsService = new MockCustomInstructionsService(); + const workspaceService = createMockWorkspaceService(); + const languageIdToFilePatterns = new Map>(); + + const result = await loadCustomInstructions( + customInstructionsService, + workspaceService, + 'diff', + languageIdToFilePatterns, + 1 + ); + + assert.deepStrictEqual(result, []); + }); + + test('loads instructions from agent instruction files', async () => { + // Create a custom service that returns agent instructions + const testUri = URI.file('/test/instructions.md'); + const customInstructionsService = { + ...new MockCustomInstructionsService(), + getAgentInstructions: () => Promise.resolve([testUri]), + fetchInstructionsFromFile: (uri: typeof testUri) => Promise.resolve({ + content: [{ instruction: 'Test instruction', languageId: undefined }] + }), + fetchInstructionsFromSetting: () => Promise.resolve([]) + }; + const workspaceService = createMockWorkspaceService(); + const languageIdToFilePatterns = new Map>(); + + const result = await loadCustomInstructions( + customInstructionsService as unknown as ICustomInstructionsService, + workspaceService, + 'selection', + languageIdToFilePatterns, + 1 + ); + + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].type, 'github.coding_guideline'); + assert.strictEqual(result[0].data.description, 'Test instruction'); + assert.deepStrictEqual(result[0].data.filePatterns, ['*']); + }); + + test('loads instructions from settings', async () => { + // Create a custom service that returns settings instructions + const customInstructionsService = { + ...new MockCustomInstructionsService(), + getAgentInstructions: () => Promise.resolve([]), + fetchInstructionsFromFile: () => Promise.resolve(undefined), + fetchInstructionsFromSetting: () => Promise.resolve([{ + content: [{ instruction: 'Settings instruction', languageId: undefined }] + }]) + }; + const workspaceService = createMockWorkspaceService(); + const languageIdToFilePatterns = new Map>(); + + const result = await loadCustomInstructions( + customInstructionsService as unknown as ICustomInstructionsService, + workspaceService, + 'selection', + languageIdToFilePatterns, + 1 + ); + + // CodeGenerationInstructions + CodeFeedbackInstructions for 'selection' kind + // Each setting config will be called, and each returns 1 instruction + assert.ok(result.length >= 1); + assert.strictEqual(result[0].type, 'github.coding_guideline'); + }); + + test('filters instructions by languageId when specified', async () => { + const testUri = URI.file('/test/instructions.md'); + const customInstructionsService = { + ...new MockCustomInstructionsService(), + getAgentInstructions: () => Promise.resolve([testUri]), + fetchInstructionsFromFile: () => Promise.resolve({ + content: [ + { instruction: 'TypeScript only', languageId: 'typescript' }, + { instruction: 'Python only', languageId: 'python' }, + { instruction: 'All languages', languageId: undefined } + ] + }), + fetchInstructionsFromSetting: () => Promise.resolve([]) + }; + const workspaceService = createMockWorkspaceService(); + // Only TypeScript is in the map, so Python instruction should be skipped + const languageIdToFilePatterns = new Map>([ + ['typescript', new Set(['*.ts', '*.tsx'])] + ]); + + const result = await loadCustomInstructions( + customInstructionsService as unknown as ICustomInstructionsService, + workspaceService, + 'selection', + languageIdToFilePatterns, + 1 + ); + + // Should have 2 instructions: TypeScript + All languages (Python skipped) + assert.strictEqual(result.length, 2); + const descriptions = result.map(r => r.data.description); + assert.ok(descriptions.includes('TypeScript only')); + assert.ok(descriptions.includes('All languages')); + assert.ok(!descriptions.includes('Python only')); + + // TypeScript instruction should have specific file patterns + const tsInstruction = result.find(r => r.data.description === 'TypeScript only'); + assert.ok(tsInstruction); + assert.deepStrictEqual(tsInstruction.data.filePatterns.sort(), ['*.ts', '*.tsx']); + }); + + test('filters settings instructions by languageId', async () => { + // Create a custom service that returns settings instructions with languageId + const customInstructionsService = { + ...new MockCustomInstructionsService(), + getAgentInstructions: () => Promise.resolve([]), + fetchInstructionsFromFile: () => Promise.resolve(undefined), + fetchInstructionsFromSetting: () => Promise.resolve([{ + content: [ + { instruction: 'JavaScript rule', languageId: 'javascript' }, + { instruction: 'Ruby rule', languageId: 'ruby' }, + { instruction: 'General rule', languageId: undefined } + ] + }]) + }; + const workspaceService = createMockWorkspaceService(); + // Only JavaScript is in the map, Ruby should be filtered out + const languageIdToFilePatterns = new Map>([ + ['javascript', new Set(['*.js'])] + ]); + + const result = await loadCustomInstructions( + customInstructionsService as unknown as ICustomInstructionsService, + workspaceService, + 'selection', + languageIdToFilePatterns, + 1 + ); + + // JavaScript + General should be included, Ruby filtered out + const descriptions = result.map(r => r.data.description); + assert.ok(descriptions.includes('JavaScript rule')); + assert.ok(descriptions.includes('General rule')); + assert.ok(!descriptions.includes('Ruby rule')); + }); + }); + + describe('githubReview', () => { + // These tests verify the integration of githubReview with mocked services + // Following the pattern from chatMLFetcherRetry.spec.ts for extending mocks + + // Common mock services shared across tests + const createMockFetcherService = (options?: { + isAbortError?: (err: unknown) => boolean; + }): IFetcherService => ({ + makeAbortController: () => ({ abort: () => { }, signal: {} }), + isAbortError: options?.isAbortError ?? (() => false), + } as unknown as IFetcherService); + + const createBaseMocks = () => ({ + domainService: { _serviceBrand: undefined, onDidChangeDomains: Event.None } as IDomainService, + fetcherService: createMockFetcherService(), + envService: { sessionId: 'test' } as IEnvService, + }); + + const createMockGitExtensionService = (): IGitExtensionService => { + const mockGitApi = { + getRepository: () => ({ rootUri: URI.file('/test') }), + repositories: [], + }; + return { + getExtensionApi: () => mockGitApi, + extensionAvailable: true, + } as unknown as IGitExtensionService; + }; + + // Factory for TestAuthenticationService with configurable token options + const createTestAuthenticationService = (tokenOptions?: { + token?: string; + code_review_enabled?: boolean; + }) => { + class TestAuthenticationService extends MockAuthenticationService { + override getCopilotToken(_force?: boolean): Promise { + return Promise.resolve(new CopilotToken(createTestExtendedTokenInfo({ + token: tokenOptions?.token ?? 'test-token', + code_review_enabled: tokenOptions?.code_review_enabled ?? true, + }))); + } + } + return new TestAuthenticationService() as unknown as IAuthenticationService; + }; + + // Factory for TestCAPIClientService with configurable response + const createTestCAPIClientService = (options: { + makeRequest?: () => Promise; + buildUrl?: (ep: unknown, path: string) => URL; + }) => { + class TestCAPIClientService extends MockCAPIClientService { + buildUrl(_ep: unknown, path: string): URL { + return options.buildUrl?.(_ep, path) ?? new URL('https://api.github.com' + path); + } + override makeRequest(): Promise { + if (options.makeRequest) { + return options.makeRequest(); + } + return Promise.resolve({} as T); + } + } + return new TestCAPIClientService() as unknown as ICAPIClientService; + }; + + // Factory for TestWorkspaceService with configurable document handling + const createTestWorkspaceService = (documents: Map) => { + class TestWorkspaceService extends MockWorkspaceService { + override openTextDocument(uri: URI): Promise { + const doc = documents.get(uri.toString()); + if (doc) { + return Promise.resolve(doc); + } + return Promise.reject(new Error(`Document not found: ${uri.toString()}`)); + } + override asRelativePath(uri: URI): string { + return uri.path.replace(/^\/test\//, ''); + } + } + return new TestWorkspaceService(); + }; + + // Helper to create a document map for TestWorkspaceService + const createDocumentMap = (files: Array<{ uri: URI; content: string; languageId: string }>) => { + const map = new Map(); + for (const file of files) { + const docData = createTextDocumentData(file.uri, file.content, file.languageId); + map.set(file.uri.toString(), docData.document); + } + return map; + }; + + test('returns success with empty comments when git extension is not available', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + const result = await githubReview( + new TestLogService(), + new NullGitExtensionService(), + new MockAuthenticationService() as unknown as IAuthenticationService, + new MockCAPIClientService() as unknown as ICAPIClientService, + domainService, + fetcherService, + envService, + new NullIgnoreService(), + new MockWorkspaceService(), + new MockCustomInstructionsService(), + { repositoryRoot: '/test', commitMessages: [], patches: [] }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + + assert.strictEqual(result.type, 'success'); + if (result.type === 'success') { + assert.deepStrictEqual(result.comments, []); + } + }); + + test('returns success with empty comments when no patches provided', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + const result = await githubReview( + new TestLogService(), + createMockGitExtensionService(), + new MockAuthenticationService() as unknown as IAuthenticationService, + new MockCAPIClientService() as unknown as ICAPIClientService, + domainService, + fetcherService, + envService, + new NullIgnoreService(), + new MockWorkspaceService(), + new MockCustomInstructionsService(), + { repositoryRoot: '/test', commitMessages: [], patches: [] }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + + assert.strictEqual(result.type, 'success'); + if (result.type === 'success') { + assert.deepStrictEqual(result.comments, []); + } + }); + + test('processes patches and returns review comments from API response', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + // Set up CAPI client to return a streaming response with a comment + const sseResponse = [ + `data: ${JSON.stringify({ + copilot_references: [{ + type: 'github.generated-pull-request-comment', + data: { + path: 'file.ts', + line: 1, + body: 'Consider using const instead of let.' + } + }] + })}\n`, + 'data: [DONE]\n' + ]; + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'let x = 1;', languageId: 'typescript' }]); + + const reportedComments: ReviewComment[] = []; + const progress = { + report: (comments: ReviewComment[]) => reportedComments.push(...comments) + }; + + const result = await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService({ code_review_enabled: true }), + createTestCAPIClientService({ + makeRequest: () => Promise.resolve(createFakeStreamResponse(sseResponse) as unknown as T) + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + progress, + CancellationToken.None + ); + + assert.strictEqual(result.type, 'success'); + if (result.type === 'success') { + assert.strictEqual(result.comments.length, 1); + assert.strictEqual(reportedComments.length, 1); + } + }); + + test('returns info error when all files are ignored', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + // Create an ignore service that ignores all files + const ignoreService = { + isCopilotIgnored: () => Promise.resolve(true), + }; + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'let x = 1;', languageId: 'typescript' }]); + + const result = await githubReview( + new TestLogService(), + createMockGitExtensionService(), + new MockAuthenticationService() as unknown as IAuthenticationService, + new MockCAPIClientService() as unknown as ICAPIClientService, + domainService, + fetcherService, + envService, + ignoreService as unknown as IIgnoreService, + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: [], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + + assert.strictEqual(result.type, 'error'); + if (result.type === 'error') { + assert.strictEqual(result.severity, 'info'); + assert.ok(result.reason.includes('ignored')); + } + }); + + test('handles cancelled request via abort signal', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, envService } = createBaseMocks(); + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'const x = 1;', languageId: 'typescript' }]); + + // Mock fetcher with abort support + const abortError = new Error('Aborted'); + const fetcherService = createMockFetcherService({ + isAbortError: (err: unknown) => err === abortError, + }); + + const result = await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService(), + createTestCAPIClientService({ + makeRequest: () => Promise.reject(abortError) as Promise, + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + + // When aborted, should return cancelled + assert.strictEqual(result.type, 'cancelled'); + }); + + test('handles HTTP 402 quota exceeded error', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'const x = 1;', languageId: 'typescript' }]); + + try { + await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService(), + createTestCAPIClientService({ + makeRequest: () => Promise.resolve({ + ok: false, + status: 402, + headers: { get: (name: string) => name === 'x-github-request-id' ? 'test-req-id' : null }, + } as unknown as T), + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + assert.fail('Should have thrown an error'); + } catch (err: unknown) { + const error = err as Error & { severity?: string }; + assert.ok(error.message.includes('quota')); + assert.strictEqual(error.severity, 'info'); + } + }); + + test('handles HTTP error response', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'const x = 1;', languageId: 'typescript' }]); + + try { + await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService(), + createTestCAPIClientService({ + makeRequest: () => Promise.resolve({ + ok: false, + status: 500, + headers: { get: (name: string) => name === 'x-github-request-id' ? 'test-req-id' : null }, + } as unknown as T), + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + assert.fail('Should have thrown an error'); + } catch (err: unknown) { + const error = err as Error; + assert.ok(error.message.includes('500')); + assert.ok(error.message.includes('test-req-id')); + } + }); + + test('propagates non-abort fetch errors', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, envService } = createBaseMocks(); + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'const x = 1;', languageId: 'typescript' }]); + + // Mock fetcher that does NOT recognize this error as abort + const networkError = new Error('Network failure'); + const fetcherService = createMockFetcherService({ + isAbortError: () => false, // Not an abort error + }); + + try { + await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService(), + createTestCAPIClientService({ + makeRequest: () => Promise.reject(networkError) as Promise, + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + assert.fail('Should have thrown an error'); + } catch (err: unknown) { + const error = err as Error; + assert.strictEqual(error.message, 'Network failure'); + } + }); + + test('ignores comments with paths not matching any change', async () => { + const { githubReview } = await import('../githubReviewAgent'); + const { domainService, fetcherService, envService } = createBaseMocks(); + + const fileUri = URI.file('/test/file.ts'); + const documents = createDocumentMap([{ uri: fileUri, content: 'const x = 1;', languageId: 'typescript' }]); + + // Response contains a comment for a different file - use proper SSE format + const sseResponse = [ + `data: ${JSON.stringify({ + copilot_references: [{ + type: 'github.generated-pull-request-comment', + data: { + path: 'other-file.ts', // Different from file.ts + line: 1, + body: 'Comment on non-existent file' + } + }] + })}\n`, + 'data: [DONE]\n' + ]; + + const result = await githubReview( + new TestLogService(), + createMockGitExtensionService(), + createTestAuthenticationService(), + createTestCAPIClientService({ + makeRequest: () => Promise.resolve(createFakeStreamResponse(sseResponse) as unknown as T), + }), + domainService, + fetcherService, + envService, + new NullIgnoreService(), + createTestWorkspaceService(documents), + new MockCustomInstructionsService(), + { + repositoryRoot: '/test', + commitMessages: ['test commit'], + patches: [{ + patch: '@@ -1,1 +1,1 @@\n-const x = 1;\n+let x = 1;', + fileUri: fileUri.toString(), + }] + }, + undefined, + { report: () => { } }, + CancellationToken.None + ); + + // Should succeed but with no comments (the mismatched path comment is skipped) + assert.strictEqual(result.type, 'success'); + if (result.type === 'success') { + assert.strictEqual(result.comments.length, 0); + } + }); + }); +});