From dc7518d18fcf4d9be08bbff26bf9dd03a2d21f23 Mon Sep 17 00:00:00 2001 From: Nikhil Mittal Date: Mon, 13 Jul 2026 10:15:26 +0530 Subject: [PATCH 1/3] fix: remove stale XML root caching in MissingFaultHandler (#2063) MissingFaultHandler cached the parsed XML root on its first execute() call and reused it for all subsequent subflow frames. This caused findings attributed to subflow files but with line numbers from the parent flow's XML tree. Remove the caching so parser.get_root() is called fresh each time, matching HardcodedId's behavior. --- .../FlowScanner/queries/optional_query.py | 9 +-------- packages/code-analyzer-flow-engine/package.json | 2 +- packages/code-analyzer-flow-engine/test/engine.test.ts | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py b/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py index 8c5eab73..61b49cc1 100644 --- a/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py +++ b/packages/code-analyzer-flow-engine/FlowScanner/queries/optional_query.py @@ -233,9 +233,6 @@ class MissingFaultHandler(LexicalQuery): fault_tags = ['recordCreates', 'recordUpdates', 'recordDeletes', 'actionCalls', 'subflows'] - def __init__(self): - self.root = None - @classmethod def get_query_description(cls) -> QueryDescription: return QueryDescription(query_id=cls.query_id, @@ -253,11 +250,7 @@ def when_to_run(self) -> list[QueryAction]: def execute(self, parser: FlowParser = None, **kwargs) -> list[QueryResult] | None: accum = [] - if self.root is None: - root = parser.get_root() - self.root = root - else: - root = self.root + root = parser.get_root() for tag_ in self.fault_tags: elems = parse_utils.get_by_tag(root, tag_) diff --git a/packages/code-analyzer-flow-engine/package.json b/packages/code-analyzer-flow-engine/package.json index 94cf8c2c..08f350a5 100644 --- a/packages/code-analyzer-flow-engine/package.json +++ b/packages/code-analyzer-flow-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-flow-engine", "description": "Plugin package that adds 'Flow Scanner' as an engine into Salesforce Code Analyzer", - "version": "0.38.0", + "version": "0.38.1-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", diff --git a/packages/code-analyzer-flow-engine/test/engine.test.ts b/packages/code-analyzer-flow-engine/test/engine.test.ts index ddbc08e4..06aaf7e3 100644 --- a/packages/code-analyzer-flow-engine/test/engine.test.ts +++ b/packages/code-analyzer-flow-engine/test/engine.test.ts @@ -125,7 +125,7 @@ describe('Tests for the FlowScannerEngine', () => { }, {}); expect(countsPerRule).toEqual({ MissingDescription: 56, - MissingFaultHandler: 9, + MissingFaultHandler: 8, PreventPassingUserDataIntoElementWithoutSharing: 5, PreventPassingUserDataIntoElementWithSharing: 2 }); From 54340546f0504a96eecaddb101942b943a9e8e53 Mon Sep 17 00:00:00 2001 From: nikhil-mittal-165 Date: Tue, 14 Jul 2026 11:14:18 +0530 Subject: [PATCH 2/3] NEW @W-22393694@ - Gracefully skip ApexGuru when user is not authenticated (#478) --- .../package.json | 2 +- .../src/apexguru-rules.ts | 64 +++++ .../src/engine.ts | 164 +++++++++--- .../src/services/ApexGuruAuthService.ts | 4 +- .../src/services/ApexGuruService.ts | 103 +++++++- .../test/ApexGuruEngine.test.ts | 240 +++++++++++++++--- .../test/ApexGuruService.test.ts | 22 +- 7 files changed, 505 insertions(+), 94 deletions(-) diff --git a/packages/code-analyzer-apexguru-engine/package.json b/packages/code-analyzer-apexguru-engine/package.json index 551d82a3..10ac6d9d 100644 --- a/packages/code-analyzer-apexguru-engine/package.json +++ b/packages/code-analyzer-apexguru-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-apexguru-engine", "description": "ApexGuru Engine Package for the Salesforce Code Analyzer", - "version": "0.39.0", + "version": "0.40.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview", diff --git a/packages/code-analyzer-apexguru-engine/src/apexguru-rules.ts b/packages/code-analyzer-apexguru-engine/src/apexguru-rules.ts index 43391edb..0b1a15bf 100644 --- a/packages/code-analyzer-apexguru-engine/src/apexguru-rules.ts +++ b/packages/code-analyzer-apexguru-engine/src/apexguru-rules.ts @@ -29,6 +29,26 @@ export const APEXGURU_RULES: RuleDescription[] = [ resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_dml_in_loop.htm&type=5'] }, + // ================================================================================================================= + // PERFORMANCE RULES - HIGH SEVERITY (PERFORMANCE ONLY - NOT RECOMMENDED) + // ================================================================================================================= + + { + name: 'SoqlInALoopOneHop', + severityLevel: SeverityLevel.High, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'SOQL query reached one method-hop away inside a loop causes performance issues and can hit governor limits', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_soql_in_loop_one_hop.htm&type=5'] + }, + + { + name: 'ExpensiveMethods', + severityLevel: SeverityLevel.High, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'Method accounts for a large share of observed Apex CPU time and is a hotspot for performance work', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_expensive_methods.htm&type=5'] + }, + // ================================================================================================================= // PERFORMANCE RULES - MODERATE SEVERITY (CRITICAL - RECOMMENDED) // ================================================================================================================= @@ -109,6 +129,42 @@ export const APEXGURU_RULES: RuleDescription[] = [ resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_sobject_map_in_for_loop.htm&type=5'] }, + { + name: 'SoqlWithoutPlatformCache', + severityLevel: SeverityLevel.Moderate, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'Frequently executed SOQL query whose results could be served from Platform Cache to reduce database load', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_soql_without_platform_cache.htm&type=5'] + }, + + // ================================================================================================================= + // PERFORMANCE RULES - LOW SEVERITY (PERFORMANCE ONLY - NOT RECOMMENDED) + // ================================================================================================================= + + { + name: 'LimitsGetHeapsizeMethods', + severityLevel: SeverityLevel.Low, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'Frequent Limits.getHeapSize() calls add runtime overhead', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_limits_getheapsize_methods.htm&type=5'] + }, + + { + name: 'ExpensiveStringComparison', + severityLevel: SeverityLevel.Low, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'Inefficient string comparison wastes CPU time', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_expensive_string_comparison.htm&type=5'] + }, + + { + name: 'ExpensiveDebugStatements', + severityLevel: SeverityLevel.Low, + tags: [COMMON_TAGS.CATEGORIES.PERFORMANCE, COMMON_TAGS.LANGUAGES.APEX], + description: 'Expensive System.debug() statements add runtime overhead', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_expensive_debug_statements.htm&type=5'] + }, + // ================================================================================================================= // BEST PRACTICES - LOW SEVERITY (RECOMMENDED) // ================================================================================================================= @@ -149,6 +205,14 @@ export const APEXGURU_RULES: RuleDescription[] = [ resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_antipattern_soql_with_unused_fields.htm&type=5'] }, + { + name: 'WritingFillerStatements', + severityLevel: SeverityLevel.Low, + tags: [COMMON_TAGS.CATEGORIES.BEST_PRACTICES, COMMON_TAGS.LANGUAGES.APEX], + description: 'Filler statements written to inflate code coverage instead of testing real behavior', + resourceUrls: ['https://help.salesforce.com/s/articleView?id=xcloud.apexguru_test_case_antipattern_filler_statements.htm&type=5'] + }, + // ================================================================================================================= // FALLBACK RULE // ================================================================================================================= diff --git a/packages/code-analyzer-apexguru-engine/src/engine.ts b/packages/code-analyzer-apexguru-engine/src/engine.ts index f7c333f8..0d5518b7 100644 --- a/packages/code-analyzer-apexguru-engine/src/engine.ts +++ b/packages/code-analyzer-apexguru-engine/src/engine.ts @@ -1,5 +1,3 @@ - - import { Engine, EngineEventEmitter, @@ -18,6 +16,7 @@ import { ApexGuruEngineConfig, DEFAULT_APEXGURU_ENGINE_CONFIG } from './config'; import { ENGINE_NAME, APEXGURU_FILE_EXTENSIONS } from './constants'; import { APEXGURU_RULES, isKnownRule, FALLBACK_RULE_NAME } from './apexguru-rules'; import * as fs from 'node:fs/promises'; +import { existsSync } from 'node:fs'; import * as path from 'node:path'; /** @@ -53,14 +52,6 @@ export class ApexGuruEngine extends EngineEventEmitter implements Engine { async describeRules(describeOptions: DescribeOptions): Promise { this.emitDescribeRulesProgressEvent(0); - // The SFAP API endpoint is environment-specific and supplied externally. - // When unset, this engine has nowhere to scan against, so it advertises no rules. - if (!process.env.SFAP_API_BASE_URL) { - this.emitLogEvent(LogLevel.Debug, 'SFAP API base URL not configured. ApexGuru engine is disabled.'); - this.emitDescribeRulesProgressEvent(100); - return []; - } - // Check if targeted files contain any Apex files if (describeOptions.workspace) { const targetedFiles = await describeOptions.workspace.getTargetedFiles(); @@ -97,15 +88,15 @@ export class ApexGuruEngine extends EngineEventEmitter implements Engine { // Get target org alias/username from config (passed by CLI --target-org flag) const targetOrg = this.getTargetOrg(); - // Initialize authentication + // Initialize authentication — skip gracefully if user is not authenticated try { await this.apexGuruService.initialize(targetOrg); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - throw new Error( - `Failed to authenticate: ${message}\n` + - 'Please authenticate with: sf org login web' - ); + const detail = error instanceof Error ? error.message : String(error); + this.apexGuruService.cleanup(); + return this.skipWithError('NO_ORG_CONNECTION', + `Failed to authenticate: ${detail}`, + "Please authenticate with 'sf org login web' or pass --target-org"); } // Get workspace root path @@ -126,7 +117,15 @@ export class ApexGuruEngine extends EngineEventEmitter implements Engine { throw new Error('ApexGuru requires a common workspace root, but the targeted files do not share one.'); } - const pathsToZip = [workspaceRoot]; + // Zip only the Apex files the user actually targeted (via --target or the workspace itself). + // Entry names are computed relative to workspaceRoot in createWorkspaceZip so project layout is preserved. + const pathsToZip = apexFiles; + + // TEMP DIAGNOSTIC: log the exact set of files being zipped and shipped to SFAP. + // Pair this with the [apexguru-diag] "Raw SFAP violation paths" log to confirm + // that only what we zipped comes back in violations. Remove once verified. + this.emitLogEvent(LogLevel.Fine, + `[apexguru-diag] Zipping ${pathsToZip.length} file(s) for SFAP: ${JSON.stringify(pathsToZip)}`); try { // Set up progress callback for polling @@ -135,28 +134,110 @@ export class ApexGuruEngine extends EngineEventEmitter implements Engine { }); // Scan (creates zip -> submits -> polls -> decodes) - const { violations: apexGuruViolations, scanMetadata } = await this.apexGuruService.scanWorkspace(workspaceRoot, pathsToZip); + const { violations: apexGuruViolations, scanMetadata, analysisMode } = await this.apexGuruService.scanWorkspace(workspaceRoot, pathsToZip); + + // TEMP DIAGNOSTIC: dump the raw file paths ApexGuru returned so we can see + // exactly what SFAP is sending back (inner-class notation? relative? absolute?). + // Remove once the "file does not exist" edge case is fully understood. + const rawFilePaths = apexGuruViolations.map(av => ({ + rule: av.rule, + file: av.locations[av.primaryLocationIndex]?.file ?? av.locations[0]?.file, + startLine: av.locations[av.primaryLocationIndex]?.startLine ?? av.locations[0]?.startLine + })); + this.emitLogEvent(LogLevel.Fine, + `[apexguru-diag] Raw SFAP violation paths (${rawFilePaths.length}): ${JSON.stringify(rawFilePaths)}`); + + // SFAP normalizes the paths it returns — it strips the longest common leading directory + // shared by all files in the zip. e.g. if we zip `unpackaged/config/foo.cls` and + // `unpackaged/config/bar.cls`, SFAP returns `foo.cls` and `bar.cls`. Reconstruct the + // real absolute path by matching each returned path as a suffix of what we actually zipped. + const resolveReturnedPath = (returnedFile: string | undefined): string | undefined => { + if (!returnedFile) return undefined; + const normalized = returnedFile.replace(/\\/g, '/'); + const match = pathsToZip.find(absPath => { + const absNormalized = absPath.replace(/\\/g, '/'); + return absNormalized === normalized || absNormalized.endsWith(`/${normalized}`); + }); + return match; + }; // Convert all ApexGuru violations to Code Analyzer format const allViolations = apexGuruViolations.map(av => { - // SFAP response includes file path in location.file - const filePath = av.locations[0]?.file ?? 'unknown'; - return toViolation(av, filePath, runOptions.includeSuggestions ?? false); + const returnedFile = av.locations[av.primaryLocationIndex]?.file ?? av.locations[0]?.file; + const resolvedFile = resolveReturnedPath(returnedFile) ?? returnedFile ?? 'unknown'; + return toViolation(av, resolvedFile, workspaceRoot, runOptions.includeSuggestions ?? false, resolveReturnedPath); + }); + + // Drop violations whose primary code location points at a non-existent file. + // Safety net for edge cases the suffix-match couldn't resolve (e.g. synthetic paths + // like inner-class notation `Foo.InnerHelper.cls`) so we never hand Core a bad path. + const validViolations = allViolations.filter(v => { + const file = v.codeLocations[v.primaryLocationIndex]?.file; + if (file && existsSync(file)) { + return true; + } + this.emitLogEvent(LogLevel.Warn, + `Dropping ${v.ruleName} violation: primary location file does not exist on disk: ${file ?? '(missing)'}`); + return false; }); // Filter violations to only include selected rules - const filteredViolations = allViolations.filter(v => selectedRulesSet.has(v.ruleName)); + const filteredViolations = validViolations.filter(v => selectedRulesSet.has(v.ruleName)); - // Return insights as scan metadata (workspace-level) - const insights: Record | undefined = scanMetadata ? { scan: scanMetadata } : undefined; + // Return insights with status: "completed", analysis mode, and scan metadata + const insights: Record = { + status: 'completed', + ...(analysisMode ? { analysisMode } : {}), + ...(scanMetadata ? { scan: scanMetadata } : {}) + }; return { violations: filteredViolations, insights }; + } catch (error) { + // Catch API failures (5xx, timeout, connection refused) and unexpected errors + const detail = error instanceof Error ? error.message : String(error); + if (this.isApiUnavailableError(error)) { + return this.skipWithError('API_UNAVAILABLE', + `ApexGuru service is unavailable: ${detail}`, + 'The ApexGuru service is temporarily unavailable. Please try again later.'); + } + return this.skipWithError('UNEXPECTED_ERROR', + `An unexpected error occurred: ${detail}`, + 'An unexpected error occurred. Please try again or file a support ticket if the issue persists.'); } finally { // Always cleanup resources this.apexGuruService.cleanup(); } } + /** + * Returns a graceful skip result with structured error insights. + * Emits a warn-level log event and completes progress before returning. + * NOTE: Caller is responsible for cleanup() — do NOT call cleanup here since + * this may be invoked from within a try-finally that already handles cleanup. + */ + private skipWithError(code: string, message: string, remediation: string): EngineRunResults { + this.emitLogEvent(LogLevel.Warn, `ApexGuru skipped: ${message}`); + this.emitRunRulesProgressEvent(100); + return { + violations: [], + insights: { + status: 'skipped', + error: { code, message, remediation } + } + }; + } + + /** + * Determines whether an error is an API unavailability issue (network/timeout/5xx). + */ + private isApiUnavailableError(error: unknown): boolean { + if (!(error instanceof Error)) return false; + const msg = error.message.toLowerCase(); + const networkIndicators = ['econnrefused', 'etimedout', 'enotfound', 'socket hang up', + 'connection refused', 'timeout', 'network', '502', '503', '504', '500']; + return networkIndicators.some(indicator => msg.includes(indicator)); + } + /** * Check if file is an Apex file based on extension */ @@ -187,7 +268,9 @@ export class ApexGuruEngine extends EngineEventEmitter implements Engine { function toViolation( av: ApexGuruViolation, filePath: string, - includeSuggestions: boolean + workspaceRoot: string, + includeSuggestions: boolean, + resolveReturnedPath: (returnedFile: string | undefined) => string | undefined ): Violation { // Map unknown rules to fallback to ensure Core validation passes const ruleName = isKnownRule(av.rule) ? av.rule : FALLBACK_RULE_NAME; @@ -195,14 +278,14 @@ function toViolation( const violation: Violation = { ruleName, message: av.message, - codeLocations: av.locations.map(loc => normalizeLocation(loc, filePath)), + codeLocations: av.locations.map(loc => normalizeLocation(loc, filePath, workspaceRoot, resolveReturnedPath)), primaryLocationIndex: av.primaryLocationIndex, resourceUrls: av.resources }; // Add suggestions if requested and available if (includeSuggestions && av.suggestions?.length) { - violation.suggestions = av.suggestions.map(suggestion => toSuggestion(suggestion, filePath)); + violation.suggestions = av.suggestions.map(suggestion => toSuggestion(suggestion, filePath, workspaceRoot, resolveReturnedPath)); } return violation; @@ -212,9 +295,14 @@ function toViolation( * Convert ApexGuru suggestion to Code Analyzer Suggestion format * Note: suggestion.message contains "// explanation\ncode" - we keep it as-is */ -function toSuggestion(apexGuruSuggestion: ApexGuruSuggestion, filePath: string): Suggestion { +function toSuggestion( + apexGuruSuggestion: ApexGuruSuggestion, + filePath: string, + workspaceRoot: string, + resolveReturnedPath: (returnedFile: string | undefined) => string | undefined +): Suggestion { return { - location: normalizeLocation(apexGuruSuggestion.location, filePath), + location: normalizeLocation(apexGuruSuggestion.location, filePath, workspaceRoot, resolveReturnedPath), message: apexGuruSuggestion.message // Keep "// explanation\ncode" as-is }; } @@ -223,21 +311,31 @@ function toSuggestion(apexGuruSuggestion: ApexGuruSuggestion, filePath: string): * Normalize location by filling in required fields * * SFAP ApexGuru API provides: - * - file (from SFAP response, workspace-relative path) + * - file (from SFAP response, path normalized against zip contents — may have common leading dirs stripped) * - startLine (required) * - comment (optional) * * We fill in: * - startColumn = 1 (required by Code Analyzer, reasonable default if not provided) - * - Use file from location if provided, else use filePath parameter + * - Prefer the fallback filePath (already resolved via suffix-match against zipped files); + * if the location's own file matches a zipped path by suffix, use that instead + * - Resolve any remaining workspace-relative paths against workspaceRoot * - endLine/endColumn are left undefined (optional fields) */ -function normalizeLocation(location: ApexGuruLocation, filePath: string): CodeLocation { +function normalizeLocation( + location: ApexGuruLocation, + filePath: string, + workspaceRoot: string, + resolveReturnedPath: (returnedFile: string | undefined) => string | undefined +): CodeLocation { const startLine = location.startLine ?? 1; const startColumn = location.startColumn ?? 1; // Default to column 1 if not provided + const locationResolved = resolveReturnedPath(location.file); + const rawFile = locationResolved ?? location.file ?? filePath; + const resolvedFile = path.isAbsolute(rawFile) ? rawFile : path.resolve(workspaceRoot, rawFile); return { - file: location.file ?? filePath, // SFAP includes file path in response + file: resolvedFile, startLine, startColumn, endLine: location.endLine, // undefined if not provided (optional) diff --git a/packages/code-analyzer-apexguru-engine/src/services/ApexGuruAuthService.ts b/packages/code-analyzer-apexguru-engine/src/services/ApexGuruAuthService.ts index c745e058..443256db 100644 --- a/packages/code-analyzer-apexguru-engine/src/services/ApexGuruAuthService.ts +++ b/packages/code-analyzer-apexguru-engine/src/services/ApexGuruAuthService.ts @@ -40,7 +40,7 @@ export class ApexGuruAuthService { return; } catch (err) { const errorMessage = err instanceof Error ? err.message : String(err); - this.emitLogEvent(LogLevel.Error, `Failed to authenticate with org '${config.targetOrg}': ${errorMessage}`); + this.emitLogEvent(LogLevel.Fine, `Failed to authenticate with org '${config.targetOrg}': ${errorMessage}`); throw new Error( `Failed to authenticate with org '${config.targetOrg}'. ` + 'Please verify the org alias/username and ensure you are authenticated:\n' + @@ -58,7 +58,7 @@ export class ApexGuruAuthService { this.emitLogEvent(LogLevel.Fine, 'Successfully authenticated to default org'); } catch (err) { const errorMessage = err instanceof Error ? err.message : String(err); - this.emitLogEvent(LogLevel.Error, `Failed to authenticate: No default org found: ${errorMessage}`); + this.emitLogEvent(LogLevel.Fine, `Failed to authenticate: No default org found: ${errorMessage}`); throw new Error( 'No default org found. Please either:\n' + ' 1. Set a default org: sf config set target-org \n' + diff --git a/packages/code-analyzer-apexguru-engine/src/services/ApexGuruService.ts b/packages/code-analyzer-apexguru-engine/src/services/ApexGuruService.ts index bfb059a0..5f9c5565 100644 --- a/packages/code-analyzer-apexguru-engine/src/services/ApexGuruService.ts +++ b/packages/code-analyzer-apexguru-engine/src/services/ApexGuruService.ts @@ -17,6 +17,10 @@ import FormData from 'form-data'; const MAX_ZIP_SIZE_BYTES = 20 * 1024 * 1024; +// Identifies Code Analyzer as the caller on ApexGuru SFAP requests. +// Not sent on the /ide/auth JWT-mint call — that's a separate service. +const APEXGURU_CLIENT_HEADER = { 'x-apexguru-client': 'CodeAnalyzer' } as const; + /** * Service for interacting with SFAP ApexGuru workspace scan APIs */ @@ -27,7 +31,7 @@ export class ApexGuruService { private readonly initialRetryMs: number; private readonly maxRetryMs: number; private readonly backoffMultiplier: number; - private readonly sfapBaseUrl = process.env.SFAP_API_BASE_URL ?? ''; + private readonly sfapBaseUrl = 'https://dev.api.salesforce.com/platform/scale/v1-beta.1'; private progressCallback?: (progress: number) => void; private isCancelled = false; @@ -76,7 +80,7 @@ export class ApexGuruService { * the archive preserves project structure (e.g. force-app/main/default/classes/Foo.cls). * Each path in pathsToZip may be a file or a folder; contents are not inspected or filtered. */ - async scanWorkspace(workspaceRoot: string, pathsToZip: string[]): Promise<{violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata}> { + async scanWorkspace(workspaceRoot: string, pathsToZip: string[]): Promise<{violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata, analysisMode?: string}> { this.isCancelled = false; let timeoutId: NodeJS.Timeout; const scanPromise = this.performScan(workspaceRoot, pathsToZip); @@ -97,7 +101,7 @@ export class ApexGuruService { /** * Perform the full scan workflow: create zip -> submit -> poll -> decode */ - private async performScan(workspaceRoot: string, pathsToZip: string[]): Promise<{violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata}> { + private async performScan(workspaceRoot: string, pathsToZip: string[]): Promise<{violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata, analysisMode?: string}> { // Step 1: Create zip of workspace const zipBuffer = await this.createWorkspaceZip(workspaceRoot, pathsToZip); @@ -171,6 +175,7 @@ export class ApexGuruService { method: 'POST', headers: { 'Authorization': `Bearer ${orgJwt}`, + ...APEXGURU_CLIENT_HEADER, ...form.getHeaders() }, body: form.getBuffer() @@ -178,7 +183,7 @@ export class ApexGuruService { if (!response.ok) { const errorText = await response.text(); - throw new Error(`SFAP API returned ${response.status}: ${errorText}`); + throw new Error(formatHttpError(response.status, errorText)); } const submitResponse = await response.json() as ApexGuruSubmitResponse; @@ -212,13 +217,14 @@ export class ApexGuruService { const response = await fetch(url, { method: 'GET', headers: { - 'Authorization': `Bearer ${orgJwt}` + 'Authorization': `Bearer ${orgJwt}`, + ...APEXGURU_CLIENT_HEADER } }); if (!response.ok) { const errorText = await response.text(); - throw new Error(`SFAP API returned ${response.status}: ${errorText}`); + throw new Error(formatHttpError(response.status, errorText)); } const pollResponse: ApexGuruPollResponse = await response.json() as ApexGuruPollResponse; @@ -251,9 +257,13 @@ export class ApexGuruService { /** * Decode base64 report and parse violations */ - private decodeResults(pollResponse: ApexGuruPollResponse): {violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata} { + private decodeResults(pollResponse: ApexGuruPollResponse): {violations: ApexGuruViolation[], scanMetadata?: ApexGuruScanMetadata, analysisMode?: string} { if (!pollResponse.report) { - return { violations: [], scanMetadata: pollResponse.scanMetadata ?? undefined }; + return { + violations: [], + scanMetadata: pollResponse.scanMetadata ?? undefined, + analysisMode: pollResponse.analysisMode ?? undefined + }; } try { @@ -262,7 +272,8 @@ export class ApexGuruService { return { violations, - scanMetadata: pollResponse.scanMetadata ?? undefined + scanMetadata: pollResponse.scanMetadata ?? undefined, + analysisMode: pollResponse.analysisMode ?? undefined }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); @@ -290,3 +301,77 @@ function isApexSourceFile(entryPath: string): boolean { const lower = entryPath.toLowerCase(); return lower.endsWith('.cls') || lower.endsWith('.trigger'); } + +/** + * Format a non-2xx HTTP response into a concise, single-line error message. + * SFAP maintenance/error pages come back as HTML — strip tags and collapse whitespace + * so the surfaced error isn't a wall of markup. + */ +function formatHttpError(status: number, body: string): string { + const statusText = STATUS_TEXT[status] ?? 'Error'; + const detail = summarizeErrorBody(body); + return detail + ? `SFAP API returned ${status} ${statusText}: ${detail}` + : `SFAP API returned ${status} ${statusText}`; +} + +const STATUS_TEXT: Record = { + 400: 'Bad Request', + 401: 'Unauthorized', + 403: 'Forbidden', + 404: 'Not Found', + 408: 'Request Timeout', + 429: 'Too Many Requests', + 500: 'Internal Server Error', + 502: 'Bad Gateway', + 503: 'Service Unavailable', + 504: 'Gateway Timeout' +}; + +const MAX_ERROR_DETAIL_LENGTH = 300; + +/** + * Extract a short, human-readable summary from an error response body. + * Handles HTML (strips tags, keeps text), JSON (extracts common error fields), + * and plain text (collapses whitespace). Truncates to keep logs readable. + */ +function summarizeErrorBody(body: string): string { + if (!body) { + return ''; + } + const trimmed = body.trim(); + + // Try JSON first — SFAP typically returns { message, error, detail } on structured errors + if (trimmed.startsWith('{') || trimmed.startsWith('[')) { + try { + const parsed = JSON.parse(trimmed) as Record; + const message = parsed.message ?? parsed.error ?? parsed.detail ?? parsed.error_description; + if (typeof message === 'string' && message.trim()) { + return truncate(message.trim()); + } + } catch { + // fall through to text/HTML handling + } + } + + // Strip HTML tags and script/style blocks, then collapse whitespace + const stripped = trimmed + .replace(//gi, '') + .replace(//gi, '') + .replace(/<[^>]+>/g, ' ') + .replace(/ /gi, ' ') + .replace(/&/gi, '&') + .replace(/</gi, '<') + .replace(/>/gi, '>') + .replace(/"/gi, '"') + .replace(/\s+/g, ' ') + .trim(); + + return truncate(stripped); +} + +function truncate(text: string): string { + return text.length > MAX_ERROR_DETAIL_LENGTH + ? `${text.slice(0, MAX_ERROR_DETAIL_LENGTH)}…` + : text; +} diff --git a/packages/code-analyzer-apexguru-engine/test/ApexGuruEngine.test.ts b/packages/code-analyzer-apexguru-engine/test/ApexGuruEngine.test.ts index 3e1224fd..5269938c 100644 --- a/packages/code-analyzer-apexguru-engine/test/ApexGuruEngine.test.ts +++ b/packages/code-analyzer-apexguru-engine/test/ApexGuruEngine.test.ts @@ -1,22 +1,24 @@ import { ApexGuruEngine } from '../src/engine'; import { ApexGuruService } from '../src/services/ApexGuruService'; -import { RunOptions, Workspace } from '@salesforce/code-analyzer-engine-api'; +import { LogLevel, RunOptions, Workspace } from '@salesforce/code-analyzer-engine-api'; import * as fs from 'node:fs/promises'; +import * as fsSync from 'node:fs'; +import * as path from 'node:path'; // Mock dependencies jest.mock('../src/services/ApexGuruService'); jest.mock('node:fs/promises'); +jest.mock('node:fs', () => ({ + ...jest.requireActual('node:fs'), + existsSync: jest.fn().mockReturnValue(true) +})); describe('ApexGuruEngine', () => { let engine: ApexGuruEngine; let mockApexGuruService: jest.Mocked; let mockWorkspace: jest.Mocked; - let originalSfapBaseUrl: string | undefined; - beforeEach(() => { jest.clearAllMocks(); - originalSfapBaseUrl = process.env.SFAP_API_BASE_URL; - process.env.SFAP_API_BASE_URL = 'https://example.test/sfap'; mockApexGuruService = { initialize: jest.fn(), @@ -39,11 +41,7 @@ describe('ApexGuruEngine', () => { }); afterEach(() => { - if (originalSfapBaseUrl === undefined) { - delete process.env.SFAP_API_BASE_URL; - } else { - process.env.SFAP_API_BASE_URL = originalSfapBaseUrl; - } + jest.restoreAllMocks(); }); describe('getName', () => { @@ -63,17 +61,6 @@ describe('ApexGuruEngine', () => { }); describe('describeRules', () => { - it('should return empty array when SFAP_API_BASE_URL is not set', async () => { - delete process.env.SFAP_API_BASE_URL; - - const rules = await engine.describeRules({ - logFolder: '/tmp/logs', - workingFolder: '/tmp/working' - }); - - expect(rules).toEqual([]); - }); - it('should return all ApexGuru rules', async () => { const rules = await engine.describeRules({ logFolder: '/tmp/logs', @@ -156,6 +143,18 @@ describe('ApexGuruEngine', () => { expect(rules.length).toBeGreaterThan(0); expect(rules.find(r => r.name === 'SoqlInALoop')).toBeDefined(); }); + + it('should return rules without attempting authentication regardless of auth state', async () => { + const rules = await engine.describeRules({ + logFolder: '/tmp/logs', + workingFolder: '/tmp/working' + }); + + expect(rules.length).toBeGreaterThan(0); + expect(rules.find(r => r.name === 'SoqlInALoop')).toBeDefined(); + expect(rules.find(r => r.name === 'DmlInALoop')).toBeDefined(); + expect(mockApexGuruService.initialize).not.toHaveBeenCalled(); + }); }); describe('runRules', () => { @@ -186,15 +185,96 @@ describe('ApexGuruEngine', () => { await engine.runRules(['SoqlInALoop'], mockRunOptions); expect(mockApexGuruService.initialize).toHaveBeenCalledWith(undefined); - expect(mockApexGuruService.scanWorkspace).toHaveBeenCalledWith('/test/workspace', ['/test/workspace']); + expect(mockApexGuruService.scanWorkspace).toHaveBeenCalledWith('/test/workspace', ['/test/workspace/Test.cls']); }); - it('should throw error if authentication fails', async () => { - mockApexGuruService.initialize.mockRejectedValue(new Error('Auth failed')); + it('should gracefully skip with NO_ORG_CONNECTION when authentication fails', async () => { + mockApexGuruService.initialize.mockRejectedValue(new Error('No default org found')); mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); + const logSpy = jest.spyOn(engine as any, 'emitLogEvent'); + const progressSpy = jest.spyOn(engine as any, 'emitRunRulesProgressEvent'); - await expect(engine.runRules(['SoqlInALoop'], mockRunOptions)) - .rejects.toThrow('Failed to authenticate'); + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toEqual([]); + expect(results.insights).toEqual({ + status: 'skipped', + error: { + code: 'NO_ORG_CONNECTION', + message: expect.stringContaining('No default org found'), + remediation: expect.stringContaining('sf org login web') + } + }); + expect(logSpy).toHaveBeenCalledWith( + LogLevel.Warn, + expect.stringContaining('Failed to authenticate') + ); + expect(mockApexGuruService.cleanup).toHaveBeenCalled(); + expect(progressSpy).toHaveBeenCalledWith(100); + // No SFAP calls should be made after auth failure + expect(mockApexGuruService.scanWorkspace).not.toHaveBeenCalled(); + }); + + it('should gracefully skip with API_UNAVAILABLE when API is unreachable', async () => { + mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); + mockApexGuruService.scanWorkspace.mockRejectedValue(new Error('connect ECONNREFUSED 127.0.0.1:443')); + const logSpy = jest.spyOn(engine as any, 'emitLogEvent'); + + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toEqual([]); + expect(results.insights).toEqual({ + status: 'skipped', + error: { + code: 'API_UNAVAILABLE', + message: expect.stringContaining('ECONNREFUSED'), + remediation: expect.stringContaining('temporarily unavailable') + } + }); + expect(logSpy).toHaveBeenCalledWith( + LogLevel.Warn, + expect.stringContaining('unavailable') + ); + expect(mockApexGuruService.cleanup).toHaveBeenCalled(); + }); + + it('should gracefully skip with API_UNAVAILABLE on timeout', async () => { + mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); + mockApexGuruService.scanWorkspace.mockRejectedValue(new Error('Request timeout after 300000ms')); + const logSpy = jest.spyOn(engine as any, 'emitLogEvent'); + + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toEqual([]); + expect(results.insights).toEqual({ + status: 'skipped', + error: { + code: 'API_UNAVAILABLE', + message: expect.stringContaining('timeout'), + remediation: expect.stringContaining('try again later') + } + }); + expect(logSpy).toHaveBeenCalledWith(LogLevel.Warn, expect.any(String)); + }); + + it('should gracefully skip with UNEXPECTED_ERROR on non-network errors', async () => { + mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); + mockApexGuruService.scanWorkspace.mockRejectedValue(new Error('Unexpected internal failure')); + const logSpy = jest.spyOn(engine as any, 'emitLogEvent'); + + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toEqual([]); + expect(results.insights).toEqual({ + status: 'skipped', + error: { + code: 'UNEXPECTED_ERROR', + message: expect.stringContaining('Unexpected internal failure'), + remediation: expect.stringContaining('file a support ticket') + } + }); + expect(logSpy).toHaveBeenCalledWith(LogLevel.Warn, expect.any(String)); + expect(mockApexGuruService.cleanup).toHaveBeenCalled(); }); it('should return empty results if no Apex files found', async () => { @@ -219,7 +299,7 @@ describe('ApexGuruEngine', () => { expect(mockApexGuruService.cleanup).toHaveBeenCalled(); }); - it('should scan workspace with all Apex files', async () => { + it('should zip only the targeted Apex files', async () => { mockWorkspace.getTargetedFiles.mockResolvedValue([ '/test/workspace/classes/Test.cls', '/test/workspace/triggers/AccountTrigger.trigger' @@ -227,7 +307,24 @@ describe('ApexGuruEngine', () => { await engine.runRules(['SoqlInALoop'], mockRunOptions); - expect(mockApexGuruService.scanWorkspace).toHaveBeenCalledWith('/test/workspace', ['/test/workspace']); + expect(mockApexGuruService.scanWorkspace).toHaveBeenCalledWith('/test/workspace', [ + '/test/workspace/classes/Test.cls', + '/test/workspace/triggers/AccountTrigger.trigger' + ]); + }); + + it('should exclude non-Apex targeted files from the zip', async () => { + mockWorkspace.getTargetedFiles.mockResolvedValue([ + '/test/workspace/classes/Test.cls', + '/test/workspace/README.md', + '/test/workspace/lwc/foo/foo.js' + ]); + + await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(mockApexGuruService.scanWorkspace).toHaveBeenCalledWith('/test/workspace', [ + '/test/workspace/classes/Test.cls' + ]); }); it('should filter violations by selected rules', async () => { @@ -346,12 +443,17 @@ describe('ApexGuruEngine', () => { it('should cleanup even when error occurs within try block', async () => { mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); - // Make scanWorkspace throw an error + // Make scanWorkspace throw an error — should be caught and return skip result mockApexGuruService.scanWorkspace.mockRejectedValue(new Error('Fatal API error')); - await expect(engine.runRules(['SoqlInALoop'], mockRunOptions)) - .rejects.toThrow('Fatal API error'); + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + // Should not throw — error is caught and returned as UNEXPECTED_ERROR skip + expect(results.violations).toEqual([]); + expect(results.insights).toEqual({ + status: 'skipped', + error: expect.objectContaining({ code: 'UNEXPECTED_ERROR' }) + }); expect(mockApexGuruService.cleanup).toHaveBeenCalled(); }); @@ -383,7 +485,7 @@ describe('ApexGuruEngine', () => { expect(mockApexGuruService.initialize).toHaveBeenCalledWith('my-org'); }); - it('should populate insights in results when scanMetadata is returned', async () => { + it('should return insights with status completed, analysis mode, and scan metadata on success', async () => { mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); const mockScanMetadata = { analysis_mode: 'full' as const, @@ -394,22 +496,28 @@ describe('ApexGuruEngine', () => { }; mockApexGuruService.scanWorkspace.mockResolvedValue({ violations: [], - scanMetadata: mockScanMetadata + scanMetadata: mockScanMetadata, + analysisMode: 'full' }); const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); expect(results.insights).toBeDefined(); + expect(results.insights!['status']).toBe('completed'); + expect(results.insights!['analysisMode']).toBe('full'); expect(results.insights!['scan']).toEqual(mockScanMetadata); }); - it('should not include insights in results when no scanMetadata is returned', async () => { + it('should return insights with status completed even without scan metadata or analysis mode', async () => { mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); mockApexGuruService.scanWorkspace.mockResolvedValue({ violations: [] }); const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); - expect(results.insights).toBeUndefined(); + expect(results.insights).toBeDefined(); + expect(results.insights!['status']).toBe('completed'); + expect(results.insights!['analysisMode']).toBeUndefined(); + expect(results.insights!['scan']).toBeUndefined(); }); it('should use file path from SFAP violation location', async () => { @@ -431,7 +539,67 @@ describe('ApexGuruEngine', () => { expect(results.violations).toHaveLength(1); expect(results.violations[0].primaryLocationIndex).toBe(0); - expect(results.violations[0].codeLocations[0].file).toBe('force-app/main/default/classes/Test.cls'); + expect(results.violations[0].codeLocations[0].file).toBe( + path.resolve('/test/workspace', 'force-app/main/default/classes/Test.cls')); + }); + + it('should reconstruct the real absolute path when SFAP strips a common leading directory', async () => { + // Simulate the NPSP case: user targeted files under .../NPSP/unpackaged/, + // SFAP stripped "unpackaged/" from the returned path. + mockWorkspace.getWorkspaceRoot.mockReturnValue('/test/workspace'); + const zippedFile = '/test/workspace/unpackaged/config/foo/classes/Bar.cls'; + mockWorkspace.getTargetedFiles.mockResolvedValue([zippedFile]); + (fsSync.existsSync as jest.Mock).mockImplementation((p: string) => p === zippedFile); + mockApexGuruService.scanWorkspace.mockResolvedValue({ + violations: [ + { + rule: 'SoqlInALoop', + message: 'stripped path', + locations: [{ startLine: 5, file: 'config/foo/classes/Bar.cls' }], + primaryLocationIndex: 0, + severity: 1, + resources: [] + } + ] + }); + + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toHaveLength(1); + expect(results.violations[0].codeLocations[0].file).toBe(zippedFile); + }); + + it('should drop violations whose primary location file does not exist on disk', async () => { + mockWorkspace.getTargetedFiles.mockResolvedValue(['/test/workspace/Test.cls']); + const realFilePath = path.resolve('/test/workspace', 'force-app/main/default/classes/Real.cls'); + // existsSync returns true only for the real file, false for the synthetic inner-class path + (fsSync.existsSync as jest.Mock).mockImplementation((p: string) => + p === realFilePath); + mockApexGuruService.scanWorkspace.mockResolvedValue({ + violations: [ + { + rule: 'SoqlInALoop', + message: 'real file', + locations: [{ startLine: 10, file: 'force-app/main/default/classes/Real.cls' }], + primaryLocationIndex: 0, + severity: 1, + resources: [] + }, + { + rule: 'SoqlInALoop', + message: 'synthetic inner-class path', + locations: [{ startLine: 20, file: 'force-app/main/default/classes/Foo.InnerHelper.cls' }], + primaryLocationIndex: 0, + severity: 1, + resources: [] + } + ] + }); + + const results = await engine.runRules(['SoqlInALoop'], mockRunOptions); + + expect(results.violations).toHaveLength(1); + expect(results.violations[0].codeLocations[0].file).toBe(realFilePath); }); }); }); diff --git a/packages/code-analyzer-apexguru-engine/test/ApexGuruService.test.ts b/packages/code-analyzer-apexguru-engine/test/ApexGuruService.test.ts index adf323a4..11390344 100644 --- a/packages/code-analyzer-apexguru-engine/test/ApexGuruService.test.ts +++ b/packages/code-analyzer-apexguru-engine/test/ApexGuruService.test.ts @@ -7,7 +7,7 @@ jest.mock('../src/services/ApexGuruAuthService'); jest.mock('archiver'); jest.mock('node:fs'); -const TEST_SFAP_BASE_URL = 'https://example.test/sfap'; +const TEST_SFAP_BASE_URL = 'https://dev.api.salesforce.com/platform/scale/v1-beta.1'; const mockFetch = jest.fn(); globalThis.fetch = mockFetch as unknown as typeof globalThis.fetch; @@ -16,12 +16,9 @@ describe('ApexGuruService', () => { let apexGuruService: ApexGuruService; let mockEmitLogEvent: jest.Mock; let mockAuthService: jest.Mocked; - let originalSfapBaseUrl: string | undefined; beforeEach(() => { jest.clearAllMocks(); - originalSfapBaseUrl = process.env.SFAP_API_BASE_URL; - process.env.SFAP_API_BASE_URL = TEST_SFAP_BASE_URL; mockEmitLogEvent = jest.fn(); mockAuthService = { @@ -45,11 +42,7 @@ describe('ApexGuruService', () => { }); afterEach(() => { - if (originalSfapBaseUrl === undefined) { - delete process.env.SFAP_API_BASE_URL; - } else { - process.env.SFAP_API_BASE_URL = originalSfapBaseUrl; - } + jest.restoreAllMocks(); }); describe('initialize', () => { @@ -149,6 +142,7 @@ describe('ApexGuruService', () => { expect(result.violations).toEqual(mockViolations); expect(result.scanMetadata).toEqual(mockScanMetadata); + expect(result.analysisMode).toBe('full'); expect(mockFetch).toHaveBeenCalledTimes(2); // submit + poll }); @@ -484,7 +478,8 @@ describe('ApexGuruService', () => { expect.objectContaining({ method: 'POST', headers: expect.objectContaining({ - 'Authorization': 'Bearer mock-jwt-token' + 'Authorization': 'Bearer mock-jwt-token', + 'x-apexguru-client': 'CodeAnalyzer' }) }) ); @@ -495,9 +490,10 @@ describe('ApexGuruService', () => { `${TEST_SFAP_BASE_URL}/apex-guru/scan/scan-endpoint-check`, expect.objectContaining({ method: 'GET', - headers: { - 'Authorization': 'Bearer mock-jwt-token' - } + headers: expect.objectContaining({ + 'Authorization': 'Bearer mock-jwt-token', + 'x-apexguru-client': 'CodeAnalyzer' + }) }) ); }); From 82017a9c94aa0fb8e04131151722f4c193fc94e7 Mon Sep 17 00:00:00 2001 From: Nikhil Mittal Date: Tue, 14 Jul 2026 11:24:22 +0530 Subject: [PATCH 3/3] correct snapshot --- packages/code-analyzer-flow-engine/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/code-analyzer-flow-engine/package.json b/packages/code-analyzer-flow-engine/package.json index 08f350a5..30dede3f 100644 --- a/packages/code-analyzer-flow-engine/package.json +++ b/packages/code-analyzer-flow-engine/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/code-analyzer-flow-engine", "description": "Plugin package that adds 'Flow Scanner' as an engine into Salesforce Code Analyzer", - "version": "0.38.1-SNAPSHOT", + "version": "0.39.0-SNAPSHOT", "author": "The Salesforce Code Analyzer Team", "license": "BSD-3-Clause", "homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",