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' + }) }) ); });