Skip to content

Commit 38c78b7

Browse files
stephen-carter-at-sfstephen-carter-at-sf
authored
FIX: @W-17821243@: Quick fix to external service race condition issues (#190)
1 parent 2ba9d3c commit 38c78b7

13 files changed

+373
-344
lines changed

‎package.json

+6-3
Original file line numberDiff line numberDiff line change
@@ -249,14 +249,16 @@
249249
],
250250
"editor/context": [
251251
{
252-
"command": "sfca.runOnActiveFile"
252+
"command": "sfca.runOnActiveFile",
253+
"when": "true"
253254
},
254255
{
255256
"command": "sfca.runDfaOnSelectedMethod",
256257
"when": "sfca.codeAnalyzerV4Enabled"
257258
},
258259
{
259-
"command": "sfca.removeDiagnosticsOnActiveFile"
260+
"command": "sfca.removeDiagnosticsOnActiveFile",
261+
"when": "true"
260262
},
261263
{
262264
"command": "sfca.runApexGuruAnalysisOnCurrentFile",
@@ -265,7 +267,8 @@
265267
],
266268
"explorer/context": [
267269
{
268-
"command": "sfca.runOnSelected"
270+
"command": "sfca.runOnSelected",
271+
"when": "true"
269272
},
270273
{
271274
"command": "sfca.removeDiagnosticsOnSelectedFile",

‎src/extension.ts

+113-96
Large diffs are not rendered by default.

‎src/lib/agentforce/agentforce-violations-fixer.ts

+40-23
Original file line numberDiff line numberDiff line change
@@ -9,47 +9,61 @@ import * as vscode from 'vscode';
99
import * as Constants from '../constants';
1010
import * as PromptConstants from './prompt-constants';
1111
import { PromptBuilder } from './prompt-builder';
12-
import { messages } from '../messages';
1312
import {CodeActionKind} from "vscode";
14-
import {LLMService} from "../external-services/llm-service";
13+
import {LLMService, LLMServiceProvider} from "../external-services/llm-service";
14+
import {messages} from "../messages";
15+
import {Logger} from "../logger";
1516

1617
export class AgentforceViolationsFixer implements vscode.CodeActionProvider {
1718
static readonly providedCodeActionKinds: CodeActionKind[] = [vscode.CodeActionKind.QuickFix];
1819

19-
private readonly llmService: LLMService;
20+
private readonly llmServiceProvider: LLMServiceProvider;
21+
private readonly logger: Logger;
2022

21-
constructor(llmService: LLMService) {
22-
this.llmService = llmService;
23+
private hasWarnedAboutUnavailableLLMService: boolean = false;
24+
25+
constructor(llmServiceProvider: LLMServiceProvider, logger: Logger) {
26+
this.llmServiceProvider = llmServiceProvider;
27+
this.logger = logger;
2328
}
2429

25-
provideCodeActions(document: vscode.TextDocument, range: vscode.Range, context: vscode.CodeActionContext,
26-
_token: vscode.CancellationToken): vscode.CodeAction[] {
30+
async provideCodeActions(document: vscode.TextDocument, range: vscode.Range, context: vscode.CodeActionContext,
31+
_token: vscode.CancellationToken): Promise<vscode.CodeAction[]> {
2732
const codeActions: vscode.CodeAction[] = [];
2833

2934
// Throw out diagnostics that aren't ours, or are for the wrong line.
30-
const filteredDiagnostics = context.diagnostics.filter(
31-
diagnostic => messages.diagnostics.source
32-
&& messages.diagnostics.source.isSource(diagnostic.source)
33-
&& range.contains(diagnostic.range)
34-
&& this.isSupportedViolationForCodeFix(diagnostic));
35-
36-
// Loop through diagnostics in the context
37-
filteredDiagnostics.forEach((diagnostic) => {
38-
// Create a code action for each diagnostic
35+
const filteredDiagnostics: vscode.Diagnostic[] = context.diagnostics.filter((diagnostic: vscode.Diagnostic) =>
36+
diagnostic.source
37+
&& diagnostic.source.endsWith(messages.diagnostics.source.suffix)
38+
&& range.contains(diagnostic.range)
39+
&& this.isSupportedViolationForCodeFix(diagnostic));
40+
41+
if (filteredDiagnostics.length == 0) {
42+
return codeActions;
43+
}
44+
45+
// Do not provide quick fix code actions if LLM service is not available. We warn once to let user know.
46+
if (!(await this.llmServiceProvider.isLLMServiceAvailable())) {
47+
if (!this.hasWarnedAboutUnavailableLLMService) {
48+
this.logger.warn(messages.agentforce.a4dQuickFixUnavailable);
49+
this.hasWarnedAboutUnavailableLLMService = true;
50+
}
51+
return codeActions;
52+
}
53+
54+
for (const diagnostic of filteredDiagnostics) {
3955
const fixAction = new vscode.CodeAction(
4056
`Fix ${this.extractDiagnosticCode(diagnostic)} using Agentforce.`,
4157
vscode.CodeActionKind.QuickFix
4258
);
43-
44-
// Pass the diagnostic as an argument to the command
59+
fixAction.diagnostics = [diagnostic] // Important (that we used to miss before): we should tie the code fix to the specific diagnostic.
4560
fixAction.command = {
4661
title: 'Fix Diagnostic Issue',
47-
command: Constants.UNIFIED_DIFF,
48-
arguments: [document, diagnostic] // Pass the diagnostic here
62+
command: Constants.UNIFIED_DIFF, // TODO: This really should be called something like QF_COMMAND_A4D_FIX... but it isn't because we are using the resolveCodeAction to do the A4D stuff instead before sending to unifieid diff.
63+
arguments: [document, diagnostic] // Pass the document and diagnostic here.
4964
};
50-
5165
codeActions.push(fixAction);
52-
});
66+
}
5367

5468
return codeActions;
5569
}
@@ -62,6 +76,8 @@ export class AgentforceViolationsFixer implements vscode.CodeActionProvider {
6276
return Constants.A4D_FIX_AVAILABLE_RULES.includes(this.extractDiagnosticCode(diagnostic));
6377
}
6478

79+
// TODO: Evaluate if this even should be used. It clearly does more than its responsibility of just resolving the code action... it performs work by invoking the callLLM stuff and updates the existing UNIFIED_DIFF command's arguments before actually executing the unified diff.
80+
// I think we will probably benefit from removing this and instead just have the code action trigger a QF_COMMAND_A4D_FIX which can optionally then trigger a COMMAND_UNIFIED_DIFF or something.
6581
public resolveCodeAction(
6682
codeAction: vscode.CodeAction,
6783
token: vscode.CancellationToken
@@ -78,7 +94,8 @@ export class AgentforceViolationsFixer implements vscode.CodeActionProvider {
7894
const prompt = this.generatePrompt(document, diagnostic);
7995

8096
// Call the LLM service with the generated prompt
81-
const llmResponse = await this.llmService.callLLM(prompt);
97+
const llmService: LLMService = await this.llmServiceProvider.getLLMService();
98+
const llmResponse = await llmService.callLLM(prompt);
8299
const codeSnippet = this.extractCodeFromResponse(llmResponse);
83100

84101
const updatedFileContent = this.replaceCodeInFile(document.getText(), codeSnippet.trim(), diagnostic.range.start.line + 1, diagnostic.range.end.line + 1, document);

‎src/lib/constants.ts

+4-3
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@ export const COMMAND_RUN_DFA_ON_SELECTED_METHOD = 'sfca.runDfaOnSelectedMethod';
1717
export const COMMAND_RUN_DFA = 'sfca.runDfa';
1818
export const COMMAND_REMOVE_DIAGNOSTICS_ON_ACTIVE_FILE = 'sfca.removeDiagnosticsOnActiveFile';
1919
export const COMMAND_REMOVE_DIAGNOSTICS_ON_SELECTED_FILE = 'sfca.removeDiagnosticsOnSelectedFile';
20-
export const COMMAND_DIAGNOSTICS_IN_RANGE = 'sfca.removeDiagnosticsInRange';
2120
export const COMMAND_RUN_APEX_GURU_ON_FILE = 'sfca.runApexGuruAnalysisOnSelectedFile';
2221
export const COMMAND_RUN_APEX_GURU_ON_ACTIVE_FILE = 'sfca.runApexGuruAnalysisOnCurrentFile';
23-
export const COMMAND_INCLUDE_APEX_GURU_SUGGESTIONS = 'sfca.includeApexGuruSuggestions';
22+
23+
// commands that are only invoked by quick fixes (which do not need to be declared in package.json since they can be registered dynamically)
24+
export const QF_COMMAND_DIAGNOSTICS_IN_RANGE = 'sfca.removeDiagnosticsInRange';
25+
export const QF_COMMAND_INCLUDE_APEX_GURU_SUGGESTIONS = 'sfca.includeApexGuruSuggestions';
2426

2527
// telemetry event keys
2628
export const TELEM_SUCCESSFUL_STATIC_ANALYSIS = 'sfdx__codeanalyzer_static_run_complete';
@@ -49,7 +51,6 @@ export const APEX_GURU_MAX_TIMEOUT_SECONDS = 60;
4951
export const APEX_GURU_RETRY_INTERVAL_MILLIS = 1000;
5052

5153
// A4D Integration
52-
export const ENABLE_A4D_INTEGRATION = false;
5354
export const A4D_FIX_AVAILABLE_RULES = ['ApexCRUDViolation', 'ApexSharingViolations', 'EmptyCatchBlock', 'EmptyTryOrFinallyBlock', 'EmptyWhileStmt', 'EmptyIfStmt'];
5455
export const UNIFIED_DIFF = 'unifiedDiff';
5556
export const A4D_PREFIX = 'SFCA_A4D_FIX';

‎src/lib/external-services/external-service-provider.ts

+89-16
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,26 @@ import {
44
ServiceType,
55
TelemetryServiceInterface
66
} from "@salesforce/vscode-service-provider";
7-
import {LiveTelemetryService, LogOnlyTelemetryService, TelemetryService} from "./telemetry-service";
7+
import {
8+
LiveTelemetryService,
9+
LogOnlyTelemetryService,
10+
TelemetryService,
11+
TelemetryServiceProvider
12+
} from "./telemetry-service";
813
import {Logger} from "../logger";
9-
import {LiveLLMService, LLMService} from "./llm-service";
14+
import {LiveLLMService, LLMService, LLMServiceProvider} from "./llm-service";
15+
import {Extension} from "vscode";
16+
import vscode from "vscode";
17+
import * as Constants from "../constants";
18+
19+
const EXTENSION_THAT_SUPPLIES_LLM_SERVICE = 'salesforce.salesforcedx-einstein-gpt';
20+
const EXTENSION_THAT_SUPPLIES_TELEMETRY_SERVICE = 'salesforce.salesforcedx-vscode-core';
21+
1022

1123
/**
1224
* Provides and caches a number of external services that we use like the LLM service, telemetry service, etc.
1325
*/
14-
export class ExternalServiceProvider {
26+
export class ExternalServiceProvider implements LLMServiceProvider, TelemetryServiceProvider {
1527
private readonly logger: Logger;
1628

1729
private cachedLLMService?: LLMService;
@@ -21,12 +33,13 @@ export class ExternalServiceProvider {
2133
this.logger = logger;
2234
}
2335

24-
async isLLMServiceAvailable(): Promise<boolean> {
25-
return await ServiceProvider.isServiceAvailable(ServiceType.LLMService);
26-
}
36+
// =================================================================================================================
37+
// === LLMServiceProvider implementation
38+
// =================================================================================================================
2739

28-
async isTelemetryServiceAvailable(): Promise<boolean> {
29-
return await ServiceProvider.isServiceAvailable(ServiceType.Telemetry);
40+
async isLLMServiceAvailable(): Promise<boolean> {
41+
return (await this.waitForExtensionToBeActivatedIfItExists(EXTENSION_THAT_SUPPLIES_LLM_SERVICE)) &&
42+
(await ServiceProvider.isServiceAvailable(ServiceType.LLMService));
3043
}
3144

3245
async getLLMService(): Promise<LLMService> {
@@ -36,19 +49,12 @@ export class ExternalServiceProvider {
3649
return this.cachedLLMService;
3750
}
3851

39-
async getTelemetryService(): Promise<TelemetryService> {
40-
if (!this.cachedTelemetryService) {
41-
this.cachedTelemetryService = await this.initializeTelemetryService();
42-
}
43-
return this.cachedTelemetryService;
44-
}
45-
4652
private async initializeLLMService(): Promise<LLMService> {
4753
if (!(await this.isLLMServiceAvailable())) {
4854
throw new Error("The initializeLLMService method should not be called if the LLM Service is unavailable.");
4955
}
5056
try {
51-
const coreLLMService: LLMServiceInterface = await ServiceProvider.getService(ServiceType.LLMService);
57+
const coreLLMService: LLMServiceInterface = await ServiceProvider.getService(ServiceType.LLMService, Constants.EXTENSION_ID);
5258
return new LiveLLMService(coreLLMService, this.logger);
5359
} catch (err) {
5460
const errMsg: string = err instanceof Error? err.stack : String(err);
@@ -57,6 +63,22 @@ export class ExternalServiceProvider {
5763
}
5864
}
5965

66+
// =================================================================================================================
67+
// === TelemetryServiceProvider implementation
68+
// =================================================================================================================
69+
70+
async isTelemetryServiceAvailable(): Promise<boolean> {
71+
return (await this.waitForExtensionToBeActivatedIfItExists(EXTENSION_THAT_SUPPLIES_TELEMETRY_SERVICE)) &&
72+
(await ServiceProvider.isServiceAvailable(ServiceType.Telemetry));
73+
}
74+
75+
async getTelemetryService(): Promise<TelemetryService> {
76+
if (!this.cachedTelemetryService) {
77+
this.cachedTelemetryService = await this.initializeTelemetryService();
78+
}
79+
return this.cachedTelemetryService;
80+
}
81+
6082
private async initializeTelemetryService(): Promise<TelemetryService> {
6183
if (!(await this.isTelemetryServiceAvailable())) {
6284
this.logger.debug('Could not establish live telemetry service since it is not available. ' +
@@ -73,4 +95,55 @@ export class ExternalServiceProvider {
7395
return new LogOnlyTelemetryService(this.logger);
7496
}
7597
}
98+
99+
100+
// =================================================================================================================
101+
102+
// TODO: The following is a temporary workaround to the problem that our extension might activate before
103+
// the extension that provides a dependent service has activated. We wait for it to activate for up to 2 seconds if
104+
// it is available and after 2 seconds just force activate it. The service provider should do this automatically for
105+
// us. Until then, we'll keep this workaround in place (which is not preferred because it requires us to hard code
106+
// the extension name that each service comes from which theoretically could be subject to change over time).
107+
// Returns true if the extension activated and false if the extension doesn't exist or could not be activated.
108+
private async waitForExtensionToBeActivatedIfItExists(extensionName: string): Promise<boolean> {
109+
const extension: Extension<unknown> = vscode.extensions.getExtension(extensionName);
110+
if (!extension) {
111+
this.logger.debug(`The extension '${extensionName}' was not found. Some functionality that depends on this extension will not be available.`);
112+
return false;
113+
} else if (extension.isActive) {
114+
return true;
115+
}
116+
117+
this.logger.debug(`The extension '${extensionName}' was found but has not yet activated. Waiting up to 5 seconds for it to activate.`);
118+
const eventuallyBecameActive: boolean = await new Promise(resolve => {
119+
const interval = setInterval(() => {
120+
if (extension.isActive) {
121+
clearInterval(interval);
122+
resolve(true);
123+
}
124+
}, 50); // Check every 50ms
125+
setTimeout(() => {
126+
clearInterval(interval);
127+
resolve(false);
128+
}, 5000); // Timeout after 5 seconds
129+
});
130+
131+
if (eventuallyBecameActive) {
132+
this.logger.debug(`The extension '${extensionName}' has activated successfully.`);
133+
return true;
134+
}
135+
136+
// Ideally we shouldn't be force activating it, but it's the best thing we can do after waiting 2 seconds as a
137+
// last attempt to get the dependent extension's service available.
138+
this.logger.debug(`The extension '${extensionName}' has still has not activated. Attempting to force activate it.`);
139+
try {
140+
await extension.activate();
141+
this.logger.debug(`The extension '${extensionName}' has activated successfully.`);
142+
return true;
143+
} catch (err) {
144+
const errMsg: string = err instanceof Error ? err.stack : String(err);
145+
this.logger.debug(`The extension '${extensionName}' could not activate due to an unexpected exception:\n${errMsg}`);
146+
return false;
147+
}
148+
}
76149
}

‎src/lib/external-services/llm-service.ts

+6-1
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,14 @@ export interface LLMService {
1010
callLLM(prompt: string, guidedJsonSchema?: string): Promise<string>
1111
}
1212

13+
export interface LLMServiceProvider {
14+
isLLMServiceAvailable(): Promise<boolean>
15+
getLLMService(): Promise<LLMService>
16+
}
17+
18+
1319
export class LiveLLMService implements LLMService {
1420
// Delegates to the "Agentforce for Developers" LLM service
15-
// See https://github.com/forcedotcom/salesforcedx-vscode-einstein-gpt/blob/beb0a7cb1b1c9d3f62cf538b93204d50263f20d8/src/services/LLMService.ts#L36
1621
private readonly coreLLMService: LLMServiceInterface;
1722
private readonly logger: Logger;
1823
private uuidGenerator: UUIDGenerator = new RandomUUIDGenerator();

‎src/lib/external-services/telemetry-service.ts

+7
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,16 @@ export interface TelemetryService {
1111
sendException(name: string, errorMessage: string, properties?: Record<string, string>): void;
1212
}
1313

14+
export interface TelemetryServiceProvider {
15+
isTelemetryServiceAvailable(): Promise<boolean>
16+
getTelemetryService(): Promise<TelemetryService>
17+
}
18+
19+
1420
export class LiveTelemetryService implements TelemetryService {
1521
// Delegates to the core telemetry service
1622
// See https://github.com/forcedotcom/salesforcedx-vscode/blob/develop/packages/salesforcedx-utils-vscode/src/services/telemetry.ts#L78
23+
// and https://github.com/forcedotcom/salesforcedx-vscode/blob/develop/packages/salesforcedx-vscode-core/src/services/telemetry/telemetryServiceProvider.ts#L19
1724
private readonly coreTelemetryService: TelemetryServiceInterface;
1825
private readonly logger: Logger;
1926

0 commit comments

Comments
 (0)