From 7c5a97eca2708fd884a2f883cbb52e8a8d01597f Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 7 May 2026 15:43:38 -0500 Subject: [PATCH 1/3] PDX-0: fix(mcp): resolve sf CLI for quality hub and defect tools (B1/B2a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: qualityHubTools and defectTools used the simple sfSpawn runSfCommand with no maxBuffer, probing, or sf_path — causing SF_NOT_FOUND on Windows standalone installer path C:\Program Files\sf\bin\sf.cmd. Fix: Elevate sfSpawn.ts to the central sf resolution module with 50 MB maxBuffer, two-phase PATH probe, Windows standalone installer paths in getSfCommonPaths, and sf_path param on all quality hub and defect tools. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/automationTools.ts | 183 +------------------------- src/mcp/tools/defectTools.ts | 70 +++++----- src/mcp/tools/qualityHubTools.ts | 117 ++++++++++------ src/mcp/tools/sfSpawn.ts | 178 +++++++++++++++++++++++-- test/unit/mcp/automationTools.test.ts | 29 ++++ test/unit/mcp/qualityHubTools.test.ts | 78 ++++++++++- 6 files changed, 386 insertions(+), 269 deletions(-) diff --git a/src/mcp/tools/automationTools.ts b/src/mcp/tools/automationTools.ts index 3433334d..b631c760 100644 --- a/src/mcp/tools/automationTools.ts +++ b/src/mcp/tools/automationTools.ts @@ -16,187 +16,10 @@ import { log } from '../logging/logger.js'; import type { ServerConfig } from '../server.js'; import { assertPathAllowed, PathPolicyError } from '../security/pathPolicy.js'; import { parseJUnitResults } from './antTools.js'; -import { sfSpawnHelper, SfNotFoundError } from './sfSpawn.js'; +import { runSfCommand } from './sfSpawn.js'; -// ── SF CLI discovery ────────────────────────────────────────────────────────── - -/** - * Returns candidate sf CLI paths in common npm/nvm/volta install locations. - * Used as a fallback when `sf` is not in PATH. - */ -export function getSfCommonPaths(): string[] { - const home = os.homedir(); - if (process.platform === 'win32') { - const appData = process.env['APPDATA'] ?? path.join(home, 'AppData', 'Roaming'); - return [ - path.join(appData, 'npm', 'sf.cmd'), - path.join('C:', 'Program Files', 'nodejs', 'sf.cmd'), - path.join('C:', 'Program Files (x86)', 'nodejs', 'sf.cmd'), - ]; - } - const candidates = [ - '/usr/local/bin/sf', - path.join(home, '.npm-global', 'bin', 'sf'), - path.join(home, '.local', 'bin', 'sf'), - path.join(home, '.volta', 'bin', 'sf'), - ]; - // nvm — scan the three most-recently installed Node versions - const nvmBinDir = path.join(process.env['NVM_DIR'] ?? path.join(home, '.nvm'), 'versions', 'node'); - if (fs.existsSync(nvmBinDir)) { - try { - for (const v of fs.readdirSync(nvmBinDir).sort().reverse().slice(0, 3)) { - candidates.push(path.join(nvmBinDir, v, 'bin', 'sf')); - } - } catch { - /* skip */ - } - } - return candidates; -} - -// ── Shared spawn helper ─────────────────────────────────────────────────────── - -const MAX_BUFFER = 50 * 1024 * 1024; // 50 MB — prevents ENOBUFS on verbose Provar runs - -interface SpawnResult { - stdout: string; - stderr: string; - exitCode: number; -} - -// Proactively resolve the sf executable path once on first use and cache it. -// This ensures sf is always found even when ENOENT is masked by other errors (e.g. ENOBUFS). -let cachedSfPath: string | null | undefined; // undefined = not yet probed - -/** - * Exposed for testing only — pre-seeds the cached sf executable path, bypassing the probe spawn. - * Pass `undefined` to reset the cache so the next call triggers a fresh probe. - */ -export function setSfPathCacheForTesting(value: string | null | undefined): void { - cachedSfPath = value; -} - -// Platform override used in tests so Windows-specific shell logic can be exercised on any OS. -let sfPlatformOverride: NodeJS.Platform | undefined; -/** Exposed for testing only — overrides process.platform for needsWindowsShell decisions. */ -export function setSfPlatformForTesting(platform: NodeJS.Platform | undefined): void { - sfPlatformOverride = platform; -} - -/** - * Returns true when spawning `executable` requires the Windows shell. - * On Windows, `.cmd` and `.bat` batch scripts cannot be executed directly by - * Node's spawnSync — they must be invoked through cmd.exe (i.e. shell: true). - * The bare name "sf" also needs this treatment on Windows because the file on - * disk is actually "sf.cmd" and Node won't auto-append the extension. - * - * The `platform` parameter defaults to `process.platform` and is exposed for - * unit testing so tests can verify both Windows and non-Windows behaviour - * without having to run on the corresponding OS. - */ -export function needsWindowsShell(executable: string, platform = process.platform): boolean { - if (platform !== 'win32') return false; - const lower = executable.toLowerCase(); - return lower.endsWith('.cmd') || lower.endsWith('.bat') || !path.extname(lower); -} - -function resolveSfExecutable(): string | null { - if (cachedSfPath !== undefined) return cachedSfPath; - const platform = sfPlatformOverride ?? process.platform; - - // Two-phase probe avoids false-positives on Windows with shell:true. - // When shell:true is used, cmd.exe spawns successfully even when `sf` is - // missing — it exits non-zero with "not recognised" in stderr but sets no - // probe.error. Trying shell:false first catches both cases correctly. - // - // First attempt: shell:false (works on Linux/macOS; gives ENOENT on Windows if - // sf.cmd is on PATH but requires the shell). - const probe = sfSpawnHelper.spawnSync('sf', ['--version'], { - encoding: 'utf-8', - shell: false, - maxBuffer: 1024 * 1024, - }); - if (!probe.error && probe.status === 0) { - cachedSfPath = 'sf'; - return cachedSfPath; - } - - // Windows fallback: retry with shell:true when the plain probe failed - // with ENOENT — meaning sf.cmd exists on PATH but can't run without the shell. - if (platform === 'win32' && (probe.error as NodeJS.ErrnoException | undefined)?.code === 'ENOENT') { - const probeShell = sfSpawnHelper.spawnSync('sf', ['--version'], { - encoding: 'utf-8', - shell: true, - maxBuffer: 1024 * 1024, - }); - if (!probeShell.error && probeShell.status === 0) { - cachedSfPath = 'sf'; - return cachedSfPath; - } - } - - // Fall back to common install locations - for (const candidate of getSfCommonPaths()) { - if (fs.existsSync(candidate)) { - cachedSfPath = candidate; - return cachedSfPath; - } - } - cachedSfPath = null; - return null; -} - -/** - * Reject shell metacharacters in an sf_path that will be executed via shell:true. - * On Windows, cmd.exe interprets & | ; < > ` ' " and newlines as shell syntax. - * A valid filesystem path should never contain these characters. - */ -function assertShellSafePath(sfPath: string): void { - if (/[&|;<>`'"\n\r]/.test(sfPath)) { - throw Object.assign( - new Error( - 'sf_path contains characters that are unsafe for shell execution on Windows ' + - '(& | ; < > ` \' " or line-breaks). Provide an absolute filesystem path to the sf executable.' - ), - { code: 'INVALID_SF_PATH' } - ); - } -} - -function runSfCommand(args: string[], sfPath?: string): SpawnResult { - // Use explicit path if provided; otherwise use cached probe result - const executable = sfPath ?? resolveSfExecutable(); - if (!executable) throw new SfNotFoundError(); - - const platform = sfPlatformOverride ?? process.platform; - const useShell = needsWindowsShell(executable, platform); - - // Guard against injection when shell:true is used with a user-supplied path. - // Common install locations returned by resolveSfExecutable() are safe by construction. - if (useShell && sfPath) { - assertShellSafePath(sfPath); - } - - const result = sfSpawnHelper.spawnSync(executable, args, { - encoding: 'utf-8', - shell: useShell, - maxBuffer: MAX_BUFFER, - }); - - if (result.error) { - const err = result.error as NodeJS.ErrnoException; - if (err.code === 'ENOENT') { - throw new SfNotFoundError(sfPath); - } - throw result.error; - } - - return { - stdout: result.stdout ?? '', - stderr: result.stderr ?? '', - exitCode: result.status ?? 1, - }; -} +// Re-export sf resolution helpers so existing test imports from automationTools continue to work +export { getSfCommonPaths, needsWindowsShell, setSfPathCacheForTesting, setSfPlatformForTesting } from './sfSpawn.js'; function handleSpawnError( err: unknown, diff --git a/src/mcp/tools/defectTools.ts b/src/mcp/tools/defectTools.ts index c408f01f..81ca6a70 100644 --- a/src/mcp/tools/defectTools.ts +++ b/src/mcp/tools/defectTools.ts @@ -45,8 +45,8 @@ interface FailureContext { // ── SF CLI helpers ───────────────────────────────────────────────────────────── -function runSfArgs(args: string[]): { stdout: string; stderr: string; exitCode: number } { - const { stdout, stderr, exitCode } = runSfCommand(args); +function runSfArgs(args: string[], sfPath?: string): { stdout: string; stderr: string; exitCode: number } { + const { stdout, stderr, exitCode } = runSfCommand(args, sfPath); return { stdout, stderr, exitCode }; } @@ -57,35 +57,22 @@ function formatSfCommandError(action: string, exitCode: number, stderr: string, : `${action} failed with exit code ${exitCode}`; } -function runQuery(soql: string, targetOrg: string): SfQueryResponse { - const { stdout, stderr, exitCode } = runSfArgs([ - 'data', - 'query', - '--query', - soql, - '--target-org', - targetOrg, - '--json', - ]); +function runQuery(soql: string, targetOrg: string, sfPath?: string): SfQueryResponse { + const { stdout, stderr, exitCode } = runSfArgs( + ['data', 'query', '--query', soql, '--target-org', targetOrg, '--json'], + sfPath + ); if (exitCode !== 0) { throw new Error(formatSfCommandError('Salesforce query', exitCode, stderr, stdout)); } return JSON.parse(stdout) as SfQueryResponse; } -function createRecord(sobject: string, values: string, targetOrg: string): string { - const { stdout, stderr, exitCode } = runSfArgs([ - 'data', - 'create', - 'record', - '--sobject', - sobject, - '--values', - values, - '--target-org', - targetOrg, - '--json', - ]); +function createRecord(sobject: string, values: string, targetOrg: string, sfPath?: string): string { + const { stdout, stderr, exitCode } = runSfArgs( + ['data', 'create', 'record', '--sobject', sobject, '--values', values, '--target-org', targetOrg, '--json'], + sfPath + ); if (exitCode !== 0) { throw new Error(formatSfCommandError(`Failed to create ${sobject}`, exitCode, stderr, stdout)); } @@ -115,12 +102,14 @@ export interface DefectCreateResult { export function createDefectsForRun( runId: string, targetOrg: string, - failedTestFilter?: string[] + failedTestFilter?: string[], + sfPath?: string ): { created: DefectCreateResult[]; skipped: number; message: string } { // Step 1: resolve job record ID from tracking ID const jobQuery = runQuery( `SELECT Id FROM provar__Test_Plan_Schedule_Job__c WHERE provar__Tracking_Id__c = '${soqlEscape(runId)}'`, - targetOrg + targetOrg, + sfPath ); if (jobQuery.result.totalSize === 0) { throw new Error(`No Test_Plan_Schedule_Job__c found with Tracking_Id__c = '${runId}'`); @@ -133,7 +122,8 @@ export function createDefectsForRun( FROM provar__Test_Cycle__c WHERE provar__Test_Plan_Schedule_Job__c = '${soqlEscape(jobId)}' LIMIT 1`, - targetOrg + targetOrg, + sfPath ); const cycle = cycleQuery.result.records[0] ?? {}; const browser = safeText(cycle['provar__Web_Browser__c'], 100); @@ -151,7 +141,8 @@ export function createDefectsForRun( FROM provar__Test_Execution__c WHERE provar__Test_Cycle__c = '${soqlEscape(cycleId)}' AND provar__Status__c = 'Failed'`, - targetOrg + targetOrg, + sfPath ); if (execQuery.result.totalSize === 0) { @@ -185,7 +176,8 @@ export function createDefectsForRun( AND provar__Result__c = 'Fail' ORDER BY provar__Sequence_No__c ASC LIMIT 1`, - targetOrg + targetOrg, + sfPath ); const step = stepQuery.result.records[0] ?? {}; @@ -227,7 +219,7 @@ export function createDefectsForRun( `provar__Description__c="${safeText(descLines, 2000)}" ` + 'provar__Status__c="Open"'; - const defectId = createRecord('provar__Defect__c', defectValues, targetOrg); + const defectId = createRecord('provar__Defect__c', defectValues, targetOrg, sfPath); // Step 5b: link TC → Defect const tcDefectValues = @@ -235,7 +227,7 @@ export function createDefectsForRun( `provar__Test_Case__c="${testCaseId}"` + (stepId ? ` provar__Test_Step__c="${stepId}"` : ''); - const tcDefectId = createRecord('provar__Test_Case_Defect__c', tcDefectValues, targetOrg); + const tcDefectId = createRecord('provar__Test_Case_Defect__c', tcDefectValues, targetOrg, sfPath); // Step 5c: link Execution → Defect (with step execution if available) const execDefectValues = @@ -243,7 +235,7 @@ export function createDefectsForRun( `provar__Test_Execution__c="${executionId}"` + (stepExecutionId ? ` provar__Test_Step_Execution__c="${stepExecutionId}"` : ''); - const execDefectId = createRecord('provar__Test_Execution_Defect__c', execDefectValues, targetOrg); + const execDefectId = createRecord('provar__Test_Execution_Defect__c', execDefectValues, targetOrg, sfPath); log('info', 'defect created', { defectId, tcDefectId, execDefectId, executionId }); @@ -281,14 +273,22 @@ export function registerQualityHubDefectCreate(server: McpServer): void { .describe( 'Optional filter — list of Test_Case__c record ID substrings to restrict defect creation to specific failures' ), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ run_id, target_org, failed_tests }) => { + ({ run_id, target_org, failed_tests, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_defect_create', { requestId, run_id, target_org }); try { - const result = createDefectsForRun(run_id, target_org, failed_tests); + const result = createDefectsForRun(run_id, target_org, failed_tests, sf_path); const response = { requestId, ...result }; return { content: [{ type: 'text' as const, text: JSON.stringify(response) }], diff --git a/src/mcp/tools/qualityHubTools.ts b/src/mcp/tools/qualityHubTools.ts index 74d037da..aa464af6 100644 --- a/src/mcp/tools/qualityHubTools.ts +++ b/src/mcp/tools/qualityHubTools.ts @@ -46,14 +46,25 @@ export function registerQualityHubConnect(server: McpServer): void { .optional() .default([]) .describe('Additional raw CLI flags to forward (e.g. ["--json"])'), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, flags }) => { + ({ target_org, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_connect', { requestId, target_org }); try { - const result = runSfCommand(['provar', 'quality-hub', 'connect', '--target-org', target_org, ...flags]); + const result = runSfCommand( + ['provar', 'quality-hub', 'connect', '--target-org', target_org, ...flags], + sf_path + ); const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr }; if (result.exitCode !== 0) { @@ -87,9 +98,17 @@ export function registerQualityHubDisplay(server: McpServer): void { inputSchema: { target_org: z.string().optional().describe('SF org alias or username (uses default if omitted)'), flags: z.array(z.string()).optional().default([]).describe('Additional raw CLI flags to forward'), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, flags }) => { + ({ target_org, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_display', { requestId, target_org }); @@ -97,7 +116,7 @@ export function registerQualityHubDisplay(server: McpServer): void { const args = ['provar', 'quality-hub', 'display', ...flags]; if (target_org) args.splice(3, 0, '--target-org', target_org); - const result = runSfCommand(args); + const result = runSfCommand(args, sf_path); const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr }; if (result.exitCode !== 0) { @@ -155,15 +174,26 @@ export function registerQualityHubTestRun(server: McpServer): void { .describe( 'Additional raw CLI flags (e.g. ["--plan-name", "SmokeTests"]). Avoid wildcards in --plan-name values — they skip QH plan-level reporting.' ), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, flags }) => { + ({ target_org, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_testrun', { requestId, target_org }); try { const wildcardWarning = detectWildcardFlags(flags); - const result = runSfCommand(['provar', 'quality-hub', 'test', 'run', '--target-org', target_org, ...flags]); + const result = runSfCommand( + ['provar', 'quality-hub', 'test', 'run', '--target-org', target_org, ...flags], + sf_path + ); const response: Record = { requestId, exitCode: result.exitCode, @@ -207,25 +237,25 @@ export function registerQualityHubTestRunReport(server: McpServer): void { target_org: z.string().describe('SF org alias or username'), run_id: z.string().describe('Test run ID returned by provar_qualityhub_testrun'), flags: z.array(z.string()).optional().default([]).describe('Additional raw CLI flags'), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, run_id, flags }) => { + ({ target_org, run_id, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_testrun_report', { requestId, target_org, run_id }); try { - const result = runSfCommand([ - 'provar', - 'quality-hub', - 'test', - 'run', - 'report', - '--target-org', - target_org, - '--run-id', - run_id, - ...flags, - ]); + const result = runSfCommand( + ['provar', 'quality-hub', 'test', 'run', 'report', '--target-org', target_org, '--run-id', run_id, ...flags], + sf_path + ); const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr }; if (result.exitCode !== 0) { @@ -279,25 +309,25 @@ export function registerQualityHubTestRunAbort(server: McpServer): void { target_org: z.string().describe('SF org alias or username'), run_id: z.string().describe('Test run ID to abort'), flags: z.array(z.string()).optional().default([]).describe('Additional raw CLI flags'), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, run_id, flags }) => { + ({ target_org, run_id, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_testrun_abort', { requestId, target_org, run_id }); try { - const result = runSfCommand([ - 'provar', - 'quality-hub', - 'test', - 'run', - 'abort', - '--target-org', - target_org, - '--run-id', - run_id, - ...flags, - ]); + const result = runSfCommand( + ['provar', 'quality-hub', 'test', 'run', 'abort', '--target-org', target_org, '--run-id', run_id, ...flags], + sf_path + ); const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr }; if (result.exitCode !== 0) { @@ -336,22 +366,25 @@ export function registerQualityHubTestcaseRetrieve(server: McpServer): void { .optional() .default([]) .describe('Additional raw CLI flags (e.g. ["--user-story", "US-123"])'), + sf_path: z + .string() + .optional() + .describe( + 'Path to the sf CLI executable when not in PATH ' + + '(e.g. "C:\\\\Program Files\\\\sf\\\\bin\\\\sf.cmd" for the Windows standalone installer). ' + + 'Leave unset to use auto-discovery.' + ), }, }, - ({ target_org, flags }) => { + ({ target_org, flags, sf_path }) => { const requestId = makeRequestId(); log('info', 'provar_qualityhub_testcase_retrieve', { requestId, target_org }); try { - const result = runSfCommand([ - 'provar', - 'quality-hub', - 'testcase', - 'retrieve', - '--target-org', - target_org, - ...flags, - ]); + const result = runSfCommand( + ['provar', 'quality-hub', 'testcase', 'retrieve', '--target-org', target_org, ...flags], + sf_path + ); const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr }; if (result.exitCode !== 0) { diff --git a/src/mcp/tools/sfSpawn.ts b/src/mcp/tools/sfSpawn.ts index 5b7fd3c3..89078cc8 100644 --- a/src/mcp/tools/sfSpawn.ts +++ b/src/mcp/tools/sfSpawn.ts @@ -5,6 +5,9 @@ * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { spawnSync as _spawnSync } from 'node:child_process'; /** @@ -20,20 +23,18 @@ export const sfSpawnHelper = { export class SfNotFoundError extends Error { public readonly code = 'SF_NOT_FOUND'; public constructor(sfPath?: string) { - const where = sfPath - ? `at explicit path "${sfPath}"` - : 'in PATH or common npm/nvm/volta install locations'; + const where = sfPath ? `at explicit path "${sfPath}"` : 'in PATH or common npm/nvm/volta install locations'; super( `sf CLI not found ${where}. ` + - 'Install Salesforce CLI (npm install -g @salesforce/cli) and ensure the install directory is in your PATH, ' + - 'or pass sf_path pointing to the sf executable directly ' + - '(e.g. "~/.nvm/versions/node/v22.0.0/bin/sf").' + 'Install Salesforce CLI (npm install -g @salesforce/cli) and ensure the install directory is in your PATH, ' + + 'or pass sf_path pointing to the sf executable directly ' + + '(e.g. "~/.nvm/versions/node/v22.0.0/bin/sf").' ); this.name = 'SfNotFoundError'; } } -// ── Shared spawn helper ─────────────────────────────────────────────────────── +// ── Shared result type ──────────────────────────────────────────────────────── export interface SpawnResult { stdout: string; @@ -41,17 +42,172 @@ export interface SpawnResult { exitCode: number; } +const MAX_BUFFER = 50 * 1024 * 1024; // 50 MB — prevents ENOBUFS on verbose Provar runs + +// ── SF CLI discovery ────────────────────────────────────────────────────────── + +/** + * Returns candidate sf CLI paths in common install locations. + * Used as a fallback when `sf` is not in PATH. + */ +export function getSfCommonPaths(): string[] { + const home = os.homedir(); + if (process.platform === 'win32') { + const appData = process.env['APPDATA'] ?? path.join(home, 'AppData', 'Roaming'); + return [ + path.join(appData, 'npm', 'sf.cmd'), + path.join('C:', 'Program Files', 'nodejs', 'sf.cmd'), + path.join('C:', 'Program Files (x86)', 'nodejs', 'sf.cmd'), + // Windows standalone installer (https://developer.salesforce.com/tools/salesforcecli) + path.join('C:', 'Program Files', 'sf', 'bin', 'sf.cmd'), + path.join('C:', 'Program Files', 'sf', 'client', 'bin', 'sf.cmd'), + ]; + } + const candidates = [ + '/usr/local/bin/sf', + path.join(home, '.npm-global', 'bin', 'sf'), + path.join(home, '.local', 'bin', 'sf'), + path.join(home, '.volta', 'bin', 'sf'), + ]; + // nvm — scan the three most-recently installed Node versions + const nvmBinDir = path.join(process.env['NVM_DIR'] ?? path.join(home, '.nvm'), 'versions', 'node'); + if (fs.existsSync(nvmBinDir)) { + try { + for (const v of fs.readdirSync(nvmBinDir).sort().reverse().slice(0, 3)) { + candidates.push(path.join(nvmBinDir, v, 'bin', 'sf')); + } + } catch { + /* skip */ + } + } + return candidates; +} + +// Proactively resolve the sf executable path once on first use and cache it. +// This ensures sf is always found even when ENOENT is masked by other errors (e.g. ENOBUFS). +let cachedSfPath: string | null | undefined; // undefined = not yet probed + +/** + * Exposed for testing only — pre-seeds the cached sf executable path, bypassing the probe spawn. + * Pass `undefined` to reset the cache so the next call triggers a fresh probe. + */ +export function setSfPathCacheForTesting(value: string | null | undefined): void { + cachedSfPath = value; +} + +// Platform override used in tests so Windows-specific shell logic can be exercised on any OS. +let sfPlatformOverride: NodeJS.Platform | undefined; + +/** Exposed for testing only — overrides process.platform for needsWindowsShell decisions. */ +export function setSfPlatformForTesting(platform: NodeJS.Platform | undefined): void { + sfPlatformOverride = platform; +} + +/** + * Returns true when spawning `executable` requires the Windows shell. + * On Windows, `.cmd` and `.bat` batch scripts cannot be executed directly by + * Node's spawnSync — they must be invoked through cmd.exe (i.e. shell: true). + * The bare name "sf" also needs this treatment on Windows because the file on + * disk is actually "sf.cmd" and Node won't auto-append the extension. + */ +export function needsWindowsShell(executable: string, platform = process.platform): boolean { + if (platform !== 'win32') return false; + const lower = executable.toLowerCase(); + return lower.endsWith('.cmd') || lower.endsWith('.bat') || !path.extname(lower); +} + +function resolveSfExecutable(): string | null { + if (cachedSfPath !== undefined) return cachedSfPath; + const platform = sfPlatformOverride ?? process.platform; + + // Two-phase probe avoids false-positives on Windows with shell:true. + // When shell:true is used, cmd.exe spawns successfully even when `sf` is + // missing — it exits non-zero with "not recognised" in stderr but sets no + // probe.error. Trying shell:false first catches both cases correctly. + // + // First attempt: shell:false (works on Linux/macOS; gives ENOENT on Windows if + // sf.cmd is on PATH but requires the shell). + const probe = sfSpawnHelper.spawnSync('sf', ['--version'], { + encoding: 'utf-8', + shell: false, + maxBuffer: 1024 * 1024, + }); + if (!probe.error && probe.status === 0) { + cachedSfPath = 'sf'; + return cachedSfPath; + } + + // Windows fallback: retry with shell:true when the plain probe failed + // with ENOENT — meaning sf.cmd exists on PATH but can't run without the shell. + if (platform === 'win32' && (probe.error as NodeJS.ErrnoException | undefined)?.code === 'ENOENT') { + const probeShell = sfSpawnHelper.spawnSync('sf', ['--version'], { + encoding: 'utf-8', + shell: true, + maxBuffer: 1024 * 1024, + }); + if (!probeShell.error && probeShell.status === 0) { + cachedSfPath = 'sf'; + return cachedSfPath; + } + } + + // Fall back to common install locations + for (const candidate of getSfCommonPaths()) { + if (fs.existsSync(candidate)) { + cachedSfPath = candidate; + return cachedSfPath; + } + } + cachedSfPath = null; + return null; +} + +/** + * Reject shell metacharacters in an sf_path that will be executed via shell:true. + * On Windows, cmd.exe interprets & | ; < > ` ' " and newlines as shell syntax. + * A valid filesystem path should never contain these characters. + */ +function assertShellSafePath(sfPath: string): void { + if (/[&|;<>`'"\n\r]/.test(sfPath)) { + throw Object.assign( + new Error( + 'sf_path contains characters that are unsafe for shell execution on Windows ' + + '(& | ; < > ` \' " or line-breaks). Provide an absolute filesystem path to the sf executable.' + ), + { code: 'INVALID_SF_PATH' } + ); + } +} + /** * Run `sf ` synchronously and return stdout, stderr, and exit code. - * Throws SfNotFoundError if the `sf` binary is not in PATH. + * Throws SfNotFoundError if the `sf` binary cannot be found. + * Pass `sfPath` to override auto-discovery with an explicit executable path. */ -export function runSfCommand(args: string[]): SpawnResult { - const result = sfSpawnHelper.spawnSync('sf', args, { encoding: 'utf-8', shell: false }); +export function runSfCommand(args: string[], sfPath?: string): SpawnResult { + // Use explicit path if provided; otherwise use cached probe result + const executable = sfPath ?? resolveSfExecutable(); + if (!executable) throw new SfNotFoundError(); + + const platform = sfPlatformOverride ?? process.platform; + const useShell = needsWindowsShell(executable, platform); + + // Guard against injection when shell:true is used with a user-supplied path. + // Common install locations returned by resolveSfExecutable() are safe by construction. + if (useShell && sfPath) { + assertShellSafePath(sfPath); + } + + const result = sfSpawnHelper.spawnSync(executable, args, { + encoding: 'utf-8', + shell: useShell, + maxBuffer: MAX_BUFFER, + }); if (result.error) { const err = result.error as NodeJS.ErrnoException; if (err.code === 'ENOENT') { - throw new SfNotFoundError(); + throw new SfNotFoundError(sfPath); } throw result.error; } diff --git a/test/unit/mcp/automationTools.test.ts b/test/unit/mcp/automationTools.test.ts index 381ec475..2a9c3c30 100644 --- a/test/unit/mcp/automationTools.test.ts +++ b/test/unit/mcp/automationTools.test.ts @@ -16,6 +16,7 @@ import { sfSpawnHelper } from '../../../src/mcp/tools/sfSpawn.js'; import { needsWindowsShell, setSfPlatformForTesting, + getSfCommonPaths, filterTestRunOutput, setSfResultsPathForTesting, } from '../../../src/mcp/tools/automationTools.js'; @@ -928,3 +929,31 @@ describe('filterTestRunOutput', () => { assert.ok(filtered.includes('INFO Done'), 'Real output should remain'); }); }); + +// ── getSfCommonPaths — B2a Windows standalone installer paths ───────────────── + +describe('getSfCommonPaths', () => { + it('includes Windows standalone installer paths on win32 (B2a fix)', () => { + const origPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + const paths = getSfCommonPaths(); + const sfBinPath = paths.find( + (p) => p.includes('Program Files') && p.includes('sf') && p.includes('bin') && !p.includes('client') + ); + const sfClientPath = paths.find((p) => p.includes('Program Files') && p.includes('sf') && p.includes('client')); + assert.ok(sfBinPath, 'Expected C:\\Program Files\\sf\\bin\\sf.cmd in win32 paths'); + assert.ok(sfClientPath, 'Expected C:\\Program Files\\sf\\client\\bin\\sf.cmd in win32 paths'); + } finally { + if (origPlatform) Object.defineProperty(process, 'platform', origPlatform); + } + }); + + it('does not include Windows standalone paths on non-Windows platforms', () => { + if (process.platform !== 'win32') { + const paths = getSfCommonPaths(); + assert.ok(!paths.some((p) => p.includes('Program Files')), 'Windows paths should not appear on non-Windows'); + assert.ok(paths.includes('/usr/local/bin/sf'), 'Linux/macOS fallback path should be present'); + } + }); +}); diff --git a/test/unit/mcp/qualityHubTools.test.ts b/test/unit/mcp/qualityHubTools.test.ts index c7e8dc2a..e35dcf2a 100644 --- a/test/unit/mcp/qualityHubTools.test.ts +++ b/test/unit/mcp/qualityHubTools.test.ts @@ -9,7 +9,7 @@ import { strict as assert } from 'node:assert'; import sinon from 'sinon'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; -import { sfSpawnHelper } from '../../../src/mcp/tools/sfSpawn.js'; +import { sfSpawnHelper, getSfCommonPaths, setSfPathCacheForTesting } from '../../../src/mcp/tools/sfSpawn.js'; // ── Minimal mock server ─────────────────────────────────────────────────────── @@ -74,6 +74,8 @@ describe('qualityHubTools', () => { beforeEach(async () => { server = new MockMcpServer(); spawnStub = sinon.stub(sfSpawnHelper, 'spawnSync'); + // Pre-seed the sf path cache to bypass the probe spawn — tests control spawnStub directly + setSfPathCacheForTesting('sf'); const { registerAllQualityHubTools } = await import('../../../src/mcp/tools/qualityHubTools.js'); registerAllQualityHubTools(server as unknown as McpServer); @@ -81,6 +83,7 @@ describe('qualityHubTools', () => { afterEach(() => { sinon.restore(); + setSfPathCacheForTesting(undefined); // reset cache so next test probes cleanly }); // ── provar_qualityhub_connect ─────────────────────────────────────────────── @@ -391,4 +394,77 @@ describe('qualityHubTools', () => { assert.equal(parseBody(result).error_code, 'SF_NOT_FOUND'); }); }); + + // ── sf_path threading ───────────────────────────────────────────────────────── + + describe('sf_path threading', () => { + it('provar_qualityhub_connect uses explicit sf_path as the executable', () => { + spawnStub.returns(makeSpawnResult('ok', '', 0)); + server.call('provar_qualityhub_connect', { + target_org: 'myorg', + flags: [], + sf_path: '/custom/bin/sf', + }); + const [cmd] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, '/custom/bin/sf'); + }); + + it('provar_qualityhub_display uses explicit sf_path as the executable', () => { + spawnStub.returns(makeSpawnResult('ok', '', 0)); + server.call('provar_qualityhub_display', { flags: [], sf_path: '/custom/bin/sf' }); + const [cmd] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, '/custom/bin/sf'); + }); + + it('returns SF_NOT_FOUND with path hint when explicit sf_path gives ENOENT', () => { + spawnStub.returns(makeEnoentResult()); + const result = server.call('provar_qualityhub_connect', { + target_org: 'myorg', + flags: [], + sf_path: '/nonexistent/sf', + }); + assert.ok(isError(result)); + const body = parseBody(result); + assert.equal(body.error_code, 'SF_NOT_FOUND'); + assert.ok((body.message as string).includes('/nonexistent/sf'), 'message should include the bad path'); + }); + + it('returns SF_NOT_FOUND when no sf found and cache is null', () => { + setSfPathCacheForTesting(null); // simulate: probe already ran, nothing found + const result = server.call('provar_qualityhub_connect', { target_org: 'myorg', flags: [] }); + assert.ok(isError(result)); + assert.equal(parseBody(result).error_code, 'SF_NOT_FOUND'); + }); + }); + + // ── getSfCommonPaths — B2a Windows standalone installer paths ───────────────── + + describe('getSfCommonPaths', () => { + it('includes Windows standalone installer paths on win32', () => { + const origPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + const paths = getSfCommonPaths(); + assert.ok( + paths.some((p) => p.includes('Program Files') && p.includes('sf') && p.includes('bin')), + 'Expected C:\\Program Files\\sf\\bin\\sf.cmd in common paths' + ); + assert.ok( + paths.some((p) => p.includes('Program Files') && p.includes('sf') && p.includes('client')), + 'Expected C:\\Program Files\\sf\\client\\bin\\sf.cmd in common paths' + ); + } finally { + if (origPlatform) Object.defineProperty(process, 'platform', origPlatform); + } + }); + + it('returns non-empty list on non-Windows platforms', () => { + // On the current test platform (Linux/macOS), paths should include /usr/local/bin/sf + if (process.platform !== 'win32') { + const paths = getSfCommonPaths(); + assert.ok(paths.length > 0); + assert.ok(paths.includes('/usr/local/bin/sf')); + } + }); + }); }); From 71e42e39508b4268fb769c252a02d2daa40da76a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 7 May 2026 15:51:45 -0500 Subject: [PATCH 2/3] PDX-0: test(mcp): add direct unit tests for sfSpawn.ts RCA: sfSpawn.ts was elevated to the central sf resolution module but had no dedicated test file, leaving 31 code paths (SfNotFoundError, soqlEscape, getSfCommonPaths, needsWindowsShell, runSfCommand probe logic, injection guard) uncovered by direct assertions. Fix: Add test/unit/mcp/sfSpawn.test.ts covering all exported functions including two-phase probe, Windows shell injection guard, B2a standalone installer paths, and ENOENT handling. Co-Authored-By: Claude Sonnet 4.6 --- test/unit/mcp/sfSpawn.test.ts | 342 ++++++++++++++++++++++++++++++++++ 1 file changed, 342 insertions(+) create mode 100644 test/unit/mcp/sfSpawn.test.ts diff --git a/test/unit/mcp/sfSpawn.test.ts b/test/unit/mcp/sfSpawn.test.ts new file mode 100644 index 00000000..9ae3a4b5 --- /dev/null +++ b/test/unit/mcp/sfSpawn.test.ts @@ -0,0 +1,342 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +import { strict as assert } from 'node:assert'; +import os from 'node:os'; +import path from 'node:path'; +import sinon from 'sinon'; +import { + sfSpawnHelper, + SfNotFoundError, + getSfCommonPaths, + needsWindowsShell, + runSfCommand, + setSfPathCacheForTesting, + setSfPlatformForTesting, + soqlEscape, +} from '../../../src/mcp/tools/sfSpawn.js'; + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +function makeSpawnOk( + stdout = '', + stderr = '' +): { stdout: string; stderr: string; status: number; error: undefined; pid: number; output: never[]; signal: null } { + return { stdout, stderr, status: 0, error: undefined, pid: 1, output: [], signal: null }; +} + +function makeSpawnFail( + exitCode = 1, + stdout = '', + stderr = '' +): { stdout: string; stderr: string; status: number; error: undefined; pid: number; output: never[]; signal: null } { + return { stdout, stderr, status: exitCode, error: undefined, pid: 1, output: [], signal: null }; +} + +function makeEnoent(): { + stdout: string; + stderr: string; + status: null; + error: Error & { code: string }; + pid: undefined; + output: never[]; + signal: null; +} { + return { + stdout: '', + stderr: '', + status: null, + error: Object.assign(new Error('spawn sf ENOENT'), { code: 'ENOENT' }), + pid: undefined, + output: [], + signal: null, + }; +} + +// ── SfNotFoundError ─────────────────────────────────────────────────────────── + +describe('SfNotFoundError', () => { + it('has code SF_NOT_FOUND', () => { + const err = new SfNotFoundError(); + assert.equal(err.code, 'SF_NOT_FOUND'); + assert.equal(err.name, 'SfNotFoundError'); + }); + + it('generic message mentions PATH and npm install hint', () => { + const err = new SfNotFoundError(); + assert.ok(err.message.includes('npm install -g @salesforce/cli')); + assert.ok(err.message.includes('PATH')); + }); + + it('path-specific message names the explicit path', () => { + const err = new SfNotFoundError('/custom/sf'); + assert.ok(err.message.includes('/custom/sf')); + assert.ok(err.message.includes('at explicit path')); + }); +}); + +// ── soqlEscape ──────────────────────────────────────────────────────────────── + +describe('soqlEscape', () => { + it('leaves strings without quotes unchanged', () => { + assert.equal(soqlEscape('hello world'), 'hello world'); + }); + + it('escapes single quotes', () => { + assert.equal(soqlEscape("O'Brien"), "O\\'Brien"); + }); + + it('escapes multiple quotes', () => { + assert.equal(soqlEscape("it's a 'test'"), "it\\'s a \\'test\\'"); + }); + + it('handles empty string', () => { + assert.equal(soqlEscape(''), ''); + }); +}); + +// ── getSfCommonPaths ────────────────────────────────────────────────────────── + +describe('getSfCommonPaths', () => { + it('returns an array of strings', () => { + const paths = getSfCommonPaths(); + assert.ok(Array.isArray(paths)); + assert.ok(paths.length > 0); + for (const p of paths) assert.equal(typeof p, 'string'); + }); + + it('includes Windows standalone installer paths on win32', () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + try { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + const paths = getSfCommonPaths(); + assert.ok( + paths.some((p) => p.includes(path.join('Program Files', 'sf', 'bin'))), + 'should include C:\\Program Files\\sf\\bin\\sf.cmd' + ); + assert.ok( + paths.some((p) => p.includes(path.join('Program Files', 'sf', 'client', 'bin'))), + 'should include C:\\Program Files\\sf\\client\\bin\\sf.cmd' + ); + } finally { + if (originalDescriptor) Object.defineProperty(process, 'platform', originalDescriptor); + } + }); + + it('all Windows paths end with .cmd', () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + try { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + const paths = getSfCommonPaths(); + for (const p of paths) assert.ok(p.endsWith('.cmd'), `expected .cmd: ${p}`); + } finally { + if (originalDescriptor) Object.defineProperty(process, 'platform', originalDescriptor); + } + }); + + it('includes home-relative paths on non-Windows', () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + try { + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + const paths = getSfCommonPaths(); + assert.ok(paths.includes('/usr/local/bin/sf')); + assert.ok(paths.some((p) => p.includes(os.homedir()))); + } finally { + if (originalDescriptor) Object.defineProperty(process, 'platform', originalDescriptor); + } + }); +}); + +// ── needsWindowsShell ───────────────────────────────────────────────────────── + +describe('needsWindowsShell', () => { + it('returns false on non-Windows regardless of executable', () => { + assert.equal(needsWindowsShell('sf', 'linux'), false); + assert.equal(needsWindowsShell('sf.cmd', 'darwin'), false); + assert.equal(needsWindowsShell('/usr/bin/sf', 'linux'), false); + }); + + it('returns true for .cmd on win32', () => { + assert.equal(needsWindowsShell('sf.cmd', 'win32'), true); + assert.equal(needsWindowsShell('C:\\npm\\sf.cmd', 'win32'), true); + }); + + it('returns true for .bat on win32', () => { + assert.equal(needsWindowsShell('sf.bat', 'win32'), true); + }); + + it('returns true for bare name (no extension) on win32', () => { + assert.equal(needsWindowsShell('sf', 'win32'), true); + }); + + it('returns false for .exe on win32', () => { + assert.equal(needsWindowsShell('sf.exe', 'win32'), false); + }); + + it('is case-insensitive for extension check', () => { + assert.equal(needsWindowsShell('SF.CMD', 'win32'), true); + assert.equal(needsWindowsShell('SF.BAT', 'win32'), true); + }); +}); + +// ── runSfCommand ────────────────────────────────────────────────────────────── + +describe('runSfCommand', () => { + let spawnStub: sinon.SinonStub; + + beforeEach(() => { + spawnStub = sinon.stub(sfSpawnHelper, 'spawnSync'); + setSfPathCacheForTesting('sf'); + setSfPlatformForTesting('linux'); + }); + + afterEach(() => { + sinon.restore(); + setSfPathCacheForTesting(undefined); + setSfPlatformForTesting(undefined); + }); + + it('returns stdout, stderr, and exitCode on success', () => { + spawnStub.returns(makeSpawnOk('output', 'warn')); + const result = runSfCommand(['data', 'query']); + assert.equal(result.stdout, 'output'); + assert.equal(result.stderr, 'warn'); + assert.equal(result.exitCode, 0); + }); + + it('passes args array to spawnSync', () => { + spawnStub.returns(makeSpawnOk()); + runSfCommand(['provar', 'quality-hub', 'connect', '--target-org', 'myorg']); + const [cmd, args] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, 'sf'); + assert.deepEqual(args, ['provar', 'quality-hub', 'connect', '--target-org', 'myorg']); + }); + + it('uses explicit sfPath instead of cached path', () => { + spawnStub.returns(makeSpawnOk()); + runSfCommand(['--version'], '/custom/path/sf'); + const [cmd] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, '/custom/path/sf'); + }); + + it('throws SfNotFoundError when cache is null and no sfPath given', () => { + setSfPathCacheForTesting(null); + assert.throws( + () => runSfCommand(['--version']), + (err) => { + assert.ok(err instanceof SfNotFoundError); + assert.equal(err.code, 'SF_NOT_FOUND'); + return true; + } + ); + }); + + it('throws SfNotFoundError with path hint when explicit sfPath gives ENOENT', () => { + spawnStub.returns(makeEnoent()); + assert.throws( + () => runSfCommand(['--version'], '/bad/path/sf'), + (err) => { + assert.ok(err instanceof SfNotFoundError); + assert.ok(err.message.includes('/bad/path/sf')); + return true; + } + ); + }); + + it('rethrows non-ENOENT errors', () => { + const genericErr = new Error('unexpected failure'); + spawnStub.returns({ + stdout: '', + stderr: '', + status: null, + error: genericErr, + pid: undefined, + output: [], + signal: null, + }); + assert.throws(() => runSfCommand(['--version']), genericErr); + }); + + it('returns exitCode 1 when status is null and no error', () => { + spawnStub.returns({ stdout: '', stderr: '', status: null, error: undefined, pid: 1, output: [], signal: null }); + const result = runSfCommand(['--version']); + assert.equal(result.exitCode, 1); + }); + + it('returns non-zero exitCode from failed command', () => { + spawnStub.returns(makeSpawnFail(2, '', 'error text')); + const result = runSfCommand(['bad', 'command']); + assert.equal(result.exitCode, 2); + assert.equal(result.stderr, 'error text'); + }); + + describe('Windows shell path injection guard', () => { + beforeEach(() => { + setSfPlatformForTesting('win32'); + }); + + it('rejects sfPath containing & on win32', () => { + // Path ends with .cmd (triggers shell) but has & in directory name + assert.throws(() => runSfCommand(['--version'], 'C:\\sf & malicious\\sf.cmd'), /unsafe for shell execution/); + }); + + it('rejects sfPath containing | on win32', () => { + assert.throws(() => runSfCommand(['--version'], 'C:\\sf|evil\\sf.cmd'), /unsafe for shell execution/); + }); + + it('accepts clean .cmd path on win32', () => { + spawnStub.returns(makeSpawnOk('ok')); + // Should not throw — clean path + const result = runSfCommand(['--version'], 'C:\\Program Files\\sf\\bin\\sf.cmd'); + assert.equal(result.exitCode, 0); + }); + }); + + describe('probe-based resolution', () => { + beforeEach(() => { + setSfPathCacheForTesting(undefined); // force a fresh probe + setSfPlatformForTesting('linux'); + }); + + it('caches "sf" when shell:false probe succeeds', () => { + spawnStub.returns(makeSpawnOk('sf/2.0.0')); + runSfCommand(['--version']); + const [, args, opts] = spawnStub.firstCall.args as [string, string[], { shell: boolean }]; + assert.deepEqual(args, ['--version']); // probe call + assert.equal(opts.shell, false); + }); + + it('returns SF_NOT_FOUND when probe fails and no common path exists', () => { + // Both probe attempts fail with ENOENT, no common paths match + spawnStub.returns(makeEnoent()); + assert.throws( + () => runSfCommand(['--version']), + (err) => { + assert.ok(err instanceof SfNotFoundError); + return true; + } + ); + }); + + it('Windows two-phase probe: retries with shell:true on ENOENT', () => { + setSfPlatformForTesting('win32'); + // First call (shell:false) → ENOENT + spawnStub.onFirstCall().returns(makeEnoent()); + // Second call (shell:true) → success + spawnStub.onSecondCall().returns(makeSpawnOk('sf/2.0.0')); + // Third call → the actual command + spawnStub.onThirdCall().returns(makeSpawnOk('result')); + + const result = runSfCommand(['--version']); + assert.equal(result.stdout, 'result'); + + // Verify second probe used shell:true + const [, , opts] = spawnStub.secondCall.args as [string, string[], { shell: boolean }]; + assert.equal(opts.shell, true); + }); + }); +}); From bc9e11ac804895d1b6bb41f0a86beb28319e5e3c Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 7 May 2026 15:56:44 -0500 Subject: [PATCH 3/3] PDX-0: fix(mcp): address PR #151 review comments RCA: Two issues flagged by Copilot review: (1) runSfCommand passed empty-string sfPath directly to spawnSync instead of falling through to auto-discovery, producing a misleading SF_NOT_FOUND with no hint; (2) getSfCommonPaths test predicate includes(bin) was too broad and would pass even if only the client path existed. Fix: Normalize empty/whitespace sfPath to undefined in runSfCommand so auto-discovery runs; tighten qualityHubTools getSfCommonPaths assertions to exact path matching via path.join; add two tests for empty/whitespace sfPath normalization. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/sfSpawn.ts | 13 ++++++++----- test/unit/mcp/qualityHubTools.test.ts | 13 +++++-------- test/unit/mcp/sfSpawn.test.ts | 15 +++++++++++++++ 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/mcp/tools/sfSpawn.ts b/src/mcp/tools/sfSpawn.ts index 89078cc8..f103385b 100644 --- a/src/mcp/tools/sfSpawn.ts +++ b/src/mcp/tools/sfSpawn.ts @@ -185,8 +185,11 @@ function assertShellSafePath(sfPath: string): void { * Pass `sfPath` to override auto-discovery with an explicit executable path. */ export function runSfCommand(args: string[], sfPath?: string): SpawnResult { - // Use explicit path if provided; otherwise use cached probe result - const executable = sfPath ?? resolveSfExecutable(); + // Treat empty/whitespace sfPath as absent so auto-discovery runs instead of + // throwing SfNotFoundError with no useful path hint. + const trimmedSfPath = sfPath?.trim(); + const resolvedSfPath = trimmedSfPath !== '' ? trimmedSfPath : undefined; + const executable = resolvedSfPath ?? resolveSfExecutable(); if (!executable) throw new SfNotFoundError(); const platform = sfPlatformOverride ?? process.platform; @@ -194,8 +197,8 @@ export function runSfCommand(args: string[], sfPath?: string): SpawnResult { // Guard against injection when shell:true is used with a user-supplied path. // Common install locations returned by resolveSfExecutable() are safe by construction. - if (useShell && sfPath) { - assertShellSafePath(sfPath); + if (useShell && resolvedSfPath) { + assertShellSafePath(resolvedSfPath); } const result = sfSpawnHelper.spawnSync(executable, args, { @@ -207,7 +210,7 @@ export function runSfCommand(args: string[], sfPath?: string): SpawnResult { if (result.error) { const err = result.error as NodeJS.ErrnoException; if (err.code === 'ENOENT') { - throw new SfNotFoundError(sfPath); + throw new SfNotFoundError(resolvedSfPath); } throw result.error; } diff --git a/test/unit/mcp/qualityHubTools.test.ts b/test/unit/mcp/qualityHubTools.test.ts index e35dcf2a..11274d2f 100644 --- a/test/unit/mcp/qualityHubTools.test.ts +++ b/test/unit/mcp/qualityHubTools.test.ts @@ -7,6 +7,7 @@ /* eslint-disable camelcase */ import { strict as assert } from 'node:assert'; +import path from 'node:path'; import sinon from 'sinon'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { sfSpawnHelper, getSfCommonPaths, setSfPathCacheForTesting } from '../../../src/mcp/tools/sfSpawn.js'; @@ -445,14 +446,10 @@ describe('qualityHubTools', () => { Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); try { const paths = getSfCommonPaths(); - assert.ok( - paths.some((p) => p.includes('Program Files') && p.includes('sf') && p.includes('bin')), - 'Expected C:\\Program Files\\sf\\bin\\sf.cmd in common paths' - ); - assert.ok( - paths.some((p) => p.includes('Program Files') && p.includes('sf') && p.includes('client')), - 'Expected C:\\Program Files\\sf\\client\\bin\\sf.cmd in common paths' - ); + const expectedBin = path.join('C:', 'Program Files', 'sf', 'bin', 'sf.cmd'); + const expectedClientBin = path.join('C:', 'Program Files', 'sf', 'client', 'bin', 'sf.cmd'); + assert.ok(paths.includes(expectedBin), `Expected ${expectedBin} in common paths`); + assert.ok(paths.includes(expectedClientBin), `Expected ${expectedClientBin} in common paths`); } finally { if (origPlatform) Object.defineProperty(process, 'platform', origPlatform); } diff --git a/test/unit/mcp/sfSpawn.test.ts b/test/unit/mcp/sfSpawn.test.ts index 9ae3a4b5..60317464 100644 --- a/test/unit/mcp/sfSpawn.test.ts +++ b/test/unit/mcp/sfSpawn.test.ts @@ -223,6 +223,21 @@ describe('runSfCommand', () => { assert.equal(cmd, '/custom/path/sf'); }); + it('falls through to auto-discovery when sfPath is empty string', () => { + // Empty string is not a valid path — should behave as if sfPath was absent + spawnStub.returns(makeSpawnOk('ok')); + runSfCommand(['--version'], ''); + const [cmd] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, 'sf'); // uses the cached path, not the empty string + }); + + it('falls through to auto-discovery when sfPath is whitespace only', () => { + spawnStub.returns(makeSpawnOk('ok')); + runSfCommand(['--version'], ' '); + const [cmd] = spawnStub.firstCall.args as [string, string[]]; + assert.equal(cmd, 'sf'); + }); + it('throws SfNotFoundError when cache is null and no sfPath given', () => { setSfPathCacheForTesting(null); assert.throws(