fix(mcp): resolve sf CLI for quality hub and defect tools (B1/B2a)#151
Merged
Conversation
…B2a) 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 <noreply@anthropic.com>
Quality Orchestrator🟢 LOW · 🧪 Tests to Run · Running 4 of 41 tests
▶ Run commandnpx playwright test \
unit/mcp/automationTools.test.ts \
unit/mcp/defectTools.test.ts \
unit/mcp/qualityHubTools.test.ts \
unit/mcp/sfSpawn.test.ts⚡ quality-orchestrator · |
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 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent Salesforce CLI (sf) resolution across MCP tools by centralizing discovery/spawn behavior in sfSpawn.ts, adding Windows standalone installer lookup paths, and threading an optional sf_path override through Quality Hub + defect tooling.
Changes:
- Centralize
sfdiscovery + spawning insrc/mcp/tools/sfSpawn.ts, including a largermaxBufferto avoidENOBUFS. - Add Windows standalone installer locations to common
sfpath discovery. - Add optional
sf_pathto Quality Hub tools and the defect-create tool, and add/extend unit tests to cover the new behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/mcp/tools/sfSpawn.ts |
Implements cached sf discovery, Windows shell handling, common-path fallback (incl. Windows installer), maxBuffer, and sf_path override support. |
src/mcp/tools/automationTools.ts |
Removes duplicated sf resolution logic, uses runSfCommand from sfSpawn.ts, and re-exports helper APIs for backward compatibility. |
src/mcp/tools/qualityHubTools.ts |
Adds sf_path input to all Quality Hub MCP tools and passes it into runSfCommand. |
src/mcp/tools/defectTools.ts |
Adds sf_path input to defect-create tool and threads it through the internal sf calls. |
test/unit/mcp/qualityHubTools.test.ts |
Adds tests for sf_path threading and getSfCommonPaths behavior. |
test/unit/mcp/automationTools.test.ts |
Adds tests validating the new Windows standalone installer common paths via the (re-exported) helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+187
to
+191
| 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(); | ||
|
|
Comment on lines
+449
to
+453
| 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')), |
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getSfCommonPaths()— fixesSF_NOT_FOUNDon systems using the official Salesforce CLI Windows installer (C:\Program Files\sf\bin\sf.cmd)sfSpawn.tswith 50 MBmaxBuffer— preventsENOBUFSon verbose Provar test runs for quality hub and defect toolsautomationTools.tsintosfSpawn.ts;automationTools.tsre-exports helpers for backward compatsf_pathparameter: Added to all 6 quality hub tools and the defect create tool so users can point to a non-PATH sf executable directlyRoot cause
provar_qualityhub_connect(and all quality hub / defect tools) called a stripped-downrunSfCommandinsfSpawn.tswith no path probing, nomaxBuffer, and nosf_pathoverride. MeanwhileautomationTools.tshad a full-featured version that was never shared.Test plan
yarn test:only)node scripts/mcp-smoke.cjs)yarn compile)sf_paththreading forprovar_qualityhub_connectandprovar_qualityhub_display;SF_NOT_FOUNDwith explicit path on ENOENT; B2a Windows standalone paths ingetSfCommonPathsprovar_qualityhub_connectwithoutsf_pathon Windows standalone installer auto-discovers; with explicitsf_pathroutes correctlyNotes for reviewers
docs/mcp.mdshould be updated to document the newsf_pathparameter on quality hub tools — flagged for follow-upTOTAL_EXPECTEDchange inmcp-smoke.cjs— no new tools, only new optional paramsGenerated with Claude Code