From 44a72592944552ec7f3df763365a9efb253d2d4c Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 5 May 2026 16:27:49 -0500 Subject: [PATCH 01/17] PDX-0: fix(ci): filter internal commits from Slack release notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: The git log extraction picked up all feat/fix commits including internal process commits (PR review addressing, merge conflict resolution, CI tooling, and implementation details that are meaningless to end users). Fix: Add two extra grep filters — one to strip (ci)-scoped commits entirely, and one deny-list for patterns that indicate purely internal work: review comments, merge conflicts, feedback items, pre-landing fixes, session references, technical implementation tokens (testCasePath, planitem, uuid lookup, buildTestCase, server.tool, registerTool, awk regex). Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/DeployManual.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 68d8c469..578cbf6d 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -76,7 +76,9 @@ jobs: 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) From 74d90e026869673436e26c6bea69c263872c5479 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 5 May 2026 16:32:03 -0500 Subject: [PATCH 02/17] PDX-0: fix(ci): use git describe to find previous tag for Slack release notes RCA: git tag --sort=-creatordate | head -1 picks the most recently created tag regardless of branch ancestry. When a dispatch runs from a branch that diverged before the last tag was cut on develop/main, the range includes commits already shipped in the previous release. Fix: Replace with git describe --tags --abbrev=0 which walks the ancestry from HEAD and returns only a tag that is a true ancestor, guaranteeing the range only covers commits introduced since the last release on this line. Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/DeployManual.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 578cbf6d..9c92a259 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -65,11 +65,12 @@ 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 || "") fi RANGE="${PREV:+${PREV}..}HEAD" From 0cbed7ae6e2f555502c48fcf7d6d28f369a57aaa Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 08:42:23 -0500 Subject: [PATCH 03/17] PDX-0: fix(mcp): case-insensitive path comparison for Windows drive letters RCA: On Windows, fs.realpathSync does not canonicalise drive-letter case, so assertPathAllowed failed when Copilot sent lowercase c:\ paths but allowed paths used uppercase C:\. Fix: Normalise both sides of the path comparison to lowercase on win32; Linux/macOS behaviour is unchanged. Four regression tests added to pathPolicy.test.ts. Co-Authored-By: Claude Opus 4.7 --- src/mcp/security/pathPolicy.ts | 11 +++++++- test/unit/mcp/pathPolicy.test.ts | 48 ++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index 00eeef1e..c8b3f7ad 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -61,9 +61,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', diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 01276d83..38aab73c 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -2,7 +2,7 @@ import { strict as assert } from 'node:assert'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { describe, it, afterEach } from 'mocha'; +import { describe, it, afterEach, before } from 'mocha'; import { assertPathAllowed, PathPolicyError } from '../../../src/mcp/security/pathPolicy.js'; const tmp = os.tmpdir(); @@ -41,9 +41,7 @@ describe('pathPolicy', () => { }); it('allows nested paths inside allowedPaths', () => { - assert.doesNotThrow(() => - assertPathAllowed(path.join(tmp, 'a', 'b', 'c', 'file.xml'), [tmp]) - ); + assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'a', 'b', 'c', 'file.xml'), [tmp])); }); it('rejects sibling directories that share a prefix', () => { @@ -63,7 +61,11 @@ describe('pathPolicy', () => { afterEach(() => { if (symlinkDir) { - try { fs.rmSync(symlinkDir, { recursive: true, force: true }); } catch { /* ignore */ } + try { + fs.rmSync(symlinkDir, { recursive: true, force: true }); + } catch { + /* ignore */ + } } }); @@ -98,4 +100,40 @@ describe('pathPolicy', () => { assert.doesNotThrow(() => assertPathAllowed(real, [allowedDir])); }); }); + + describe('Windows case-insensitive comparison', () => { + before(function () { + if (process.platform !== 'win32') this.skip(); + }); + + it('allows a path with lowercase drive letter when allowed has uppercase', () => { + // Reproduces GitHub Copilot bug: agent sends `c:\...` but allowed path is `C:\...` + const allowed = tmp; // typically `C:\Users\\AppData\Local\Temp` + const lowerDrive = allowed.charAt(0).toLowerCase() + allowed.slice(1); + assert.doesNotThrow(() => assertPathAllowed(path.join(lowerDrive, 'foo.java'), [allowed])); + }); + + it('allows a path with uppercase drive letter when allowed has lowercase', () => { + const upperDrive = tmp.charAt(0).toUpperCase() + tmp.slice(1); + const lowerAllowed = tmp.charAt(0).toLowerCase() + tmp.slice(1); + assert.doesNotThrow(() => assertPathAllowed(path.join(upperDrive, 'foo.java'), [lowerAllowed])); + }); + + it('allows a path with mixed-case directory segments', () => { + const mixed = tmp.toUpperCase(); + assert.doesNotThrow(() => assertPathAllowed(path.join(mixed, 'foo.java'), [tmp])); + }); + + it('still rejects a sibling path that differs only after the prefix (case-insensitive)', () => { + const allowed = path.join(tmp, 'myproject'); + const sibling = path.join(tmp.toUpperCase(), 'myproject-evil', 'secret.txt'); + try { + assertPathAllowed(sibling, [allowed]); + assert.fail('Expected PathPolicyError to be thrown'); + } catch (e) { + assert.ok(e instanceof PathPolicyError, 'Expected PathPolicyError'); + assert.equal(e.code, 'PATH_NOT_ALLOWED'); + } + }); + }); }); From 5cd635fe88d74afddb4aec9a38c8744db1e74383 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 08:45:30 -0500 Subject: [PATCH 04/17] PDX-0: fix(ci): replace || "" with || true in PREV tag lookup RCA: In bash, using || "" attempts to execute an empty string as a command, which fails with "command not found" in strict environments and produces noisy stderr output in CI logs. Fix: Replace || "" with || true so PREV is empty string when no ancestor tag exists; the ${PREV:+${PREV}..}HEAD range expansion already handles the empty case correctly. Co-Authored-By: Claude Opus 4.7 --- .github/workflows/DeployManual.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 9c92a259..3072a922 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -70,7 +70,7 @@ jobs: else # 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 || "") + PREV=$(git describe --tags --abbrev=0 HEAD 2>/dev/null || true) fi RANGE="${PREV:+${PREV}..}HEAD" From 346d50cde5a865846b21e30dfc4a0f064215db1f Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 08:52:15 -0500 Subject: [PATCH 05/17] PDX-0: fix(ci): quote $RANGE, fix dot.notation regex, add unit tests to CI matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Three issues in CI workflows — $RANGE unquoted in git log allows shell injection via crafted tag names; dot.notation in grep filter was an unescaped regex matching any character; unit tests never ran in CI so Windows path-policy regression tests had zero automated coverage. Fix: Quote "$RANGE" in DeployManual.yml git log call; escape dot\.notation in grep -Eiv pattern; add windows-latest to CI_Execution.yml default OS matrix and add a unit test step before smoke tests so all platform-specific code paths (including Windows case-insensitive path comparison) run on every PR. Co-Authored-By: Claude Opus 4.7 --- .github/workflows/CI_Execution.yml | 4 +++- .github/workflows/DeployManual.yml | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/CI_Execution.yml b/.github/workflows/CI_Execution.yml index 35bb7abf..63c24dcd 100644 --- a/.github/workflows/CI_Execution.yml +++ b/.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: node node_modules/.bin/mocha "test/**/*.test.ts" --timeout 30000 - name: MCP smoke test timeout-minutes: 5 env: diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 3072a922..10da189e 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -75,11 +75,11 @@ jobs: 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|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" \ + | 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) From 014e86fd222c0e94f6923e3ef7a21b739ed9e3fa Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 08:53:40 -0500 Subject: [PATCH 06/17] PDX-0: chore: bump version to 1.5.0-beta.16 RCA: CI improvements and Windows path-policy fix constitute a release-worthy change set that requires a new beta version. Fix: Increment beta suffix from .15 to .16 in package.json and server.json; both top-level version and packages[0].version updated to stay in sync. Co-Authored-By: Claude Opus 4.7 --- package.json | 2 +- server.json | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 72041167..60481051 100644 --- a/package.json +++ b/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": [ diff --git a/server.json b/server.json index 3b203868..54eca322 100644 --- a/server.json +++ b/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", From 596abf2ae4f1812a0e8e3fea2156fe7f9d48311a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 08:56:20 -0500 Subject: [PATCH 07/17] PDX-0: docs: document Windows case-insensitive path comparison in security model RCA: The pathPolicy change (case-insensitive comparison on win32) was undocumented in the security model sections of mcp.md and mcp-pilot-guide.md, leaving users and integrators with no explanation of why c:\ and C:\ paths are treated as equivalent. Fix: Add one paragraph to each doc's path-policy section explaining the Windows case-insensitive behavior and the fs.realpathSync limitation that motivates it. Co-Authored-By: Claude Opus 4.7 --- docs/mcp-pilot-guide.md | 2 ++ docs/mcp.md | 2 ++ 2 files changed, 4 insertions(+) diff --git a/docs/mcp-pilot-guide.md b/docs/mcp-pilot-guide.md index 7da00fc1..a9589182 100644 --- a/docs/mcp-pilot-guide.md +++ b/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: diff --git a/docs/mcp.md b/docs/mcp.md index 5b25a7f7..2cff0fbd 100644 --- a/docs/mcp.md +++ b/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 From 5a43cbc40e2dea5667de016525abbaca4c9c3499 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 10:50:26 -0500 Subject: [PATCH 08/17] PDX-0: fix(mcp): fix macOS symlink resolution and backslash path normalization RCA: Two pre-existing bugs were exposed by adding macOS to CI matrix and the unit test step. On macOS, os.tmpdir() returns /var/... which is a symlink to /private/var/...; the path policy fallback only tried one parent level and fell back to path.resolve() which does not follow symlinks, causing allowed-path comparisons to fail for non-existent output paths. Separately, testPlanTools built absolute paths with path.join() before normalizing backslashes, so Windows-style paths from test fixtures resolved incorrectly on macOS/Linux where backslash is a literal character. Fix: Replace single-level parent fallback with an ancestor walk-up loop in assertPathAllowed so realpathSync is called on the deepest existing ancestor, preserving symlink resolution. Hoist normalizedTestCasePath (using existing toForwardSlashes helper) to before path.join() in testPlanTools add-instance handler so backslashes are normalized before any path ops. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/mcp/security/pathPolicy.ts | 17 +++++++++++++---- src/mcp/tools/testPlanTools.ts | 10 +++++----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index c8b3f7ad..c7a46560 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/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; } } diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index 8c01d9e3..c53e65a4 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,8 +259,9 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } - // Resolve testcase absolute path - const absoluteTestCasePath = path.join(projectRoot, test_case_path); + // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux + const normalizedTestCasePath = toForwardSlashes(test_case_path); + const absoluteTestCasePath = path.join(projectRoot, normalizedTestCasePath); if (!fs.existsSync(absoluteTestCasePath)) { return { isError: true, @@ -274,7 +275,7 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon ], }; } - if (!test_case_path.endsWith('.testcase')) { + if (!normalizedTestCasePath.endsWith('.testcase')) { return { isError: true, content: [ @@ -331,7 +332,7 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon } // Determine filename and full path - const instanceFileName = path.basename(test_case_path, '.testcase') + '.testinstance'; + const instanceFileName = path.basename(normalizedTestCasePath, '.testcase') + '.testinstance'; const instanceFilePath = path.join(instanceDir, instanceFileName); if (!overwrite && fs.existsSync(instanceFilePath)) { @@ -354,7 +355,6 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon // Build XML const guid = randomUUID(); - const normalizedTestCasePath = toForwardSlashes(test_case_path); const xmlContent = buildTestInstanceXml(guid, testCaseId, normalizedTestCasePath); if (!dry_run) { From 8c4e6a43668ed22a9ef08517e3f2de1016e92390 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 11:02:27 -0500 Subject: [PATCH 09/17] PDX-0: docs(mcp): add step-reference fallback guidance to testcase tool descriptions RCA: Three tools that generate or edit XML test steps (provar_testcase_generate, provar_testcase_step_edit, provar_testcase_validate) lacked the fallback instruction to consult the provar://docs/step-reference MCP resource when the corpus API returns count:0 with a warning field. Agents calling these tools directly (outside a loop prompt) had no guidance on where to look for step structure when corpus is unavailable. Fix: Add the standard corpus fallback sentence to all three tool descriptions, matching the pattern already used in loopPrompts.ts and migrationPrompts.ts. Update MockMcpServer/CapturingServer in the three test files to capture descriptions, and add one describe block per tool verifying the step-reference URI appears. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/testCaseGenerate.ts | 1 + src/mcp/tools/testCaseStepTools.ts | 1 + src/mcp/tools/testCaseValidate.ts | 2 +- test/unit/mcp/testCaseGenerate.test.ts | 22 ++++++++++++++++++++- test/unit/mcp/testCaseStepTools.test.ts | 22 ++++++++++++++++++++- test/unit/mcp/testCaseValidate.test.ts | 26 +++++++++++++++++++++++++ 6 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 9fd4d55f..5abd6ff7 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/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 { diff --git a/src/mcp/tools/testCaseStepTools.ts b/src/mcp/tools/testCaseStepTools.ts index 09eca655..5a24b75f 100644 --- a/src/mcp/tools/testCaseStepTools.ts +++ b/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 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'), diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index cc2f4d5f..eb327e80 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/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), 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), 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'), diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index e7334825..45804700 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -21,14 +21,17 @@ import type { ServerConfig } from '../../../src/mcp/server.js'; type ToolHandler = (args: Record) => unknown; class MockMcpServer { + public registrations: Array<{ name: string; description: string }> = []; private handlers = new Map(); public tool(name: string, _description: string, _schema: unknown, handler: ToolHandler): void { this.handlers.set(name, handler); } - public registerTool(name: string, _config: unknown, handler: ToolHandler): void { + public registerTool(name: string, config: unknown, handler: ToolHandler): void { this.handlers.set(name, handler); + const desc = (config as Record)['description']; + if (typeof desc === 'string') this.registrations.push({ name, description: desc }); } public call(name: string, args: Record): ReturnType { @@ -69,6 +72,23 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); +// ── tool description ────────────────────────────────────────────────────────── + +describe('provar_testcase_generate description', () => { + it('references corpus tool and step-reference fallback', () => { + const reg = server.registrations.find((r) => r.name === 'provar_testcase_generate'); + assert.ok(reg, 'tool should be registered'); + assert.ok( + reg.description.includes('provar_qualityhub_examples_retrieve'), + 'description should reference corpus tool' + ); + assert.ok( + reg.description.includes('provar://docs/step-reference'), + 'description should include step-reference fallback' + ); + }); +}); + // ── provar_testcase_generate ─────────────────────────────────────────────────── describe('provar_testcase_generate', () => { diff --git a/test/unit/mcp/testCaseStepTools.test.ts b/test/unit/mcp/testCaseStepTools.test.ts index 9e879b76..809cf3cd 100644 --- a/test/unit/mcp/testCaseStepTools.test.ts +++ b/test/unit/mcp/testCaseStepTools.test.ts @@ -18,14 +18,17 @@ import { registerAllTestCaseStepTools } from '../../../src/mcp/tools/testCaseSte type ToolHandler = (args: Record) => unknown; class MockMcpServer { + public registrations: Array<{ name: string; description: string }> = []; private handlers = new Map(); public tool(name: string, _desc: string, _schema: unknown, handler: ToolHandler): void { this.handlers.set(name, handler); } - public registerTool(name: string, _config: unknown, handler: ToolHandler): void { + public registerTool(name: string, config: unknown, handler: ToolHandler): void { this.handlers.set(name, handler); + const desc = (config as Record)['description']; + if (typeof desc === 'string') this.registrations.push({ name, description: desc }); } public call(name: string, args: Record): ReturnType { @@ -78,6 +81,23 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); +// ── tool description ────────────────────────────────────────────────────────── + +describe('provar_testcase_step_edit description', () => { + it('references corpus tool and step-reference fallback', () => { + const reg = server.registrations.find((r) => r.name === 'provar_testcase_step_edit'); + assert.ok(reg, 'tool should be registered'); + assert.ok( + reg.description.includes('provar_qualityhub_examples_retrieve'), + 'description should reference corpus tool' + ); + assert.ok( + reg.description.includes('provar://docs/step-reference'), + 'description should include step-reference fallback' + ); + }); +}); + // ── provar_testcase_step_edit ────────────────────────────────────────────────── describe('provar_testcase_step_edit', () => { diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index d735d7ba..81f828a7 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -797,6 +797,7 @@ describe('registerTestCaseValidate handler', () => { // Cast to McpServer via unknown — safe because registerTestCaseValidate only calls server.tool(). class CapturingServer { public capturedHandler: ((args: Record) => Promise) | null = null; + public capturedDescription: string | null = null; // eslint-disable-next-line @typescript-eslint/no-explicit-any public tool(...args: any[]): void { this.capturedHandler = args[args.length - 1] as (args: Record) => Promise; @@ -804,6 +805,8 @@ describe('registerTestCaseValidate handler', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any public registerTool(...args: any[]): void { + const config = args[1] as { description?: string }; + if (config?.description) this.capturedDescription = config.description; this.capturedHandler = args[args.length - 1] as (args: Record) => Promise; } } @@ -935,3 +938,26 @@ describe('validateTestCaseXml', () => { assert.throws(() => validateTestCaseXml(outside, makeConfig(tmpDir))); }); }); + +// ── tool description ────────────────────────────────────────────────────────── + +describe('provar_testcase_validate description', () => { + class DescriptionCapturingServer { + public capturedDescription: string | null = null; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + public registerTool(...args: any[]): void { + const config = args[1] as { description?: string }; + if (config?.description) this.capturedDescription = config.description; + } + } + + it('includes step-reference guidance', () => { + const srv = new DescriptionCapturingServer(); + registerTestCaseValidate(srv as unknown as McpServer, { allowedPaths: [] }); + assert.ok(srv.capturedDescription, 'description should be captured'); + assert.ok( + String(srv.capturedDescription).includes('provar://docs/step-reference'), + 'description should include step-reference guidance' + ); + }); +}); From dcd27c67023c913b6b806896b9440688bf3b9958 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 11:05:48 -0500 Subject: [PATCH 10/17] PDX-0: fix(ci): use npx mocha instead of node .bin/mocha for Windows compatibility RCA: node_modules/.bin/mocha is a bash shebang script on all platforms. Calling it as `node node_modules/.bin/mocha` works on Unix because npm adds a proper launcher, but on Windows Node.js tries to parse the bash script as JavaScript and throws SyntaxError. CI started failing on the windows-latest runner as soon as the unit tests step was added to the matrix. Fix: Replace `node node_modules/.bin/mocha` with `npx mocha`. npx resolves the platform-correct entry point automatically: .cmd wrapper on Windows, bash script on Unix. Uses the locally installed version (no registry download). Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/CI_Execution.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/CI_Execution.yml b/.github/workflows/CI_Execution.yml index 63c24dcd..3d1558ad 100644 --- a/.github/workflows/CI_Execution.yml +++ b/.github/workflows/CI_Execution.yml @@ -100,7 +100,7 @@ jobs: sf plugins link . yarn prepack - name: Unit tests - run: node node_modules/.bin/mocha "test/**/*.test.ts" --timeout 30000 + run: npx mocha "test/**/*.test.ts" --timeout 30000 - name: MCP smoke test timeout-minutes: 5 env: From 9eaf4eb5c0414e84a22e524cd8cb648c68f94963 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 11:47:09 -0500 Subject: [PATCH 11/17] PDX-0: feat(mcp): add compound value generation and VAR-REF-002 detection RCA: Tools lacked compound XML generation and embedded variable detection Fix: Add buildCompoundValue() to testCaseGenerate and VAR-REF-002 rule to testCaseValidate with full test coverage (888 passing) Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/testCaseGenerate.ts | 28 ++++++- src/mcp/tools/testCaseValidate.ts | 54 ++++++++----- test/unit/mcp/testCaseGenerate.test.ts | 70 +++++++++++++++++ test/unit/mcp/testCaseValidate.test.ts | 102 +++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 18 deletions(-) diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 5abd6ff7..6310a810 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -279,7 +279,29 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // ── XML builder ─────────────────────────────────────────────────────────────── -// Build the 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}${escapeXmlContent(before)}`); + const pathElements = m[1] + .split('.') + .map((p) => `${i} `) + .join('\n'); + parts.push(`${i}\n${pathElements}\n${i}`); + last = m.index + m[0].length; + } + const tail = val.slice(last); + if (tail) parts.push(`${i}${escapeXmlContent(tail)}`); + return `${indent}\n${i}\n${parts.join('\n')}\n${i}\n${indent}`; +} + +// Build the 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 { @@ -292,6 +314,10 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal .join('\n'); return `${indent}\n${pathElements}\n${indent}`; } + // F1/F3: {VarName} embedded in surrounding text → class="compound" with . + 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'))) { diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index eb327e80..7ef8d3d3 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -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 = /]+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 . 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 = /]+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 . 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 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); diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index 45804700..6de289a1 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -695,6 +695,76 @@ describe('provar_testcase_generate', () => { }); }); + describe('F1 — Compound values for {VarName} embedded in surrounding text', () => { + it('"Hello {Name}" emits class="compound" with literal and variable parts', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Compound Value Test', + steps: [ + { + api_id: 'UiDoAction', + name: 'Enter greeting', + attributes: { value: 'Hello {Name}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="compound"'), 'Expected class="compound" for mixed value'); + assert.ok(xml.includes(''), 'Expected element'); + assert.ok(xml.includes('valueClass="string">Hello '), 'Expected literal prefix as string part'); + assert.ok(xml.includes(''), 'Expected element for the token'); + assert.ok(xml.includes(''), 'Expected '); + assert.ok(!xml.includes('valueClass="string">Hello {Name}'), 'Must NOT emit raw {Name} as string literal'); + }); + + it('"{A} and {B}" emits compound with two variable parts', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Multi-Var Compound Test', + steps: [ + { + api_id: 'UiDoAction', + name: 'Combine two vars', + attributes: { value: '{First} and {Last}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="compound"'), 'Expected compound for two variables'); + assert.ok(xml.includes(''), 'Expected path for First'); + assert.ok(xml.includes(''), 'Expected path for Last'); + }); + + it('pure {VarName} alone still uses class="variable" (not compound)', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Pure Var Test', + steps: [ + { + api_id: 'UiDoAction', + name: 'Pure var', + attributes: { value: '{AccountId}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="variable"'), 'Pure token should still use class="variable"'); + assert.ok(!xml.includes('class="compound"'), 'Should not emit compound for a pure variable token'); + }); + }); + describe('D7 — Cleanup warning for ApexDeleteObject', () => { it('includes cleanup warning when ApexDeleteObject is in the step list', () => { const result = server.call('provar_testcase_generate', { diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index 81f828a7..fa8ed15f 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -788,6 +788,108 @@ describe('validateTestCase', () => { ); }); }); + + describe('VAR-REF-002', () => { + it('warns when {VarName} is embedded inside a larger string value', () => { + const r = validateTestCase( + ` + + + + + + SELECT Id FROM Account WHERE Id = '{AccountId}' + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + `Expected VAR-REF-002, got: ${JSON.stringify(r.issues.map((i) => i.rule_id))}` + ); + const issue = r.issues.find((i) => i.rule_id === 'VAR-REF-002')!; + assert.equal(issue.severity, 'WARNING'); + assert.ok(issue.message.includes('AccountId'), `Message should mention the variable: ${issue.message}`); + assert.ok(issue.suggestion?.includes('compound'), `Suggestion should mention compound: ${issue.suggestion}`); + }); + + it('warns for {NOW} system variable embedded in a SetValues string', () => { + const r = validateTestCase( + ` + + + + + + startDate=prefix_{NOW} + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + `Expected VAR-REF-002, got: ${JSON.stringify(r.issues.map((i) => i.rule_id))}` + ); + const issue = r.issues.find((i) => i.rule_id === 'VAR-REF-002')!; + assert.ok(issue.message.includes('NOW'), `Message should mention NOW: ${issue.message}`); + }); + + it('does NOT fire VAR-REF-002 for a pure {VarName} value (that is VAR-REF-001)', () => { + const r = validateTestCase( + ` + + + + + + {AccountId} + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + 'VAR-REF-002 must not fire for a pure {VarName} value' + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'VAR-REF-001 should still fire for pure {VarName}' + ); + }); + + it('does NOT fire VAR-REF-002 for correct class="compound" XML', () => { + const r = validateTestCase( + ` + + + + + + + + SELECT Id FROM Account WHERE Id = ' + + ' + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + 'VAR-REF-002 must not fire when class="compound" is used correctly' + ); + }); + }); }); // ── Handler-level tests (registerTestCaseValidate) ──────────────────────────── From 18216d84890a61a3bbfa81da77fb439cba252466 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 11:55:29 -0500 Subject: [PATCH 12/17] PDX-0: docs(mcp): document VAR-REF-002 error code for embedded variable strings RCA: docs/mcp.md lacked a VAR-REF-002 entry after the rule was added to the validator; CLAUDE.md requires new error codes to be documented. Fix: Add VAR-REF-002 bullet to the testcase validate error code section explaining compound XML usage and provar_testcase_generate auto-handling. Co-Authored-By: Claude Sonnet 4.6 --- docs/mcp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/mcp.md b/docs/mcp.md index 2cff0fbd..5024925e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -752,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 `` 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 `` 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 `` 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. --- From 21891def6512c17733d558a52ec6b362efe7afc1 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 13:35:42 -0500 Subject: [PATCH 13/17] PDX-0: feat(mcp): emit class="compound" for {VarName} tokens embedded in strings Req: When {VarName} was mixed with surrounding literal text in a step argument, the generator emitted a plain valueClass="string" element; Provar runtime passes these tokens literally rather than resolving the variable reference. The validator also lacked a rule for this compound-variable pattern. Fix: Add buildCompoundValue() in testCaseGenerate to emit class="compound"/ when a string mixes literals and {VarName} tokens; split VAR-REF-001/002 in testCaseValidate to detect both pure and embedded variable misuse. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/testCaseGenerate.ts | 28 +++++- src/mcp/tools/testCaseValidate.ts | 54 ++++++++---- test/unit/mcp/testCaseGenerate.test.ts | 117 +++++++++++++++++++++++++ test/unit/mcp/testCaseValidate.test.ts | 104 ++++++++++++++++++++++ 4 files changed, 285 insertions(+), 18 deletions(-) diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 9fd4d55f..19cba65b 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -278,7 +278,29 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // ── XML builder ─────────────────────────────────────────────────────────────── -// Build the 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}${escapeXmlContent(before)}`); + const pathElements = m[1] + .split('.') + .map((p) => `${i} `) + .join('\n'); + parts.push(`${i}\n${pathElements}\n${i}`); + last = m.index + m[0].length; + } + const tail = val.slice(last); + if (tail) parts.push(`${i}${escapeXmlContent(tail)}`); + return `${indent}\n${i}\n${parts.join('\n')}\n${i}\n${indent}`; +} + +// Build the 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 +313,10 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal .join('\n'); return `${indent}\n${pathElements}\n${indent}`; } + // F1/F3: {VarName} embedded in surrounding text → class="compound" with . + 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'))) { diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index cc2f4d5f..d4f5da8b 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -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 = /]+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 . 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 = /]+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 . 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 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); diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index e7334825..534e5070 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -744,4 +744,121 @@ describe('provar_testcase_generate', () => { assert.ok(!('validation' in body), 'validation field should be absent when validate_after_edit=false'); }); }); + + describe('F1/F3 — compound value emission for embedded {VarName} tokens', () => { + it('emits class="compound" with when a SOQL query embeds a variable (F1)', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'SOQL Compound Test', + steps: [ + { + api_id: 'ApexSoqlQuery', + name: 'Query account', + attributes: { soqlQuery: "SELECT Id, Name FROM Account WHERE Id = '{AccountId}'" }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="compound"'), 'Expected class="compound" for embedded variable in SOQL'); + assert.ok(xml.includes(''), 'Expected element inside compound value'); + assert.ok(xml.includes(''), 'Expected element for the AccountId reference'); + assert.ok(xml.includes(''), 'Expected '); + assert.ok( + !xml.includes('valueClass="string">{AccountId}'), + 'Must NOT emit {AccountId} as a plain string literal' + ); + }); + + it('emits class="compound" for Provar system variables embedded in a string (F3: {NOW})', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'NOW Compound Test', + steps: [ + { + api_id: 'SetValues', + name: 'Set account name', + attributes: { AccountName: 'Acme Corp CRUD Test {NOW}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="compound"'), 'Expected class="compound" inside namedValues'); + assert.ok(xml.includes(''), 'Expected for system variable'); + assert.ok( + !xml.includes('valueClass="string">Acme Corp CRUD Test {NOW}'), + 'Must NOT emit {NOW} as a literal string' + ); + }); + + it('emits with correct literal fragments around the variable', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Fragment Test', + steps: [ + { + api_id: 'ApexSoqlQuery', + name: 'Query with prefix and suffix', + attributes: { soqlQuery: "SELECT Id FROM Contact WHERE Email = '{Email}' LIMIT 1" }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes("SELECT Id FROM Contact WHERE Email = '"), 'Expected literal prefix fragment'); + assert.ok(xml.includes("' LIMIT 1"), 'Expected literal suffix fragment'); + assert.ok(xml.includes(''), 'Expected variable path element'); + }); + + it('handles multiple embedded variables in one string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Multi Var Test', + steps: [ + { + api_id: 'ApexSoqlQuery', + name: 'Query by two fields', + attributes: { soqlQuery: "SELECT Id FROM Case WHERE AccountId='{AccId}' AND OwnerId='{OwnerId}'" }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes(''), 'Expected first variable path'); + assert.ok(xml.includes(''), 'Expected second variable path'); + const compoundCount = (xml.match(/class="compound"/g) ?? []).length; + assert.equal(compoundCount, 1, 'Should be exactly one compound element for the soqlQuery argument'); + }); + + it('pure {VarName} value (entire argument) still uses class="variable", not compound', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'Pure Var Test', + steps: [ + { + api_id: 'ApexDeleteObject', + name: 'Delete account', + attributes: { recordId: '{AccountId}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="variable"'), 'Pure {VarName} should use class="variable"'); + assert.ok(!xml.includes('class="compound"'), 'Pure {VarName} must NOT use class="compound"'); + }); + }); }); diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index d735d7ba..6fd4f70b 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -788,6 +788,110 @@ describe('validateTestCase', () => { ); }); }); + + describe('VAR-REF-002', () => { + it('warns when {VarName} is embedded in a larger SOQL string (F1)', () => { + const r = validateTestCase( + ` + + + + + + SELECT Id, Name FROM Account WHERE Id = '{AccountId}' + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + 'Expected VAR-REF-002 for embedded variable in SOQL string' + ); + const issue = r.issues.find((i) => i.rule_id === 'VAR-REF-002')!; + assert.equal(issue.severity, 'WARNING'); + assert.ok(issue.message.includes('AccountId'), `Message should include variable name: ${issue.message}`); + assert.ok(issue.suggestion?.includes('compound'), 'Suggestion should mention compound format'); + }); + + it('warns for multiple embedded variables in one string (F3 / system vars)', () => { + const r = validateTestCase( + ` + + + + + + + + + Acme Corp CRUD Test {NOW} + + + + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + 'Expected VAR-REF-002 for {NOW} embedded in SetValues string' + ); + assert.ok(r.issues.find((i) => i.rule_id === 'VAR-REF-002')!.message.includes('NOW')); + }); + + it('does NOT fire for a pure {VarName} value (VAR-REF-001 owns that case)', () => { + const r = validateTestCase( + ` + + + + + + {AccountId} + + + + +` + ); + assert.ok(!r.issues.some((i) => i.rule_id === 'VAR-REF-002'), 'VAR-REF-002 must not double-fire on pure vars'); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'VAR-REF-001 should still fire' + ); + }); + + it('does NOT fire for a correct class="compound" value', () => { + const r = validateTestCase( + ` + + + + + + + + SELECT Id FROM Account WHERE Id = ' + + ' + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'VAR-REF-002'), + 'VAR-REF-002 must not fire on correct compound value' + ); + }); + }); }); // ── Handler-level tests (registerTestCaseValidate) ──────────────────────────── From f43e6b6d5614041a08b1c97818b37ef6e70ab0be Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 13:36:06 -0500 Subject: [PATCH 14/17] PDX-0: fix(security): address PR 143 review findings in pathPolicy, DeployManual, and testPlanTools Req: PR 143 review identified three bugs: pathPolicy containment check produced double separators for root paths (e.g. C:\ or //), DeployManual used tail -1 on a descending-sorted tag list so it selected the oldest tag instead of newest, and testPlanTools did not call assertPathAllowed on test_case_path before file access allowing directory traversal. Fix: Strip trailing separator before prefix check in pathPolicy; change tail -1 to head -1 in DeployManual; add assertPathAllowed(absoluteTestCasePath) in testPlanTools; add path traversal unit test covering the escape-via-dotdot case. Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/DeployManual.yml | 2 +- src/mcp/security/pathPolicy.ts | 4 +++- src/mcp/tools/testPlanTools.ts | 3 ++- test/unit/mcp/testPlanTools.test.ts | 22 ++++++++++++++++++++++ 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 10da189e..5612e182 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -66,7 +66,7 @@ jobs: # Auto-extract from git log since the previous tag if [ "${{ github.event_name }}" = "release" ]; then # 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) + PREV=$(git describe --tags --abbrev=0 HEAD^ 2>/dev/null || git tag --sort=-version:refname | head -1) else # Manual dispatch: find the nearest ancestor tag from HEAD # (git describe respects branch ancestry; avoids pulling in commits from sibling branches) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index c8b3f7ad..e0a52bf4 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -71,7 +71,9 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi resolvedAllowed.length > 0 && !resolvedAllowed.some((base) => { const baseKey = normalizeForCompare(base); - return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); + // Strip a trailing separator so roots like '/' or 'C:\' don't produce '//' or 'C:\\' + const baseKeyNorm = baseKey.endsWith(path.sep) ? baseKey.slice(0, -1) : baseKey; + return resolvedKey === baseKey || resolvedKey.startsWith(baseKeyNorm + path.sep); }) ) { throw new PathPolicyError( diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index 8c01d9e3..53a8ac54 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,8 +259,9 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } - // Resolve testcase absolute path + // Resolve testcase absolute path and enforce path policy before any fs access const absoluteTestCasePath = path.join(projectRoot, test_case_path); + assertPathAllowed(absoluteTestCasePath, config.allowedPaths); if (!fs.existsSync(absoluteTestCasePath)) { return { isError: true, diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index cbef770c..06a55ea2 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -507,6 +507,28 @@ describe('provar_testplan_add-instance', () => { }); }); + describe('path policy on test_case_path', () => { + it('returns PATH_NOT_ALLOWED when test_case_path escapes project root via ..', () => { + const strictServer = new MockMcpServer(); + // Use projectDir as the allowed root so ../outside.testcase is outside allowed paths + registerAllTestPlanTools(strictServer as never, { allowedPaths: [projectDir] }); + makeProject(projectDir); + makePlan(projectDir, 'P'); + + const result = strictServer.call('provar_testplan_add-instance', { + project_path: projectDir, + test_case_path: '../outside.testcase', + plan_name: 'P', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + const code = errorCode(result); + assert.ok(code === 'PATH_NOT_ALLOWED' || code === 'PATH_TRAVERSAL', `Unexpected error code: ${code}`); + }); + }); + describe('testCasePath forward-slash normalization', () => { it('normalizes backslashes to forward slashes in written XML', () => { makeProject(projectDir); From 3d71c5583e3f2734e5581f7d94637ec11b2ac1a8 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 14:08:41 -0500 Subject: [PATCH 15/17] PDX-0: fix(security): catch PATH_TRAVERSAL in test_case_path before path.join normalizes '..' away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Req: path.join(projectRoot, test_case_path) normalizes '..' segments before assertPathAllowed inspects the path, so a traversal input like '../escape.testcase' bypassed the PATH_TRAVERSAL check and was only caught by containment — meaning it went undetected entirely when allowedPaths is empty (unrestricted mode). Fix: Explicitly reject '..' segments in normalizedTestCasePath before calling path.join(), ensuring PATH_TRAVERSAL is raised regardless of allowedPaths configuration; update the unit test to use an unrestricted server (allowedPaths: []) to prove the fix covers the unrestricted-mode gap. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/testPlanTools.ts | 8 ++++++-- test/unit/mcp/testPlanTools.test.ts | 14 +++++++------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index 0461d6bc..d93d7253 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,9 +259,13 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } - // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux, - // then enforce path policy before any fs access + // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux. + // Check for '..' before path.join() normalizes them away; otherwise traversal goes undetected + // when allowedPaths is empty (unrestricted mode). Then enforce containment on the resolved path. const normalizedTestCasePath = toForwardSlashes(test_case_path); + if (normalizedTestCasePath.split('/').some((seg) => seg === '..')) { + throw new PathPolicyError('PATH_TRAVERSAL', `Path traversal detected in test_case_path: ${test_case_path}`); + } const absoluteTestCasePath = path.join(projectRoot, normalizedTestCasePath); assertPathAllowed(absoluteTestCasePath, config.allowedPaths); if (!fs.existsSync(absoluteTestCasePath)) { diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index 06a55ea2..0193ebcf 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -508,14 +508,15 @@ describe('provar_testplan_add-instance', () => { }); describe('path policy on test_case_path', () => { - it('returns PATH_NOT_ALLOWED when test_case_path escapes project root via ..', () => { - const strictServer = new MockMcpServer(); - // Use projectDir as the allowed root so ../outside.testcase is outside allowed paths - registerAllTestPlanTools(strictServer as never, { allowedPaths: [projectDir] }); + it('returns PATH_TRAVERSAL when test_case_path contains .. (rejected before path.join normalizes it)', () => { makeProject(projectDir); makePlan(projectDir, 'P'); - const result = strictServer.call('provar_testplan_add-instance', { + // Use unrestricted server (empty allowedPaths) to confirm '..' is caught even without containment check + const unrestrictedServer = new MockMcpServer(); + registerAllTestPlanTools(unrestrictedServer as never, { allowedPaths: [] }); + + const result = unrestrictedServer.call('provar_testplan_add-instance', { project_path: projectDir, test_case_path: '../outside.testcase', plan_name: 'P', @@ -524,8 +525,7 @@ describe('provar_testplan_add-instance', () => { }); assert.equal(isError(result), true); - const code = errorCode(result); - assert.ok(code === 'PATH_NOT_ALLOWED' || code === 'PATH_TRAVERSAL', `Unexpected error code: ${code}`); + assert.equal(errorCode(result), 'PATH_TRAVERSAL'); }); }); From eeda92129cd8e68173299d3370e492d4befa5b8a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 15:22:32 -0500 Subject: [PATCH 16/17] PDX-0: fix(mcp): address PR #145 review comments for security hardening RCA: Copilot review on PR #145 identified two security gaps: (1) pathPolicy.ts did not strip trailing separators from allowed-root keys, causing double-sep prefix checks ("/tmp//") and equality mismatches that incorrectly reject valid child paths when users pass roots like "/tmp/"; (2) testPlanTools.ts accepted absolute test_case_path values, allowing path.join(projectRoot, absolutePath) to silently discard projectRoot on Windows (drive-letter) and Unix, enabling reads outside the project. Fix: Strip trailing path.sep from allowed-root keys in pathPolicy (except filesystem roots "/" and "C:\"). Reject absolute test_case_path values in add-instance handler before path.join. Add two new tests: trailing-sep cases in pathPolicy.test.ts and an absolute-path rejection in testPlanTools.test.ts. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/security/pathPolicy.ts | 7 ++++++- src/mcp/tools/testPlanTools.ts | 16 ++++++++++++++++ test/unit/mcp/pathPolicy.test.ts | 8 ++++++++ test/unit/mcp/testPlanTools.test.ts | 16 ++++++++++++++++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index c7a46560..04d25f18 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -79,7 +79,12 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi if ( resolvedAllowed.length > 0 && !resolvedAllowed.some((base) => { - const baseKey = normalizeForCompare(base); + const rawBaseKey = normalizeForCompare(base); + // Strip trailing separator unless base is a filesystem root (/ on Unix, C:\ on Windows). + // A trailing sep from user input like "/tmp/" would otherwise cause double-sep prefix + // checks ("startsWith('/tmp//')") and equality mismatches ("/tmp" !== "/tmp/"). + const isRoot = rawBaseKey === path.sep || (isWindows && /^[a-z]:[/\\]$/.test(rawBaseKey)); + const baseKey = !isRoot && rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); }) ) { diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index c53e65a4..aa781982 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,6 +259,22 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } + // Reject absolute test_case_path: path.join(root, absPath) ignores root on Windows (drive-letter paths) + // and Unix, allowing the caller to escape the project directory entirely. + if (path.isAbsolute(test_case_path)) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify( + makeError('INVALID_PATH', 'test_case_path must be relative to project_path, not absolute', requestId) + ), + }, + ], + }; + } + // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux const normalizedTestCasePath = toForwardSlashes(test_case_path); const absoluteTestCasePath = path.join(projectRoot, normalizedTestCasePath); diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 38aab73c..2c0ac3c3 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -44,6 +44,14 @@ describe('pathPolicy', () => { assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'a', 'b', 'c', 'file.xml'), [tmp])); }); + it('allows path when allowed root has a trailing separator', () => { + assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'foo.java'), [tmp + path.sep])); + }); + + it('allows exact match when allowed root has a trailing separator', () => { + assert.doesNotThrow(() => assertPathAllowed(tmp, [tmp + path.sep])); + }); + it('rejects sibling directories that share a prefix', () => { const allowed = path.join(tmp, 'myproject'); const sibling = path.join(tmp, 'myproject-evil', 'secret.txt'); diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index cbef770c..1baa5bce 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -369,6 +369,22 @@ describe('provar_testplan_add-instance', () => { assert.equal(errorCode(result), 'FILE_NOT_FOUND'); }); + it('returns INVALID_PATH when test_case_path is absolute', () => { + makeProject(projectDir); + makePlan(projectDir, 'MyPlan'); + + const result = server.call('provar_testplan_add-instance', { + project_path: projectDir, + test_case_path: path.join(projectDir, 'tests', 'MyTest.testcase'), + plan_name: 'MyPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + assert.equal(errorCode(result), 'INVALID_PATH'); + }); + it('returns INVALID_PATH when test_case_path does not end with .testcase', () => { makeProject(projectDir); // Create the file but with wrong extension From 9b5f32d360626c43d58a76261d73cb9dc8ed47df Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 15:55:40 -0500 Subject: [PATCH 17/17] PDX-0: fix(mcp): address PR #146 review comments for root-path and test robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: PR #146 Copilot review found three issues in the previous security fix: (1) filesystem root allowed-paths (/ or C:\) were broken — appending path.sep to a root key produces // or C:\ so startsWith always fails for children, meaning only the root itself was allowed instead of everything under it; (2) no regression test existed for the filesystem-root case; (3) the absolute-path rejection test relied on projectDir being absolute rather than constructing the absolute path explicitly. Fix: Handle root base keys separately in assertPathAllowed — use startsWith(rawBaseKey) directly since the root already ends with its own separator. Add a filesystem-root regression test using path.parse(tmp).root. Rewrite the absolute-path test to derive the escape path from path.parse(projectDir).root so it is always absolute regardless of how projectDir is constructed. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/security/pathPolicy.ts | 7 ++++++- test/unit/mcp/pathPolicy.test.ts | 8 ++++++++ test/unit/mcp/testPlanTools.test.ts | 5 ++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index 04d25f18..3797cbe5 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -84,7 +84,12 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi // A trailing sep from user input like "/tmp/" would otherwise cause double-sep prefix // checks ("startsWith('/tmp//')") and equality mismatches ("/tmp" !== "/tmp/"). const isRoot = rawBaseKey === path.sep || (isWindows && /^[a-z]:[/\\]$/.test(rawBaseKey)); - const baseKey = !isRoot && rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; + if (isRoot) { + // Root path already ends with its own separator (/ or C:\). + // Appending path.sep would produce // or C:\\, breaking startsWith for all children. + return resolvedKey.startsWith(rawBaseKey); + } + const baseKey = rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); }) ) { diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 2c0ac3c3..35bbe6ff 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -52,6 +52,14 @@ describe('pathPolicy', () => { assert.doesNotThrow(() => assertPathAllowed(tmp, [tmp + path.sep])); }); + it('allows child paths when allowed root is the filesystem root', () => { + // path.parse gives the drive root on Windows (C:\) or / on Unix. + // Regression: prior code appended path.sep to the root, producing "//" or "C:\\\\", + // causing startsWith to fail for all children. + const fsRoot = path.parse(tmp).root; + assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'foo.java'), [fsRoot])); + }); + it('rejects sibling directories that share a prefix', () => { const allowed = path.join(tmp, 'myproject'); const sibling = path.join(tmp, 'myproject-evil', 'secret.txt'); diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index cb5f7c9a..ceb09eff 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -373,9 +373,12 @@ describe('provar_testplan_add-instance', () => { makeProject(projectDir); makePlan(projectDir, 'MyPlan'); + // Construct the absolute path from the filesystem root so the test is robust + // regardless of whether projectDir happens to be absolute or relative. + const absolutePath = path.join(path.parse(projectDir).root, 'escape', 'MyTest.testcase'); const result = server.call('provar_testplan_add-instance', { project_path: projectDir, - test_case_path: path.join(projectDir, 'tests', 'MyTest.testcase'), + test_case_path: absolutePath, plan_name: 'MyPlan', overwrite: false, dry_run: false,