diff --git a/package.json b/package.json index 5039b74b71..d077496338 100644 --- a/package.json +++ b/package.json @@ -4317,6 +4317,15 @@ "experimental" ] }, + "github.copilot.chat.autopilot.codeReview.enabled": { + "type": "boolean", + "default": false, + "markdownDescription": "%github.copilot.config.autopilot.codeReview.enabled%", + "tags": [ + "advanced", + "experimental" + ] + }, "github.copilot.chat.agent.largeToolResultsToDisk.enabled": { "type": "boolean", "default": true, diff --git a/package.nls.json b/package.nls.json index 5ed02879b3..8020ff05c0 100644 --- a/package.nls.json +++ b/package.nls.json @@ -386,6 +386,7 @@ "github.copilot.config.codesearch.agent.enabled": "Enable code search capabilities in agent mode.", "github.copilot.config.agent.temperature": "Temperature setting for agent mode requests.", "github.copilot.config.agent.omitFileAttachmentContents": "Omit summarized file contents from file attachments in agent mode, to encourage the agent to properly read and explore.", + "github.copilot.config.autopilot.codeReview.enabled": "Enable this to automatically run code review on your changes when Autopilot completes a task. Review comments are fed back to the agent so it can address them before finishing.", "github.copilot.config.agent.largeToolResultsToDisk.enabled": "When enabled, large tool results are written to disk instead of being included directly in the context, helping manage context window usage.", "github.copilot.config.agent.largeToolResultsToDisk.thresholdBytes": "The size threshold in bytes above which tool results are written to disk. Only applies when largeToolResultsToDisk.enabled is true.", "github.copilot.config.instantApply.shortContextModelName": "Model name for short context instant apply.", diff --git a/src/extension/intents/node/toolCallingLoop.ts b/src/extension/intents/node/toolCallingLoop.ts index 593b417931..4ad2fe846d 100644 --- a/src/extension/intents/node/toolCallingLoop.ts +++ b/src/extension/intents/node/toolCallingLoop.ts @@ -24,6 +24,7 @@ import { OpenAIContextManagementResponse } from '../../../platform/networking/co import { CopilotChatAttr, emitAgentTurnEvent, emitSessionStartEvent, GenAiAttr, GenAiMetrics, GenAiOperationName, GenAiProviderName, resolveWorkspaceOTelMetadata, StdAttr, truncateForOTel, workspaceMetadataToOTelAttributes } from '../../../platform/otel/common/index'; import { IOTelService, SpanKind, SpanStatusCode } from '../../../platform/otel/common/otelService'; import { getCurrentCapturingToken, IRequestLogger } from '../../../platform/requestLogger/node/requestLogger'; +import { CodeReviewComment, CodeReviewFileInput, CodeReviewInput } from '../../../platform/review/common/reviewCommand'; import { IExperimentationService } from '../../../platform/telemetry/common/nullExperimentationService'; import { ITelemetryService } from '../../../platform/telemetry/common/telemetry'; import { computePromptTokenDetails } from '../../../platform/tokenizer/node/promptTokenDetails'; @@ -49,6 +50,7 @@ import { PseudoStopStartResponseProcessor } from '../../prompt/node/pseudoStartS import { ResponseProcessorContext } from '../../prompt/node/responseProcessorContext'; import { SummarizedConversationHistoryMetadata } from '../../prompts/node/agent/summarizedConversationHistory'; import { ToolFailureEncountered, ToolResultMetadata } from '../../prompts/node/panel/toolCalling'; +import { reviewFileChanges } from '../../review/node/doReview'; import { ToolName } from '../../tools/common/toolNames'; import { IToolsService, ToolCallCancelledError } from '../../tools/common/toolsService'; import { ReadFileParams } from '../../tools/node/readFileTool'; @@ -166,12 +168,29 @@ export abstract class ToolCallingLoop = new Set([ + ToolName.EditFile, + ToolName.ReplaceString, + ToolName.MultiReplaceString, + ToolName.ApplyPatch, + ToolName.CreateFile, + ToolName.EditNotebook, + ]); + private toolCallResults: Record = Object.create(null); private toolCallRounds: IToolCallRound[] = []; private stopHookReason: string | undefined; private additionalHookContext: string | undefined; private stopHookUserInitiated = false; + /** + * Stores file content snapshots captured *before* autopilot's first edit to each file. + * Used by {@link performAutopilotCodeReview} so that the review only covers what + * autopilot changed, not pre-existing uncommitted changes. + */ + private readonly preEditSnapshots = new Map(); + public appendAdditionalHookContext(context: string): void { if (!context) { return; @@ -350,6 +369,7 @@ export abstract class ToolCallingLoop | undefined; /** @@ -441,6 +461,205 @@ export abstract class ToolCallingLoop round.toolCalls.some(tc => ToolCallingLoop.EDIT_TOOL_NAMES.has(tc.name)) + ); + } + + /** + * Extracts deduplicated file paths from a single round's edit tool calls. + */ + private static extractEditedFilePathsFromRound(round: IToolCallRound): string[] { + const paths = new Set(); + for (const tc of round.toolCalls) { + if (!ToolCallingLoop.EDIT_TOOL_NAMES.has(tc.name)) { + continue; + } + try { + const args = JSON.parse(tc.arguments); + if (tc.name === ToolName.MultiReplaceString) { + const replacements: { filePath?: string }[] = args.replacements ?? []; + for (const r of replacements) { + if (r.filePath) { + paths.add(r.filePath); + } + } + } else if (tc.name === ToolName.ApplyPatch) { + const patchText: string = args.input ?? ''; + const headerRe = /\*\*\*\s+(?:Add|Update|Delete)\s+File:\s+(.+)/g; + let m: RegExpExecArray | null; + while ((m = headerRe.exec(patchText)) !== null) { + paths.add(m[1].trim()); + } + } else if (args.filePath) { + paths.add(args.filePath); + } + } catch { + // malformed arguments — skip + } + } + return [...paths]; + } + + /** + * Scans tool call rounds for code-editing tools and extracts the deduplicated + * set of file paths that were edited. + */ + protected getEditedFilePaths(): URI[] { + const paths = new Set(); + for (const round of this.toolCallRounds) { + for (const p of ToolCallingLoop.extractEditedFilePathsFromRound(round)) { + paths.add(p); + } + } + return [...paths].map(p => URI.file(p)); + } + + /** + * After a round's tool calls are known but before they execute (next iteration), + * capture the current file content so we have a pre-edit snapshot for code review. + * Only captures each file once — the first snapshot represents the state before + * autopilot's first edit to that file. + */ + private async capturePreEditSnapshots(round: IToolCallRound): Promise { + if (this.options.request.permissionLevel !== 'autopilot') { + return; + } + const filePaths = ToolCallingLoop.extractEditedFilePathsFromRound(round); + const newPaths = filePaths.filter(p => !this.preEditSnapshots.has(p)); + if (!newPaths.length) { + return; + } + await Promise.all(newPaths.map(async filePath => { + try { + const bytes = await this._fileSystemService.readFile(URI.file(filePath)); + if (!this.preEditSnapshots.has(filePath)) { + this.preEditSnapshots.set(filePath, new TextDecoder().decode(bytes)); + } + } catch { + // File doesn't exist yet (e.g. create_file) — record empty string + if (!this.preEditSnapshots.has(filePath)) { + this.preEditSnapshots.set(filePath, ''); + } + } + })); + } + + /** + * Formats code review comments into a continuation message for the model. + */ + private formatReviewCommentsForModel(comments: readonly CodeReviewComment[]): string { + const lines: string[] = [ + 'Code review found the following issues with your changes. Please address each one:\n', + ]; + for (const comment of comments) { + const file = comment.uri.fsPath; + const startLine = comment.range.start.line + 1; + const endLine = comment.range.end.line + 1; + const loc = startLine === endLine ? `line ${startLine}` : `lines ${startLine}-${endLine}`; + lines.push(`- **${file}** (${loc}) [${comment.severity}]: ${comment.body}`); + } + lines.push( + '', + 'After addressing all review comments, call task_complete with a brief summary of what was fixed.', + ); + return lines.join('\n'); + } + + /** + * Runs code review on files edited during autopilot, if applicable. + * Returns `true` if review comments were found and the loop should continue + * so the model can address them. + */ + private async performAutopilotCodeReview( + outputStream: ChatResponseStream | undefined, + token: CancellationToken, + ): Promise { + if (this.options.request.permissionLevel !== 'autopilot') { + return false; + } + if (!this._configurationService.getConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled)) { + return false; + } + if (!this.taskCompleted || this.autopilotCodeReviewCompleted || token.isCancellationRequested) { + return false; + } + if (!this.hadCodeEdits()) { + this._logService.info('[ToolCallingLoop] Autopilot code review: no code edits detected, skipping'); + return false; + } + + this._logService.info('[ToolCallingLoop] Autopilot code review: starting review of edited files'); + + // Mark review as completed so we don't re-run after the fix cycle. + this.autopilotCodeReviewCompleted = true; + + const editedFiles = this.getEditedFilePaths(); + if (!editedFiles.length) { + this._logService.info('[ToolCallingLoop] Autopilot code review: could not extract file paths, skipping'); + return false; + } + + // Show which files are being reviewed. + const fileNames = editedFiles.map(uri => uri.path.split('/').pop() ?? uri.path); + const fileList = fileNames.length <= 3 + ? fileNames.join(', ') + : `${fileNames.slice(0, 3).join(', ')} (+${fileNames.length - 3} more)`; + this.showAutopilotProgress( + outputStream, + l10n.t('Reviewing {0} edited files: {1}', editedFiles.length, fileList), + l10n.t('Reviewed {0} edited files', editedFiles.length), + ); + + try { + // Use pre-edit snapshots captured before each file's first edit so + // the review only covers what autopilot changed in this turn, not + // pre-existing uncommitted changes. + const files: CodeReviewFileInput[] = editedFiles.map(uri => { + const snapshot = this.preEditSnapshots.get(uri.fsPath); + return { currentUri: uri, baseContent: snapshot }; + }); + + const input: CodeReviewInput = { files }; + + const result = await this._instantiationService.invokeFunction( + accessor => reviewFileChanges(accessor, input, token) + ); + + this.resolveAutopilotProgress(); + + if (result.type === 'success' && result.comments.length > 0) { + const affectedFiles = new Set(result.comments.map(c => c.uri.path.split('/').pop() ?? c.uri.path)); + this._logService.info(`[ToolCallingLoop] Autopilot code review: found ${result.comments.length} comments, continuing to address them`); + outputStream?.progress(l10n.t( + 'Found {0} review comments in {1} files — addressing them now', + result.comments.length, + affectedFiles.size, + )); + const continuationMessage = this.formatReviewCommentsForModel(result.comments); + this.stopHookReason = continuationMessage; + this.taskCompleted = false; + this.autopilotStopHookActive = false; + this.autopilotIterationCount = 0; + return true; + } + + this._logService.info(`[ToolCallingLoop] Autopilot code review: ${result.type === 'success' ? 'no comments found' : `skipped (${result.type})`}`); + if (result.type === 'success') { + outputStream?.progress(l10n.t('Code review passed — no issues found')); + } + return false; + } catch (err) { + this._logService.error('[ToolCallingLoop] Autopilot code review failed', err); + this.resolveAutopilotProgress(); + return false; + } + } + /** * Whether the loop should auto-retry after a failed fetch in auto-approve/autopilot mode. * Does not retry rate-limited, quota-exceeded, or cancellation errors. @@ -872,6 +1091,12 @@ export abstract class ToolCallingLoop tc.name === ToolCallingLoop.TASK_COMPLETE_TOOL_NAME)) { @@ -946,6 +1171,12 @@ export abstract class ToolCallingLoop ({ + reviewFileChanges: vi.fn(), +})); + +const mockReviewFileChanges = vi.mocked(reviewFileChanges); + /** * Concrete test implementation that exposes autopilot-related protected methods. */ @@ -69,6 +81,41 @@ class AutopilotTestToolCallingLoop extends ToolCallingLoop { + return (this as any).performAutopilotCodeReview(undefined, token); + } + + /** + * Set the taskCompleted flag for testing. + */ + public setTaskCompleted(value: boolean): void { + (this as any).taskCompleted = value; + } + + /** + * Set a pre-edit snapshot for testing. + */ + public setPreEditSnapshot(filePath: string, content: string): void { + (this as any).preEditSnapshots.set(filePath, content); + } } function createMockChatRequest(overrides: Partial = {}): ChatRequest { @@ -384,4 +431,376 @@ describe('ToolCallingLoop autopilot', () => { expect(result).toBe(tools); }); }); + + describe('hadCodeEdits', () => { + it('should return false when no tool calls were made', () => { + const loop = createLoop('autopilot'); + expect(loop.testHadCodeEdits()).toBe(false); + }); + + it('should return false when only non-edit tools were called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound(['read_file', 'grep_search', 'list_dir'])); + expect(loop.testHadCodeEdits()).toBe(false); + }); + + it('should return true when replace_string_in_file was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.ReplaceString])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should return true when insert_edit_into_file was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.EditFile])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should return true when apply_patch was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.ApplyPatch])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should return true when create_file was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.CreateFile])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should return true when multi_replace_string_in_file was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.MultiReplaceString])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should return true when edit_notebook_file was called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound([ToolName.EditNotebook])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + + it('should detect edit tools in later rounds', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound(['read_file'])); + loop.addToolCallRound(createMockRound(['grep_search'])); + loop.addToolCallRound(createMockRound([ToolName.ReplaceString])); + expect(loop.testHadCodeEdits()).toBe(true); + }); + }); + + describe('getEditedFilePaths', () => { + function createRoundWithArgs(toolName: string, args: string): IToolCallRound { + return { + id: generateUuid(), + response: 'test response', + toolInputRetry: 0, + toolCalls: [{ + id: generateUuid(), + name: toolName, + arguments: args, + }], + }; + } + + it('should return empty array when no edit tools were called', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createMockRound(['read_file'])); + expect(loop.testGetEditedFilePaths()).toEqual([]); + }); + + it('should extract filePath from replace_string_in_file', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/src/index.ts', oldString: 'foo', newString: 'bar' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(1); + expect(paths[0].fsPath).toBe('/home/user/project/src/index.ts'); + }); + + it('should extract filePath from insert_edit_into_file', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.EditFile, + JSON.stringify({ filePath: '/home/user/project/src/app.ts', code: 'hello' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(1); + expect(paths[0].fsPath).toBe('/home/user/project/src/app.ts'); + }); + + it('should extract filePath from create_file', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.CreateFile, + JSON.stringify({ filePath: '/home/user/project/new-file.ts', content: 'export {}' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(1); + expect(paths[0].fsPath).toBe('/home/user/project/new-file.ts'); + }); + + it('should extract file paths from multi_replace_string_in_file replacements', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.MultiReplaceString, + JSON.stringify({ + explanation: 'test', + replacements: [ + { filePath: '/home/user/project/a.ts', oldString: 'x', newString: 'y' }, + { filePath: '/home/user/project/b.ts', oldString: 'x', newString: 'y' }, + ] + }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(2); + expect(paths.map(p => p.fsPath)).toEqual(['/home/user/project/a.ts', '/home/user/project/b.ts']); + }); + + it('should extract file paths from apply_patch headers', () => { + const loop = createLoop('autopilot'); + const patchText = [ + '*** Begin Patch', + '*** Update File: /home/user/project/src/main.ts', + '@@ function hello()', + '-console.log("hello")', + '+console.log("world")', + '*** Add File: /home/user/project/src/new.ts', + '+export const x = 1;', + '*** End Patch', + ].join('\n'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ApplyPatch, + JSON.stringify({ input: patchText, explanation: 'test' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(2); + expect(paths.map(p => p.fsPath)).toContain('/home/user/project/src/main.ts'); + expect(paths.map(p => p.fsPath)).toContain('/home/user/project/src/new.ts'); + }); + + it('should deduplicate file paths', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/src/index.ts', oldString: 'a', newString: 'b' }) + )); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/src/index.ts', oldString: 'c', newString: 'd' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(1); + }); + + it('should handle malformed arguments gracefully', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs(ToolName.ReplaceString, 'not valid json')); + loop.addToolCallRound(createRoundWithArgs( + ToolName.CreateFile, + JSON.stringify({ filePath: '/home/user/project/good.ts' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(1); + expect(paths[0].fsPath).toBe('/home/user/project/good.ts'); + }); + + it('should collect paths from multiple rounds and tool types', () => { + const loop = createLoop('autopilot'); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/a.ts', oldString: 'x', newString: 'y' }) + )); + loop.addToolCallRound(createRoundWithArgs( + ToolName.CreateFile, + JSON.stringify({ filePath: '/home/user/project/b.ts', content: '' }) + )); + loop.addToolCallRound(createRoundWithArgs( + ToolName.EditFile, + JSON.stringify({ filePath: '/home/user/project/c.ts', code: '' }) + )); + const paths = loop.testGetEditedFilePaths(); + expect(paths).toHaveLength(3); + }); + }); + + describe('performAutopilotCodeReview', () => { + function createRoundWithArgs(toolName: string, args: string): IToolCallRound { + return { + id: generateUuid(), + response: 'test response', + toolInputRetry: 0, + toolCalls: [{ + id: generateUuid(), + name: toolName, + arguments: args, + }], + }; + } + + function createLoopWithEdits(permissionLevel: string): AutopilotTestToolCallingLoop { + const loop = createLoop(permissionLevel); + loop.setTaskCompleted(true); + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/src/index.ts', oldString: 'a', newString: 'b' }) + )); + return loop; + } + + beforeEach(() => { + mockReviewFileChanges.mockReset(); + }); + + it('should return false when permissionLevel is not autopilot', async () => { + const loop = createLoopWithEdits('agent'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + expect(mockReviewFileChanges).not.toHaveBeenCalled(); + }); + + it('should return false when config is disabled', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, false); + const loop = createLoopWithEdits('autopilot'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + expect(mockReviewFileChanges).not.toHaveBeenCalled(); + }); + + it('should return false when task is not completed', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + const loop = createLoop('autopilot'); + // taskCompleted defaults to false + loop.addToolCallRound(createRoundWithArgs( + ToolName.ReplaceString, + JSON.stringify({ filePath: '/home/user/project/src/index.ts', oldString: 'a', newString: 'b' }) + )); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should return false when no code edits were made', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + const loop = createLoop('autopilot'); + loop.setTaskCompleted(true); + loop.addToolCallRound(createMockRound(['read_file'])); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should return false when token is cancelled', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + const loop = createLoopWithEdits('autopilot'); + tokenSource.cancel(); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should return true and set stopHookReason when review finds comments', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + const reviewResult: CodeReviewResult = { + type: 'success', + comments: [{ + uri: URI.file('/home/user/project/src/index.ts'), + range: { start: { line: 0, character: 0 }, end: { line: 0, character: 10 } } as any, + body: 'Consider using const instead of let', + kind: 'suggestion', + severity: 'warning', + }], + }; + mockReviewFileChanges.mockResolvedValue(reviewResult); + const loop = createLoopWithEdits('autopilot'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(true); + expect(mockReviewFileChanges).toHaveBeenCalledOnce(); + }); + + it('should return false when review finds no comments', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'success', comments: [] }); + const loop = createLoopWithEdits('autopilot'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should return false when review returns an error', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'error', reason: 'Code review is not enabled' }); + const loop = createLoopWithEdits('autopilot'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should return false and not crash when reviewFileChanges throws', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockRejectedValue(new Error('Network error')); + const loop = createLoopWithEdits('autopilot'); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + }); + + it('should not run review a second time', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'success', comments: [] }); + const loop = createLoopWithEdits('autopilot'); + await loop.testPerformAutopilotCodeReview(tokenSource.token); + // Second call should be a no-op because autopilotCodeReviewCompleted is true + loop.setTaskCompleted(true); + const result = await loop.testPerformAutopilotCodeReview(tokenSource.token); + expect(result).toBe(false); + expect(mockReviewFileChanges).toHaveBeenCalledOnce(); + }); + + it('should pass pre-edit snapshot as baseContent when available', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'success', comments: [] }); + + const loop = createLoopWithEdits('autopilot'); + loop.setPreEditSnapshot('/home/user/project/src/index.ts', 'const original = true;'); + await loop.testPerformAutopilotCodeReview(tokenSource.token); + + expect(mockReviewFileChanges).toHaveBeenCalledOnce(); + const input = mockReviewFileChanges.mock.calls[0][1]; + expect(input.files[0].baseContent).toBe('const original = true;'); + }); + + it('should pass undefined baseContent when no snapshot exists', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'success', comments: [] }); + + const loop = createLoopWithEdits('autopilot'); + // No snapshot set — simulates a file we couldn't capture + await loop.testPerformAutopilotCodeReview(tokenSource.token); + + expect(mockReviewFileChanges).toHaveBeenCalledOnce(); + const input = mockReviewFileChanges.mock.calls[0][1]; + expect(input.files[0].baseContent).toBeUndefined(); + }); + + it('should forward the cancellation token to reviewFileChanges', async () => { + const configService = instantiationService.invokeFunction(accessor => accessor.get(IConfigurationService)) as InMemoryConfigurationService; + await configService.setConfig(ConfigKey.Advanced.AutopilotCodeReviewEnabled, true); + mockReviewFileChanges.mockResolvedValue({ type: 'success', comments: [] }); + + const loop = createLoopWithEdits('autopilot'); + await loop.testPerformAutopilotCodeReview(tokenSource.token); + + expect(mockReviewFileChanges).toHaveBeenCalledOnce(); + const passedToken = mockReviewFileChanges.mock.calls[0][2]; + expect(passedToken).toBe(tokenSource.token); + }); + }); }); diff --git a/src/extension/review/node/doReview.ts b/src/extension/review/node/doReview.ts index dd67febb63..db11c16180 100644 --- a/src/extension/review/node/doReview.ts +++ b/src/extension/review/node/doReview.ts @@ -386,6 +386,7 @@ async function review( export async function reviewFileChanges( accessor: ServicesAccessor, input: CodeReviewInput, + externalToken?: CancellationToken, ): Promise { const logService = accessor.get(ILogService); const authService = accessor.get(IAuthenticationService); @@ -402,11 +403,11 @@ export async function reviewFileChanges( return { type: 'error', reason: 'Code review is not enabled for this account.' }; } - const tokenSource = new CancellationTokenSource(); + const tokenSource = new CancellationTokenSource(externalToken); try { const fileInputs = await Promise.all(input.files.map(async file => { - let baseContent = ''; - if (file.baseUri) { + let baseContent = file.baseContent ?? ''; + if (!baseContent && file.baseUri) { const bytes = await fileSystemService.readFile(file.baseUri); baseContent = new TextDecoder().decode(bytes); } diff --git a/src/platform/configuration/common/configurationService.ts b/src/platform/configuration/common/configurationService.ts index 19924e1dcb..cfb3404a08 100644 --- a/src/platform/configuration/common/configurationService.ts +++ b/src/platform/configuration/common/configurationService.ts @@ -699,6 +699,8 @@ export namespace ConfigKey { export const OTelCaptureContent = defineSetting('chat.otel.captureContent', ConfigType.Simple, false); export const OTelOutfile = defineSetting('chat.otel.outfile', ConfigType.Simple, ''); export const OTelDbSpanExporter = defineSetting('chat.otel.dbSpanExporter.enabled', ConfigType.Simple, false); + + export const AutopilotCodeReviewEnabled = defineSetting('chat.autopilot.codeReview.enabled', ConfigType.Simple, false); } /** diff --git a/src/platform/review/common/reviewCommand.ts b/src/platform/review/common/reviewCommand.ts index 92dcc28455..69676354f3 100644 --- a/src/platform/review/common/reviewCommand.ts +++ b/src/platform/review/common/reviewCommand.ts @@ -12,6 +12,8 @@ import { ReviewComment, ReviewSuggestion } from './reviewService'; export interface CodeReviewFileInput { readonly currentUri: vscode.Uri; readonly baseUri?: vscode.Uri; + /** Optional pre-resolved base content. When provided, takes precedence over reading from {@link baseUri}. */ + readonly baseContent?: string; } /**