Merge pull request #143 from ProvarTesting/fix/mcp-tool-names-underscore

v1.5.0-beta.16 fix(mcp): Windows path case-insensitivity + CI hardening

Author: mrdailey99

.github/workflows/CI_Execution.yml

@@ -22,7 +22,7 @@ jobs:
   provardx-ci-execution:
     strategy:
       matrix:
-        os: ${{ fromJSON(inputs.OS && format('[{0}]', inputs.OS) || '["ubuntu-latest", "macos-latest"]') }}
+        os: ${{ fromJSON(inputs.OS && format('[{0}]', inputs.OS) || '["ubuntu-latest", "macos-latest", "windows-latest"]') }}
         nodeversion: [20]
     runs-on: ${{ matrix.os }}
     steps:
@@ -99,6 +99,8 @@ jobs:
         run: |
           sf plugins link .
           yarn prepack
+      - name: Unit tests
+        run: npx mocha "test/**/*.test.ts" --timeout 30000
       - name: MCP smoke test
         timeout-minutes: 5
         env:

.github/workflows/DeployManual.yml

@@ -65,18 +65,21 @@ jobs:
           else
             # Auto-extract from git log since the previous tag
             if [ "${{ github.event_name }}" = "release" ]; then
-              # For a release event HEAD is already the new tag; find the one before it
-              PREV=$(git tag --sort=-creatordate | awk -v tag="${GITHUB_REF_NAME}" '$0==tag{found=1;next} found{print;exit}')
+              # Release event: HEAD is the new tag — find the nearest ancestor tag before it
+              PREV=$(git describe --tags --abbrev=0 HEAD^ 2>/dev/null || git tag --sort=-version:refname | tail -1)
             else
-              # For a manual dispatch use the most recent existing tag
-              PREV=$(git tag --sort=-creatordate | head -1)
+              # Manual dispatch: find the nearest ancestor tag from HEAD
+              # (git describe respects branch ancestry; avoids pulling in commits from sibling branches)
+              PREV=$(git describe --tags --abbrev=0 HEAD 2>/dev/null || true)
             fi
 
             RANGE="${PREV:+${PREV}..}HEAD"
 
