From 63d75979825cf6055ac4f0a8ebec7eb1f0212a31 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 15:33:44 -0500 Subject: [PATCH 1/4] =?UTF-8?q?PDX-485:=20chore(mcp)=20=E2=80=94=20introdu?= =?UTF-8?q?ce=20shared=20warningCodes.ts=20enum=20for=20cross-thread=20fee?= =?UTF-8?q?dback=20codes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Six sibling Provar MCP thread PRs (validation, properties, automation, RCA, JUnit, parallel-mode tuning) each independently emit warnings with ad-hoc string prefixes — `WARNING:`, `[provarHome]`, `WARN —` — producing inconsistent surface area for AI agents downstream and making cross-tool typo guidance ("Did you mean...?") harder to standardise. PDX-485 wants a single canonical enum the threads can import, so warning codes are coined once, formatted once, and documented once. Fix: Adds src/mcp/utils/warningCodes.ts exporting WARNING_CODES (PROVARHOME-001, DATA-001, PARALLEL-001, SCHEMA-001, RUN-001, JUNIT-001), a WarningCode type derived from the enum, and a formatWarning(code, message, suggestion?) helper that emits `WARNING []: ` and appends ` Did you mean ''?` when a suggestion is provided. No call sites are touched in this PR — surface area is intentionally minimal so the six sibling thread PRs can import and adopt without merge conflicts. docs/mcp.md gains a new "Warning codes" reference table linked from the table of contents; per-row meanings are placeholders that subsequent thread PRs will refine. Tests: New test/unit/mcp/utils/warningCodes.test.ts covers (1) each WARNING_CODES key maps to its expected wire string, (2) formatWarning without a suggestion returns the prefixed message exactly, (3) formatWarning with a suggestion appends the "Did you mean" suffix exactly, (4) an empty-string suggestion is treated as no suggestion. Validation: yarn compile clean, yarn lint clean, full mocha 1159 passing / 0 failing. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 18 ++++++++ src/mcp/utils/warningCodes.ts | 22 ++++++++++ test/unit/mcp/utils/warningCodes.test.ts | 53 ++++++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 src/mcp/utils/warningCodes.ts create mode 100644 test/unit/mcp/utils/warningCodes.test.ts diff --git a/docs/mcp.md b/docs/mcp.md index 33d23421..3ec6bbe9 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -78,6 +78,7 @@ The Provar DX CLI ships with a built-in **Model Context Protocol (MCP) server** - [Quality scores explained](#quality-scores-explained) - [API compatibility — `xml` vs `xml_content`](#api-compatibility--xml-vs-xml_content) - [Performance Tuning](#performance-tuning) +- [Warning codes](#warning-codes) --- @@ -531,6 +532,23 @@ On **Windows**, path comparisons are performed case-insensitively to account for --- +## Warning codes + +Cross-cutting warning codes surfaced by validation, configuration, and run tooling. These complement the per-tool `rule_id` codes (e.g. `TC_001`, `VAR-REF-001`) documented under [Available tools](#available-tools). Subsequent revisions will refine the meanings as the relevant tool surfaces stabilise. + +| Code | Surfaced by | Meaning | +| ---------------- | --------------------------------------- | ------------------------------------------------------------------------- | +| `PROVARHOME-001` | properties / automation tooling | `provarHome` is missing, blank, or does not point to a Provar install | +| `DATA-001` | `provar_testcase_validate` | `` iteration is silently ignored in CLI standalone execution | +| `PARALLEL-001` | automation / run tooling | Parallel-mode cache mismatch between properties and active runtime config | +| `SCHEMA-001` | strict properties / config validators | Unknown or misspelled key in a JSON / properties schema (typo guard) | +| `RUN-001` | `provar_automation_testrun` and friends | Test run produced no executable results — check input selection | +| `JUNIT-001` | report / RCA tooling | JUnit results file is missing, empty, or not parseable | + +Warnings emitted programmatically follow the shape `WARNING []: ` — and when a typo is detected, the message is suffixed with `Did you mean ''?`. See `src/mcp/utils/warningCodes.ts` for the canonical enum. + +--- + ## Available tools ### `provardx_ping` diff --git a/src/mcp/utils/warningCodes.ts b/src/mcp/utils/warningCodes.ts new file mode 100644 index 00000000..9da5d9a3 --- /dev/null +++ b/src/mcp/utils/warningCodes.ts @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +export const WARNING_CODES = { + PROVARHOME_001: 'PROVARHOME-001', + DATA_001: 'DATA-001', + PARALLEL_001: 'PARALLEL-001', + SCHEMA_001: 'SCHEMA-001', + RUN_001: 'RUN-001', + JUNIT_001: 'JUNIT-001', +} as const; + +export type WarningCode = (typeof WARNING_CODES)[keyof typeof WARNING_CODES]; + +export function formatWarning(code: WarningCode, message: string, suggestion?: string): string { + const base = `WARNING [${code}]: ${message}`; + return suggestion ? `${base} Did you mean '${suggestion}'?` : base; +} diff --git a/test/unit/mcp/utils/warningCodes.test.ts b/test/unit/mcp/utils/warningCodes.test.ts new file mode 100644 index 00000000..3afa5b01 --- /dev/null +++ b/test/unit/mcp/utils/warningCodes.test.ts @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +import { strict as assert } from 'node:assert'; +import { describe, it } from 'mocha'; +import { WARNING_CODES, formatWarning } from '../../../../src/mcp/utils/warningCodes.js'; + +describe('WARNING_CODES', () => { + it('maps each key to its expected wire string', () => { + const expected: Record = { + PROVARHOME_001: 'PROVARHOME-001', + DATA_001: 'DATA-001', + PARALLEL_001: 'PARALLEL-001', + SCHEMA_001: 'SCHEMA-001', + RUN_001: 'RUN-001', + JUNIT_001: 'JUNIT-001', + }; + for (const [key, value] of Object.entries(expected)) { + assert.equal( + (WARNING_CODES as Record)[key], + value, + `WARNING_CODES.${key} should equal '${value}'` + ); + } + }); +}); + +describe('formatWarning', () => { + it('returns the prefixed message exactly when no suggestion is provided', () => { + assert.equal( + formatWarning(WARNING_CODES.PROVARHOME_001, 'provarHome is missing'), + 'WARNING [PROVARHOME-001]: provarHome is missing' + ); + }); + + it('appends the "Did you mean" suffix exactly when a suggestion is provided', () => { + assert.equal( + formatWarning(WARNING_CODES.SCHEMA_001, "unknown key 'parralelMode'", 'parallelMode'), + "WARNING [SCHEMA-001]: unknown key 'parralelMode' Did you mean 'parallelMode'?" + ); + }); + + it('omits the suffix when suggestion is an empty string', () => { + assert.equal( + formatWarning(WARNING_CODES.DATA_001, 'data-table iteration not bound', ''), + 'WARNING [DATA-001]: data-table iteration not bound' + ); + }); +}); From 583a1d94610aa2a7ec4e399f7bb178c173efa7bb Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:16:56 -0500 Subject: [PATCH 2/4] =?UTF-8?q?PDX-489:=20feat(mcp)=20=E2=80=94=20warn=20o?= =?UTF-8?q?n=20dataTable=20used=20in=20direct=20testCase-mode=20(DATA-001)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Provar's `` element only iterates row-by-row when the test case runs through a test-plan instance (`.testinstance`). When the project's provardx-properties.json references the test case directly via top-level `testCase` or `testCases`, the runtime silently ignores the data table and every variable reference resolves to null. The test run reports success against an empty row set — a silent-pass mode that AI agents and humans both miss until a downstream assertion fails for an unrelated reason. Prior to this change the validator emitted a generic DATA-001 advisory whenever a `` element was present, with no awareness of how the test would be wired at run time. That generic warning was also noisy in plan-mode projects where data-driven execution works correctly. Fix: Introduce `src/mcp/utils/testCasePlanMode.ts`, a resolver that consults the active `~/.sf/config.json` (`PROVARDX_PROPERTIES_FILE_PATH`) for the project's properties file, resolves `projectPath`, walks `plans/**/*.testinstance` for `testCasePath=` references to the file under validation, and returns `'direct' | 'plan' | 'unknown'`. The MCP handler in `registerTestCaseValidate` and the disk-backed `validateTestCaseXml` entry both call the resolver and pass the mode through `validateTestCase` via a new `ValidateTestCaseOptions` parameter. DATA-001 is now suppressed under plan mode, fires with the `formatWarning(WARNING_CODES.DATA_001, ...)` advisory under direct mode (recommending `provar_testplan_add-instance`), and falls back to the structural advisory under unknown mode (pure function called without file resolution — keeps existing callers' behaviour). The new helper, the new mode-aware emission helper (`maybeEmitDataTableWarning`), and the handler-level integration are covered by 12 new tests in `test/unit/mcp/testCasePlanMode.test.ts` and the PDX-489 block of `test/unit/mcp/testCaseValidate.test.ts`. Documentation: the tool description for `provar_testcase_validate` references the new behaviour, a new "Data-driven execution" subsection in `docs/mcp.md` explains the direct-vs-plan modes and the recommended workaround, the warning-codes table cross-links to it, and the DATA-001 entry in the validator section spells out the suppression rule. Thread E (testCaseValidate.ts) of the Sessions 6–7 plan; does not touch automationTools.ts or testCaseGenerate.ts per the per-thread file-ownership map (the `provar_automation_testrun` description-text reference noted in the original brief is deferred to Thread B which owns that file). --- docs/mcp.md | 51 ++++-- src/mcp/tools/testCaseValidate.ts | 108 ++++++++++--- src/mcp/utils/testCasePlanMode.ts | 214 +++++++++++++++++++++++++ test/unit/mcp/testCasePlanMode.test.ts | 177 ++++++++++++++++++++ test/unit/mcp/testCaseValidate.test.ts | 157 +++++++++++++++++- 5 files changed, 673 insertions(+), 34 deletions(-) create mode 100644 src/mcp/utils/testCasePlanMode.ts create mode 100644 test/unit/mcp/testCasePlanMode.test.ts diff --git a/docs/mcp.md b/docs/mcp.md index 3ec6bbe9..190a042d 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -50,6 +50,7 @@ The Provar DX CLI ships with a built-in **Model Context Protocol (MCP) server** - [provar_testplan_add-instance](#provar_testplan_add-instance) - [provar_testplan_create-suite](#provar_testplan_create-suite) - [provar_testplan_remove-instance](#provar_testplan_remove-instance) +- [Data-driven execution](#data-driven-execution) - [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) - [provar_nitrox_discover](#provar_nitrox_discover) - [provar_nitrox_read](#provar_nitrox_read) @@ -536,14 +537,14 @@ On **Windows**, path comparisons are performed case-insensitively to account for Cross-cutting warning codes surfaced by validation, configuration, and run tooling. These complement the per-tool `rule_id` codes (e.g. `TC_001`, `VAR-REF-001`) documented under [Available tools](#available-tools). Subsequent revisions will refine the meanings as the relevant tool surfaces stabilise. -| Code | Surfaced by | Meaning | -| ---------------- | --------------------------------------- | ------------------------------------------------------------------------- | -| `PROVARHOME-001` | properties / automation tooling | `provarHome` is missing, blank, or does not point to a Provar install | -| `DATA-001` | `provar_testcase_validate` | `` iteration is silently ignored in CLI standalone execution | -| `PARALLEL-001` | automation / run tooling | Parallel-mode cache mismatch between properties and active runtime config | -| `SCHEMA-001` | strict properties / config validators | Unknown or misspelled key in a JSON / properties schema (typo guard) | -| `RUN-001` | `provar_automation_testrun` and friends | Test run produced no executable results — check input selection | -| `JUNIT-001` | report / RCA tooling | JUnit results file is missing, empty, or not parseable | +| Code | Surfaced by | Meaning | +| ---------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | +| `PROVARHOME-001` | properties / automation tooling | `provarHome` is missing, blank, or does not point to a Provar install | +| `DATA-001` | `provar_testcase_validate` | `` iteration is silently ignored when a test case runs in direct `testCase`-mode (see [Data-driven execution](#data-driven-execution)) | +| `PARALLEL-001` | automation / run tooling | Parallel-mode cache mismatch between properties and active runtime config | +| `SCHEMA-001` | strict properties / config validators | Unknown or misspelled key in a JSON / properties schema (typo guard) | +| `RUN-001` | `provar_automation_testrun` and friends | Test run produced no executable results — check input selection | +| `JUNIT-001` | report / RCA tooling | JUnit results file is missing, empty, or not parseable | Warnings emitted programmatically follow the shape `WARNING []: ` — and when a typo is detected, the message is suffixed with `Did you mean ''?`. See `src/mcp/utils/warningCodes.ts` for the canonical enum. @@ -843,7 +844,7 @@ Validates an XML test case for schema correctness (validity score) and best prac **Warning rules:** -- **DATA-001** — `testCase` declares a `` element. CLI standalone execution does not bind CSV column variables; steps using variable references will resolve to null. Use `SetValues` (Test scope) steps instead, or add the test to a test plan. +- **DATA-001** — `testCase` declares a `` element. When the validator is called with `file_path` and the project's `provardx-properties.json` references that test case directly via top-level `testCase` or `testCases` (rather than via a `.testinstance` inside a plan), the warning carries the `WARNING [DATA-001]:` prefix and recommends wiring the test into a plan via `provar_testplan_add-instance`. When `file_path` is not supplied (or the project context cannot be resolved), the warning falls back to a structural advisory recommending `SetValues` (Test scope) steps. The warning is suppressed entirely when a `.testinstance` references the test case, because data-driven iteration works in that mode. See also [Data-driven execution](#data-driven-execution). - **ASSERT-001** — An `AssertValues` step uses the `argument id="values"` (namedValues) format, which is designed for UI element attribute assertions. For Apex/SOQL result or variable comparisons this silently passes as `null=null`. Use separate `expectedValue`, `actualValue`, and `comparisonType` arguments instead. - **UI-TARGET-001** — A UiWithScreen or UiWithRow `target` argument uses the wrong XML class (e.g. `class="value"`). Must be `class="uiTarget"` or the screen binding is silently ignored at runtime. - **UI-LOCATOR-001** — A UiDoAction or UiAssert `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element. @@ -1702,6 +1703,38 @@ Remove a `.testinstance` file from a plan suite. Path is validated to stay withi --- +## Data-driven execution + +Provar's data-driven execution relies on the `` element inside a `.testcase` XML. The runtime only **iterates rows** when the test case is launched through a test-plan instance (a `.testinstance` file under `plans/`). When the same test case is launched directly via the top-level `testCase` or `testCases` property in `provardx-properties.json`, the runtime ignores the data table entirely — every step referencing a `` resolves to `null`, and the test typically completes "successfully" against an empty row set. + +This produces silent-pass behaviour that is hard to spot from a log: the run exits 0, JUnit shows one test case, and the data-driven assertions never fire. The MCP server detects this configuration mismatch and surfaces a **`DATA-001`** warning so an AI agent can recover before the next run. + +**When does `DATA-001` fire?** + +| Condition (validator called with `file_path`) | DATA-001 emitted? | Severity | +| -------------------------------------------------------------------------------------------- | ----------------- | -------- | +| `` present **and** referenced from a `.testinstance` inside `plans/` | No | — | +| `` present **and** referenced via top-level `testCase` / `testCases` array | Yes | WARNING | +| `` present **and** project context cannot be resolved (no active properties file) | Yes (structural) | WARNING | +| No `` element | No | — | + +The plan-mode resolver consults the properties file registered by [`provar_automation_config_load`](#provar_automation_config_load) (`PROVARDX_PROPERTIES_FILE_PATH` in `~/.sf/config.json`), reads `projectPath`, then: + +1. Walks `/plans/**/*.testinstance` for any `testCasePath="..."` referencing the test under validation. If found → `plan` mode → DATA-001 suppressed. +2. Otherwise checks `testCase` / `testCases` for a direct reference. If found → `direct` mode → DATA-001 with the PDX-489 advisory. +3. Falls back to `unknown` mode when no project context is resolvable — DATA-001 still fires (structural fallback) so authors editing a test case in isolation are still warned. + +**Recommended workaround** + +When `DATA-001` fires in direct mode, wire the test case into a plan via [`provar_testplan_add-instance`](#provar_testplan_add-instance) and run via the `testPlan` property in `provardx-properties.json` instead of `testCase` / `testCases`. The pattern is: + +1. Use [`provar_testplan_create-suite`](#provar_testplan_create-suite) to add a suite under an existing plan if needed. +2. Use [`provar_testplan_add-instance`](#provar_testplan_add-instance) to create the `.testinstance` linking the test case to the suite. +3. Update `provardx-properties.json` to reference the plan (and remove the direct `testCase` entry if it no longer applies) before invoking [`provar_automation_testrun`](#provar_automation_testrun). +4. Re-run [`provar_testcase_validate`](#provar_testcase_validate) on the test case file — DATA-001 should no longer appear. + +The constraint is also referenced in the [`provar_testcase_generate`](#provar_testcase_generate) tool description so an agent constructing a new data-driven test case sees the limitation up front, and in [`provar_automation_testrun`](#provar_automation_testrun) so an agent triggering a run is reminded that direct-mode execution will not iterate. + --- ## NitroX — Hybrid Model page objects diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index b374db21..15e0e233 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -36,6 +36,8 @@ import { resolveValidationDir, type DiffableViolation, } from '../utils/validationDiff.js'; +import { resolveTestCasePlanMode, type TestCasePlanMode } from '../utils/testCasePlanMode.js'; +import { WARNING_CODES, formatWarning } from '../utils/warningCodes.js'; import { runBestPractices } from './bestPracticesEngine.js'; import { desc } from './descHelper.js'; @@ -75,15 +77,20 @@ function tcStorageDir(): string { async function resolveBaseResult( source: string, apiKey: string | null, - requestId: string + requestId: string, + planMode: TestCasePlanMode = 'unknown' ): Promise { if (!apiKey) { - return { ...validateTestCase(source), validation_source: 'local', validation_warning: ONBOARDING_MESSAGE }; + return { + ...validateTestCase(source, undefined, { planMode }), + validation_source: 'local', + validation_warning: ONBOARDING_MESSAGE, + }; } const baseUrl = getQualityHubBaseUrl(); try { const apiResult = await qualityHubClient.validateTestCaseViaApi(source, apiKey, baseUrl); - const localMeta = validateTestCase(source); + const localMeta = validateTestCase(source, undefined, { planMode }); log('info', 'provar_testcase_validate: quality_hub', { requestId }); return { ...apiResult, @@ -107,7 +114,11 @@ async function resolveBaseResult( warning = UNREACHABLE_WARNING; log('warn', 'provar_testcase_validate: api unreachable, falling back', { requestId }); } - return { ...validateTestCase(source), validation_source: 'local_fallback', validation_warning: warning }; + return { + ...validateTestCase(source, undefined, { planMode }), + validation_source: 'local_fallback', + validation_warning: warning, + }; } } @@ -123,7 +134,7 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig { title: 'Validate Test Case', description: desc( - '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. Every response includes run_id — pass it as baseline_run_id in the next call to receive only new/resolved issues. When structural errors are returned, consult the provar://docs/step-reference MCP resource for correct step attribute schemas.', + "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. Every response includes run_id — pass it as baseline_run_id in the next call to receive only new/resolved issues. Data-driven note (DATA-001): when file_path is supplied and the project's provardx-properties.json references the test case directly via top-level `testCase` / `testCases` rather than via a `.testinstance` inside a plan, the validator emits DATA-001 warning a declaration will resolve all variables to null in direct testCase-mode — wire the test into a plan via provar_testplan_add-instance to enable data-driven iteration. When structural errors are returned, consult the provar://docs/step-reference MCP resource for correct step attribute schemas.", 'Validate a Provar XML test case: structure, UUIDs, steps, quality scoring; run_id for baseline diff.' ), inputSchema: { @@ -181,7 +192,10 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig } const apiKey = resolveApiKey(); - const baseResult = await resolveBaseResult(source, apiKey, requestId); + const planMode: TestCasePlanMode = file_path + ? resolveTestCasePlanMode({ testCaseFilePath: file_path, allowedPaths: config.allowedPaths }).mode + : 'unknown'; + const baseResult = await resolveBaseResult(source, apiKey, requestId, planMode); const storageDir = tcStorageDir(); const context = tcRunContext(file_path, source); @@ -306,7 +320,11 @@ export function validateTestCaseXml(filePath: string, config: ServerConfig): Tes if (!fs.existsSync(resolved)) { throw Object.assign(new Error(`File not found: ${resolved}`), { code: 'TESTCASE_FILE_NOT_FOUND' }); } - return validateTestCase(fs.readFileSync(resolved, 'utf-8')); + const planMode = resolveTestCasePlanMode({ + testCaseFilePath: resolved, + allowedPaths: config.allowedPaths, + }).mode; + return validateTestCase(fs.readFileSync(resolved, 'utf-8'), undefined, { planMode }); } /** TC_010/TC_011: validate testCase id and guid attributes. */ @@ -348,8 +366,69 @@ function checkTestCaseIdAndGuid(tcId: string | null, tcGuid: string | undefined, } } +/** + * Emit the DATA-001 warning when the test case declares a ``. + * + * PDX-489: when the validator handler resolves the project's + * `provardx-properties.json` and finds the test case is referenced from a + * `.testinstance` (plan mode), suppress the warning — data-driven execution + * works there. When the project references the test case directly via + * top-level `testCase` / `testCases`, emit the PDX-489 advisory. When no + * project context is available (plan mode unknown — pure function called + * without file resolution), keep the structural advisory so the warning + * surface stays backward-compatible. + */ +function maybeEmitDataTableWarning( + tc: Record, + planMode: TestCasePlanMode, + issues: ValidationIssue[] +): void { + const hasDataTable = 'dataTable' in tc && tc['dataTable'] != null; + if (!hasDataTable || planMode === 'plan') return; + if (planMode === 'direct') { + issues.push({ + rule_id: 'DATA-001', + severity: 'WARNING', + message: formatWarning( + WARNING_CODES.DATA_001, + ' only iterates when run through a test plan instance. Direct testCase-mode execution will resolve all data-driven variables as null. Add this test to a plan via provar_testplan_add-instance.' + ), + applies_to: 'testCase', + suggestion: + 'Move this test case under a test plan and reference it through a .testinstance — use provar_testplan_add-instance to wire it up.', + }); + return; + } + issues.push({ + rule_id: 'DATA-001', + severity: 'WARNING', + message: + 'testCase declares a but CLI standalone execution does not bind CSV column variables — steps using references will resolve to null.', + applies_to: 'testCase', + suggestion: + 'Use SetValues (Test scope) steps to bind data for standalone CLI execution, or add this test case to a test plan.', + }); +} + +/** + * Validator options threaded through from the MCP handler. `planMode` controls + * how DATA-001 (the data-driven `` warning) is surfaced. + * + * `direct` emits the PDX-489 warning recommending a plan instance. `plan` + * suppresses DATA-001 entirely because data-driven execution works under plan + * mode. `unknown` (default) emits the structural DATA-001 warning so callers + * without project context still receive the original advisory. + */ +export interface ValidateTestCaseOptions { + planMode?: TestCasePlanMode; +} + /** Pure function — exported for unit testing */ -export function validateTestCase(xmlContent: string, testName?: string): TestCaseValidationResult { +export function validateTestCase( + xmlContent: string, + testName?: string, + options: ValidateTestCaseOptions = {} +): TestCaseValidationResult { const issues: ValidationIssue[] = []; // TC_001: XML declaration @@ -423,18 +502,7 @@ export function validateTestCase(xmlContent: string, testName?: string): TestCas return finalize(issues, tcId, tcName, 0, xmlContent, testName); } - // DATA-001: binding is silently ignored in standalone CLI execution - if ('dataTable' in tc && tc['dataTable'] != null) { - issues.push({ - rule_id: 'DATA-001', - severity: 'WARNING', - message: - 'testCase declares a but CLI standalone execution does not bind CSV column variables — steps using references will resolve to null.', - applies_to: 'testCase', - suggestion: - 'Use SetValues (Test scope) steps to bind data for standalone CLI execution, or add this test case to a test plan.', - }); - } + maybeEmitDataTableWarning(tc, options.planMode ?? 'unknown', issues); // Same self-closing guard for → fast-xml-parser yields '' const rawSteps = tc['steps']; diff --git a/src/mcp/utils/testCasePlanMode.ts b/src/mcp/utils/testCasePlanMode.ts new file mode 100644 index 00000000..f1fee581 --- /dev/null +++ b/src/mcp/utils/testCasePlanMode.ts @@ -0,0 +1,214 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +/* eslint-disable camelcase */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { assertPathAllowed } from '../security/pathPolicy.js'; + +/** + * How the project's provardx-properties.json runs this test case. + * + * `direct` means the test case path appears in `testCase` or `testCases` and + * no `.testinstance` references it — data-driven `` rows will not + * iterate. `plan` means the test case is referenced by at least one + * `.testinstance` under `plans/`, so data-driven execution works. `unknown` + * means the properties file could not be resolved or parsed (no + * `~/.sf/config.json`, file outside allowed paths, missing project root, etc.) + * — callers should default to the safe behaviour (emit the structural + * warning) when the mode is unknown. + */ +export type TestCasePlanMode = 'direct' | 'plan' | 'unknown'; + +export interface ResolvedTestCaseMode { + mode: TestCasePlanMode; + /** The properties-file path consulted (when resolved). */ + propertiesFilePath?: string; + /** The project root resolved from the properties file (when present). */ + projectPath?: string; +} + +interface ResolveOptions { + /** Path to the test case file under validation. */ + testCaseFilePath: string; + /** Allowed-paths policy from the MCP server config. */ + allowedPaths: string[]; + /** Override of ~/.sf/config.json location for testing. */ + sfConfigPathOverride?: string; + /** Override of provardx-properties.json location for testing. */ + propertiesFilePathOverride?: string; +} + +/** + * Resolve whether a given test case file is referenced directly via + * `testCase`/`testCases` or via a `.testinstance` inside a plan. + * + * The resolution flow is intentionally best-effort and silent: any file + * read failure, JSON parse error, or path-policy violation collapses to + * `mode: 'unknown'` so the caller falls back to default behaviour rather + * than surfacing a confusing error from the validator. + */ +export function resolveTestCasePlanMode(opts: ResolveOptions): ResolvedTestCaseMode { + const propsPath = readPropertiesFilePath(opts); + if (!propsPath) return { mode: 'unknown' }; + + let propsObj: Record; + try { + propsObj = JSON.parse(fs.readFileSync(propsPath, 'utf-8')) as Record; + } catch { + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + const projectPath = typeof propsObj['projectPath'] === 'string' ? propsObj['projectPath'] : null; + if (!projectPath) { + // Without a project root we cannot resolve relative `testCase` entries or + // walk `plans/`. The mode is unknown. + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + let resolvedProjectPath: string; + try { + resolvedProjectPath = path.resolve(projectPath); + assertPathAllowed(resolvedProjectPath, opts.allowedPaths); + } catch { + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + const resolvedTestCasePath = path.resolve(opts.testCaseFilePath); + + // A `.testinstance` reference inside any plan wins — plan mode supports + // data-driven iteration. + if (isReferencedFromPlanInstance(resolvedProjectPath, resolvedTestCasePath)) { + return { mode: 'plan', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; + } + + if (isReferencedDirectly(propsObj, resolvedProjectPath, resolvedTestCasePath)) { + return { mode: 'direct', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; + } + + return { mode: 'unknown', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; +} + +/** + * Resolve the active properties file path. Prefers an explicit override + * (used by tests), then `~/.sf/config.json`'s `PROVARDX_PROPERTIES_FILE_PATH`. + */ +function readPropertiesFilePath(opts: ResolveOptions): string | null { + if (opts.propertiesFilePathOverride) { + return opts.propertiesFilePathOverride; + } + try { + const sfConfigPath = opts.sfConfigPathOverride ?? path.join(os.homedir(), '.sf', 'config.json'); + if (!fs.existsSync(sfConfigPath)) return null; + const sfConfig = JSON.parse(fs.readFileSync(sfConfigPath, 'utf-8')) as Record; + const propsPath = sfConfig['PROVARDX_PROPERTIES_FILE_PATH'] as string | undefined; + if (!propsPath) return null; + if (!fs.existsSync(propsPath)) return null; + // Honour allowed-paths for the properties file too. + assertPathAllowed(propsPath, opts.allowedPaths); + return propsPath; + } catch { + return null; + } +} + +/** + * Does the properties file's top-level `testCase` or `testCases` array + * reference this test case file? Entries are interpreted relative to + * `/tests/` (per the Provar runtime convention). + */ +function isReferencedDirectly(props: Record, projectPath: string, testCaseFilePath: string): boolean { + const entries: string[] = []; + const tc = props['testCase']; + const tcs = props['testCases']; + for (const candidate of [tc, tcs]) { + if (typeof candidate === 'string') { + entries.push(candidate); + } else if (Array.isArray(candidate)) { + for (const e of candidate as unknown[]) { + if (typeof e === 'string') entries.push(e); + } + } + } + if (entries.length === 0) return false; + + const testsDir = path.join(projectPath, 'tests'); + const targetNorm = path.resolve(testCaseFilePath).toLowerCase(); + + for (const entry of entries) { + // Provar accepts both bare names ("MyTest") and relative paths + // ("Module/MyTest.testcase"). Allow either; match with and without the + // `.testcase` extension. + const variants: string[] = []; + const trimmed = entry.replace(/^[/\\]+/, ''); + variants.push(path.resolve(testsDir, trimmed)); + if (!/\.testcase$/i.test(trimmed)) { + variants.push(path.resolve(testsDir, `${trimmed}.testcase`)); + } + for (const v of variants) { + if (v.toLowerCase() === targetNorm) return true; + } + } + return false; +} + +/** + * Walk `/plans/` for `.testinstance` files referencing this + * test case via `testCasePath="..."`. Best-effort — any read error skips + * the offending file. + */ +function isReferencedFromPlanInstance(projectPath: string, testCaseFilePath: string): boolean { + const plansDir = path.join(projectPath, 'plans'); + if (!fs.existsSync(plansDir)) return false; + + const testsDir = path.join(projectPath, 'tests'); + const targetNorm = path.resolve(testCaseFilePath).toLowerCase(); + let found = false; + + const walk = (dir: string): void => { + if (found) return; + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + if (found) return; + if (entry.name.startsWith('.') && !entry.name.endsWith('.testinstance')) continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith('.testinstance')) continue; + let content: string; + try { + content = fs.readFileSync(full, 'utf-8'); + } catch { + continue; + } + const matches = content.matchAll(/testCasePath=["']([^"']+)["']/g); + for (const m of matches) { + const rel = m[1].replace(/\\/g, '/'); + // testCasePath in Provar testinstances is conventionally relative to + // `/tests/`. Also tolerate paths relative to project root. + const candidates = [path.resolve(testsDir, rel), path.resolve(projectPath, rel)]; + for (const c of candidates) { + if (c.toLowerCase() === targetNorm) { + found = true; + return; + } + } + } + } + }; + + walk(plansDir); + return found; +} diff --git a/test/unit/mcp/testCasePlanMode.test.ts b/test/unit/mcp/testCasePlanMode.test.ts new file mode 100644 index 00000000..dff71800 --- /dev/null +++ b/test/unit/mcp/testCasePlanMode.test.ts @@ -0,0 +1,177 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +/* eslint-disable camelcase */ +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, beforeEach, afterEach } from 'mocha'; +import { resolveTestCasePlanMode } from '../../../src/mcp/utils/testCasePlanMode.js'; + +/** + * Build a minimal Provar project on disk for plan-mode resolution tests. + * + * Layout: `/tests/Module/MyTest.testcase`, + * `/plans/Plan1/Suite1/MyTest.testinstance` (only when + * `wirePlan=true`), and `/provardx-properties.json`. + * + * Returns the test-case path and the properties-file path so callers can + * point the resolver at the right inputs. + */ +function buildProject(opts: { + root: string; + references: 'direct-testCase' | 'direct-testCases' | 'none'; + wirePlan: boolean; +}): { testCasePath: string; propertiesPath: string; projectPath: string } { + const projectPath = path.join(opts.root, 'project'); + fs.mkdirSync(path.join(projectPath, 'tests', 'Module'), { recursive: true }); + const testCasePath = path.join(projectPath, 'tests', 'Module', 'MyTest.testcase'); + fs.writeFileSync(testCasePath, ''); + + if (opts.wirePlan) { + fs.mkdirSync(path.join(projectPath, 'plans', 'Plan1', 'Suite1'), { recursive: true }); + const inst = path.join(projectPath, 'plans', 'Plan1', 'Suite1', 'MyTest.testinstance'); + fs.writeFileSync(inst, ''); + } + + const props: Record = { + projectPath, + provarHome: '/tmp/provarHome', + resultsPath: 'ANT/Results', + }; + if (opts.references === 'direct-testCase') { + props.testCase = ['Module/MyTest.testcase']; + } else if (opts.references === 'direct-testCases') { + props.testCases = ['Module/MyTest.testcase']; + } + + const propertiesPath = path.join(projectPath, 'provardx-properties.json'); + fs.writeFileSync(propertiesPath, JSON.stringify(props, null, 2)); + return { testCasePath, propertiesPath, projectPath }; +} + +describe('resolveTestCasePlanMode', () => { + let tmp: string; + + beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-mode-')); + }); + + afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + it('returns "direct" when properties references via top-level testCase', () => { + const { testCasePath, propertiesPath, projectPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + assert.equal(result.propertiesFilePath, propertiesPath); + assert.equal(result.projectPath, projectPath); + }); + + it('returns "direct" when properties references via testCases (plural)', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCases', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + }); + + it('returns "plan" when a .testinstance references the test case (even if testCase array also lists it)', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: true, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'plan', 'Plan-instance reference must win over direct testCase array'); + }); + + it('returns "plan" when only a .testinstance references it', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'none', + wirePlan: true, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'plan'); + }); + + it('returns "unknown" when neither testCase nor a .testinstance references it', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'none', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when the properties file does not exist', () => { + const result = resolveTestCasePlanMode({ + testCaseFilePath: path.join(tmp, 'nope.testcase'), + allowedPaths: [tmp], + propertiesFilePathOverride: path.join(tmp, 'missing.json'), + }); + // Override is honoured but fs read fails → 'unknown' + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when sf config has no PROVARDX_PROPERTIES_FILE_PATH entry', () => { + const sfDir = path.join(tmp, 'sf'); + fs.mkdirSync(sfDir); + fs.writeFileSync(path.join(sfDir, 'config.json'), JSON.stringify({})); + const result = resolveTestCasePlanMode({ + testCaseFilePath: path.join(tmp, 'nope.testcase'), + allowedPaths: [tmp], + sfConfigPathOverride: path.join(sfDir, 'config.json'), + }); + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when properties file is outside allowed paths', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + // Allowed paths intentionally does NOT include the properties path. + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [path.join(tmp, 'unrelated-dir')], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'unknown'); + }); +}); diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index 9ff5bd2e..ad7a1391 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -226,16 +226,16 @@ describe('validateTestCase', () => { }); describe('DATA-001', () => { - it('warns when testCase has a child element', () => { - const r = validateTestCase( - ` + const DATA_TABLE_TC = ` mydata.csv -` - ); +`; + + it('warns (structural) when testCase has a child element and mode is unknown', () => { + const r = validateTestCase(DATA_TABLE_TC); assert.ok( r.issues.some((i) => i.rule_id === 'DATA-001'), 'Expected DATA-001' @@ -248,6 +248,42 @@ describe('validateTestCase', () => { const r = validateTestCase(VALID_TC); assert.ok(!r.issues.some((i) => i.rule_id === 'DATA-001'), 'DATA-001 should not fire for valid test case'); }); + + it('PDX-489: emits DATA-001 with formatWarning text when planMode is direct', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'direct' }); + const issue = r.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 to fire in direct mode'); + assert.ok( + issue.message.startsWith('WARNING [DATA-001]:'), + `Message must use formatWarning prefix, got: ${issue.message}` + ); + assert.ok( + issue.message.includes('only iterates when run through a test plan instance'), + `Message must reference the plan-instance fix: ${issue.message}` + ); + assert.ok( + issue.message.includes('provar_testplan_add-instance'), + `Message must reference provar_testplan_add-instance: ${issue.message}` + ); + }); + + it('PDX-489: suppresses DATA-001 when planMode is plan', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'plan' }); + assert.ok( + !r.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when the test case is referenced from a .testinstance' + ); + }); + + it('PDX-489: keeps structural DATA-001 when planMode is unknown', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'unknown' }); + const issue = r.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 to fire in unknown mode'); + assert.ok( + !issue.message.startsWith('WARNING [DATA-001]:'), + 'Unknown-mode message should NOT use the formatWarning prefix (preserves prior behaviour)' + ); + }); }); describe('ASSERT-001', () => { @@ -1140,6 +1176,117 @@ describe('validateTestCaseXml', () => { }); }); +// ── PDX-489 handler-level DATA-001 integration ──────────────────────────────── + +/** + * Build a project that wires a dataTable test case directly (via testCase / + * testCases array) OR through a .testinstance — and writes ~/.sf/config.json + * pointing at the project's provardx-properties.json so the handler can + * resolve plan mode without explicit overrides. + */ +function buildDataTableProject( + tmpRoot: string, + references: 'direct-testCase' | 'direct-testCases' | 'plan' +): { testCasePath: string; allowedPaths: string[] } { + const projectPath = path.join(tmpRoot, 'project'); + fs.mkdirSync(path.join(projectPath, 'tests', 'Module'), { recursive: true }); + const testCasePath = path.join(projectPath, 'tests', 'Module', 'DataTest.testcase'); + fs.writeFileSync( + testCasePath, + ` + + mydata.csv + + + +` + ); + + const props: Record = { + projectPath, + provarHome: '/tmp/provarHome', + resultsPath: 'ANT/Results', + }; + + if (references === 'direct-testCase') { + props.testCase = ['Module/DataTest.testcase']; + } else if (references === 'direct-testCases') { + props.testCases = ['Module/DataTest.testcase']; + } else { + fs.mkdirSync(path.join(projectPath, 'plans', 'Plan1', 'Suite1'), { recursive: true }); + fs.writeFileSync( + path.join(projectPath, 'plans', 'Plan1', 'Suite1', 'DataTest.testinstance'), + '' + ); + } + + const propertiesPath = path.join(projectPath, 'provardx-properties.json'); + fs.writeFileSync(propertiesPath, JSON.stringify(props, null, 2)); + + // Wire ~/.sf/config.json so the resolver picks up the properties file. + const sfDir = path.join(tmpRoot, '.sf'); + fs.mkdirSync(sfDir, { recursive: true }); + fs.writeFileSync(path.join(sfDir, 'config.json'), JSON.stringify({ PROVARDX_PROPERTIES_FILE_PATH: propertiesPath })); + + return { testCasePath, allowedPaths: [tmpRoot] }; +} + +describe('PDX-489: DATA-001 handler integration via validateTestCaseXml', () => { + let tmpRoot: string; + let origHomedir: () => string; + + beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-handler-')); + origHomedir = os.homedir; + (os as unknown as { homedir: () => string }).homedir = (): string => tmpRoot; + }); + + afterEach(() => { + (os as unknown as { homedir: () => string }).homedir = origHomedir; + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + it('fires DATA-001 with formatWarning prefix when properties uses top-level testCase', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCase'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + const issue = result.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 in direct testCase mode'); + assert.ok(issue.message.startsWith('WARNING [DATA-001]:'), `Expected formatWarning prefix: ${issue.message}`); + assert.ok( + issue.message.includes('provar_testplan_add-instance'), + `Expected guidance to plan-instance: ${issue.message}` + ); + }); + + it('fires DATA-001 when properties uses testCases (plural) — typo-tolerant', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCases'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + const issue = result.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 when testCases array references the test'); + assert.ok(issue.message.startsWith('WARNING [DATA-001]:')); + }); + + it('does NOT fire DATA-001 when test case is referenced from a .testinstance', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'plan'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + assert.ok( + !result.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when the test case is wired via a plan instance' + ); + }); + + it('does NOT fire DATA-001 when test case has no (direct mode)', () => { + // Reuse the direct project but rewrite the test case content without a . + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCase'); + fs.writeFileSync(testCasePath, VALID_TC); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + assert.ok( + !result.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when no is present' + ); + }); +}); + // ── tool description ────────────────────────────────────────────────────────── describe('provar_testcase_validate description', () => { From e46b7a948714b316db06743519223dd5822df84d Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:20:46 -0500 Subject: [PATCH 3/4] =?UTF-8?q?PDX-489:=20fix(mcp)=20=E2=80=94=20enforce?= =?UTF-8?q?=20assertPathAllowed=20on=20propertiesFilePathOverride=20+=20re?= =?UTF-8?q?pair=20docs=20TOC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review on PR #187 flagged a path-policy bypass in resolveTestCasePlanMode: the propertiesFilePathOverride parameter was returned directly from readPropertiesFilePath without going through assertPathAllowed, so a future caller could read a properties file outside the configured allowedPaths tree. A separate Copilot finding caught a broken docs TOC — adding the Data-driven execution top-level bullet left the NitroX, Quality Hub, and Org metadata bullets indented as if nested, misrepresenting the section hierarchy. Fix: Funnel both the override and the value read from ~/.sf/config.json through a shared enforceAllowed helper that resolves the path, calls assertPathAllowed, canonicalises symlinks via realpathSync when the file exists, and collapses any policy violation or missing file to null to preserve the resolver's best-effort silent-fallback contract. Re-leveled the docs/mcp.md TOC so NitroX and Quality Hub sit at the top level (matching their ## headings) with Org metadata nested under Quality Hub (matching its ### heading). Added two regression tests: override inside allowed paths is consulted normally; override outside allowed paths collapses to mode 'unknown' without throwing and without exposing the rejected path. --- docs/mcp.md | 16 ++++----- src/mcp/utils/testCasePlanMode.ts | 34 +++++++++++++++--- test/unit/mcp/testCasePlanMode.test.ts | 49 ++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 190a042d..272af0df 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -51,14 +51,14 @@ The Provar DX CLI ships with a built-in **Model Context Protocol (MCP) server** - [provar_testplan_create-suite](#provar_testplan_create-suite) - [provar_testplan_remove-instance](#provar_testplan_remove-instance) - [Data-driven execution](#data-driven-execution) - - [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) - - [provar_nitrox_discover](#provar_nitrox_discover) - - [provar_nitrox_read](#provar_nitrox_read) - - [provar_nitrox_validate](#provar_nitrox_validate) - - [provar_nitrox_generate](#provar_nitrox_generate) - - [provar_nitrox_patch](#provar_nitrox_patch) - - [Quality Hub API tools](#quality-hub-api-tools) - - [provar_qualityhub_examples_retrieve](#provar_qualityhub_examples_retrieve) +- [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) + - [provar_nitrox_discover](#provar_nitrox_discover) + - [provar_nitrox_read](#provar_nitrox_read) + - [provar_nitrox_validate](#provar_nitrox_validate) + - [provar_nitrox_generate](#provar_nitrox_generate) + - [provar_nitrox_patch](#provar_nitrox_patch) +- [Quality Hub API tools](#quality-hub-api-tools) + - [provar_qualityhub_examples_retrieve](#provar_qualityhub_examples_retrieve) - [Org metadata via Salesforce Hosted MCP](#org-metadata-via-salesforce-hosted-mcp) - [MCP Prompts](#mcp-prompts) - [Migration prompts](#migration-prompts) diff --git a/src/mcp/utils/testCasePlanMode.ts b/src/mcp/utils/testCasePlanMode.ts index f1fee581..0f98ac74 100644 --- a/src/mcp/utils/testCasePlanMode.ts +++ b/src/mcp/utils/testCasePlanMode.ts @@ -97,10 +97,15 @@ export function resolveTestCasePlanMode(opts: ResolveOptions): ResolvedTestCaseM /** * Resolve the active properties file path. Prefers an explicit override * (used by tests), then `~/.sf/config.json`'s `PROVARDX_PROPERTIES_FILE_PATH`. + * + * Both the override and the value read from `~/.sf/config.json` are funnelled + * through `assertPathAllowed` so a caller cannot trick the resolver into + * reading a properties file outside the configured `allowedPaths`. Any policy + * violation collapses to `null` to preserve the helper's best-effort contract. */ function readPropertiesFilePath(opts: ResolveOptions): string | null { if (opts.propertiesFilePathOverride) { - return opts.propertiesFilePathOverride; + return enforceAllowed(opts.propertiesFilePathOverride, opts.allowedPaths); } try { const sfConfigPath = opts.sfConfigPathOverride ?? path.join(os.homedir(), '.sf', 'config.json'); @@ -108,10 +113,29 @@ function readPropertiesFilePath(opts: ResolveOptions): string | null { const sfConfig = JSON.parse(fs.readFileSync(sfConfigPath, 'utf-8')) as Record; const propsPath = sfConfig['PROVARDX_PROPERTIES_FILE_PATH'] as string | undefined; if (!propsPath) return null; - if (!fs.existsSync(propsPath)) return null; - // Honour allowed-paths for the properties file too. - assertPathAllowed(propsPath, opts.allowedPaths); - return propsPath; + return enforceAllowed(propsPath, opts.allowedPaths); + } catch { + return null; + } +} + +/** + * Resolve a candidate properties-file path through `assertPathAllowed`, then + * canonicalize via `fs.realpathSync` when the file exists so a symlink + * inside an allowed directory cannot redirect the read outside it. Returns + * `null` on any policy violation, missing file, or unexpected error — the + * caller treats `null` as "mode unknown" and falls back to default behaviour. + */ +function enforceAllowed(candidate: string, allowedPaths: string[]): string | null { + try { + const resolved = path.resolve(candidate); + assertPathAllowed(resolved, allowedPaths); + if (!fs.existsSync(resolved)) return null; + try { + return fs.realpathSync(resolved); + } catch { + return resolved; + } } catch { return null; } diff --git a/test/unit/mcp/testCasePlanMode.test.ts b/test/unit/mcp/testCasePlanMode.test.ts index dff71800..0f6cd345 100644 --- a/test/unit/mcp/testCasePlanMode.test.ts +++ b/test/unit/mcp/testCasePlanMode.test.ts @@ -174,4 +174,53 @@ describe('resolveTestCasePlanMode', () => { }); assert.equal(result.mode, 'unknown'); }); + + it('honours assertPathAllowed on propertiesFilePathOverride: override inside allowed paths is consulted', () => { + // Security regression guard: a properties-file override path must pass the + // path-policy check before the resolver opens it. When the override sits + // inside the allowed-paths tree the helper should consult it normally and + // return the resolved mode (here: "direct"). + const { testCasePath, propertiesPath, projectPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + assert.equal(result.projectPath, projectPath); + }); + + it('honours assertPathAllowed on propertiesFilePathOverride: override outside allowed paths is ignored without throwing', () => { + // Security regression guard for the Copilot-flagged path-policy bypass: + // the override must be funnelled through assertPathAllowed and a + // violation must collapse to 'unknown' rather than reading the file or + // throwing — the helper's contract is best-effort silent fallback. + const outsideRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-outside-')); + try { + const { testCasePath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + // Write a properties file OUTSIDE the allowed-paths tree. + const outsideProps = path.join(outsideRoot, 'provardx-properties.json'); + fs.writeFileSync( + outsideProps, + JSON.stringify({ projectPath: outsideRoot, provarHome: '/tmp', resultsPath: 'r' }) + ); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: outsideProps, + }); + assert.equal(result.mode, 'unknown'); + assert.equal(result.propertiesFilePath, undefined, 'must not expose a path it rejected'); + } finally { + fs.rmSync(outsideRoot, { recursive: true, force: true }); + } + }); }); From dbe4f37bb82a63eb1298ff9c09727b43309e5d04 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 08:32:38 -0500 Subject: [PATCH 4/4] =?UTF-8?q?PDX-489:=20fix(test)=20=E2=80=94=20realpath?= =?UTF-8?q?=20tmp=20root=20in=20plan-mode=20test=20to=20match=20symlink=20?= =?UTF-8?q?canonicalisation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: macOS CI surfaced a test failure in resolveTestCasePlanMode's "direct" assertion. On Mac, os.tmpdir() returns /var/folders/... but /var is a symlink to /private/var. The Copilot-fix commit (e46b7a9) added fs.realpathSync to enforceAllowed for symlink canonicalisation — correct security behaviour — but the test compared result.propertiesFilePath against a path built from un-realpath'd os.tmpdir(), so the resolved /private/var/... did not match the expected /var/... and the assertion blew up on Mac CI (passed on Windows because /var is a regular dir there). Fix: Wrap the mkdtempSync return in fs.realpathSync inside beforeEach so the tmp root is already canonicalised. Every derived path (projectPath, testCasePath, propertiesPath) now uses the canonical /private/var/... form on Mac, matching what the resolver returns. No code change to the helper — its realpathSync behaviour is correct and stays. Windows behaviour unchanged because realpathSync is a no-op on paths without symlinks. Full mocha 1176 passing, yarn lint clean. --- test/unit/mcp/testCasePlanMode.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/unit/mcp/testCasePlanMode.test.ts b/test/unit/mcp/testCasePlanMode.test.ts index 0f6cd345..1dcdc328 100644 --- a/test/unit/mcp/testCasePlanMode.test.ts +++ b/test/unit/mcp/testCasePlanMode.test.ts @@ -59,7 +59,12 @@ describe('resolveTestCasePlanMode', () => { let tmp: string; beforeEach(() => { - tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-mode-')); + // Realpath the tmp root immediately so every derived path uses the canonical form. + // Required on macOS where os.tmpdir() returns /var/folders/... but realpathSync + // canonicalises through the /var → /private/var symlink. The resolver under test + // now calls fs.realpathSync internally, so comparisons against constructed paths + // would otherwise diverge from the resolved result on Mac. + tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-mode-'))); }); afterEach(() => {