-            RAW=$(git log --pretty=format:"%s" $RANGE \
+            RAW=$(git log --pretty=format:"%s" "$RANGE" \
               | sed 's/^[A-Z][A-Z0-9]*-[0-9]*: //' \
-              | grep -Ev "^(Merge |chore)" \
+              | grep -Ev "^(Merge |chore|bump|increment)" \
+              | grep -Ev "\(ci\):" \
+              | grep -Eiv "review comments?|merge conflict|feedback items?|test fixtures?|pre-landing|address session|address PR|session [0-9]|resolving merge|PR #[0-9]|dot\.notation|server\.tool|registerTool|bump version|increment version|testCasePath|planitem|uuid lookup|coverage (count|skew)|buildTestCase|awk regex|deploy workflow" \
               | head -20)
 
             FEATS=$(printf '%s\n' "$RAW" | grep '^feat' | sed 's/^feat[^:]*: /• /' | head -8)

docs/mcp-pilot-guide.md

@@ -502,6 +502,8 @@ PathPolicy: assertPathAllowed(filePath, allowedPaths)
 
 This check runs before every file read and write, including all path-type input fields — not just output file paths. Symlinks are dereferenced so that a symlink inside an allowed directory cannot escape containment. The allowed roots are set at server startup via `--allowed-paths` and cannot be changed while the server is running.
 
+On **Windows**, all path comparisons are performed case-insensitively. `fs.realpathSync` does not always canonicalize drive-letter case (e.g. `c:\` vs `C:\`), so the policy normalizes both the candidate path and the allowed roots to lowercase before comparing. This means `C:\Projects\MyProject` and `c:\projects\myproject` are treated as the same path for containment purposes.
+
 ### Audit log
 
 All tool invocations are logged to **stderr** with a unique `requestId` per call. The log format is structured JSON:

docs/mcp.md

@@ -473,6 +473,8 @@ All file-system operations (read, write, generate) are restricted to the paths s
 
 Symlinks are resolved via `fs.realpathSync` before the containment check, so a symlink inside an allowed directory that points outside it cannot bypass the restriction. For tools that accept multiple path inputs (such as `provar_ant_generate`'s `provar_home`, `project_path`, and `results_path`), all path fields are validated before any file operation occurs — not just the output path.
 
+On **Windows**, path comparisons are performed case-insensitively to account for the fact that `fs.realpathSync` does not always canonicalize drive-letter case (e.g. `c:\` vs `C:\`). This means `C:\Projects\my-project` and `c:\projects\my-project` are treated as equivalent when checking against `--allowed-paths`.
+
 ---
 
 ## Available tools
@@ -750,6 +752,7 @@ Validates an XML test case for schema correctness (validity score) and best prac
 - **UI-LOCATOR-001** — A UiDoAction or UiAssert `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element.
 - **SETVALUES-STRUCTURE-001** (ERROR) — A `SetValues` step's `values` argument uses `class="value"` (plain string) instead of `class="valueList"` with `<namedValues>` children. This causes an immediate `ClassCastException` at runtime.
 - **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `<path>` elements.
+- **VAR-REF-002** — A `{VarName}` token is embedded inside a larger plain string (e.g. `SELECT Id FROM Account WHERE Id = '{AccountId}'`). Provar does not perform `{…}` interpolation in string values at runtime; the braces are emitted literally. Use `class="compound"` with `<parts>` children to split the literal text and variable references. In `provar_testcase_generate`, pass the value with `{VarName}` placeholders — the generator emits compound XML automatically.
 
 ---
 

package.json

@@ -1,7 +1,7 @@
 {
   "name": "@provartesting/provardx-cli",
   "description": "A plugin for the Salesforce CLI to orchestrate testing activities and report quality metrics to Provar Quality Hub",
-  "version": "1.5.0-beta.15",
+  "version": "1.5.0-beta.16",
   "mcpName": "io.github.ProvarTesting/provar",
   "license": "BSD-3-Clause",
   "plugins": [

server.json

@@ -14,19 +14,28 @@
     "url": "https://github.com/ProvarTesting/provardx-cli",
     "source": "github"
   },
-  "version": "1.5.0-beta.15",
+  "version": "1.5.0-beta.16",
   "packages": [
     {
       "registryType": "npm",
       "identifier": "@provartesting/provardx-cli",
-      "version": "1.5.0-beta.15",
+      "version": "1.5.0-beta.16",
       "transport": {
         "type": "stdio"
       },
       "runtimeArguments": [
-        { "type": "positional", "value": "provar" },
-        { "type": "positional", "value": "mcp" },
-        { "type": "positional", "value": "start" },
+        {
+          "type": "positional",
+          "value": "provar"
+        },
+        {
+          "type": "positional",
+          "value": "mcp"
+        },
+        {
+          "type": "positional",
+          "value": "start"
+        },
         {
           "type": "named",
           "name": "--allowed-paths",

src/mcp/security/pathPolicy.ts

@@ -44,12 +44,21 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi
   try {
     resolved = fs.realpathSync(filePath);
   } catch {
-    // Path doesn't exist — resolve the parent (which should exist) to catch symlinks there
-    const parent = path.dirname(path.resolve(filePath));
+    // Path doesn't exist — walk up the ancestor hierarchy to find the deepest existing directory,
+    // resolve symlinks there, then re-attach the non-existent tail segments. This handles macOS
+    // where os.tmpdir() returns /var/... (a symlink to /private/var/...) and intermediate dirs
+    // for a new output path may not yet exist.
+    const full = path.resolve(filePath);
+    let cur = full;
+    const tail: string[] = [];
+    while (!fs.existsSync(cur) && cur !== path.dirname(cur)) {
+      tail.unshift(path.basename(cur));
+      cur = path.dirname(cur);
+    }
     try {
-      resolved = path.join(fs.realpathSync(parent), path.basename(filePath));
+      resolved = path.join(fs.realpathSync(cur), ...tail);
     } catch {
-      resolved = path.resolve(filePath);
+      resolved = full;
     }
   }
 
@@ -61,9 +70,18 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi
     }
   });
 
+  // Windows file paths are case-insensitive; fs.realpathSync does not always
+  // canonicalize drive-letter case (e.g. `c:\` vs `C:\`), so compare case-insensitively.
+  const isWindows = process.platform === 'win32';
+  const normalizeForCompare = (p: string): string => (isWindows ? p.toLowerCase() : p);
+  const resolvedKey = normalizeForCompare(resolved);
+
   if (
     resolvedAllowed.length > 0 &&
-    !resolvedAllowed.some((base) => resolved === base || resolved.startsWith(base + path.sep))
+    !resolvedAllowed.some((base) => {
+      const baseKey = normalizeForCompare(base);
+      return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep);
+    })
   ) {
     throw new PathPolicyError(
       'PATH_NOT_ALLOWED',

src/mcp/tools/testCaseGenerate.ts

@@ -150,6 +150,7 @@ const TOOL_DESCRIPTION = [
   'Cleanup warning: ApexDeleteObject steps near end of test will be skipped if an earlier step fails (stopOnError=false). Use a TearDown callable.',
   'Validation: when validate_after_edit=true (default) the response includes a validation field and returns TESTCASE_INVALID if the generated XML fails structural checks.',
   'Grounding: call provar_qualityhub_examples_retrieve before generating to get corpus examples for the scenario — correct XML structure for the step types you need.',
+  'If the response has count: 0 with a warning field (API unavailable or not configured), fall back: read the provar://docs/step-reference MCP resource for step types and attribute formats, then continue.',
 ].join(' ');
 
 export function registerTestCaseGenerate(server: McpServer, config: ServerConfig): void {
@@ -278,7 +279,29 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig
 
 // ── XML builder ───────────────────────────────────────────────────────────────
 
-// Build the <value> element for a single argument (D2/D4 aware).
+// F1/F3: build class="compound" for strings that mix literal text with {VarName} tokens.
+function buildCompoundValue(val: string, indent: string): string {
+  const i = `${indent}  `;
+  const parts: string[] = [];
+  const tokenRe = /\{([\w.]+)\}/g;
+  let last = 0;
+  let m: RegExpExecArray | null;
+  while ((m = tokenRe.exec(val)) !== null) {
+    const before = val.slice(last, m.index);
+    if (before) parts.push(`${i}<value valueClass="string">${escapeXmlContent(before)}</value>`);
+    const pathElements = m[1]
+      .split('.')
+      .map((p) => `${i}  <path element="${escapeXmlAttr(p)}"/>`)
+      .join('\n');
+    parts.push(`${i}<variable>\n${pathElements}\n${i}</variable>`);
+    last = m.index + m[0].length;
+  }
+  const tail = val.slice(last);
+  if (tail) parts.push(`${i}<value valueClass="string">${escapeXmlContent(tail)}</value>`);
+  return `${indent}<value class="compound">\n${i}<parts>\n${parts.join('\n')}\n${i}</parts>\n${indent}</value>`;
+}
+
+// Build the <value> element for a single argument (D2/D4/F1 aware).
 // inNamedValues: when true (inside SetValues namedValues), skip uiTarget/uiLocator dispatch.
 // apiId: resolved API ID used to restrict key-name dispatch to the correct UI APIs.
 function buildArgumentValue(key: string, val: string, indent: string, inNamedValues = false, apiId = ''): string {
@@ -291,6 +314,10 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal
       .join('\n');
     return `${indent}<value class="variable">\n${pathElements}\n${indent}</value>`;
   }
+  // F1/F3: {VarName} embedded in surrounding text → class="compound" with <parts>.
+  if (/\{[\w.]+\}/.test(val)) {
+    return buildCompoundValue(val, indent);
+  }
   if (!inNamedValues) {
     // D2: 'target' argument → class="uiTarget" (only for UiWithScreen / UiWithRow).
     if (key === 'target' && (apiId.includes('UiWithScreen') || apiId.includes('UiWithRow'))) {

src/mcp/tools/testCaseStepTools.ts

@@ -98,6 +98,7 @@ export function registerTestCaseStepEdit(server: McpServer, config: ServerConfig
         'Returns STEP_NOT_FOUND (with all_test_item_ids list) when the target step is absent.',
         'Returns INVALID_STEP_XML when step_xml cannot be parsed or contains ≠1 <apiCall> elements.',
         'Returns INVALID_XML_AFTER_EDIT (backup restored) when the mutated file fails validation.',
+        'Grounding for step_xml: call provar_qualityhub_examples_retrieve for corpus examples of the step type you need; if the response has count: 0 with a warning field, fall back: read the provar://docs/step-reference MCP resource.',
       ].join(' '),
       inputSchema: {
         test_case_path: z.string().describe('Absolute path to the .testcase XML file; must be within --allowed-paths'),

src/mcp/tools/testCaseValidate.ts

@@ -47,7 +47,7 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig
     {
       title: 'Validate Test Case',
       description:
-        'Validate a Provar XML test case for structural correctness and quality. Checks XML declaration, root element, required attributes (guid UUID v4, testItemId integer), <steps> presence, and applies best-practice rules. When a Provar API key is configured (via sf provar auth login or PROVAR_API_KEY env var), calls the Quality Hub API for full 170-rule scoring. Falls back to local validation if no key is set or the API is unavailable. Returns validity_score (schema compliance), quality_score (best practices, 0–100), and validation_source indicating which ruleset was applied.',
+        'Validate a Provar XML test case for structural correctness and quality. Checks XML declaration, root element, required attributes (guid UUID v4, testItemId integer), <steps> presence, and applies best-practice rules. When a Provar API key is configured (via sf provar auth login or PROVAR_API_KEY env var), calls the Quality Hub API for full 170-rule scoring. Falls back to local validation if no key is set or the API is unavailable. Returns validity_score (schema compliance), quality_score (best practices, 0–100), and validation_source indicating which ruleset was applied. When structural errors are returned, consult the provar://docs/step-reference MCP resource for correct step attribute schemas.',
       inputSchema: {
         content: z.string().optional().describe('XML content to validate directly (alias: xml)'),
         xml: z.string().optional().describe('XML content to validate — API-compatible alias for content'),
@@ -329,23 +329,43 @@ export function validateTestCase(xmlContent: string, testName?: string): TestCas
     validateApiCall(call, issues);
   }
 
-  // VAR-REF-001 (gap in both local and quality-hub-agents backend):
-  // Detect {VarName} or {Obj.Field} literals stored as plain string values.
-  // Provar will pass these as-is to the API rather than resolving them as variable references.
-  const varLiteralRe = /<value[^>]+valueClass="string"[^>]*>\{([\w.]+)\}<\/value>/g;
-  let varMatch: RegExpExecArray | null;
-  while ((varMatch = varLiteralRe.exec(xmlContent)) !== null) {
-    issues.push({
-      rule_id: 'VAR-REF-001',
-      severity: 'WARNING',
-      message: `Argument value "{${varMatch[1]}}" looks like a variable reference but is stored as a plain string — Provar will not resolve it at runtime.`,
-      applies_to: 'argument',
-      suggestion: `Replace with <value class="variable"><path element="${varMatch[1]
-        .split('.')
-        .join(
-          '"/><path element="'
-        )}"/></value>. In provar_testcase_generate, use the {VarName} syntax in the attributes object — the generator converts it automatically.`,
-    });
+  // VAR-REF-001 / VAR-REF-002: detect {VarName} tokens inside valueClass="string" elements.
+  // Provar does not interpolate {…} tokens in plain string values at runtime — they must use
+  // class="variable" (pure reference) or class="compound" (embedded in surrounding text).
+  const stringValueRe = /<value[^>]+valueClass="string"[^>]*>([^<]+)<\/value>/g;
+  let stringMatch: RegExpExecArray | null;
+  while ((stringMatch = stringValueRe.exec(xmlContent)) !== null) {
+    const rawContent = stringMatch[1];
+    if (!/\{[\w.]+\}/.test(rawContent)) continue;
+    const isPure = /^\{[\w.]+\}$/.test(rawContent.trim());
+    const varNames = [...rawContent.matchAll(/\{([\w.]+)\}/g)].map((m) => m[1]);
+    if (isPure) {
+      const varName = varNames[0];
+      issues.push({
+        rule_id: 'VAR-REF-001',
+        severity: 'WARNING',
+        message: `Argument value "{${varName}}" looks like a variable reference but is stored as a plain string — Provar will not resolve it at runtime.`,
+        applies_to: 'argument',
+        suggestion: `Replace with <value class="variable"><path element="${varName
+          .split('.')
+          .join(
+            '"/><path element="'
+          )}"/></value>. In provar_testcase_generate, use the {VarName} syntax in the attributes object — the generator converts it automatically.`,
+      });
+    } else {
+      const preview = rawContent.length > 60 ? rawContent.slice(0, 57) + '…' : rawContent;
+      issues.push({
+        rule_id: 'VAR-REF-002',
+        severity: 'WARNING',
+        message: `Argument value "${preview}" contains {${varNames.join(
+          '}, {'
+        )}} embedded in a plain string — Provar does not interpolate {…} tokens in string values at runtime.`,
+        applies_to: 'argument',
+        suggestion:
+          'Use class="compound" with <parts> to split literal text and variable references at each {VarName} boundary. ' +
+          'In provar_testcase_generate, pass the value with {VarName} placeholders in the attributes object — the generator emits compound XML automatically.',
+      });
+    }
   }
 
   return finalize(issues, tcId, tcName, apiCalls.length, xmlContent, testName);