From 63d75979825cf6055ac4f0a8ebec7eb1f0212a31 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 15:33:44 -0500 Subject: [PATCH 01/17] =?UTF-8?q?PDX-485:=20chore(mcp)=20=E2=80=94=20intro?= =?UTF-8?q?duce=20shared=20warningCodes.ts=20enum=20for=20cross-thread=20f?= =?UTF-8?q?eedback=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 d0335193b41506efa07491134aa72a0a1f717b6e Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 15:59:59 -0500 Subject: [PATCH 02/17] =?UTF-8?q?PDX-493:=20feat(mcp)=20=E2=80=94=20emit?= =?UTF-8?q?=20valueClass=3Ddate|datetime|boolean|integer=20via=20inferSale?= =?UTF-8?q?sforceValueClass=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Provar runtime silently discards Salesforce date/datetime fields whose XML emits valueClass="string" — the failure surfaces only when a later validation rule reads the empty field. The existing buildArgumentValue dispatch in src/mcp/tools/testCaseGenerate.ts fell through to a hard-coded valueClass="string" fallback after handling variable / compound / uiTarget / uiLocator cases, so every literal date or boolean argument flowed out untyped. A prior helper (inferValueClass) introduced in 3c1545c that covered the boolean case alone was refactored away in 2d4240c, regressing literal-true/false handling too. Datetime, integer, and explicit org-describe-driven type hints were never covered. Fix: Introduce an exported inferSalesforceValueClass(key, val, fieldTypeHint?) helper that returns one of 'date' | 'datetime' | 'boolean' | 'integer' | 'string'. Detection order: explicit fieldTypeHint wins; then ISO-8601 datetime regex /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/; then ISO-8601 date regex /^\d{4}-\d{2}-\d{2}$/; then literal 'true'/'false'; then integer-only string /^-?\d+$/; else string. buildArgumentValue now calls this helper before the (now-removed) string fallback and emits …. The fieldTypeHint param is wired into the helper signature but not yet exposed on the MCP tool input schema — that exposure lands in PDX-492 H2b, which sits behind PDX-491 H2a (provar_org_describe). Until then callers omit the hint and detection falls through to the regex layer. Tool description and docs/mcp.md argument-conventions table updated to spell out the auto-detection rules. Tests: New PDX-493 — inferSalesforceValueClass helper block exercises the helper directly across all six branches (datetime with fractional seconds + zone, plain date, true, false, positive integer, negative integer, plain string), the edge case where a short numeric string like "12" must resolve to integer not date (date regex requires full ISO yyyy-mm-dd), strings that look like dates but miss the ISO format must stay string, and explicit fieldTypeHint wins over format detection in all three directions (string-shape + date hint, date-shape + string hint, integer-shape + boolean hint). A second PDX-493 — valueClass emission in generated XML block round-trips through provar_testcase_generate and asserts the inferred attribute reaches the emitted XML for date, datetime, boolean (true and false), positive integer, negative integer, and plain string. Validation: node_modules/.bin/nyc node_modules/.bin/mocha "test/**/*.test.ts" — 1172 passing / 0 failing; yarn compile clean; yarn lint clean (lint:script-names + lint). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 20 ++- src/mcp/tools/testCaseGenerate.ts | 39 +++++- test/unit/mcp/testCaseGenerate.test.ts | 185 ++++++++++++++++++++++++- 3 files changed, 235 insertions(+), 9 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 33d23421..4068f543 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -729,13 +729,19 @@ The tool's chip-level `title` — `Generate Test Case (full steps in one call)` **Argument XML conventions** (automatically applied by the generator): -| Argument key / value pattern | Emitted XML class | API context | -| ------------------------------------ | ----------------------------------- | ----------------------- | -| `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | -| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert | -| Value matches `{VarName}` or `{A.B}` | `class="variable"` + `` | Any step | -| SetValues attributes | `class="valueList"/` | SetValues only | -| All other values | `class="value" valueClass="string"` | Any step | +| Argument key / value pattern | Emitted XML class | API context | +| --------------------------------------- | ------------------------------------- | ----------------------- | +| `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | +| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert | +| Value matches `{VarName}` or `{A.B}` | `class="variable"` + `` | Any step | +| SetValues attributes | `class="valueList"/` | SetValues only | +| Value `YYYY-MM-DDTHH:MM:SS…` (ISO-8601) | `class="value" valueClass="datetime"` | Any step | +| Value `YYYY-MM-DD` (ISO-8601) | `class="value" valueClass="date"` | Any step | +| Value `true` / `false` | `class="value" valueClass="boolean"` | Any step | +| Value matches `^-?\d+$` | `class="value" valueClass="integer"` | Any step | +| All other values | `class="value" valueClass="string"` | Any step | + +`valueClass` is inferred automatically by `inferSalesforceValueClass(key, val, fieldTypeHint?)`. Detection order: explicit `fieldTypeHint` (wired in a follow-up tool surface — `field_type_hints` param) → ISO-8601 datetime → ISO-8601 date → boolean → integer → string. Provar runtime silently discards date fields emitted as `valueClass="string"`, so always pass date / datetime values in ISO-8601 form. AssertValues uses **flat** argument structure (`expectedValue`, `actualValue`, `comparisonType`) — not the `valueList`/namedValues format. diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 6e97b736..244533a9 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -150,6 +150,10 @@ const TOOL_DESCRIPTION = [ 'Variable references: pass values as "{VarName}" (braces); emitted as class="variable" .', 'target argument (UiWithScreen/UiWithRow): pass the URI value; emitted as class="uiTarget" uri="...".', 'locator argument (UiDoAction/UiAssert): pass the URI value; emitted as class="uiLocator" uri="...".', + 'valueClass auto-detection: argument values are typed automatically before XML emission. ' + + 'ISO-8601 date "YYYY-MM-DD" → valueClass="date"; ISO-8601 datetime "YYYY-MM-DDTHH:MM:SS" → "datetime"; ' + + '"true"/"false" → "boolean"; integer-only string (e.g. "42", "-5") → "integer"; otherwise "string". ' + + 'Pass dates / booleans / integers in those formats — Provar runtime silently discards date fields emitted as valueClass="string".', 'Edit page objects: action=Edit targets require a compiled page object for the SF object. ' + 'If none exists in the project page-objects directory, the locator binding will fail at runtime. ' + 'For objects without a compiled Edit page object, use inline edit instead: sfIleActivate to activate the field, ' + @@ -374,6 +378,35 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // ── XML builder ─────────────────────────────────────────────────────────────── +// PDX-493 (H3): infer the Salesforce `valueClass` attribute that should be emitted on a +// `` element from an argument's key + string value. +// +// Detection order: +// 1. Explicit `fieldTypeHint` (from `field_type_hints` param or `provar_org_describe` cache — wired +// in PDX-492 H2b) wins if provided. +// 2. ISO-8601 datetime → 'datetime' (e.g. "2026-05-19T10:30:00", with optional fractional/zone). +// 3. ISO-8601 date → 'date' (e.g. "2026-05-19"). +// 4. Literal 'true' / 'false' → 'boolean'. +// 5. Integer-only string (optional leading '-') → 'integer'. +// 6. Else → 'string'. +// +// The `key` argument is reserved for future heuristics (e.g. SF naming conventions like *__c) +// but is intentionally not consulted today — explicit hints + value-shape regexes are the +// safer signal until org-describe is wired. +export function inferSalesforceValueClass( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + key: string, + val: string, + fieldTypeHint?: 'date' | 'datetime' | 'boolean' | 'integer' | 'string' +): 'date' | 'datetime' | 'boolean' | 'integer' | 'string' { + if (fieldTypeHint) return fieldTypeHint; + if (/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/.test(val)) return 'datetime'; + if (/^\d{4}-\d{2}-\d{2}$/.test(val)) return 'date'; + if (val === 'true' || val === 'false') return 'boolean'; + if (/^-?\d+$/.test(val)) return 'integer'; + return 'string'; +} + // F1/F3: build class="compound" for strings that mix literal text with {VarName} tokens. function buildCompoundValue(val: string, indent: string): string { const i = `${indent} `; @@ -423,7 +456,11 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal return `${indent}`; } } - return `${indent}${escapeXmlContent(val)}`; + // PDX-493 (H3): infer valueClass for date / datetime / boolean / integer / string. The + // `fieldTypeHint` parameter on `inferSalesforceValueClass` is intentionally not threaded + // through here yet — it lands in PDX-492 (H2b) along with the `field_type_hints` tool input. + const inferred = inferSalesforceValueClass(key, val); + return `${indent}${escapeXmlContent(val)}`; } function buildArgumentsXml(attributes: Record, baseIndent = ' ', apiId = ''): string { diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index 95c7579b..66460f05 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -11,7 +11,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { describe, it, beforeEach, afterEach } from 'mocha'; -import { registerTestCaseGenerate } from '../../../src/mcp/tools/testCaseGenerate.js'; +import { registerTestCaseGenerate, inferSalesforceValueClass } from '../../../src/mcp/tools/testCaseGenerate.js'; import type { ServerConfig } from '../../../src/mcp/server.js'; // ── Minimal McpServer mock ───────────────────────────────────────────────────── @@ -699,6 +699,189 @@ describe('provar_testcase_generate', () => { }); }); + // PDX-493 (H3): date/datetime/boolean/integer valueClass dispatch via inferSalesforceValueClass. + describe('PDX-493 — inferSalesforceValueClass helper', () => { + it('returns "datetime" for ISO-8601 datetime string', () => { + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00'), 'datetime'); + }); + + it('returns "datetime" for ISO-8601 datetime with fractional seconds + zone', () => { + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00.123Z'), 'datetime'); + }); + + it('returns "date" for ISO-8601 date string', () => { + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19'), 'date'); + }); + + it('returns "boolean" for "true"', () => { + assert.equal(inferSalesforceValueClass('IsActive', 'true'), 'boolean'); + }); + + it('returns "boolean" for "false"', () => { + assert.equal(inferSalesforceValueClass('IsActive', 'false'), 'boolean'); + }); + + it('returns "integer" for positive integer string', () => { + assert.equal(inferSalesforceValueClass('Quantity', '42'), 'integer'); + }); + + it('returns "integer" for negative integer string', () => { + assert.equal(inferSalesforceValueClass('Delta', '-5'), 'integer'); + }); + + it('returns "string" for plain text', () => { + assert.equal(inferSalesforceValueClass('Name', 'Acme Corp'), 'string'); + }); + + it('returns "integer" not "date" for a short numeric string like "12"', () => { + // Edge case: the date regex requires the full ISO yyyy-mm-dd form, so a bare "12" + // is integer, not date. Guards against false-positive date detection on numeric IDs. + assert.equal(inferSalesforceValueClass('Code', '12'), 'integer'); + }); + + it('returns "string" for date-looking strings that miss the ISO format', () => { + // Confirms the regex is strict: month/day shape matters. + assert.equal(inferSalesforceValueClass('CloseDate', '2026/05/19'), 'string'); + assert.equal(inferSalesforceValueClass('CloseDate', '05-19-2026'), 'string'); + }); + + it('explicit fieldTypeHint wins over format detection', () => { + // Value looks like a string but the hint says it's a date — hint wins. + assert.equal(inferSalesforceValueClass('CloseDate', 'today', 'date'), 'date'); + // Value looks like a date but the hint says string — hint wins (e.g. an external + // ID that happens to look like a date). + assert.equal(inferSalesforceValueClass('ExternalId', '2026-05-19', 'string'), 'string'); + // Value is integer, hint says boolean — hint wins. + assert.equal(inferSalesforceValueClass('IsActive', '1', 'boolean'), 'boolean'); + }); + }); + + describe('PDX-493 — valueClass emission in generated XML', () => { + it('emits valueClass="date" for an ISO-8601 date string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'DateField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Opp', + attributes: { CloseDate: '2026-05-19' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok( + xml.includes('valueClass="date">2026-05-19'), + `Expected valueClass="date" for ISO date; got: ${xml}` + ); + }); + + it('emits valueClass="datetime" for an ISO-8601 datetime string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'DatetimeField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Event', + attributes: { StartTime: '2026-05-19T10:30:00' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok( + xml.includes('valueClass="datetime">2026-05-19T10:30:00'), + `Expected valueClass="datetime" for ISO datetime; got: ${xml}` + ); + }); + + it('emits valueClass="boolean" for "true" / "false" literals', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'BoolField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Account', + attributes: { IsActive: 'true', IsDeleted: 'false' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok( + xml.includes('valueClass="boolean">true'), + `Expected valueClass="boolean" for "true"; got: ${xml}` + ); + assert.ok( + xml.includes('valueClass="boolean">false'), + `Expected valueClass="boolean" for "false"; got: ${xml}` + ); + }); + + it('emits valueClass="integer" for an integer-only string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'IntField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Opp', + attributes: { Quantity: '42' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('valueClass="integer">42'), `Expected valueClass="integer" for "42"; got: ${xml}`); + }); + + it('emits valueClass="integer" for a negative integer string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'NegIntField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Adjustment', + attributes: { Delta: '-5' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('valueClass="integer">-5'), `Expected valueClass="integer" for "-5"; got: ${xml}`); + }); + + it('emits valueClass="string" for plain text', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'StringField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Account', + attributes: { Name: 'Acme Corp' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok( + xml.includes('valueClass="string">Acme Corp'), + `Expected valueClass="string" for "Acme Corp"; got: ${xml}` + ); + }); + }); + describe('target_uri — non-SF page object (ui:) nesting', () => { it('wraps steps in UiWithScreen when target_uri uses ?pageId= format', () => { const result = server.call('provar_testcase_generate', { From 2b67b0a4cd50bc9e26ab65ad4d51b1490e392a4a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:08:18 -0500 Subject: [PATCH 03/17] =?UTF-8?q?PDX-486:=20feat(mcp)=20=E2=80=94=20strict?= =?UTF-8?q?=20validator=20unknown-key=20detection=20(SCHEMA-001=20warning)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: A user typo of 'testCases' (plural) instead of 'testCase' (singular) in provardx-properties.json slipped past provar_properties_validate because the validator only checked required fields and enum values — unknown keys were silently accepted. The subsequent test run exited 0 with zero tests executed, falsely reporting success. Two safety nets failed in series: the lenient validator was the first. Fix: Extend validateProperties to emit a SCHEMA-001 warning for any unknown top-level, metadata.*, or environment.* key. Canonical key sets are exported constants (CANONICAL_TOP_LEVEL_KEYS / CANONICAL_METADATA_KEYS / CANONICAL_ENVIRONMENT_KEYS) so sibling PRs PDX-488 (PROVARHOME-001) and PDX-494 (PARALLEL-001) can extend them. A minimal inline Levenshtein helper suggests the closest canonical key within distance 2 ("Did you mean 'testCase'?"). Unknown keys are warnings (not errors) so additive Provar schema versions do not break older MCP clients. The validate tool description now references the testCases → testCase example explicitly. A regression fixture lives at test/fixtures/properties/testcases-typo.json and is shared with the VALIDATE-TYPO-B half (sibling PR, Thread B). Out of scope for this PR (Thread A, PR 1 of 3): the testrun zero-tests guard (VALIDATE-TYPO-B, Thread B), the config_load wiring (final Thread B commit), PROVARHOME-001 (PDX-488), and PARALLEL-001 (PDX-494). Depends on #182 (PDX-485 thread-prep: shared warningCodes.ts enum). --- docs/mcp.md | 19 +-- src/mcp/tools/propertiesTools.ts | 121 ++++++++++++++++++- test/fixtures/properties/testcases-typo.json | 21 ++++ test/unit/mcp/propertiesTools.test.ts | 103 ++++++++++++++++ 4 files changed, 256 insertions(+), 8 deletions(-) create mode 100644 test/fixtures/properties/testcases-typo.json diff --git a/docs/mcp.md b/docs/mcp.md index 3ec6bbe9..100d548d 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1104,7 +1104,7 @@ Updates one or more fields in a `provardx-properties.json` file. Only the suppli ### `provar_properties_validate` -Validates a `provardx-properties.json` file against the ProvarDX schema. Checks required fields, valid enum values, and warns about unfilled `${PLACEHOLDER}` values. Accepts either a file path or inline JSON content. +Validates a `provardx-properties.json` file against the ProvarDX schema. Checks required fields, valid enum values, and warns about unfilled `${PLACEHOLDER}` values. Also surfaces a `SCHEMA-001` warning for any unknown top-level, `metadata.*`, or `environment.*` key, with a "Did you mean ..." suggestion when a canonical key is within Levenshtein distance 2. Accepts either a file path or inline JSON content. **Input** @@ -1115,12 +1115,17 @@ Validates a `provardx-properties.json` file against the ProvarDX schema. Checks **Output** -| Field | Description | -| --------------- | ----------------------------------------------- | -| `is_valid` | `true` if no errors | -| `error_count` | Number of validation errors | -| `warning_count` | Number of warnings (e.g. unfilled placeholders) | -| `issues` | Array of `{ field, severity, message }` | +| Field | Description | +| --------------- | ----------------------------------------------------------- | +| `is_valid` | `true` if no errors (warnings alone do not flip `is_valid`) | +| `error_count` | Number of validation errors | +| `warning_count` | Number of warnings (placeholders, unknown keys, etc.) | +| `errors` | Array of `{ field, severity: 'error', message }` | +| `warnings` | Array of `{ field, severity: 'warning', message }` | + +**Warning codes (`warnings` array):** + +- `SCHEMA-001` — unknown key at top-level / `metadata.*` / `environment.*`. Example: `WARNING [SCHEMA-001]: Unknown field 'testCases' at top-level. Did you mean 'testCase'?` Unknown keys are **warnings, not errors**, so additive Provar versions do not break older MCP clients. The classic instance is the `testCases` (plural) typo for the canonical `testCase` (singular) — if you see SCHEMA-001 on `testCases`, fix the spelling before running any tests. **Error codes:** `MISSING_INPUT`, `PROPERTIES_FILE_NOT_FOUND`, `MALFORMED_JSON`, `PATH_NOT_ALLOWED` diff --git a/src/mcp/tools/propertiesTools.ts b/src/mcp/tools/propertiesTools.ts index f7b558df..06d9846f 100644 --- a/src/mcp/tools/propertiesTools.ts +++ b/src/mcp/tools/propertiesTools.ts @@ -16,6 +16,7 @@ import type { ServerConfig } from '../server.js'; import { assertPathAllowed, PathPolicyError } from '../security/pathPolicy.js'; import { makeError, makeRequestId } from '../schemas/common.js'; import { log } from '../logging/logger.js'; +import { WARNING_CODES, formatWarning } from '../utils/warningCodes.js'; import { desc } from './descHelper.js'; // ── Validation helpers ──────────────────────────────────────────────────────── @@ -30,12 +31,118 @@ const TOP_REQUIRED = ['provarHome', 'projectPath', 'resultsPath', 'metadata', 'e const METADATA_REQUIRED = ['metadataLevel', 'cachePath'] as const; const ENV_REQUIRED = ['webBrowser', 'webBrowserConfig', 'webBrowserProviderName', 'webBrowserDeviceName'] as const; +/** + * Canonical key sets for provardx-properties.json. Sourced from the documented schema in + * `docs/mcp.md` (provar_properties_set updates schema) and the `propertyFileContent` template + * from `@provartesting/provardx-plugins-utils`. Sibling PRs (PDX-488, PDX-494) may extend + * these sets — keep them exported and additive. + * + * NOTE: This set is intentionally lenient on "future Provar additions" — unknown keys emit a + * SCHEMA-001 warning (not a hard error) so older MCP clients keep working when new keys ship. + */ +export const CANONICAL_TOP_LEVEL_KEYS: readonly string[] = [ + // Required + 'provarHome', + 'projectPath', + 'resultsPath', + 'metadata', + 'environment', + // Optional — documented in docs/mcp.md properties_set schema + 'resultsPathDisposition', + 'testOutputLevel', + 'pluginOutputlevel', + 'stopOnError', + 'excludeCallable', + 'testprojectSecrets', + 'testCase', + 'testPlan', + 'connectionOverride', + // Optional — present in the standard template (propertyFileContent.js) + 'smtpPath', + 'lightningMode', + 'connectionRefreshType', + 'testplanFeatures', +]; + +export const CANONICAL_METADATA_KEYS: readonly string[] = ['metadataLevel', 'cachePath']; + +export const CANONICAL_ENVIRONMENT_KEYS: readonly string[] = [ + 'testEnvironment', + 'webBrowser', + 'webBrowserConfig', + 'webBrowserProviderName', + 'webBrowserDeviceName', +]; + const VALID_RESULTS_DISPOSITION = ['Increment', 'Replace', 'Fail']; const VALID_OUTPUT_LEVELS = ['BASIC', 'DETAILED', 'DIAGNOSTIC']; const VALID_PLUGIN_LEVELS = ['SEVERE', 'WARNING', 'INFO', 'FINE', 'FINER', 'FINEST']; const VALID_BROWSERS = ['Chrome', 'Safari', 'Edge', 'Edge_Legacy', 'Firefox', 'IE', 'Chrome_Headless']; const VALID_METADATA_LEVELS = ['Reuse', 'Reload', 'Refresh']; +/** + * Iterative Levenshtein distance — small, no-dependency implementation used solely for + * "did you mean ..." suggestions in SCHEMA-001 warnings. Returns the edit distance between + * `a` and `b`. Case-sensitive. + */ +function levenshtein(a: string, b: string): number { + if (a === b) return 0; + if (a.length === 0) return b.length; + if (b.length === 0) return a.length; + let prev: number[] = Array.from({ length: b.length + 1 }, (_, i) => i); + for (let i = 1; i <= a.length; i++) { + const curr: number[] = [i]; + for (let j = 1; j <= b.length; j++) { + const cost = a[i - 1] === b[j - 1] ? 0 : 1; + curr.push(Math.min(curr[j - 1] + 1, prev[j] + 1, prev[j - 1] + cost)); + } + prev = curr; + } + return prev[b.length]; +} + +/** + * Returns the canonical key whose Levenshtein distance from `key` is <= 2, or `undefined` + * if no key is within that threshold. Picks the closest match (smallest distance, then + * lexicographic) to keep suggestions stable across runs. + */ +function closestCanonicalKey(key: string, canonical: readonly string[]): string | undefined { + let best: { key: string; dist: number } | undefined; + for (const candidate of canonical) { + const dist = levenshtein(key, candidate); + if (dist > 2) continue; + if (!best || dist < best.dist || (dist === best.dist && candidate < best.key)) { + best = { key: candidate, dist }; + } + } + return best?.key; +} + +/** + * Emit SCHEMA-001 warnings for any keys in `actual` that are not in `canonical`. The + * `pathLabel` is the human-readable scope (`top-level`, `metadata`, `environment`). + */ +function findUnknownKeys( + actual: Record, + canonical: readonly string[], + pathLabel: string, + fieldPrefix: string +): ValidationError[] { + const results: ValidationError[] = []; + const canonicalSet = new Set(canonical); + for (const key of Object.keys(actual)) { + if (canonicalSet.has(key)) continue; + const suggestion = closestCanonicalKey(key, canonical); + const message = formatWarning(WARNING_CODES.SCHEMA_001, `Unknown field '${key}' at ${pathLabel}.`, suggestion); + results.push({ + field: fieldPrefix ? `${fieldPrefix}.${key}` : key, + message, + severity: 'warning', + }); + } + return results; +} + // eslint-disable-next-line complexity function validateProperties(props: Record): ValidationError[] { const errors: ValidationError[] = []; @@ -134,6 +241,15 @@ function validateProperties(props: Record): ValidationError[] { } } + // SCHEMA-001: unknown keys at any level — warning only so additive Provar versions don't break old clients + errors.push(...findUnknownKeys(props, CANONICAL_TOP_LEVEL_KEYS, 'top-level', '')); + if (meta && typeof meta === 'object') { + errors.push(...findUnknownKeys(meta, CANONICAL_METADATA_KEYS, 'metadata', 'metadata')); + } + if (env && typeof env === 'object') { + errors.push(...findUnknownKeys(env, CANONICAL_ENVIRONMENT_KEYS, 'environment', 'environment')); + } + return errors; } @@ -624,9 +740,12 @@ export function registerPropertiesValidate(server: McpServer, config: ServerConf [ 'Validate a provardx-properties.json file against the ProvarDX schema.', 'Checks required fields, valid enum values, and warns about unfilled placeholder values.', + 'Also emits a SCHEMA-001 warning for any unknown top-level, metadata.*, or environment.* key', + "(e.g. 'testCases' → did you mean 'testCase'?). Unknown keys are warnings, not errors,", + 'so additive keys in future Provar versions do not break older MCP clients.', 'Accepts either a file path or inline JSON content.', ].join(' '), - 'Validate a provardx-properties.json against required fields and enum values.' + 'Validate a provardx-properties.json; warns on unknown keys (SCHEMA-001) like testCases vs testCase.' ), inputSchema: { file_path: z diff --git a/test/fixtures/properties/testcases-typo.json b/test/fixtures/properties/testcases-typo.json new file mode 100644 index 00000000..bdef42eb --- /dev/null +++ b/test/fixtures/properties/testcases-typo.json @@ -0,0 +1,21 @@ +{ + "provarHome": "/opt/provar", + "projectPath": "/projects/sample-project", + "resultsPath": "/projects/sample-project/ANT/Results", + "resultsPathDisposition": "Replace", + "testOutputLevel": "BASIC", + "pluginOutputlevel": "WARNING", + "stopOnError": false, + "metadata": { + "metadataLevel": "Reuse", + "cachePath": "/projects/sample-project/ANT/.provarCaches" + }, + "environment": { + "testEnvironment": "default", + "webBrowser": "Chrome_Headless", + "webBrowserConfig": "Full Screen", + "webBrowserProviderName": "Desktop", + "webBrowserDeviceName": "Full Screen" + }, + "testCases": ["tests/SmokeFlow.testcase"] +} diff --git a/test/unit/mcp/propertiesTools.test.ts b/test/unit/mcp/propertiesTools.test.ts index d405b084..442e0369 100644 --- a/test/unit/mcp/propertiesTools.test.ts +++ b/test/unit/mcp/propertiesTools.test.ts @@ -535,4 +535,107 @@ describe('provar_properties_validate', () => { 'Expected error on metadata.metadataLevel' ); }); + + // ── SCHEMA-001: unknown-key detection (PDX-486 VALIDATE-TYPO-A) ────────────── + + describe('SCHEMA-001 unknown-key detection', () => { + it('fires for unknown top-level key with did-you-mean suggestion within distance 2', () => { + const props = validProps(); + // Classic typo: testCases (plural) vs canonical testCase + props['testCases'] = ['tests/foo.testcase']; + + const result = server.call('provar_properties_validate', { content: JSON.stringify(props) }); + + const body = parseText(result); + const warnings = body['warnings'] as Array<{ field: string; message: string; severity: string }>; + const w = warnings.find((x) => x.field === 'testCases'); + assert.ok(w, 'Expected SCHEMA-001 warning for testCases'); + assert.ok(w.message.includes('SCHEMA-001'), 'Warning should reference SCHEMA-001'); + assert.ok(w.message.includes("Unknown field 'testCases'"), 'Warning should name the offending key'); + assert.ok(w.message.includes('top-level'), 'Warning should label the scope as top-level'); + assert.ok(w.message.includes("'testCase'"), `Expected "did you mean 'testCase'" suggestion, got: ${w.message}`); + }); + + it("fires for unknown metadata.* key (e.g. 'metadataLvel') with suggestion", () => { + const props = validProps(); + (props['metadata'] as Record)['metadataLvel'] = 'Reuse'; + + const result = server.call('provar_properties_validate', { content: JSON.stringify(props) }); + + const body = parseText(result); + const warnings = body['warnings'] as Array<{ field: string; message: string }>; + const w = warnings.find((x) => x.field === 'metadata.metadataLvel'); + assert.ok(w, 'Expected SCHEMA-001 warning for metadata.metadataLvel'); + assert.ok(w.message.includes('SCHEMA-001')); + assert.ok(w.message.includes('metadata'), 'Warning should label the scope as metadata'); + assert.ok(w.message.includes("'metadataLevel'"), `Expected suggestion, got: ${w.message}`); + }); + + it("fires for unknown environment.* key (e.g. 'testEnvironments' → testEnvironment)", () => { + const props = validProps(); + (props['environment'] as Record)['testEnvironments'] = 'QA'; + + const result = server.call('provar_properties_validate', { content: JSON.stringify(props) }); + + const body = parseText(result); + const warnings = body['warnings'] as Array<{ field: string; message: string }>; + const w = warnings.find((x) => x.field === 'environment.testEnvironments'); + assert.ok(w, 'Expected SCHEMA-001 warning for environment.testEnvironments'); + assert.ok(w.message.includes('SCHEMA-001')); + assert.ok(w.message.includes('environment')); + assert.ok(w.message.includes("'testEnvironment'"), `Expected testEnvironment suggestion, got: ${w.message}`); + }); + + it('does NOT fire SCHEMA-001 for known top-level / metadata / environment keys', () => { + const result = server.call('provar_properties_validate', { content: JSON.stringify(validProps()) }); + + const body = parseText(result); + const warnings = body['warnings'] as Array<{ message: string }>; + assert.ok( + warnings.every((w) => !w.message.includes('SCHEMA-001')), + `Expected zero SCHEMA-001 warnings on a valid file, got: ${JSON.stringify(warnings)}` + ); + }); + + it('emits SCHEMA-001 with no "Did you mean" when no canonical key is within distance 2', () => { + const props = validProps(); + props['totallyUnrelatedKey'] = 'x'; + + const result = server.call('provar_properties_validate', { content: JSON.stringify(props) }); + + const body = parseText(result); + const warnings = body['warnings'] as Array<{ field: string; message: string }>; + const w = warnings.find((x) => x.field === 'totallyUnrelatedKey'); + assert.ok(w, 'Expected SCHEMA-001 warning for totallyUnrelatedKey'); + assert.ok(w.message.includes('SCHEMA-001')); + assert.ok(!w.message.includes('Did you mean'), `Should not include suggestion, got: ${w.message}`); + }); + + it('is_valid stays true when the only issues are SCHEMA-001 warnings (no errors)', () => { + const props = validProps(); + props['testCases'] = ['x']; + + const result = server.call('provar_properties_validate', { content: JSON.stringify(props) }); + + const body = parseText(result); + assert.equal(body['is_valid'], true, 'Unknown keys are warnings only — is_valid must remain true'); + assert.equal(body['error_count'], 0); + assert.ok((body['warning_count'] as number) >= 1); + }); + + it("loads test/fixtures/properties/testcases-typo.json and reports SCHEMA-001 with 'testCase' suggestion", () => { + // Tests run from the repo root via wireit/yarn; resolve relative to cwd to avoid ESM __dirname. + const fixturePath = path.resolve(process.cwd(), 'test', 'fixtures', 'properties', 'testcases-typo.json'); + const content = fs.readFileSync(fixturePath, 'utf-8'); + + const result = server.call('provar_properties_validate', { content }); + + const body = parseText(result); + assert.equal(body['is_valid'], true, 'Fixture should pass structural validation (warnings only)'); + const warnings = body['warnings'] as Array<{ field: string; message: string }>; + const w = warnings.find((x) => x.field === 'testCases'); + assert.ok(w, 'Expected SCHEMA-001 warning for the testCases typo in the fixture'); + assert.ok(w.message.includes("Did you mean 'testCase'?"), `Expected suggestion, got: ${w.message}`); + }); + }); }); From e78683a8d1abedd549d7a91da1547af977377d05 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:11:20 -0500 Subject: [PATCH 04/17] =?UTF-8?q?PDX-490:=20feat(mcp)=20=E2=80=94=20add=20?= =?UTF-8?q?error=5Fcategory=20and=20retryable=20fields=20to=20test-run=20f?= =?UTF-8?q?ailures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Consumers of provar_automation_testrun.steps[] and provar_testrun_rca.failures[] currently have no machine-friendly retry hint and must re-parse human-readable strings to decide whether a failure is transient or deterministic, so agents either retry locator failures forever or give up on a single ECONNRESET blip. Fix: Add optional error_category (INFRASTRUCTURE|ASSERTION|LOCATOR|TIMEOUT|OTHER) and retryable (boolean) fields to both FailureReport (rcaTools.ts) and JUnitStepResult (antTools.ts), populated by an additive pattern-match classifier; retryable=true only for INFRASTRUCTURE and TIMEOUT, undefined when no pattern matches — non-breaking — with tool descriptions and docs/mcp.md updated. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 37 ++++++++------ src/mcp/tools/antTools.ts | 43 +++++++++++++++- src/mcp/tools/automationTools.ts | 2 + src/mcp/tools/rcaTools.ts | 56 ++++++++++++++++++++- test/unit/mcp/antTools.test.ts | 51 +++++++++++++++++++ test/unit/mcp/rcaTools.test.ts | 86 ++++++++++++++++++++++++++++++++ 6 files changed, 258 insertions(+), 17 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 33d23421..1a17f1cf 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1386,12 +1386,17 @@ After each run, the tool scans the results directory for JUnit XML files and add ```json "steps": [ { "testItemId": "1", "title": "TC-Login-001-LoginAndVerify.testcase", "status": "pass" }, - { "testItemId": "2", "title": "TC-Login-002-ForgotPassword.testcase", "status": "fail", "errorMessage": "Execution failed: Element not found" } + { "testItemId": "2", "title": "TC-Login-002-ForgotPassword.testcase", "status": "fail", "errorMessage": "TimeoutException: page did not load", "error_category": "TIMEOUT", "retryable": true } ] ``` Each entry represents one test case. `status` is `"pass"`, `"fail"`, or `"skip"`. If the results directory cannot be located or contains no JUnit XML, `details.warning` explains why and `steps` is absent. +Failed steps may include two optional classification fields: + +- `error_category` — one of `INFRASTRUCTURE`, `ASSERTION`, `LOCATOR`, `TIMEOUT`, `OTHER`, set when the failure text matches a known pattern. +- `retryable` — `true` when `error_category` is `INFRASTRUCTURE` or `TIMEOUT` (transient causes), `false` for `ASSERTION`/`LOCATOR`/`OTHER`. Absent when no pattern matched. + **Error codes:** `AUTOMATION_TESTRUN_FAILED`, `SF_NOT_FOUND` --- @@ -1535,22 +1540,26 @@ Use `mode="failures"` when you only need the list of failing test case names wit **`FailureReport` fields (mode=rca only):** -| Field | Description | -| --------------------- | -------------------------------------------------------- | -| `test_case` | Test case filename from JUnit `` | -| `error_class` | Extracted exception class name | -| `error_message` | First 500 chars of failure/error text | -| `root_cause_category` | One of 12 categories (see table below) | -| `root_cause_summary` | Human-readable cause description | -| `recommendation` | Suggested fix action | -| `page_object` | Extracted from `Page Object: ...` pattern, or `null` | -| `operation` | Extracted from `operation: ...` pattern, or `null` | -| `report_html` | Path to per-test HTML report if found, else `null` | -| `screenshot_dir` | Path to `Artifacts/` directory if it exists, else `null` | -| `pre_existing` | `true` if the same test failed in a prior Increment run | +| Field | Description | +| --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `test_case` | Test case filename from JUnit `` | +| `error_class` | Extracted exception class name | +| `error_message` | First 500 chars of failure/error text | +| `root_cause_category` | One of 12 categories (see table below) | +| `root_cause_summary` | Human-readable cause description | +| `recommendation` | Suggested fix action | +| `page_object` | Extracted from `Page Object: ...` pattern, or `null` | +| `operation` | Extracted from `operation: ...` pattern, or `null` | +| `report_html` | Path to per-test HTML report if found, else `null` | +| `screenshot_dir` | Path to `Artifacts/` directory if it exists, else `null` | +| `pre_existing` | `true` if the same test failed in a prior Increment run | +| `error_category` | Optional. One of `INFRASTRUCTURE` \| `ASSERTION` \| `LOCATOR` \| `TIMEOUT` \| `OTHER`. Absent when no known pattern matched. | +| `retryable` | Optional. `true` when `error_category` is `INFRASTRUCTURE` or `TIMEOUT` (transient causes); `false` otherwise. Absent when `error_category` is absent. | **Root cause categories:** `DRIVER_VERSION_MISMATCH`, `LOCATOR_STALE`, `TIMEOUT`, `ASSERTION_FAILED`, `CREDENTIAL_FAILURE`, `MISSING_CALLABLE`, `METADATA_CACHE`, `PAGE_OBJECT_COMPILE`, `CONNECTION_REFUSED`, `DATA_SETUP`, `LICENSE_INVALID`, `SALESFORCE_VALIDATION`, `SALESFORCE_PICKLIST`, `SALESFORCE_REFERENCE`, `SALESFORCE_ACCESS`, `SALESFORCE_TRIGGER`, `UNKNOWN` +**Error category vs. root cause category:** `root_cause_category` is fine-grained (17 buckets) and drives the human-readable `recommendation`. `error_category` is coarse-grained (5 buckets) and drives automated retry policy via `retryable`. The two are independent classifiers over the same failure text — both may be set on the same failure. + Salesforce DML error categories (`SALESFORCE_*`) represent test-data failures — they appear in `failures[].root_cause_category` but are **not** included in `infrastructure_issues`. **Error codes:** `RESULTS_NOT_CONFIGURED`, `PATH_NOT_ALLOWED`, `PATH_TRAVERSAL` diff --git a/src/mcp/tools/antTools.ts b/src/mcp/tools/antTools.ts index 9efc0c93..92d5354f 100644 --- a/src/mcp/tools/antTools.ts +++ b/src/mcp/tools/antTools.ts @@ -979,11 +979,46 @@ function finalizeAnt( // ── JUnit XML step parsing ──────────────────────────────────────────────────── +export type JUnitErrorCategory = 'INFRASTRUCTURE' | 'ASSERTION' | 'LOCATOR' | 'TIMEOUT' | 'OTHER'; + export interface JUnitStepResult { testItemId: string; title: string; status: 'pass' | 'fail' | 'skip'; errorMessage?: string; + error_category?: JUnitErrorCategory; + retryable?: boolean; +} + +/** + * Classify a failure message into a coarse-grained category used for retry decisions. + * Mirrors the classifier in rcaTools.ts (PDX-490) so a downstream consumer sees the + * same labelling whether they consume `provar_automation_testrun.steps[]` or + * `provar_testrun_rca.failures[]`. + * + * Returns `undefined` when no pattern matches. + */ +export function classifyStepErrorCategory(errorText: string): JUnitErrorCategory | undefined { + if (/Connection reset|Failed to read client socket message|socket hang up|ECONNRESET/i.test(errorText)) { + return 'INFRASTRUCTURE'; + } + if (/NoSuchElementException/i.test(errorText)) return 'LOCATOR'; + if (/TimeoutException/i.test(errorText)) return 'TIMEOUT'; + if (/AssertionException/i.test(errorText)) return 'ASSERTION'; + if ( + /SessionNotCreatedException|WebDriverException|ClassNotFoundException|LicenseException|InvalidPasswordException/i.test( + errorText + ) + ) { + return 'OTHER'; + } + return undefined; +} + +/** Only transient categories (INFRASTRUCTURE, TIMEOUT) are retryable. */ +export function isStepRetryable(category: JUnitErrorCategory | undefined): boolean | undefined { + if (category === undefined) return undefined; + return category === 'INFRASTRUCTURE' || category === 'TIMEOUT'; } export interface JUnitParseResult { @@ -1043,7 +1078,13 @@ function extractStepsFromJUnit(parsed: Record): JUnitStepResult const errorMessage = extractFailureText(tc['failure'] ?? tc['error']); const step: JUnitStepResult = { testItemId: String(idx), title, status }; - if (errorMessage) step.errorMessage = errorMessage; + if (errorMessage) { + step.errorMessage = errorMessage; + const error_category = classifyStepErrorCategory(errorMessage); + const retryable = isStepRetryable(error_category); + if (error_category !== undefined) step.error_category = error_category; + if (retryable !== undefined) step.retryable = retryable; + } steps.push(step); } } diff --git a/src/mcp/tools/automationTools.ts b/src/mcp/tools/automationTools.ts index ccc712df..7b42c4c2 100644 --- a/src/mcp/tools/automationTools.ts +++ b/src/mcp/tools/automationTools.ts @@ -241,6 +241,8 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi 'Output buffer: a 50 MB maxBuffer is set so ENOBUFS on verbose Provar runs is now rare.', 'If ENOBUFS still occurs (extremely verbose logging), run `sf provar automation test run --json` directly in the terminal and pipe or tail the output instead of retrying this tool.', 'Typical local AI loop: config.load → compile → testrun → inspect results.', + 'Each failed step in `steps[]` may include optional error_category (INFRASTRUCTURE|ASSERTION|LOCATOR|TIMEOUT|OTHER)', + 'and retryable (boolean) fields when the failure text matches a known pattern — use these to drive automated retry policy.', ].join(' '), 'Run local Provar tests via sf CLI; requires config_load first.' ), diff --git a/src/mcp/tools/rcaTools.ts b/src/mcp/tools/rcaTools.ts index f37b6a97..7b310864 100644 --- a/src/mcp/tools/rcaTools.ts +++ b/src/mcp/tools/rcaTools.ts @@ -32,6 +32,8 @@ interface LocateResult { resolution_source: string; } +type ErrorCategory = 'INFRASTRUCTURE' | 'ASSERTION' | 'LOCATOR' | 'TIMEOUT' | 'OTHER'; + interface FailureReport { test_case: string; error_class: string | null; @@ -44,6 +46,48 @@ interface FailureReport { report_html: string | null; screenshot_dir: string | null; pre_existing: boolean; + error_category?: ErrorCategory; + retryable?: boolean; +} + +/** + * Classify a failure message into a structured error category for retry decisions. + * + * Categories are coarse-grained and intended to drive automated retry policy + * downstream (e.g. retry INFRASTRUCTURE/TIMEOUT, never retry ASSERTION/LOCATOR). + * + * Returns `undefined` when no pattern matches — callers should leave the field unset. + */ +export function classifyErrorCategory(errorText: string): ErrorCategory | undefined { + // INFRASTRUCTURE — transient network / socket / browser-process failures. + if (/Connection reset|Failed to read client socket message|socket hang up|ECONNRESET/i.test(errorText)) { + return 'INFRASTRUCTURE'; + } + // LOCATOR — page object selector no longer matches the rendered DOM. + if (/NoSuchElementException/i.test(errorText)) return 'LOCATOR'; + // TIMEOUT — element or operation did not complete in time. + if (/TimeoutException/i.test(errorText)) return 'TIMEOUT'; + // ASSERTION — explicit assertion failure raised by the test or framework. + if (/AssertionException/i.test(errorText)) return 'ASSERTION'; + // OTHER — known exception class but not fitting the four primary buckets. + if ( + /SessionNotCreatedException|WebDriverException|ClassNotFoundException|LicenseException|InvalidPasswordException/i.test( + errorText + ) + ) { + return 'OTHER'; + } + return undefined; +} + +/** + * A failure is retryable only when the underlying cause is transient. + * INFRASTRUCTURE (network blips) and TIMEOUT (slow page) can succeed on retry; + * ASSERTION / LOCATOR / OTHER are deterministic and should not be retried. + */ +export function isRetryable(category: ErrorCategory | undefined): boolean | undefined { + if (category === undefined) return undefined; + return category === 'INFRASTRUCTURE' || category === 'TIMEOUT'; } // ── Root cause classification ───────────────────────────────────────────────── @@ -667,7 +711,9 @@ function buildFailureReports( const poMatch = /Page Object:\s*([\w.]+)/i.exec(failureText); const opMatch = /operation:\s*(\w+)/i.exec(failureText); const matchingHtml = htmlFiles.find((f) => path.basename(f) === `${tc.name}.html`); - reports.push({ + const error_category = classifyErrorCategory(failureText); + const retryable = isRetryable(error_category); + const report: FailureReport = { test_case: tc.name, error_class, error_message: failureText.slice(0, 500), @@ -679,7 +725,10 @@ function buildFailureReports( report_html: matchingHtml ?? null, screenshot_dir: screenshotDir, pre_existing: priorFailed.has(tc.name), - }); + }; + if (error_category !== undefined) report.error_category = error_category; + if (retryable !== undefined) report.retryable = retryable; + reports.push(report); } return reports; } @@ -699,6 +748,9 @@ export function registerTestRunRca(server: McpServer, config: ServerConfig): voi 'Use mode="failures" to get a lightweight array of failed test cases', '([{ testItemId, title, errorMessage }]) without the full RCA classification — useful when you', 'need failure names quickly without loading the HTML report.', + 'Each failure includes optional error_category (INFRASTRUCTURE|ASSERTION|LOCATOR|TIMEOUT|OTHER)', + 'and retryable (boolean) fields when the failure text matches a known pattern — INFRASTRUCTURE/TIMEOUT', + 'are flagged retryable, others are not.', ].join(' '), 'Parse a Provar test run JUnit.xml and produce an RCA report with failure classification.' ), diff --git a/test/unit/mcp/antTools.test.ts b/test/unit/mcp/antTools.test.ts index 74e37bc2..f2fd6365 100644 --- a/test/unit/mcp/antTools.test.ts +++ b/test/unit/mcp/antTools.test.ts @@ -847,4 +847,55 @@ describe('parseJUnitResults', () => { assert.ok(result.steps[0].errorMessage?.includes('Execution failed')); assert.ok(result.steps[0].errorMessage?.includes('stack trace here')); }); + + // ── PDX-490: error_category + retryable on step results ───────────────────── + + function writeFailureJunit(dir: string, failureBody: string): void { + const xml = `${failureBody}`; + fs.writeFileSync(path.join(dir, 'JUnit.xml'), xml); + } + + it('populates error_category=INFRASTRUCTURE and retryable=true for Connection reset', () => { + writeFailureJunit(junitTmpDir, 'Connection reset by peer while reading response'); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].error_category, 'INFRASTRUCTURE'); + assert.equal(result.steps[0].retryable, true); + }); + + it('populates error_category=LOCATOR and retryable=false for NoSuchElementException', () => { + writeFailureJunit(junitTmpDir, 'NoSuchElementException: Unable to locate element'); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].error_category, 'LOCATOR'); + assert.equal(result.steps[0].retryable, false); + }); + + it('populates error_category=TIMEOUT and retryable=true for TimeoutException', () => { + writeFailureJunit(junitTmpDir, 'TimeoutException: operation did not complete'); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].error_category, 'TIMEOUT'); + assert.equal(result.steps[0].retryable, true); + }); + + it('populates error_category=ASSERTION and retryable=false for AssertionException', () => { + writeFailureJunit(junitTmpDir, 'AssertionException: expected X but was Y'); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].error_category, 'ASSERTION'); + assert.equal(result.steps[0].retryable, false); + }); + + it('leaves error_category and retryable undefined when no pattern matches', () => { + writeFailureJunit(junitTmpDir, 'something completely unrecognised XYZ_BANANA'); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].error_category, undefined); + assert.equal(result.steps[0].retryable, undefined); + }); + + it('does not set error_category or retryable on passing steps', () => { + const xml = ''; + fs.writeFileSync(path.join(junitTmpDir, 'JUnit.xml'), xml); + const result = parseJUnitResults(junitTmpDir); + assert.equal(result.steps[0].status, 'pass'); + assert.equal(result.steps[0].error_category, undefined); + assert.equal(result.steps[0].retryable, undefined); + }); }); diff --git a/test/unit/mcp/rcaTools.test.ts b/test/unit/mcp/rcaTools.test.ts index 42ce4ec1..48f01ee9 100644 --- a/test/unit/mcp/rcaTools.test.ts +++ b/test/unit/mcp/rcaTools.test.ts @@ -717,3 +717,89 @@ describe('provar_testrun_rca mode=failures', () => { ); }); }); + +// ── PDX-490: error_category + retryable on failures[] ───────────────────────── + +describe('provar_testrun_rca error_category and retryable (PDX-490)', () => { + function junitWithFailure(testName: string, failureText: string): string { + return ` + + + + ${failureText} + + +`; + } + + function getFirstFailure(junit: string, name: string): Record { + const resultsDir = makeResultsDir(path.join(tmpDir, name), junit); + const body = parseText(server.call('provar_testrun_rca', { project_path: tmpDir, results_path: resultsDir })); + const failures = body['failures'] as Array>; + assert.equal(failures.length, 1, `expected exactly one failure in ${name}`); + return failures[0]; + } + + it('classifies "Connection reset" as INFRASTRUCTURE and retryable=true', () => { + const f = getFirstFailure(junitWithFailure('NetTest', 'Connection reset by peer'), 'cat-infra'); + assert.equal(f['error_category'], 'INFRASTRUCTURE'); + assert.equal(f['retryable'], true); + }); + + it('classifies "ECONNRESET" as INFRASTRUCTURE and retryable=true', () => { + const f = getFirstFailure(junitWithFailure('NetTest2', 'socket error: ECONNRESET while reading'), 'cat-econnreset'); + assert.equal(f['error_category'], 'INFRASTRUCTURE'); + assert.equal(f['retryable'], true); + }); + + it('classifies NoSuchElementException as LOCATOR and retryable=false', () => { + const f = getFirstFailure( + junitWithFailure('LocTest', 'NoSuchElementException: Unable to locate element #submit'), + 'cat-locator' + ); + assert.equal(f['error_category'], 'LOCATOR'); + assert.equal(f['retryable'], false); + }); + + it('classifies TimeoutException as TIMEOUT and retryable=true', () => { + const f = getFirstFailure( + junitWithFailure('TimeoutTest', 'TimeoutException: page did not load in 30s'), + 'cat-timeout' + ); + assert.equal(f['error_category'], 'TIMEOUT'); + assert.equal(f['retryable'], true); + }); + + it('classifies AssertionException as ASSERTION and retryable=false', () => { + const f = getFirstFailure( + junitWithFailure('AssertTest', 'AssertionException: expected "yes" but got "no"'), + 'cat-assertion' + ); + assert.equal(f['error_category'], 'ASSERTION'); + assert.equal(f['retryable'], false); + }); + + it('a plain "expected X but was Y" assertion text with no exception class is undefined (no match)', () => { + const f = getFirstFailure(junitWithFailure('PlainAssert', 'expected 5 but was 3'), 'cat-undef'); + // No known exception class → error_category should be undefined / absent and retryable absent. + assert.equal(f['error_category'], undefined); + assert.equal(f['retryable'], undefined); + }); + + it('classifies WebDriverException (matches errorClassPatterns but not primary buckets) as OTHER and retryable=false', () => { + const f = getFirstFailure(junitWithFailure('DriverTest', 'WebDriverException: chrome not reachable'), 'cat-other'); + assert.equal(f['error_category'], 'OTHER'); + assert.equal(f['retryable'], false); + }); + + it('error_class still populated alongside the new fields (additive — no breaking change)', () => { + const f = getFirstFailure(junitWithFailure('LocTest2', 'NoSuchElementException: missing button'), 'cat-additive'); + assert.equal(f['error_class'], 'NoSuchElementException'); + assert.equal(f['error_category'], 'LOCATOR'); + assert.equal(f['retryable'], false); + // Existing fields must remain intact: + assert.equal(f['root_cause_category'], 'LOCATOR_STALE'); + assert.ok(typeof f['error_message'] === 'string'); + assert.equal(f['pre_existing'], false); + }); +}); From 67f3653bb0ef294e150ca9f7c24a171d9066ed53 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:11:30 -0500 Subject: [PATCH 05/17] =?UTF-8?q?PDX-486:=20feat(mcp)=20=E2=80=94=20testru?= =?UTF-8?q?n=20zero-tests=20guard=20(RUN-001=20warning)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: provar_automation_testrun returned exitCode 0 with an empty steps[] array when a misspelled testCase/testCases key in provardx-properties.json caused the run to select nothing. Agents (and humans) had no signal to disambiguate "intentionally zero tests ran" from "typo silently dropped my entire selector", leading to wasted retry loops and downstream confusion when later tooling complained about missing results. Fix: Introduce a single JUnit introspection helper (introspectJUnit) that returns stepCount, parseWarning, and a resultsPathResolved flag. When sf exits 0 and the JUnit XML for the resolved results dir contains zero test cases, the response gains a warnings[] entry built via formatWarning(WARNING_CODES.RUN_001, ...) pointing the agent at the testCase/testCases spelling pitfall. The warning is additive — exitCode and isError remain unchanged. The helper is structured so future PDX-491 JUNIT-001 (expected-vs-actual mismatch) can read stepCount from the same struct without re-parsing. Updated the tool description, docs/mcp.md (RUN-001 output documentation), and added four targeted tests (positive, success-with-steps negative, non-zero-exit negative, unresolved-results-dir silent case). --- docs/mcp.md | 5 +- src/mcp/tools/automationTools.ts | 63 +++++++++++++--- test/unit/mcp/automationTools.test.ts | 103 ++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 3ec6bbe9..c2061f40 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1395,7 +1395,7 @@ Triggers a Provar Automation test run using the currently loaded properties file | --------- | -------- | -------- | ------------------------------------------------------------------------ | | `flags` | string[] | no | Raw CLI flags to forward (e.g. `["--project-path", "/path/to/project"]`) | -**Output** — `{ requestId, exitCode, stdout, stderr[, output_lines_suppressed][, steps][, details.warning] }` +**Output** — `{ requestId, exitCode, stdout, stderr[, output_lines_suppressed][, steps][, details.warning][, warnings] }` The `stdout` field is filtered before returning: Java schema-validator lines (`com.networknt.schema.*`) and stale logger-lock `SEVERE` warnings are stripped. If any lines were suppressed, `output_lines_suppressed` contains the count. @@ -1410,7 +1410,10 @@ After each run, the tool scans the results directory for JUnit XML files and add Each entry represents one test case. `status` is `"pass"`, `"fail"`, or `"skip"`. If the results directory cannot be located or contains no JUnit XML, `details.warning` explains why and `steps` is absent. +**Zero-tests guard (RUN-001):** when the sf command exits 0 but the JUnit report contains zero executed test cases, the response includes a `warnings[]` array containing a [`RUN-001`](#warning-codes) message. This is almost always a typo such as `testCase` vs `testCases` (or some other unknown key) in `provardx-properties.json` — the run silently selected nothing. The warning is additive and never flips `exitCode` or sets `isError`; the failure surface remains driven by the underlying `sf` exit code. + **Error codes:** `AUTOMATION_TESTRUN_FAILED`, `SF_NOT_FOUND` +**Warning codes:** `RUN-001` (zero tests executed despite success) --- diff --git a/src/mcp/tools/automationTools.ts b/src/mcp/tools/automationTools.ts index ccc712df..41dd804a 100644 --- a/src/mcp/tools/automationTools.ts +++ b/src/mcp/tools/automationTools.ts @@ -15,6 +15,7 @@ import { makeError, makeRequestId } from '../schemas/common.js'; import { log } from '../logging/logger.js'; import type { ServerConfig } from '../server.js'; import { assertPathAllowed, PathPolicyError } from '../security/pathPolicy.js'; +import { WARNING_CODES, formatWarning } from '../utils/warningCodes.js'; import { parseJUnitResults } from './antTools.js'; import { runSfCommand } from './sfSpawn.js'; import { desc } from './descHelper.js'; @@ -226,6 +227,31 @@ function readResultsPathFromSfConfig(config: ServerConfig): string | null { // ── Tool: provar_automation_testrun ─────────────────────────────────────────── +/** + * JUnit introspection for the testrun response. Returns enough structure that + * downstream warning emitters (RUN-001 zero-tests, future JUNIT-001 expected-vs- + * actual mismatch) can read a single object instead of re-parsing. + */ +type JUnitIntrospection = { + steps: ReturnType['steps']; + stepCount: number; + parseWarning: string | undefined; + resultsPathResolved: boolean; +}; + +function introspectJUnit(config: ServerConfig): JUnitIntrospection { + const resultsPath = readResultsPathFromSfConfig(config); + if (!resultsPath) { + return { steps: [], stepCount: 0, parseWarning: undefined, resultsPathResolved: false }; + } + const { steps, warning } = parseJUnitResults(resultsPath); + return { steps, stepCount: steps.length, parseWarning: warning, resultsPathResolved: true }; +} + +const ZERO_TESTS_MESSAGE = + 'Test run exited successfully but zero tests were executed. ' + + 'Check the testCase / testCases (note spelling) field in provardx-properties.json.'; + export function registerAutomationTestRun(server: McpServer, config: ServerConfig): void { server.registerTool( 'provar_automation_testrun', @@ -240,9 +266,10 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi 'For grid/CI execution via Provar Quality Hub instead of running locally, use provar_qualityhub_testrun.', 'Output buffer: a 50 MB maxBuffer is set so ENOBUFS on verbose Provar runs is now rare.', 'If ENOBUFS still occurs (extremely verbose logging), run `sf provar automation test run --json` directly in the terminal and pipe or tail the output instead of retrying this tool.', + 'Zero-tests guard: if the sf exit code is 0 but the JUnit report contains no executed tests, a RUN-001 warning is added to `warnings[]` — usually a typo such as `testCase` vs `testCases` in provardx-properties.json.', 'Typical local AI loop: config.load → compile → testrun → inspect results.', ].join(' '), - 'Run local Provar tests via sf CLI; requires config_load first.' + 'Run local Provar tests via sf CLI; requires config_load first. Surfaces RUN-001 on zero-tests-executed.' ), inputSchema: { flags: z @@ -274,11 +301,10 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi const result = runSfCommand(['provar', 'automation', 'test', 'run', ...flags], sf_path); const { filtered, suppressed } = filterTestRunOutput(result.stdout); - // Attempt to enrich the response with structured step data from JUnit XML - const resultsPath = readResultsPathFromSfConfig(config); - const { steps, warning: junitWarning } = resultsPath - ? parseJUnitResults(resultsPath) - : { steps: [], warning: undefined }; + // Enrich the response with structured step data + warning hooks from JUnit XML. + // Single introspection call keeps the wiring extensible (e.g. future JUNIT-001 + // expected-vs-actual mismatch can read stepCount from the same struct). + const junit = introspectJUnit(config); if (result.exitCode !== 0) { const { filtered: filteredErr, suppressed: suppressedErr } = filterTestRunOutput( @@ -288,11 +314,11 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi ...makeError('AUTOMATION_TESTRUN_FAILED', filteredErr, requestId), ...(suppressedErr > 0 ? { output_lines_suppressed: suppressedErr } : {}), }; - if (steps.length > 0) errBody['steps'] = steps; - if (!resultsPath || junitWarning) { + if (junit.steps.length > 0) errBody['steps'] = junit.steps; + if (!junit.resultsPathResolved || junit.parseWarning) { errBody['details'] = { warning: - junitWarning ?? + junit.parseWarning ?? 'Could not locate results directory — step-level output unavailable. Run provar_automation_config_load first.', }; } @@ -306,8 +332,23 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi stderr: result.stderr, }; if (suppressed > 0) response['output_lines_suppressed'] = suppressed; - if (steps.length > 0) response['steps'] = steps; - if (junitWarning) response['details'] = { warning: junitWarning }; + if (junit.steps.length > 0) response['steps'] = junit.steps; + if (junit.parseWarning) response['details'] = { warning: junit.parseWarning }; + + // RUN-001: sf reported success but zero tests actually executed. + // Almost always a typo in the testCase / testCases field of provardx-properties.json. + // Only fires when the results dir was located (otherwise the absent step count is + // just "we don't know what ran", not "zero tests ran" — that case is already + // surfaced via details.warning above). Fires regardless of parse warning so the + // agent gets the typo hint even when the JUnit XML is degenerate. + if (junit.resultsPathResolved && junit.stepCount === 0) { + const warningStr = formatWarning(WARNING_CODES.RUN_001, ZERO_TESTS_MESSAGE); + // Append rather than overwrite so future warning emitters (e.g. JUNIT-001 mismatch + // in PDX-491) can coexist on the same response without stepping on each other. + const existing = response['warnings'] as string[] | undefined; + response['warnings'] = existing ? existing.concat(warningStr) : [warningStr]; + } + return { content: [{ type: 'text' as const, text: JSON.stringify(response) }], structuredContent: response }; } catch (err) { return handleSpawnError(err, requestId, 'provar_automation_testrun'); diff --git a/test/unit/mcp/automationTools.test.ts b/test/unit/mcp/automationTools.test.ts index 2a9c3c30..8590167d 100644 --- a/test/unit/mcp/automationTools.test.ts +++ b/test/unit/mcp/automationTools.test.ts @@ -20,6 +20,7 @@ import { filterTestRunOutput, setSfResultsPathForTesting, } from '../../../src/mcp/tools/automationTools.js'; +import { WARNING_CODES, formatWarning } from '../../../src/mcp/utils/warningCodes.js'; // ── Minimal mock server ─────────────────────────────────────────────────────── @@ -232,6 +233,108 @@ describe('automationTools', () => { fs.rmSync(tmpDir, { recursive: true, force: true }); } }); + + // ── RUN-001 zero-tests guard (PDX-486) ────────────────────────────────── + + describe('RUN-001 zero-tests-executed warning', () => { + const expectedWarning = formatWarning( + WARNING_CODES.RUN_001, + 'Test run exited successfully but zero tests were executed. ' + + 'Check the testCase / testCases (note spelling) field in provardx-properties.json.' + ); + + it('fires when exit is 0 and JUnit step count is 0 (empty )', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'junit-zero-')); + // Valid JUnit XML but containing zero elements — the canonical + // signal that the test selector (e.g. a misspelled testCase field) matched nothing. + const emptySuiteXml = ` +`; + fs.writeFileSync(path.join(tmpDir, 'results.xml'), emptySuiteXml, 'utf-8'); + setSfResultsPathForTesting(tmpDir); + + try { + spawnStub.returns(makeSpawnResult('tests done', '', 0)); + const result = server.call('provar_automation_testrun', { flags: [] }); + assert.ok(!isError(result), 'tool should still report success — RUN-001 is additive'); + const body = parseBody(result); + assert.equal(body.exitCode, 0); + assert.ok(Array.isArray(body.warnings), 'warnings[] should be present'); + const warnings = body.warnings as string[]; + assert.equal(warnings.length, 1); + assert.equal(warnings[0], expectedWarning, 'warning string must match formatWarning output exactly'); + // Sanity-check the format envelope without inline strings: + assert.ok(warnings[0].startsWith(`WARNING [${WARNING_CODES.RUN_001}]:`)); + } finally { + setSfResultsPathForTesting(undefined); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('does NOT fire when exit is 0 and step count >= 1', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'junit-some-')); + const oneCaseXml = ` + + +`; + fs.writeFileSync(path.join(tmpDir, 'results.xml'), oneCaseXml, 'utf-8'); + setSfResultsPathForTesting(tmpDir); + + try { + spawnStub.returns(makeSpawnResult('tests done', '', 0)); + const result = server.call('provar_automation_testrun', { flags: [] }); + const body = parseBody(result); + assert.equal(body.exitCode, 0); + const steps = body.steps as unknown[] | undefined; + assert.ok(steps && steps.length === 1, 'steps should reflect the one parsed testcase'); + assert.equal(body.warnings, undefined, 'warnings[] must be absent when at least one test ran'); + } finally { + setSfResultsPathForTesting(undefined); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('does NOT fire when exit is non-zero (genuine failure path)', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'junit-fail-')); + // Even if zero steps were extracted, a non-zero exit is a real failure, not a typo. + const emptySuiteXml = ` +`; + fs.writeFileSync(path.join(tmpDir, 'results.xml'), emptySuiteXml, 'utf-8'); + setSfResultsPathForTesting(tmpDir); + + try { + spawnStub.returns(makeSpawnResult('', 'compilation error', 1)); + const result = server.call('provar_automation_testrun', { flags: [] }); + assert.ok(isError(result), 'should report error on non-zero exit'); + const body = parseBody(result); + assert.equal(body.error_code, 'AUTOMATION_TESTRUN_FAILED'); + // Failure path uses the error body shape — RUN-001 lives on the success-path response only. + assert.equal(body.warnings, undefined, 'warnings[] must not be set on the failure path'); + } finally { + setSfResultsPathForTesting(undefined); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('does NOT fire when results dir cannot be located (stay silent — config_load issue, not a typo)', () => { + // Explicitly disable the results path override so introspection returns "not resolved". + // RUN-001 must stay silent here so the agent isn't misdirected toward a typo when the + // real issue is "config_load was never called" (or the results dir is outside allowed paths). + // The existing failure-path details.warning channel covers the config_load case on errors; + // on the success path with no resolvable results dir we simply don't emit RUN-001 — silence + // is correct because we genuinely don't know whether tests ran or not. + setSfResultsPathForTesting(null); + + try { + spawnStub.returns(makeSpawnResult('tests done', '', 0)); + const result = server.call('provar_automation_testrun', { flags: [] }); + const body = parseBody(result); + assert.equal(body.exitCode, 0); + assert.equal(body.warnings, undefined, 'no RUN-001 when results dir is unresolved'); + } finally { + setSfResultsPathForTesting(undefined); + } + }); + }); }); // ── provar_automation_compile ───────────────────────────────────────────── From 583a1d94610aa2a7ec4e399f7bb178c173efa7bb Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:16:56 -0500 Subject: [PATCH 06/17] =?UTF-8?q?PDX-489:=20feat(mcp)=20=E2=80=94=20warn?= =?UTF-8?q?=20on=20dataTable=20used=20in=20direct=20testCase-mode=20(DATA-?= =?UTF-8?q?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 46f691e91fd9fbae482857996d6d1852acc33b5e Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:25:57 -0500 Subject: [PATCH 07/17] =?UTF-8?q?PDX-485:=20test(mcp)=20=E2=80=94=20add=20?= =?UTF-8?q?exact=20key/value=20set=20assertions=20to=20WARNING=5FCODES=20t?= =?UTF-8?q?est?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review on PR #182 flagged that the parity test only iterated the expected map and asserted forward-direction equality (expected key -> WARNING_CODES[key]). A new enum entry added without updating the test, or an accidentally-removed entry, would not fail the build. Bidirectional coverage was missing. Fix: Hoist the expected map to the describe scope so it is reusable. Add two new assertions: (a) Object.keys(WARNING_CODES).sort() deepEqual Object.keys(expected).sort() — guards against additions and omissions in the key set; (b) Object.values(WARNING_CODES).sort() deepEqual Object.values(expected).sort() — guards against value drift. Tests now fail loudly on silent enum drift in either direction. --- test/unit/mcp/utils/warningCodes.test.ts | 27 +++++++++++++++++------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/test/unit/mcp/utils/warningCodes.test.ts b/test/unit/mcp/utils/warningCodes.test.ts index 3afa5b01..3ae7ff07 100644 --- a/test/unit/mcp/utils/warningCodes.test.ts +++ b/test/unit/mcp/utils/warningCodes.test.ts @@ -10,15 +10,16 @@ import { describe, it } from 'mocha'; import { WARNING_CODES, formatWarning } from '../../../../src/mcp/utils/warningCodes.js'; describe('WARNING_CODES', () => { + 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', + }; + 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], @@ -27,6 +28,16 @@ describe('WARNING_CODES', () => { ); } }); + + // Guards against silent enum drift: a new code added without updating this test, + // or an accidentally-removed code, must fail the build. + it('contains exactly the expected key set (no additions, no omissions)', () => { + assert.deepEqual(Object.keys(WARNING_CODES).sort(), Object.keys(expected).sort()); + }); + + it('contains exactly the expected value set (no additions, no omissions)', () => { + assert.deepEqual(Object.values(WARNING_CODES).sort(), Object.values(expected).sort()); + }); }); describe('formatWarning', () => { From 9a6b7846d339cea53eae5e1e77e4ab9ca332f564 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Tue, 19 May 2026 16:34:48 -0500 Subject: [PATCH 08/17] =?UTF-8?q?PDX-492:=20feat(mcp)=20=E2=80=94=20add=20?= =?UTF-8?q?provar=5Forg=5Fdescribe=20tool=20(reads=20workspace=20.metadata?= =?UTF-8?q?=20cache)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces provar_org_describe, a read-only MCP tool that surfaces cached Salesforce describe data from the Provar IDE workspace .metadata directory so authoring tools (provar_testcase_generate) can produce field-correct data steps without a live SF API call. Workspace discovery walks three candidates in order: 1. /workspace-/ (sibling pattern) 2. /Provar_Workspaces/workspace-/ 3. ~/Provar/workspace-/ (user-home fallback) Returns a structured cache-miss response with details.suggestion when the connection cache is absent, so the agent can either prompt the user to load the connection in Provar IDE or fall back to inline field hints. Registered under the existing 'inspect' tool group. H2b (sibling thread) consumes this tool's output to populate generator hints. RCA: provar_testcase_generate had no source of truth for which fields on a Salesforce object are required and what their types are. Agents either guessed (producing brittle tests), hard-coded names, or called the live SF API (slow + auth-dependent). The Provar IDE already caches describe data on disk after a connection is loaded — this PR exposes that cache as a read-only MCP tool so the cache becomes useful outside the IDE. Fix: New tool src/mcp/tools/orgDescribeTools.ts with strict path-policy checks on both project_path and connection_name (separator/traversal rejected). Cache schema is one file per object (.json preferred, .xml / .object accepted) so the existing IDE writer needs no change. Cache miss returns a stable shape with suggestion rather than an isError response, so callers do not need a try/catch path. 14 unit tests cover all 7 plan scenarios (workspace discovery, fallback, cache miss, path policy, happy path, field_filter, objects filter). Two smoke entries cover happy + miss. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp-pilot-guide.md | 31 ++ docs/mcp.md | 108 +++++++ scripts/mcp-smoke.cjs | 30 ++ src/mcp/server.ts | 3 +- src/mcp/tools/orgDescribeTools.ts | 430 +++++++++++++++++++++++++ test/unit/mcp/orgDescribeTools.test.ts | 405 +++++++++++++++++++++++ 6 files changed, 1006 insertions(+), 1 deletion(-) create mode 100644 src/mcp/tools/orgDescribeTools.ts create mode 100644 test/unit/mcp/orgDescribeTools.test.ts diff --git a/docs/mcp-pilot-guide.md b/docs/mcp-pilot-guide.md index bf7e0073..27381cfd 100644 --- a/docs/mcp-pilot-guide.md +++ b/docs/mcp-pilot-guide.md @@ -483,6 +483,37 @@ If any FAIL indicator appears, report it to the Provar team with the prompt and --- +### Scenario 13: Org-Aware Test Case Generation + +The `provar_org_describe` tool surfaces cached Salesforce describe data from the Provar IDE workspace so the agent knows which fields on each object are required and what their types are — without making a live Salesforce API call. Use it as a hint source before generating data-heavy steps. + +**Prerequisite:** the project must have been opened in Provar IDE at least once with the named connection loaded, so the `.metadata//` directory is populated. + +**Try this prompt:** + +> "Before generating a test case that creates an Account, call `provar_org_describe` against my project at `/path/to/MyProject` for connection `MyOrg` and the `Account` object only. Use the required-field list to populate the create form." + +The tool returns the discovered workspace path, a cache age, and per-object required-field metadata. Example call: + +```jsonc +{ + "project_path": "/Users/you/git/MyProject", + "connection_name": "MyOrg", + "objects": ["Account"], + "field_filter": "required" +} +``` + +**What to look for (PASS):** + +- Response includes `workspace_path` resolved to a real `workspace-*` directory. +- `objects[0].required_fields` contains at least one field with `nillable: false`. +- The follow-up `provar_testcase_generate` call uses field names from the response. + +**Cache-miss behaviour (also PASS):** if the cache directory does not exist the tool returns `details.suggestion` telling the agent how to recover — either open the project in Provar IDE to populate the cache, or pass field-type hints inline. + +--- + ## Security Model ### What the server does diff --git a/docs/mcp.md b/docs/mcp.md index 33d23421..00405b29 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -50,6 +50,8 @@ 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) + - [Org metadata access](#org-metadata-access) + - [provar_org_describe](#provar_org_describe) - [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) - [provar_nitrox_discover](#provar_nitrox_discover) - [provar_nitrox_read](#provar_nitrox_read) @@ -1686,6 +1688,112 @@ Remove a `.testinstance` file from a plan suite. Path is validated to stay withi --- +## Org metadata access + +Tools that surface Salesforce org metadata to authoring tools without making a live API call. These read from data that has already been written to disk by the Provar IDE — they do **not** trigger metadata downloads themselves and they do **not** require an authenticated session. + +> **Distinct from `.provarCaches`:** the runtime cache used by `provar_automation_testrun` lives at `/.provarCaches/` and is regenerated per run. The cache read by `provar_org_describe` lives in the Provar IDE **workspace** (`/.metadata//`) and is updated when a user opens the project and loads a connection in the IDE. + +### `provar_org_describe` + +Read cached Salesforce describe data for one connection from the Provar workspace `.metadata` cache. Returns the object list, required-field schema, and a cache age. Use this before calling `provar_testcase_generate` so the generator can produce steps with correctly-typed field values. + +**Prerequisite:** the project must have been opened in Provar IDE at least once with the named connection loaded. If the cache is missing, the tool returns a structured response with `details.suggestion` rather than an error. + +**Workspace discovery heuristic** — the tool walks candidate directories in this order and uses the first one that exists: + +1. `/workspace-/` — sibling workspace pattern (default for Provar IDE in this workspace layout). +2. `/Provar_Workspaces/workspace-/` — shared `Provar_Workspaces` directory. +3. `~/Provar/workspace-/` — user-home fallback. + +`` is the project's basename with whitespace collapsed to single dashes and lowercased: `"My Project"` → `"my-project"`. + +| Input | Type | Required | Default | Description | +| ----------------- | ----------------------- | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `project_path` | string | yes | — | Absolute path to the Provar test project root (the directory containing `.testproject`). Must be within `--allowed-paths`. | +| `connection_name` | string | yes | — | Connection name as defined in `.testproject` (e.g. `"MyOrg"`). Must match the `.metadata` subdirectory exactly. Path separators in this value are rejected (`PATH_TRAVERSAL`). | +| `objects` | string[] | no | all | Filter — only return data for these object API names. When omitted, lists every object cached under the connection directory. | +| `field_filter` | `'required'` \| `'all'` | no | `'required'` | Which fields to return. `'required'` includes only fields with `nillable=false`; `'all'` returns every cached field. | + +| Output field | Description | +| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | +| `workspace_path` | Absolute resolved path to the discovered workspace, or `null` when none of the three candidate directories exists. | +| `cache_age_ms` | `mtime` delta in milliseconds of the connection cache directory, or `null` when the cache is missing. | +| `objects[]` | Array of `{ name, exists, required_fields, field_count }`. `exists` is `true` (cached), `false` (requested but not cached), or `null` (cache miss). | +| `details.suggestion` | Present **only** on cache miss. Tells the agent how to populate the cache (open Provar IDE) or how to proceed without it (inline hints). | + +**Example — happy path:** + +```jsonc +// Request +{ + "project_path": "/Users/me/git/MyProject", + "connection_name": "MyOrg", + "objects": ["Account", "Contact"], + "field_filter": "required" +} + +// Response +{ + "workspace_path": "/Users/me/git/workspace-MyProject", + "cache_age_ms": 1839200, + "objects": [ + { + "name": "Account", + "exists": true, + "required_fields": [ + { "name": "Name", "type": "string", "default_value": null, "nillable": false } + ], + "field_count": 24 + }, + { + "name": "Contact", + "exists": true, + "required_fields": [ + { "name": "LastName", "type": "string", "default_value": null, "nillable": false } + ], + "field_count": 31 + } + ] +} +``` + +**Example — cache miss:** + +```jsonc +// Response when the .metadata/ directory does not exist +{ + "workspace_path": "/Users/me/git/workspace-MyProject", + "cache_age_ms": null, + "objects": [{ "name": "Account", "exists": null, "required_fields": [], "field_count": 0 }], + "details": { + "suggestion": "Open this project in Provar IDE and load the 'MyOrg' connection, or pass field-type hints inline to provar_testcase_generate." + } +} +``` + +**On-disk cache schema (one file per object).** The tool reads `.json` first, then `.xml`, then `.object` as a fallback: + +```jsonc +// /.metadata//Account.json +{ + "name": "Account", + "fields": [ + { "name": "Name", "type": "string", "defaultValue": null, "nillable": false }, + { "name": "Phone", "type": "phone", "defaultValue": null, "nillable": true } + ] +} +``` + +**Error codes:** + +| Code | Meaning | +| ------------------ | ---------------------------------------------------------------------------------------------- | +| `PATH_NOT_ALLOWED` | `project_path` or the resolved workspace path is outside `--allowed-paths`. | +| `PATH_TRAVERSAL` | `project_path` contains `..` segments, or `connection_name` contains a path separator or `..`. | + +--- + ## NitroX — Hybrid Model page objects NitroX is Provar's **Hybrid Model** for locators. Instead of hand-written Java Page Objects it uses component-based `.po.json` files that map UI elements for any Salesforce component type: LWC, Screen Flow, Industry / OmniStudio, Experience Cloud, and standard HTML5. These files live in `nitroX/` directories inside your Provar project. diff --git a/scripts/mcp-smoke.cjs b/scripts/mcp-smoke.cjs index 6cea444b..ae54a7f4 100644 --- a/scripts/mcp-smoke.cjs +++ b/scripts/mcp-smoke.cjs @@ -461,6 +461,36 @@ async function runTests() { test_item_id: '1', }); + // ── 54. provar_org_describe — cache miss ───────────────────────────────── + // TMP has no workspace at all → cache-miss response with details.suggestion + if (inGroup('inspect')) + await callTool('provar_org_describe', { + project_path: TMP, + connection_name: 'SmokeOrg', + objects: ['Account'], + }); + + // ── 55. provar_org_describe — happy path ───────────────────────────────── + // Set up a sibling workspace + .metadata/ with one fake object. + if (inGroup('inspect')) { + const fs = require('fs'); + const orgProject = path.join(TMP, 'org-describe-smoke-project'); + fs.mkdirSync(orgProject, { recursive: true }); + const cxnDir = path.join(TMP, 'workspace-org-describe-smoke-project', '.metadata', 'SmokeOrg'); + fs.mkdirSync(cxnDir, { recursive: true }); + fs.writeFileSync( + path.join(cxnDir, 'Account.json'), + JSON.stringify({ + name: 'Account', + fields: [{ name: 'Name', type: 'string', defaultValue: null, nillable: false }], + }) + ); + await callTool('provar_org_describe', { + project_path: orgProject, + connection_name: 'SmokeOrg', + }); + } + server.stdin.end(); } diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 20429dba..1f9de722 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -34,6 +34,7 @@ import { registerAllTestPlanTools } from './tools/testPlanTools.js'; import { registerAllNitroXTools } from './tools/nitroXTools.js'; import { registerAllTestCaseStepTools } from './tools/testCaseStepTools.js'; import { registerAllConnectionTools } from './tools/connectionTools.js'; +import { registerAllOrgDescribeTools } from './tools/orgDescribeTools.js'; import { registerAllPrompts } from './prompts/index.js'; import { createDepthGuardState, @@ -64,7 +65,7 @@ const TOOL_GROUPS: Record/.metadata//.json + * + * Each JSON file contains: { name: "Account", fields: [ { name, type, defaultValue, nillable }, ... ] } + * + * As a fallback, .xml / .object files (CustomObject metadata) are also accepted + * to ease migration from the legacy Provar IDE Eclipse cache layout. + */ +interface CachedField { + name: string; + type?: string; + defaultValue?: string | null; + nillable?: boolean; +} + +interface CachedObject { + name?: string; + fields?: CachedField[]; +} + +// ── Workspace discovery ─────────────────────────────────────────────────────── + +/** + * Normalise a project basename for use in fallback workspace dir names: + * "My Project Path " → "my-project-path". + */ +export function projectNameDashes(projectPath: string): string { + return path.basename(projectPath).trim().replace(/\s+/g, '-').toLowerCase(); +} + +/** + * Build the ordered list of candidate workspace directories. + * First existing wins. + * 1. /workspace-/ — sibling workspace pattern. + * 2. /Provar_Workspaces/workspace-/ + * 3. ~/Provar/workspace-/ — user-home fallback. + */ +export function workspaceCandidates(projectPath: string): string[] { + const resolved = path.resolve(projectPath); + const parent = path.dirname(resolved); + const basename = path.basename(resolved); + const dashes = projectNameDashes(resolved); + return [ + path.join(parent, `workspace-${basename}`), + path.join(parent, 'Provar_Workspaces', `workspace-${dashes}`), + path.join(os.homedir(), 'Provar', `workspace-${dashes}`), + ]; +} + +/** Returns the first candidate workspace that exists, or null. */ +export function discoverWorkspace(projectPath: string): string | null { + for (const candidate of workspaceCandidates(projectPath)) { + try { + if (fs.existsSync(candidate) && fs.statSync(candidate).isDirectory()) { + return candidate; + } + } catch { + // Permission errors etc. — skip and try next candidate + } + } + return null; +} + +// ── Cache reading ───────────────────────────────────────────────────────────── + +const XML_PARSER = new XMLParser({ + ignoreAttributes: false, + attributeNamePrefix: '@_', + parseAttributeValue: false, + isArray: (name): boolean => name === 'fields', +}); + +/** Parse a JSON cache file into the canonical CachedObject shape. */ +function readJsonCacheFile(filePath: string): CachedObject { + const raw = fs.readFileSync(filePath, 'utf-8'); + return JSON.parse(raw) as CachedObject; +} + +/** + * Parse a legacy .object XML file (CustomObject metadata) into the canonical shape. + * Only extracts the bare minimum the tool needs: field name, type, nillable. + */ +function readXmlCacheFile(filePath: string): CachedObject { + const raw = fs.readFileSync(filePath, 'utf-8'); + const parsed = XML_PARSER.parse(raw) as Record; + const root = (parsed['CustomObject'] ?? parsed['toolingObjectInfo'] ?? {}) as Record; + const fieldsRaw = root['fields']; + if (!Array.isArray(fieldsRaw)) return { name: path.basename(filePath, path.extname(filePath)), fields: [] }; + + const fields: CachedField[] = []; + for (const f of fieldsRaw as Array>) { + const name = (f['fullName'] ?? f['name']) as string | undefined; + if (!name) continue; + fields.push({ + name, + type: (f['type'] as string | undefined) ?? 'unknown', + defaultValue: (f['defaultValue'] as string | undefined) ?? null, + // XML defaults: required = !nillable. In the .object format, "required" is rare, + // so we default to nillable=true (optional) unless explicitly required. + nillable: f['required'] === 'true' ? false : true, + }); + } + return { name: path.basename(filePath, path.extname(filePath)), fields }; +} + +/** Look up the cache file for one object, trying .json then .xml. */ +function findObjectCacheFile(connectionDir: string, objectName: string): string | null { + const jsonPath = path.join(connectionDir, `${objectName}.json`); + if (fs.existsSync(jsonPath)) return jsonPath; + const xmlPath = path.join(connectionDir, `${objectName}.xml`); + if (fs.existsSync(xmlPath)) return xmlPath; + // Legacy CustomObject layout (.object extension) + const objPath = path.join(connectionDir, `${objectName}.object`); + if (fs.existsSync(objPath)) return objPath; + return null; +} + +/** List all cached object names (stripped of extension) in a connection directory. */ +function listCachedObjectNames(connectionDir: string): string[] { + let entries: string[]; + try { + entries = fs.readdirSync(connectionDir); + } catch { + return []; + } + const names = new Set(); + for (const entry of entries) { + const ext = path.extname(entry); + if (ext === '.json' || ext === '.xml' || ext === '.object') { + names.add(path.basename(entry, ext)); + } + } + return [...names].sort(); +} + +/** Read one object's cache file and convert it to an OrgDescribeObject. */ +function readObject(connectionDir: string, objectName: string, fieldFilter: 'required' | 'all'): OrgDescribeObject { + const file = findObjectCacheFile(connectionDir, objectName); + if (!file) { + return { name: objectName, exists: false, required_fields: [], field_count: 0 }; + } + let cached: CachedObject; + try { + cached = path.extname(file) === '.json' ? readJsonCacheFile(file) : readXmlCacheFile(file); + } catch (e) { + log('warn', 'org_describe: failed to parse cache file', { file, error: (e as Error).message }); + return { name: objectName, exists: false, required_fields: [], field_count: 0 }; + } + + const allFields = cached.fields ?? []; + const filtered = fieldFilter === 'required' ? allFields.filter((f) => f.nillable === false) : allFields; + + const fields: OrgDescribeField[] = filtered.map((f) => ({ + name: f.name, + type: f.type ?? 'unknown', + default_value: f.defaultValue ?? null, + nillable: f.nillable ?? true, + })); + + return { + name: cached.name ?? objectName, + exists: true, + required_fields: fields, + field_count: allFields.length, + }; +} + +/** Compute the mtime delta (ms) of a directory. Returns null on error. */ +function cacheAgeMs(dir: string): number | null { + try { + const stat = fs.statSync(dir); + return Math.max(0, Date.now() - stat.mtimeMs); + } catch { + return null; + } +} + +// ── Suggestion strings ──────────────────────────────────────────────────────── + +function cacheMissSuggestion(connectionName: string): string { + return ( + `Open this project in Provar IDE and load the '${connectionName}' connection, ` + + 'or pass field-type hints inline to provar_testcase_generate.' + ); +} + +// ── Core logic ──────────────────────────────────────────────────────────────── + +interface DescribeArgs { + project_path: string; + connection_name: string; + objects?: string[]; + field_filter?: 'required' | 'all'; +} + +/** + * Resolve & policy-check the workspace + connection directory. + * Returns the connection directory if it exists and is allowed, otherwise null. + */ +function resolveConnectionDir( + workspacePath: string | null, + connectionName: string, + allowedPaths: string[] +): { connectionDir: string | null; resolvedWorkspace: string | null } { + if (!workspacePath) return { connectionDir: null, resolvedWorkspace: null }; + + // Reject path-shaped connection names outright. A real connection name from a + // .testproject is an identifier (e.g. "MyOrg"); any separator or traversal + // segment is almost certainly a misuse or injection attempt. + if (connectionName.includes('/') || connectionName.includes('\\') || connectionName.split(/[/\\]+/).includes('..')) { + throw new PathPolicyError( + 'PATH_TRAVERSAL', + `Invalid connection_name (contains path separators): ${connectionName}` + ); + } + + // Path policy: workspace MUST be inside allowed paths before any fs call against it. + const resolvedWorkspace = path.resolve(workspacePath); + assertPathAllowed(resolvedWorkspace, allowedPaths); + + const connectionDir = path.resolve(resolvedWorkspace, '.metadata', connectionName); + // Belt-and-braces check after composition. + assertPathAllowed(connectionDir, allowedPaths); + + if (!fs.existsSync(connectionDir) || !fs.statSync(connectionDir).isDirectory()) { + return { connectionDir: null, resolvedWorkspace }; + } + return { connectionDir, resolvedWorkspace }; +} + +function buildCacheMissResponse( + resolvedWorkspace: string | null, + args: DescribeArgs, + requestId: string +): Record { + const objects: OrgDescribeObject[] = (args.objects ?? []).map((name) => ({ + name, + exists: null, + required_fields: [], + field_count: 0, + })); + return { + requestId, + workspace_path: resolvedWorkspace, + cache_age_ms: null, + objects, + details: { suggestion: cacheMissSuggestion(args.connection_name) }, + }; +} + +function buildHappyResponse( + resolvedWorkspace: string, + connectionDir: string, + args: DescribeArgs, + requestId: string +): Record { + const fieldFilter = args.field_filter ?? 'required'; + const requestedNames = args.objects?.length ? args.objects : listCachedObjectNames(connectionDir); + const objects = requestedNames.map((name) => readObject(connectionDir, name, fieldFilter)); + return { + requestId, + workspace_path: resolvedWorkspace, + cache_age_ms: cacheAgeMs(connectionDir), + objects, + }; +} + +// ── Tool registration ───────────────────────────────────────────────────────── + +export function registerOrgDescribe(server: McpServer, config: ServerConfig): void { + server.registerTool( + 'provar_org_describe', + { + title: 'Describe Org Objects From Workspace Cache', + description: desc( + [ + 'Read cached Salesforce describe data for one connection from the Provar workspace .metadata cache.', + 'Prerequisite: the project must have been opened in Provar IDE at least once with the named connection loaded', + '— this tool is read-only and does NOT trigger a metadata download.', + 'Workspace discovery tries in order: /workspace-, ', + '/Provar_Workspaces/workspace-, then ~/Provar/workspace-.', + 'Returns an empty result with details.suggestion when the cache is missing.', + 'Distinct from the runtime .provarCaches cache used by test execution.', + ].join(' '), + 'Read cached describe data for one connection from the Provar workspace .metadata cache.' + ), + inputSchema: { + project_path: z + .string() + .describe( + desc( + 'Absolute path to the Provar test project root (the directory containing .testproject). Must be within --allowed-paths.', + 'string, absolute path to Provar test project root' + ) + ), + connection_name: z + .string() + .describe( + desc( + 'Connection name as defined in the .testproject file (e.g. "MyOrg"). The .metadata cache subdirectory must match this exactly.', + 'string, connection name as defined in .testproject' + ) + ), + objects: z + .array(z.string()) + .optional() + .describe( + desc( + 'Optional filter — only return data for these object API names (e.g. ["Account","Contact"]). When omitted, lists all cached objects for the connection.', + 'string[], optional; restrict to these object API names' + ) + ), + field_filter: z + .enum(['required', 'all']) + .optional() + .default('required') + .describe( + desc( + 'Which fields to include. "required" (default) returns only fields with nillable=false; "all" returns every cached field.', + "'required' | 'all'; default 'required'" + ) + ), + }, + }, + (args: DescribeArgs) => { + const requestId = makeRequestId(); + log('info', 'provar_org_describe', { + requestId, + connection_name: args.connection_name, + object_count: args.objects?.length ?? null, + }); + + try { + assertPathAllowed(args.project_path, config.allowedPaths); + const workspacePath = discoverWorkspace(args.project_path); + const { connectionDir, resolvedWorkspace } = resolveConnectionDir( + workspacePath, + args.connection_name, + config.allowedPaths + ); + + if (!connectionDir) { + const response = buildCacheMissResponse(resolvedWorkspace, args, requestId); + return { + content: [{ type: 'text' as const, text: JSON.stringify(response) }], + structuredContent: response, + }; + } + + const response = buildHappyResponse(resolvedWorkspace!, connectionDir, args, requestId); + return { + content: [{ type: 'text' as const, text: JSON.stringify(response) }], + structuredContent: response, + }; + } catch (err: unknown) { + const error = err as Error & { code?: string }; + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify( + makeError( + error instanceof PathPolicyError ? error.code : error.code ?? 'ORG_DESCRIBE_ERROR', + error.message, + requestId + ) + ), + }, + ], + }; + } + } + ); +} + +export function registerAllOrgDescribeTools(server: McpServer, config: ServerConfig): void { + registerOrgDescribe(server, config); +} diff --git a/test/unit/mcp/orgDescribeTools.test.ts b/test/unit/mcp/orgDescribeTools.test.ts new file mode 100644 index 00000000..a8820f27 --- /dev/null +++ b/test/unit/mcp/orgDescribeTools.test.ts @@ -0,0 +1,405 @@ +/* + * 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 { + registerOrgDescribe, + discoverWorkspace, + projectNameDashes, + workspaceCandidates, +} from '../../../src/mcp/tools/orgDescribeTools.js'; +import type { ServerConfig } from '../../../src/mcp/server.js'; + +// ── Minimal McpServer mock ───────────────────────────────────────────────────── + +type ToolHandler = (args: Record) => unknown; + +class MockMcpServer { + private handlers = new Map(); + + public registerTool(name: string, _config: unknown, handler: ToolHandler): void { + this.handlers.set(name, handler); + } + + public call(name: string, args: Record): ReturnType { + const h = this.handlers.get(name); + if (!h) throw new Error(`Tool not registered: ${name}`); + return h(args); + } +} + +// ── Helpers ──────────────────────────────────────────────────────────────────── + +function parseText(result: unknown): Record { + const r = result as { content: Array<{ type: string; text: string }> }; + return JSON.parse(r.content[0].text) as Record; +} + +function isError(result: unknown): boolean { + return (result as { isError?: boolean }).isError === true; +} + +interface CachedField { + name: string; + type: string; + defaultValue: string | null; + nillable: boolean; +} + +/** Write a JSON cache file for one object. */ +function writeJsonCache(connectionDir: string, objectName: string, fields: CachedField[]): void { + fs.mkdirSync(connectionDir, { recursive: true }); + fs.writeFileSync( + path.join(connectionDir, `${objectName}.json`), + JSON.stringify({ name: objectName, fields }), + 'utf-8' + ); +} + +// ── Test setup ───────────────────────────────────────────────────────────────── + +let tmpRoot: string; +let projectPath: string; +let server: MockMcpServer; +let config: ServerConfig; + +beforeEach(() => { + // Use realpathSync to canonicalise the path on macOS (/var → /private/var) so + // assertPathAllowed comparisons match the realpath the policy resolves to. + tmpRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'org-describe-test-'))); + projectPath = path.join(tmpRoot, 'MyProject'); + fs.mkdirSync(projectPath, { recursive: true }); + + server = new MockMcpServer(); + // tmpRoot must be allowed so both the project path and any sibling workspace + // candidate (also under tmpRoot) pass the path policy check. + config = { allowedPaths: [tmpRoot] }; + registerOrgDescribe(server as never, config); +}); + +afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +// ── projectNameDashes / workspaceCandidates ─────────────────────────────────── + +describe('projectNameDashes', () => { + it('lowercases and replaces whitespace with single dashes', () => { + assert.equal(projectNameDashes('/x/My Project Path'), 'my-project-path'); + assert.equal(projectNameDashes('/x/ Spaced Name '), 'spaced-name'); + }); +}); + +describe('workspaceCandidates', () => { + it('returns three candidates in expected order', () => { + const cands = workspaceCandidates('/Users/alice/projects/My Project'); + assert.equal(cands.length, 3); + assert.ok( + cands[0].endsWith(`${path.sep}workspace-My Project`), + `Expected sibling workspace first, got: ${cands[0]}` + ); + assert.ok( + cands[1].includes('Provar_Workspaces') && cands[1].endsWith('workspace-my-project'), + `Expected Provar_Workspaces second, got: ${cands[1]}` + ); + assert.ok(cands[2].endsWith(`${path.sep}Provar${path.sep}workspace-my-project`)); + }); +}); + +// ── (a) Workspace discovery — sibling pattern ───────────────────────────────── + +describe('provar_org_describe — workspace discovery', () => { + it('(a) finds the sibling workspace at /workspace-', () => { + // /workspace-MyProject is the sibling pattern + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [{ name: 'Name', type: 'string', defaultValue: null, nillable: false }]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['workspace_path'], siblingWorkspace, 'should discover sibling workspace'); + const objects = body['objects'] as Array<{ name: string; exists: boolean | null; field_count: number }>; + assert.equal(objects.length, 1); + assert.equal(objects[0].name, 'Account'); + assert.equal(objects[0].exists, true); + assert.equal(objects[0].field_count, 1); + }); + + it('(b) falls back to user-home workspace when sibling missing (via override)', () => { + // Stand in for ~/Provar by using a HOME override. The tool uses os.homedir(), + // and we override $HOME for this test only. Set the home to a tmp dir so the + // path is inside allowed paths. + const fakeHome = path.join(tmpRoot, 'fakehome'); + fs.mkdirSync(fakeHome, { recursive: true }); + + const homeWorkspace = path.join(fakeHome, 'Provar', 'workspace-myproject'); + const connectionDir = path.join(homeWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Contact', [ + { name: 'LastName', type: 'string', defaultValue: null, nillable: false }, + ]); + + // Override HOME and USERPROFILE so os.homedir() returns fakeHome + const oldHome = process.env['HOME']; + const oldUserProfile = process.env['USERPROFILE']; + process.env['HOME'] = fakeHome; + process.env['USERPROFILE'] = fakeHome; + try { + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + }); + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['workspace_path'], homeWorkspace, 'should discover user-home workspace'); + const objects = body['objects'] as Array<{ name: string; exists: boolean | null }>; + assert.ok( + objects.some((o) => o.name === 'Contact' && o.exists === true), + 'should list Contact from home workspace cache' + ); + } finally { + if (oldHome === undefined) delete process.env['HOME']; + else process.env['HOME'] = oldHome; + if (oldUserProfile === undefined) delete process.env['USERPROFILE']; + else process.env['USERPROFILE'] = oldUserProfile; + } + }); + + it('discoverWorkspace returns null when no candidate exists', () => { + assert.equal(discoverWorkspace(projectPath), null); + }); +}); + +// ── (c) Cache miss ──────────────────────────────────────────────────────────── + +describe('provar_org_describe — cache miss', () => { + it('(c) returns suggestion when workspace exists but .metadata/ absent', () => { + // Create the sibling workspace dir but NOT the connection subdir + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + fs.mkdirSync(path.join(siblingWorkspace, '.metadata'), { recursive: true }); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MissingOrg', + objects: ['Account'], + }); + + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['workspace_path'], siblingWorkspace); + assert.equal(body['cache_age_ms'], null); + + const details = body['details'] as { suggestion: string }; + assert.ok(details, 'details should be present on cache miss'); + assert.ok( + details.suggestion.includes('Provar IDE') && details.suggestion.includes('MissingOrg'), + `suggestion should mention IDE and connection name; got: ${details.suggestion}` + ); + + const objects = body['objects'] as Array<{ name: string; exists: boolean | null; required_fields: unknown[] }>; + assert.equal(objects.length, 1); + assert.equal(objects[0].name, 'Account'); + assert.equal(objects[0].exists, null, 'exists must be null when cache missing entirely'); + }); + + it('returns suggestion when no workspace at all is discoverable', () => { + // No HOME override + no sibling workspace ⇒ workspace_path null. But os.homedir() + // will still produce a path; set HOME to a non-existent dir so the candidate doesn't exist. + const fakeHome = path.join(tmpRoot, 'nope'); + const oldHome = process.env['HOME']; + const oldUserProfile = process.env['USERPROFILE']; + process.env['HOME'] = fakeHome; + process.env['USERPROFILE'] = fakeHome; + try { + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'AnyOrg', + objects: ['Account', 'Contact'], + }); + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['workspace_path'], null); + assert.ok(body['details'], 'suggestion should be present'); + } finally { + if (oldHome === undefined) delete process.env['HOME']; + else process.env['HOME'] = oldHome; + if (oldUserProfile === undefined) delete process.env['USERPROFILE']; + else process.env['USERPROFILE'] = oldUserProfile; + } + }); +}); + +// ── (d) Path policy ─────────────────────────────────────────────────────────── + +describe('provar_org_describe — path policy', () => { + it('(d) rejects project_path outside allowed paths with PATH_NOT_ALLOWED', () => { + const strictServer = new MockMcpServer(); + registerOrgDescribe(strictServer as never, { allowedPaths: [tmpRoot] }); + + const result = strictServer.call('provar_org_describe', { + project_path: path.join(os.tmpdir(), 'definitely-outside'), + connection_name: 'MyOrg', + }); + + assert.equal(isError(result), true); + const code = parseText(result)['error_code'] as string; + assert.ok(code === 'PATH_NOT_ALLOWED' || code === 'PATH_TRAVERSAL', `Unexpected error_code: ${code}`); + }); + + it('rejects connection_name that would escape workspace dir via traversal', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + fs.mkdirSync(path.join(siblingWorkspace, '.metadata'), { recursive: true }); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: '../../escape', + }); + + assert.equal(isError(result), true); + const code = parseText(result)['error_code'] as string; + assert.ok(code === 'PATH_TRAVERSAL' || code === 'PATH_NOT_ALLOWED', `Unexpected error_code: ${code}`); + }); +}); + +// ── (e) Happy path ──────────────────────────────────────────────────────────── + +describe('provar_org_describe — happy path', () => { + it('(e) returns the expected shape for two cached objects', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [ + { name: 'Name', type: 'string', defaultValue: null, nillable: false }, + { name: 'AccountNumber', type: 'string', defaultValue: null, nillable: true }, + ]); + writeJsonCache(connectionDir, 'Contact', [ + { name: 'LastName', type: 'string', defaultValue: null, nillable: false }, + { name: 'Email', type: 'email', defaultValue: null, nillable: true }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['workspace_path'], siblingWorkspace); + assert.ok(typeof body['cache_age_ms'] === 'number' && body['cache_age_ms'] >= 0); + + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + required_fields: Array<{ name: string; nillable: boolean }>; + field_count: number; + }>; + assert.equal(objects.length, 2); + + const account = objects.find((o) => o.name === 'Account'); + assert.ok(account); + assert.equal(account.exists, true); + assert.equal(account.field_count, 2, 'field_count reports total cached fields, not filtered'); + // default field_filter is "required" → only nillable=false fields included + assert.equal(account.required_fields.length, 1); + assert.equal(account.required_fields[0].name, 'Name'); + }); +}); + +// ── (f) field_filter ────────────────────────────────────────────────────────── + +describe('provar_org_describe — field_filter', () => { + it('(f) field_filter=required excludes nillable fields', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [ + { name: 'Name', type: 'string', defaultValue: null, nillable: false }, + { name: 'Phone', type: 'phone', defaultValue: null, nillable: true }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + field_filter: 'required', + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ required_fields: Array<{ name: string }> }>; + const fields = objects[0].required_fields.map((f) => f.name); + assert.deepEqual(fields, ['Name'], 'only nillable=false fields should appear'); + }); + + it('(f) field_filter=all includes nillable fields', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [ + { name: 'Name', type: 'string', defaultValue: null, nillable: false }, + { name: 'Phone', type: 'phone', defaultValue: null, nillable: true }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + field_filter: 'all', + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ required_fields: Array<{ name: string }> }>; + const names = objects[0].required_fields.map((f) => f.name).sort(); + assert.deepEqual(names, ['Name', 'Phone']); + }); +}); + +// ── (g) objects filter ──────────────────────────────────────────────────────── + +describe('provar_org_describe — objects filter', () => { + it('(g) restricts response to requested object names only', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [{ name: 'Name', type: 'string', defaultValue: null, nillable: false }]); + writeJsonCache(connectionDir, 'Contact', [ + { name: 'LastName', type: 'string', defaultValue: null, nillable: false }, + ]); + writeJsonCache(connectionDir, 'Lead', [{ name: 'Company', type: 'string', defaultValue: null, nillable: false }]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + objects: ['Account', 'Lead'], + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ name: string }>; + const names = objects.map((o) => o.name).sort(); + assert.deepEqual(names, ['Account', 'Lead'], 'Contact should be excluded'); + }); + + it('reports exists=false for a requested object not present in cache', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeJsonCache(connectionDir, 'Account', [{ name: 'Name', type: 'string', defaultValue: null, nillable: false }]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + objects: ['Account', 'Ghost'], + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ name: string; exists: boolean | null }>; + const ghost = objects.find((o) => o.name === 'Ghost'); + assert.ok(ghost); + assert.equal(ghost.exists, false, 'object not in cache → exists=false'); + }); +}); From a8c05fc6ee1ac579696a832e8c1c7a383540107c Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:10:49 -0500 Subject: [PATCH 09/17] =?UTF-8?q?PDX-486:=20docs(mcp)=20=E2=80=94=20clarif?= =?UTF-8?q?y=20that=20WARNING=20[code]=20shape=20applies=20only=20to=20for?= =?UTF-8?q?matWarning()=20messages?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: The "Warning codes" section in docs/mcp.md previously stated that "Warnings emitted programmatically follow the shape WARNING []: ". That claim is too broad: provar_properties_validate emits placeholder warnings (pre-existing, non-coded) whose message is a plain string with no WARNING [...] prefix. A reader looking at that line would reasonably expect every warning surface in the codebase to carry a code, then be surprised by the properties validator output and treat it as a regression. Fix: Reword the sentence to scope the structured shape to warnings built via formatWarning() — those have the WARNING []: prefix and the optional Did you mean ''? suffix. Free-form warnings without a structured code (called out explicitly via the properties validator example) remain plain strings. Keeps the reference to src/mcp/utils/warningCodes.ts for the canonical enum. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/mcp.md b/docs/mcp.md index 100d548d..01ba1430 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -545,7 +545,7 @@ Cross-cutting warning codes surfaced by validation, configuration, and run tooli | `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. +Warning-code messages emitted via `formatWarning()` follow the shape `WARNING []: ` (optionally suffixed with ` Did you mean ''?` when a typo is detected). Other free-form warnings without a structured code — such as the placeholder warnings emitted by `provar_properties_validate` — remain plain strings. See `src/mcp/utils/warningCodes.ts` for the canonical enum. --- From 669b5170dac183d7c0c958500bcb6ba644df1ddf Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:16:07 -0500 Subject: [PATCH 10/17] =?UTF-8?q?PDX-490:=20docs(mcp)=20=E2=80=94=20clarif?= =?UTF-8?q?y=20error=5Fcategory=20scope=20(rca=20mode=20only)=20+=20fix=20?= =?UTF-8?q?root=5Fcause=5Fcategory=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review on PR #186 flagged two doc/description inconsistencies — (1) the provar_testrun_rca tool description implied error_category/retryable appear in mode="failures" output, but mode="failures" only emits { testItemId, title, errorMessage } and the new fields live on FailureReport entries in mode="rca" only; (2) docs/mcp.md FailureReport table said root_cause_category is "One of 12 categories" while the enumerated list contains 17 buckets (and the new paragraph added in this PR explicitly says "17 buckets"). These are doc/description mismatches, not runtime behaviour bugs. Fix: Reword the rcaTools.ts tool description so error_category/retryable are explicitly scoped to failures[] entries in mode="rca", with a "NOT included in mode=failures output" clarifier so agents reading the description cold pick the right mode. Update docs/mcp.md root_cause_category row from "One of 12 categories (see table below)" to "One of 17 categories (see list below)" — count now matches the enumerated list (DRIVER_VERSION_MISMATCH, LOCATOR_STALE, TIMEOUT, ASSERTION_FAILED, CREDENTIAL_FAILURE, MISSING_CALLABLE, METADATA_CACHE, PAGE_OBJECT_COMPILE, CONNECTION_REFUSED, DATA_SETUP, LICENSE_INVALID, SALESFORCE_VALIDATION, SALESFORCE_PICKLIST, SALESFORCE_REFERENCE, SALESFORCE_ACCESS, SALESFORCE_TRIGGER, UNKNOWN = 17). Also corrected "see table below" → "see list below" since the categories are an inline comma-separated list, not a table. Verified with yarn compile (clean), yarn lint (clean), and full mocha suite (1169 passing). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 2 +- src/mcp/tools/rcaTools.ts | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 1a17f1cf..90414295 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1545,7 +1545,7 @@ Use `mode="failures"` when you only need the list of failing test case names wit | `test_case` | Test case filename from JUnit `` | | `error_class` | Extracted exception class name | | `error_message` | First 500 chars of failure/error text | -| `root_cause_category` | One of 12 categories (see table below) | +| `root_cause_category` | One of 17 categories (see list below) | | `root_cause_summary` | Human-readable cause description | | `recommendation` | Suggested fix action | | `page_object` | Extracted from `Page Object: ...` pattern, or `null` | diff --git a/src/mcp/tools/rcaTools.ts b/src/mcp/tools/rcaTools.ts index 7b310864..e07a9808 100644 --- a/src/mcp/tools/rcaTools.ts +++ b/src/mcp/tools/rcaTools.ts @@ -748,9 +748,10 @@ export function registerTestRunRca(server: McpServer, config: ServerConfig): voi 'Use mode="failures" to get a lightweight array of failed test cases', '([{ testItemId, title, errorMessage }]) without the full RCA classification — useful when you', 'need failure names quickly without loading the HTML report.', - 'Each failure includes optional error_category (INFRASTRUCTURE|ASSERTION|LOCATOR|TIMEOUT|OTHER)', - 'and retryable (boolean) fields when the failure text matches a known pattern — INFRASTRUCTURE/TIMEOUT', - 'are flagged retryable, others are not.', + 'In mode="rca" (default), each entry in failures[] additionally includes optional error_category', + '(INFRASTRUCTURE|ASSERTION|LOCATOR|TIMEOUT|OTHER) and retryable (boolean) fields when the failure', + 'text matches a known pattern — INFRASTRUCTURE/TIMEOUT are flagged retryable, others are not.', + 'These fields are NOT included in mode="failures" output.', ].join(' '), 'Parse a Provar test run JUnit.xml and produce an RCA report with failure classification.' ), From 8def6af8fe98d29c02816bb218f5c02df1ee095a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:19:53 -0500 Subject: [PATCH 11/17] =?UTF-8?q?PDX-493:=20fix(mcp)=20=E2=80=94=20align?= =?UTF-8?q?=20numeric=20valueClass=20with=20canonical=20reference=20(integ?= =?UTF-8?q?er=20=E2=86=92=20decimal)=20+=20anchor=20datetime=20regex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: The H3 plan AC specified `valueClass="integer"` for integer-only strings, but `docs/PROVAR_TEST_STEP_REFERENCE.md` lines 1338 and 1428 are explicit: numbers in Provar test steps use `valueClass="decimal"` (covering both `42` and `3.14`). No `integer` valueClass exists in the canonical reference grammar, so the initial H3 implementation would have emitted XML that diverges from the Provar runtime contract for every integer field — silently produced bad XML for any numeric Salesforce field. Separately, the datetime detection regex `/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/` was not end-anchored, so trailing garbage after seconds (e.g. `2026-05-19T10:30:00not-a-zone`) was accepted as a valid datetime and emitted as `valueClass="datetime"`. The canonical reference permits optional fractional seconds and timezone — the regex must enforce that shape explicitly. Fix: In `inferSalesforceValueClass` (src/mcp/tools/testCaseGenerate.ts): - Replace the `'integer'` arm of the return-type union with `'decimal'`. Both the parameter and return type now use the canonical-reference set: `'date' | 'datetime' | 'boolean' | 'decimal' | 'string'`. - Broaden the numeric regex from `/^-?\d+$/` to `/^-?\d+(\.\d+)?$/` so it matches integers and decimals; return `'decimal'` for both. - Anchor the datetime regex end with explicit optional fractional-seconds and timezone groups: `/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(Z|[+-]\d{2}:?\d{2})?$/`. - Update doc-comment to spell out the canonical reference (lines 1338/1428). Update tool description text (line ~155) to say "numeric string → decimal" and explicitly call out that there is no separate `integer` valueClass. Update `docs/mcp.md` argument-conventions table row to match (one merged numeric row covering `42`, `-5`, `3.14`). Tests: - Rewrite `integer` assertions in test/unit/mcp/testCaseGenerate.test.ts to `decimal` (positive int, negative int, short numeric string). - Add positive coverage for decimals (`3.14`, `-12.5`) at both helper and XML-emission levels. - Add datetime end-anchor coverage (numeric timezone offset accepted; trailing garbage rejected as `string`). Addresses Copilot review feedback on PR #183. --- docs/mcp.md | 26 +++++----- src/mcp/tools/testCaseGenerate.ts | 26 ++++++---- test/unit/mcp/testCaseGenerate.test.ts | 68 +++++++++++++++++++++----- 3 files changed, 84 insertions(+), 36 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 4068f543..1be19505 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -729,19 +729,19 @@ The tool's chip-level `title` — `Generate Test Case (full steps in one call)` **Argument XML conventions** (automatically applied by the generator): -| Argument key / value pattern | Emitted XML class | API context | -| --------------------------------------- | ------------------------------------- | ----------------------- | -| `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | -| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert | -| Value matches `{VarName}` or `{A.B}` | `class="variable"` + `` | Any step | -| SetValues attributes | `class="valueList"/` | SetValues only | -| Value `YYYY-MM-DDTHH:MM:SS…` (ISO-8601) | `class="value" valueClass="datetime"` | Any step | -| Value `YYYY-MM-DD` (ISO-8601) | `class="value" valueClass="date"` | Any step | -| Value `true` / `false` | `class="value" valueClass="boolean"` | Any step | -| Value matches `^-?\d+$` | `class="value" valueClass="integer"` | Any step | -| All other values | `class="value" valueClass="string"` | Any step | - -`valueClass` is inferred automatically by `inferSalesforceValueClass(key, val, fieldTypeHint?)`. Detection order: explicit `fieldTypeHint` (wired in a follow-up tool surface — `field_type_hints` param) → ISO-8601 datetime → ISO-8601 date → boolean → integer → string. Provar runtime silently discards date fields emitted as `valueClass="string"`, so always pass date / datetime values in ISO-8601 form. +| Argument key / value pattern | Emitted XML class | API context | +| ---------------------------------------------------------------------------------------------- | ------------------------------------- | ----------------------- | +| `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | +| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert | +| Value matches `{VarName}` or `{A.B}` | `class="variable"` + `` | Any step | +| SetValues attributes | `class="valueList"/` | SetValues only | +| Value `YYYY-MM-DDTHH:MM:SS` + optional `.fff` + optional `Z`/`±HH:MM` (ISO-8601, end-anchored) | `class="value" valueClass="datetime"` | Any step | +| Value `YYYY-MM-DD` (ISO-8601) | `class="value" valueClass="date"` | Any step | +| Value `true` / `false` | `class="value" valueClass="boolean"` | Any step | +| Numeric value `^-?\d+(\.\d+)?$` (`42`, `-5`, `3.14`) | `class="value" valueClass="decimal"` | Any step | +| All other values | `class="value" valueClass="string"` | Any step | + +`valueClass` is inferred automatically by `inferSalesforceValueClass(key, val, fieldTypeHint?)`. Detection order: explicit `fieldTypeHint` (wired in a follow-up tool surface — `field_type_hints` param) → ISO-8601 datetime (with optional fractional seconds and timezone, end-anchored) → ISO-8601 date → boolean → decimal → string. Per the canonical Provar reference numbers always emit as `valueClass="decimal"` (there is no separate `integer` valueClass). Provar runtime silently discards date fields emitted as `valueClass="string"`, so always pass date / datetime values in ISO-8601 form. AssertValues uses **flat** argument structure (`expectedValue`, `actualValue`, `comparisonType`) — not the `valueList`/namedValues format. diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 244533a9..cea87f73 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -151,9 +151,10 @@ const TOOL_DESCRIPTION = [ 'target argument (UiWithScreen/UiWithRow): pass the URI value; emitted as class="uiTarget" uri="...".', 'locator argument (UiDoAction/UiAssert): pass the URI value; emitted as class="uiLocator" uri="...".', 'valueClass auto-detection: argument values are typed automatically before XML emission. ' + - 'ISO-8601 date "YYYY-MM-DD" → valueClass="date"; ISO-8601 datetime "YYYY-MM-DDTHH:MM:SS" → "datetime"; ' + - '"true"/"false" → "boolean"; integer-only string (e.g. "42", "-5") → "integer"; otherwise "string". ' + - 'Pass dates / booleans / integers in those formats — Provar runtime silently discards date fields emitted as valueClass="string".', + 'ISO-8601 date "YYYY-MM-DD" → valueClass="date"; ISO-8601 datetime "YYYY-MM-DDTHH:MM:SS" (optional fractional seconds + timezone) → "datetime"; ' + + '"true"/"false" → "boolean"; numeric string (e.g. "42", "-5", "3.14") → "decimal"; otherwise "string". ' + + 'Pass dates / booleans / numbers in those formats — Provar runtime silently discards date fields emitted as valueClass="string". ' + + 'Note: numbers always emit valueClass="decimal" per the canonical Provar reference (there is no separate "integer" valueClass).', 'Edit page objects: action=Edit targets require a compiled page object for the SF object. ' + 'If none exists in the project page-objects directory, the locator binding will fail at runtime. ' + 'For objects without a compiled Edit page object, use inline edit instead: sfIleActivate to activate the field, ' + @@ -384,10 +385,15 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // Detection order: // 1. Explicit `fieldTypeHint` (from `field_type_hints` param or `provar_org_describe` cache — wired // in PDX-492 H2b) wins if provided. -// 2. ISO-8601 datetime → 'datetime' (e.g. "2026-05-19T10:30:00", with optional fractional/zone). +// 2. ISO-8601 datetime → 'datetime' (e.g. "2026-05-19T10:30:00", with optional fractional seconds +// and optional timezone). The regex is end-anchored so trailing garbage (e.g. +// "2026-05-19T10:30:00not-a-zone") is rejected as plain 'string'. // 3. ISO-8601 date → 'date' (e.g. "2026-05-19"). // 4. Literal 'true' / 'false' → 'boolean'. -// 5. Integer-only string (optional leading '-') → 'integer'. +// 5. Numeric string (integer or decimal, optional leading '-') → 'decimal'. +// Per `docs/PROVAR_TEST_STEP_REFERENCE.md` (lines 1338, 1428) the canonical Provar +// valueClass for numbers is `decimal` — there is no `integer` valueClass in the +// reference grammar, so both `42` and `3.14` emit as `valueClass="decimal"`. // 6. Else → 'string'. // // The `key` argument is reserved for future heuristics (e.g. SF naming conventions like *__c) @@ -397,13 +403,13 @@ export function inferSalesforceValueClass( // eslint-disable-next-line @typescript-eslint/no-unused-vars key: string, val: string, - fieldTypeHint?: 'date' | 'datetime' | 'boolean' | 'integer' | 'string' -): 'date' | 'datetime' | 'boolean' | 'integer' | 'string' { + fieldTypeHint?: 'date' | 'datetime' | 'boolean' | 'decimal' | 'string' +): 'date' | 'datetime' | 'boolean' | 'decimal' | 'string' { if (fieldTypeHint) return fieldTypeHint; - if (/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/.test(val)) return 'datetime'; + if (/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(Z|[+-]\d{2}:?\d{2})?$/.test(val)) return 'datetime'; if (/^\d{4}-\d{2}-\d{2}$/.test(val)) return 'date'; if (val === 'true' || val === 'false') return 'boolean'; - if (/^-?\d+$/.test(val)) return 'integer'; + if (/^-?\d+(\.\d+)?$/.test(val)) return 'decimal'; return 'string'; } @@ -456,7 +462,7 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal return `${indent}`; } } - // PDX-493 (H3): infer valueClass for date / datetime / boolean / integer / string. The + // PDX-493 (H3): infer valueClass for date / datetime / boolean / decimal / string. The // `fieldTypeHint` parameter on `inferSalesforceValueClass` is intentionally not threaded // through here yet — it lands in PDX-492 (H2b) along with the `field_type_hints` tool input. const inferred = inferSalesforceValueClass(key, val); diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index 66460f05..7c286e1a 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -699,7 +699,9 @@ describe('provar_testcase_generate', () => { }); }); - // PDX-493 (H3): date/datetime/boolean/integer valueClass dispatch via inferSalesforceValueClass. + // PDX-493 (H3): date/datetime/boolean/decimal valueClass dispatch via inferSalesforceValueClass. + // Numbers emit `valueClass="decimal"` per canonical reference (PROVAR_TEST_STEP_REFERENCE.md + // lines 1338, 1428) — there is no `integer` valueClass in the Provar grammar. describe('PDX-493 — inferSalesforceValueClass helper', () => { it('returns "datetime" for ISO-8601 datetime string', () => { assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00'), 'datetime'); @@ -709,6 +711,17 @@ describe('provar_testcase_generate', () => { assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00.123Z'), 'datetime'); }); + it('returns "datetime" for ISO-8601 datetime with numeric timezone offset', () => { + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00+05:30'), 'datetime'); + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00-0800'), 'datetime'); + }); + + it('returns "string" for datetime-looking values with trailing garbage (end-anchored)', () => { + // Guards against the un-anchored regex bug: trailing junk after seconds must not + // be silently accepted as datetime. + assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19T10:30:00not-a-zone'), 'string'); + }); + it('returns "date" for ISO-8601 date string', () => { assert.equal(inferSalesforceValueClass('CloseDate', '2026-05-19'), 'date'); }); @@ -721,22 +734,30 @@ describe('provar_testcase_generate', () => { assert.equal(inferSalesforceValueClass('IsActive', 'false'), 'boolean'); }); - it('returns "integer" for positive integer string', () => { - assert.equal(inferSalesforceValueClass('Quantity', '42'), 'integer'); + it('returns "decimal" for positive integer string', () => { + assert.equal(inferSalesforceValueClass('Quantity', '42'), 'decimal'); + }); + + it('returns "decimal" for negative integer string', () => { + assert.equal(inferSalesforceValueClass('Delta', '-5'), 'decimal'); }); - it('returns "integer" for negative integer string', () => { - assert.equal(inferSalesforceValueClass('Delta', '-5'), 'integer'); + it('returns "decimal" for positive decimal string', () => { + assert.equal(inferSalesforceValueClass('Amount', '3.14'), 'decimal'); + }); + + it('returns "decimal" for negative decimal string', () => { + assert.equal(inferSalesforceValueClass('Adjustment', '-12.5'), 'decimal'); }); it('returns "string" for plain text', () => { assert.equal(inferSalesforceValueClass('Name', 'Acme Corp'), 'string'); }); - it('returns "integer" not "date" for a short numeric string like "12"', () => { + it('returns "decimal" not "date" for a short numeric string like "12"', () => { // Edge case: the date regex requires the full ISO yyyy-mm-dd form, so a bare "12" - // is integer, not date. Guards against false-positive date detection on numeric IDs. - assert.equal(inferSalesforceValueClass('Code', '12'), 'integer'); + // is decimal, not date. Guards against false-positive date detection on numeric IDs. + assert.equal(inferSalesforceValueClass('Code', '12'), 'decimal'); }); it('returns "string" for date-looking strings that miss the ISO format', () => { @@ -751,7 +772,7 @@ describe('provar_testcase_generate', () => { // Value looks like a date but the hint says string — hint wins (e.g. an external // ID that happens to look like a date). assert.equal(inferSalesforceValueClass('ExternalId', '2026-05-19', 'string'), 'string'); - // Value is integer, hint says boolean — hint wins. + // Value is decimal, hint says boolean — hint wins. assert.equal(inferSalesforceValueClass('IsActive', '1', 'boolean'), 'boolean'); }); }); @@ -824,7 +845,7 @@ describe('provar_testcase_generate', () => { ); }); - it('emits valueClass="integer" for an integer-only string', () => { + it('emits valueClass="decimal" for an integer-only string', () => { const result = server.call('provar_testcase_generate', { test_case_name: 'IntField', steps: [ @@ -839,10 +860,10 @@ describe('provar_testcase_generate', () => { }); const xml = parseText(result)['xml_content'] as string; - assert.ok(xml.includes('valueClass="integer">42'), `Expected valueClass="integer" for "42"; got: ${xml}`); + assert.ok(xml.includes('valueClass="decimal">42'), `Expected valueClass="decimal" for "42"; got: ${xml}`); }); - it('emits valueClass="integer" for a negative integer string', () => { + it('emits valueClass="decimal" for a negative integer string', () => { const result = server.call('provar_testcase_generate', { test_case_name: 'NegIntField', steps: [ @@ -857,7 +878,28 @@ describe('provar_testcase_generate', () => { }); const xml = parseText(result)['xml_content'] as string; - assert.ok(xml.includes('valueClass="integer">-5'), `Expected valueClass="integer" for "-5"; got: ${xml}`); + assert.ok(xml.includes('valueClass="decimal">-5'), `Expected valueClass="decimal" for "-5"; got: ${xml}`); + }); + + it('emits valueClass="decimal" for a positive decimal string', () => { + const result = server.call('provar_testcase_generate', { + test_case_name: 'DecimalField', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create Opp', + attributes: { Amount: '3.14' }, + }, + ], + dry_run: true, + overwrite: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok( + xml.includes('valueClass="decimal">3.14'), + `Expected valueClass="decimal" for "3.14"; got: ${xml}` + ); }); it('emits valueClass="string" for plain text', () => { From e46b7a948714b316db06743519223dd5822df84d Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:20:46 -0500 Subject: [PATCH 12/17] =?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 3ca9379a8917c7b2c041eb35f2fbaca7926f6435 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:26:08 -0500 Subject: [PATCH 13/17] =?UTF-8?q?PDX-486:=20fix(mcp)=20=E2=80=94=20gate=20?= =?UTF-8?q?RUN-001=20on=20parsedAny=20to=20avoid=20false=20positives=20whe?= =?UTF-8?q?n=20JUnit=20data=20missing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: The RUN-001 zero-tests-executed warning fired whenever `resultsPathResolved === true && stepCount === 0`. That condition is also true when the results dir was located but contained no XML files, or when every XML file failed to parse — in those cases the tool has no data on which to claim "zero tests ran" yet would still emit RUN-001, misdirecting the agent toward a properties-file typo when the real problem was an unreadable/unwritten results dir. Fix: Extended parseJUnitResults to return parsedAny: boolean (true iff at least one JUnit XML file was located AND parsed without throwing). Tightened the RUN-001 gate in introspectJUnit/registerAutomationTestRun to require `resultsPathResolved && parsedAny && stepCount === 0`. The "no XML found" and "all XML unparseable" branches now stay silent on warnings[] and surface only through details.warning, as the docs already described. Aligned the in-tree tool description and docs/mcp.md to spell out the parsedAny requirement, and added/strengthened tests covering the missing-XML, malformed-XML, and empty- (legit RUN-001) cases. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 4 +++- src/mcp/tools/antTools.ts | 16 +++++++++++++-- src/mcp/tools/automationTools.ts | 29 ++++++++++++++++++--------- test/unit/mcp/antTools.test.ts | 15 ++++++++++++++ test/unit/mcp/automationTools.test.ts | 19 +++++++++++++++--- 5 files changed, 68 insertions(+), 15 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index c2061f40..8d876f06 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1410,7 +1410,9 @@ After each run, the tool scans the results directory for JUnit XML files and add Each entry represents one test case. `status` is `"pass"`, `"fail"`, or `"skip"`. If the results directory cannot be located or contains no JUnit XML, `details.warning` explains why and `steps` is absent. -**Zero-tests guard (RUN-001):** when the sf command exits 0 but the JUnit report contains zero executed test cases, the response includes a `warnings[]` array containing a [`RUN-001`](#warning-codes) message. This is almost always a typo such as `testCase` vs `testCases` (or some other unknown key) in `provardx-properties.json` — the run silently selected nothing. The warning is additive and never flips `exitCode` or sets `isError`; the failure surface remains driven by the underlying `sf` exit code. +**Zero-tests guard (RUN-001):** when the sf command exits 0, the results directory was located, **and at least one JUnit XML file parsed successfully** but contains zero executed test cases, the response includes a `warnings[]` array containing a [`RUN-001`](#warning-codes) message. This is almost always a typo such as `testCase` vs `testCases` (or some other unknown key) in `provardx-properties.json` — the run silently selected nothing. The warning is additive and never flips `exitCode` or sets `isError`; the failure surface remains driven by the underlying `sf` exit code. + +> **Why RUN-001 stays silent when no JUnit data is available:** if the results directory cannot be located, contains no XML files, or every XML file fails to parse, the tool genuinely has no data on which to assert "zero tests ran" — the absence of parsed results is just "we don't know what ran". In those cases the response carries `details.warning` (explaining why structured step data is missing) and RUN-001 is suppressed to avoid misdirecting the agent toward a typo when the real issue is a missing/unreadable results dir. **Error codes:** `AUTOMATION_TESTRUN_FAILED`, `SF_NOT_FOUND` **Warning codes:** `RUN-001` (zero tests executed despite success) diff --git a/src/mcp/tools/antTools.ts b/src/mcp/tools/antTools.ts index 9efc0c93..18162779 100644 --- a/src/mcp/tools/antTools.ts +++ b/src/mcp/tools/antTools.ts @@ -989,6 +989,12 @@ export interface JUnitStepResult { export interface JUnitParseResult { steps: JUnitStepResult[]; warning?: string; + /** + * True iff at least one JUnit XML file was located AND parsed without throwing. + * Distinguishes "we have data and the test selector matched zero cases" (legit RUN-001 signal) + * from "we have no data because nothing parsed" (insufficient info — must stay silent). + */ + parsedAny: boolean; } function extractFailureText(el: unknown): string | undefined { @@ -1071,7 +1077,7 @@ function findXmlFiles(dir: string): string[] { */ export function parseJUnitResults(resultsDir: string): JUnitParseResult { if (!fs.existsSync(resultsDir)) { - return { steps: [], warning: `Results directory not found: ${resultsDir}` }; + return { steps: [], warning: `Results directory not found: ${resultsDir}`, parsedAny: false }; } const xmlFiles = findXmlFiles(resultsDir); @@ -1079,6 +1085,7 @@ export function parseJUnitResults(resultsDir: string): JUnitParseResult { return { steps: [], warning: 'No JUnit XML files found in results directory — structured step output unavailable.', + parsedAny: false, }; } @@ -1111,12 +1118,17 @@ export function parseJUnitResults(resultsDir: string): JUnitParseResult { return { steps: [], warning: 'JUnit XML files found but could not be parsed — structured step output unavailable.', + parsedAny: false, }; } if (allSteps.length === 0) { + // We did parse at least one file; the file just had zero entries (or none we could + // recognise as steps). This is the legitimate "selector matched nothing" signal that RUN-001 + // is built to catch. return { steps: [], warning: 'JUnit XML found but no test steps could be extracted — files may not be standard JUnit format.', + parsedAny: true, }; } @@ -1124,7 +1136,7 @@ export function parseJUnitResults(resultsDir: string): JUnitParseResult { parseFailures > 0 ? `${parseFailures} JUnit XML file(s) could not be parsed — step data may be incomplete.` : undefined; - return { steps: allSteps, warning }; + return { steps: allSteps, warning, parsedAny: true }; } // ── Registration ────────────────────────────────────────────────────────────── diff --git a/src/mcp/tools/automationTools.ts b/src/mcp/tools/automationTools.ts index 41dd804a..0efc3869 100644 --- a/src/mcp/tools/automationTools.ts +++ b/src/mcp/tools/automationTools.ts @@ -237,15 +237,22 @@ type JUnitIntrospection = { stepCount: number; parseWarning: string | undefined; resultsPathResolved: boolean; + /** + * True iff at least one JUnit XML file was located AND parsed without throwing. + * Gates RUN-001: a `stepCount === 0` only means "zero tests executed" when we know we + * actually have parseable data. With `parsedAny === false` the count is "we don't know", + * which must stay silent (details.warning already covers it). + */ + parsedAny: boolean; }; function introspectJUnit(config: ServerConfig): JUnitIntrospection { const resultsPath = readResultsPathFromSfConfig(config); if (!resultsPath) { - return { steps: [], stepCount: 0, parseWarning: undefined, resultsPathResolved: false }; + return { steps: [], stepCount: 0, parseWarning: undefined, resultsPathResolved: false, parsedAny: false }; } - const { steps, warning } = parseJUnitResults(resultsPath); - return { steps, stepCount: steps.length, parseWarning: warning, resultsPathResolved: true }; + const { steps, warning, parsedAny } = parseJUnitResults(resultsPath); + return { steps, stepCount: steps.length, parseWarning: warning, resultsPathResolved: true, parsedAny }; } const ZERO_TESTS_MESSAGE = @@ -266,7 +273,7 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi 'For grid/CI execution via Provar Quality Hub instead of running locally, use provar_qualityhub_testrun.', 'Output buffer: a 50 MB maxBuffer is set so ENOBUFS on verbose Provar runs is now rare.', 'If ENOBUFS still occurs (extremely verbose logging), run `sf provar automation test run --json` directly in the terminal and pipe or tail the output instead of retrying this tool.', - 'Zero-tests guard: if the sf exit code is 0 but the JUnit report contains no executed tests, a RUN-001 warning is added to `warnings[]` — usually a typo such as `testCase` vs `testCases` in provardx-properties.json.', + 'Zero-tests guard: if the sf exit code is 0, the results directory was located, and at least one JUnit XML file parsed successfully but contains zero executed tests, a RUN-001 warning is added to `warnings[]` — usually a typo such as `testCase` vs `testCases` in provardx-properties.json. When no JUnit data is available (dir missing or all XML unparseable), `details.warning` is set instead and RUN-001 stays silent.', 'Typical local AI loop: config.load → compile → testrun → inspect results.', ].join(' '), 'Run local Provar tests via sf CLI; requires config_load first. Surfaces RUN-001 on zero-tests-executed.' @@ -337,11 +344,15 @@ export function registerAutomationTestRun(server: McpServer, config: ServerConfi // RUN-001: sf reported success but zero tests actually executed. // Almost always a typo in the testCase / testCases field of provardx-properties.json. - // Only fires when the results dir was located (otherwise the absent step count is - // just "we don't know what ran", not "zero tests ran" — that case is already - // surfaced via details.warning above). Fires regardless of parse warning so the - // agent gets the typo hint even when the JUnit XML is degenerate. - if (junit.resultsPathResolved && junit.stepCount === 0) { + // Only fires when: + // 1. The results dir was located (resultsPathResolved), AND + // 2. At least one JUnit XML file was successfully parsed (parsedAny). + // Without (2) `stepCount === 0` just means "we don't have parseable data" — not + // "zero tests ran" — and the agent would be misdirected toward a typo when the + // real issue is a missing/unparseable results dir. That case is already surfaced + // via `details.warning` from the parse layer. With parsedAny === true and zero + // extracted steps, we know the selector genuinely matched nothing. + if (junit.resultsPathResolved && junit.parsedAny && junit.stepCount === 0) { const warningStr = formatWarning(WARNING_CODES.RUN_001, ZERO_TESTS_MESSAGE); // Append rather than overwrite so future warning emitters (e.g. JUNIT-001 mismatch // in PDX-491) can coexist on the same response without stepping on each other. diff --git a/test/unit/mcp/antTools.test.ts b/test/unit/mcp/antTools.test.ts index 74e37bc2..dadde6de 100644 --- a/test/unit/mcp/antTools.test.ts +++ b/test/unit/mcp/antTools.test.ts @@ -800,12 +800,14 @@ describe('parseJUnitResults', () => { const result = parseJUnitResults(path.join(junitTmpDir, 'nonexistent')); assert.deepEqual(result.steps, []); assert.ok(result.warning?.includes('not found')); + assert.equal(result.parsedAny, false, 'parsedAny must be false when dir is missing'); }); it('returns warning when directory contains no XML files', () => { const result = parseJUnitResults(junitTmpDir); assert.deepEqual(result.steps, []); assert.ok(result.warning?.includes('No JUnit XML')); + assert.equal(result.parsedAny, false, 'parsedAny must be false when no XML files exist'); }); it('extracts steps from a bare JUnit file', () => { @@ -818,6 +820,7 @@ describe('parseJUnitResults', () => { assert.equal(result.steps[1].status, 'fail'); assert.ok(result.steps[1].errorMessage?.includes('Element not found')); assert.equal(result.warning, undefined); + assert.equal(result.parsedAny, true, 'parsedAny must be true when at least one file parsed'); }); it('extracts steps from a wrapper JUnit file', () => { @@ -836,6 +839,18 @@ describe('parseJUnitResults', () => { const result = parseJUnitResults(junitTmpDir); assert.deepEqual(result.steps, []); assert.ok((result.warning?.length ?? 0) > 0); + // parsedAny must be TRUE here: the file was readable and parsed, it just has zero + // entries. This is the legitimate RUN-001 signal — distinct from "we have + // no data at all". + assert.equal(result.parsedAny, true, 'parsedAny must be true when XML parsed but had no steps'); + }); + + it('returns parsedAny=false when all XML files fail to parse', () => { + fs.writeFileSync(path.join(junitTmpDir, 'broken.xml'), ' { diff --git a/test/unit/mcp/automationTools.test.ts b/test/unit/mcp/automationTools.test.ts index 8590167d..2f63ab92 100644 --- a/test/unit/mcp/automationTools.test.ts +++ b/test/unit/mcp/automationTools.test.ts @@ -199,7 +199,10 @@ describe('automationTools', () => { } }); - it('omits steps[] and adds details.warning when results dir has no XML', () => { + it('omits steps[] and adds details.warning when results dir has no XML — RUN-001 stays silent', () => { + // PDX-486 follow-up: when the results dir is resolvable but contains zero XML files, + // we have no data on which to claim "zero tests executed". parsedAny is false, so + // RUN-001 must NOT fire; the missing-data case is surfaced via details.warning only. const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'junit-empty-')); setSfResultsPathForTesting(tmpDir); @@ -211,15 +214,22 @@ describe('automationTools', () => { assert.ok(body.details, 'details should be present'); const details = body.details as Record; assert.ok(typeof details.warning === 'string', 'details.warning should be a string'); + assert.equal(body.warnings, undefined, 'RUN-001 must NOT fire when no XML data is parseable'); } finally { setSfResultsPathForTesting(undefined); fs.rmSync(tmpDir, { recursive: true, force: true }); } }); - it('omits steps[] and adds details.warning when results dir contains malformed XML', () => { + it('omits steps[] and adds details.warning when results dir contains malformed XML — RUN-001 stays silent', () => { + // PDX-486 follow-up: if every XML file in the results dir fails to parse, parsedAny + // is false. RUN-001 must stay silent — the agent should be steered toward "why is + // the JUnit output malformed?" (via details.warning), not toward a properties typo. const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'junit-bad-')); - fs.writeFileSync(path.join(tmpDir, 'results.xml'), '', 'utf-8'); + // fast-xml-parser is permissive about *some* malformed text — this input shape (truncated + // before any matchable close tag) is one that genuinely throws under the strict isArray + // config used in antTools.parseJUnitResults, exercising the parsedAny=false branch. + fs.writeFileSync(path.join(tmpDir, 'results.xml'), ' { const body = parseBody(result); assert.equal(body.steps, undefined, 'steps should be absent when XML is malformed'); assert.ok(body.details, 'details should be present with warning'); + const details = body.details as Record; + assert.ok(typeof details.warning === 'string', 'details.warning should describe the parse failure'); + assert.equal(body.warnings, undefined, 'RUN-001 must NOT fire when XML data is unparseable'); } finally { setSfResultsPathForTesting(undefined); fs.rmSync(tmpDir, { recursive: true, force: true }); From f04eeff971f7d3ba96678951481512c3608ea462 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 07:29:55 -0500 Subject: [PATCH 14/17] =?UTF-8?q?PDX-492:=20fix(mcp)=20=E2=80=94=20orgDesc?= =?UTF-8?q?ribe=20path-policy=20hardening,=20XML=20required-flag=20bug,=20?= =?UTF-8?q?exists-true=20on=20parse=20error,=20docs/tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review of PR #188 flagged six issues across correctness, security, and contract: (1) the .xml/.object fallback compared required==='true' as a string, but fast-xml-parser with parseTagValue=true (its default) coerces the value to boolean true, silently misclassifying required fields as nillable; (2) discoverWorkspace probed fs.existsSync/statSync against candidate dirs (including the ~/Provar home fallback) BEFORE any path-policy check, contradicting the project's --allowed-paths contract and potentially touching paths outside the policy; (3) when a cache file existed but failed to parse, readObject returned exists=false — indistinguishable from "object not cached", so callers could not detect corrupt/unsupported cache files; (4) docs/examples omitted the requestId field that the tool actually returns, making the documented shape drift from runtime; (5) unit tests covered only the .json cache path, leaving the legacy .xml and .object parsers (where the required-flag bug lived) untested; (6) the PATH_TRAVERSAL message read "contains path separators" but the validator also rejects bare ".." with no separators, so the message was inaccurate for that branch. Fix: (1) readXmlCacheFile now treats both boolean true and string "true" as required, so nillable is computed correctly regardless of parser config; (2) discoverWorkspace accepts allowedPaths and runs assertPathAllowed per candidate BEFORE fs.existsSync/statSync — a candidate outside policy is silently skipped (not a hard error) so discovery falls through to the next candidate naturally; (3) readObject parse failures now return exists=true with field_count=0 and a per-object error_message describing the parse failure, letting callers distinguish corrupt from missing; (4) docs/mcp.md adds requestId to the output table, adds it to both example responses, documents the new error_message field shape, and adds a third example showing the parse-error response; (5) added (h.1) .xml format test (regression guard for the required-flag bug), (h.2) .object format test, (i) parse-error test asserting exists=true + error_message, and (j) bare ".." connection_name test asserting the broadened message; (6) the PATH_TRAVERSAL message now reads "must not contain path separators or directory-traversal segments ('..')", covering both rejection conditions. 19/19 orgDescribe tests pass, full mocha 1174/1174, yarn lint clean, yarn compile clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/mcp.md | 36 ++++- src/mcp/tools/orgDescribeTools.ts | 53 +++++-- test/unit/mcp/orgDescribeTools.test.ts | 191 ++++++++++++++++++++++++- 3 files changed, 265 insertions(+), 15 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 00405b29..a4da48ab 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1715,12 +1715,14 @@ Read cached Salesforce describe data for one connection from the Provar workspac | `objects` | string[] | no | all | Filter — only return data for these object API names. When omitted, lists every object cached under the connection directory. | | `field_filter` | `'required'` \| `'all'` | no | `'required'` | Which fields to return. `'required'` includes only fields with `nillable=false`; `'all'` returns every cached field. | -| Output field | Description | -| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | -| `workspace_path` | Absolute resolved path to the discovered workspace, or `null` when none of the three candidate directories exists. | -| `cache_age_ms` | `mtime` delta in milliseconds of the connection cache directory, or `null` when the cache is missing. | -| `objects[]` | Array of `{ name, exists, required_fields, field_count }`. `exists` is `true` (cached), `false` (requested but not cached), or `null` (cache miss). | -| `details.suggestion` | Present **only** on cache miss. Tells the agent how to populate the cache (open Provar IDE) or how to proceed without it (inline hints). | +| Output field | Description | +| ------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `requestId` | UUID for this invocation. Echoed in MCP server logs for cross-correlation. Consistent with the rest of the MCP tool surface. | +| `workspace_path` | Absolute resolved path to the discovered workspace, or `null` when none of the three candidate directories exists (or all candidates were outside `--allowed-paths`). | +| `cache_age_ms` | `mtime` delta in milliseconds of the connection cache directory, or `null` when the cache is missing. | +| `objects[]` | Array of `{ name, exists, required_fields, field_count, error_message? }`. `exists` is `true` (cache file present), `false` (requested but not cached), or `null` (cache miss — the whole `.metadata/` directory is absent). | +| `objects[].error_message` | Present **only** when a cache file existed but failed to parse (`exists: true, field_count: 0`). Lets the agent distinguish a corrupt / unsupported cache file from a missing one. | +| `details.suggestion` | Present **only** on cache miss. Tells the agent how to populate the cache (open Provar IDE) or how to proceed without it (inline hints). | **Example — happy path:** @@ -1735,6 +1737,7 @@ Read cached Salesforce describe data for one connection from the Provar workspac // Response { + "requestId": "01HEXX...K7P", "workspace_path": "/Users/me/git/workspace-MyProject", "cache_age_ms": 1839200, "objects": [ @@ -1763,6 +1766,7 @@ Read cached Salesforce describe data for one connection from the Provar workspac ```jsonc // Response when the .metadata/ directory does not exist { + "requestId": "01HEXX...K7P", "workspace_path": "/Users/me/git/workspace-MyProject", "cache_age_ms": null, "objects": [{ "name": "Account", "exists": null, "required_fields": [], "field_count": 0 }], @@ -1772,6 +1776,26 @@ Read cached Salesforce describe data for one connection from the Provar workspac } ``` +**Example — parse error on a cached file:** + +```jsonc +// Response when Account.json exists but is corrupt / unparseable +{ + "requestId": "01HEXX...K7P", + "workspace_path": "/Users/me/git/workspace-MyProject", + "cache_age_ms": 1839200, + "objects": [ + { + "name": "Account", + "exists": true, + "required_fields": [], + "field_count": 0, + "error_message": "Failed to parse cache file (Account.json): Unexpected token } in JSON at position 42" + } + ] +} +``` + **On-disk cache schema (one file per object).** The tool reads `.json` first, then `.xml`, then `.object` as a fallback: ```jsonc diff --git a/src/mcp/tools/orgDescribeTools.ts b/src/mcp/tools/orgDescribeTools.ts index b5b273bf..26d52649 100644 --- a/src/mcp/tools/orgDescribeTools.ts +++ b/src/mcp/tools/orgDescribeTools.ts @@ -37,6 +37,8 @@ export interface OrgDescribeObject { exists: boolean | null; required_fields: OrgDescribeField[]; field_count: number; + /** Present only when the cache file existed but failed to parse. */ + error_message?: string; } export interface OrgDescribeResult { @@ -98,9 +100,26 @@ export function workspaceCandidates(projectPath: string): string[] { ]; } -/** Returns the first candidate workspace that exists, or null. */ -export function discoverWorkspace(projectPath: string): string | null { +/** + * Returns the first candidate workspace that exists AND is within allowedPaths, or null. + * + * Path policy is enforced PER CANDIDATE before any filesystem call: a candidate that + * sits outside `--allowed-paths` is silently skipped (it is not an error — discovery + * just moves on to the next). This means we never call fs.existsSync / fs.statSync + * against directories that the operator has explicitly placed off-limits, including + * the user-home fallback (~/Provar/...) when home sits outside the policy. + * + * When allowedPaths is empty (unrestricted mode), assertPathAllowed is a no-op and + * all candidates are probed exactly as before. + */ +export function discoverWorkspace(projectPath: string, allowedPaths: string[] = []): string | null { for (const candidate of workspaceCandidates(projectPath)) { + try { + assertPathAllowed(candidate, allowedPaths); + } catch { + // Candidate outside policy — skip without touching the filesystem. + continue; + } try { if (fs.existsSync(candidate) && fs.statSync(candidate).isDirectory()) { return candidate; @@ -142,13 +161,18 @@ function readXmlCacheFile(filePath: string): CachedObject { for (const f of fieldsRaw as Array>) { const name = (f['fullName'] ?? f['name']) as string | undefined; if (!name) continue; + // fast-xml-parser with parseTagValue=true (the default) coerces `true` + // into the boolean true; with parseTagValue=false it would stay as the string "true". + // Accept BOTH forms so we don't misclassify required fields as nillable on either path. + const requiredRaw = f['required']; + const isRequired = requiredRaw === true || requiredRaw === 'true'; fields.push({ name, type: (f['type'] as string | undefined) ?? 'unknown', defaultValue: (f['defaultValue'] as string | undefined) ?? null, // XML defaults: required = !nillable. In the .object format, "required" is rare, // so we default to nillable=true (optional) unless explicitly required. - nillable: f['required'] === 'true' ? false : true, + nillable: !isRequired, }); } return { name: path.basename(filePath, path.extname(filePath)), fields }; @@ -194,8 +218,19 @@ function readObject(connectionDir: string, objectName: string, fieldFilter: 'req try { cached = path.extname(file) === '.json' ? readJsonCacheFile(file) : readXmlCacheFile(file); } catch (e) { - log('warn', 'org_describe: failed to parse cache file', { file, error: (e as Error).message }); - return { name: objectName, exists: false, required_fields: [], field_count: 0 }; + const errorMessage = (e as Error).message; + log('warn', 'org_describe: failed to parse cache file', { file, error: errorMessage }); + // The cache file is present but unreadable. Report exists=true so the caller + // can distinguish "cache corrupt / unsupported format" from "object not cached" + // (exists=false). field_count=0 since we have no parsed fields, and error_message + // carries the underlying parse failure for diagnostics. + return { + name: objectName, + exists: true, + required_fields: [], + field_count: 0, + error_message: `Failed to parse cache file (${path.basename(file)}): ${errorMessage}`, + }; } const allFields = cached.fields ?? []; @@ -258,10 +293,12 @@ function resolveConnectionDir( // Reject path-shaped connection names outright. A real connection name from a // .testproject is an identifier (e.g. "MyOrg"); any separator or traversal // segment is almost certainly a misuse or injection attempt. - if (connectionName.includes('/') || connectionName.includes('\\') || connectionName.split(/[/\\]+/).includes('..')) { + const hasSeparator = connectionName.includes('/') || connectionName.includes('\\'); + const hasTraversal = connectionName === '..' || connectionName.split(/[/\\]+/).includes('..'); + if (hasSeparator || hasTraversal) { throw new PathPolicyError( 'PATH_TRAVERSAL', - `Invalid connection_name (contains path separators): ${connectionName}` + `Invalid connection_name (must not contain path separators or directory-traversal segments ('..')): ${connectionName}` ); } @@ -383,7 +420,7 @@ export function registerOrgDescribe(server: McpServer, config: ServerConfig): vo try { assertPathAllowed(args.project_path, config.allowedPaths); - const workspacePath = discoverWorkspace(args.project_path); + const workspacePath = discoverWorkspace(args.project_path, config.allowedPaths); const { connectionDir, resolvedWorkspace } = resolveConnectionDir( workspacePath, args.connection_name, diff --git a/test/unit/mcp/orgDescribeTools.test.ts b/test/unit/mcp/orgDescribeTools.test.ts index a8820f27..e0b851d6 100644 --- a/test/unit/mcp/orgDescribeTools.test.ts +++ b/test/unit/mcp/orgDescribeTools.test.ts @@ -65,6 +65,31 @@ function writeJsonCache(connectionDir: string, objectName: string, fields: Cache ); } +/** + * Write a legacy .object / .xml cache file (CustomObject metadata) with the given fields. + * `required` is emitted as the string "true" / "false" — but fast-xml-parser with the + * default parseTagValue=true will coerce it to a boolean before reaching the reader. + * The implementation must accept BOTH forms, which is what the legacy-format tests assert. + */ +function writeXmlCache( + connectionDir: string, + objectName: string, + fields: Array<{ name: string; type: string; required: boolean }>, + ext: '.xml' | '.object' = '.xml' +): void { + fs.mkdirSync(connectionDir, { recursive: true }); + const fieldsXml = fields + .map( + (f) => + ` \n ${f.name}\n ${f.type}\n ${String( + f.required + )}\n ` + ) + .join('\n'); + const xml = `\n\n${fieldsXml}\n\n`; + fs.writeFileSync(path.join(connectionDir, `${objectName}${ext}`), xml, 'utf-8'); +} + // ── Test setup ───────────────────────────────────────────────────────────────── let tmpRoot: string; @@ -179,7 +204,29 @@ describe('provar_org_describe — workspace discovery', () => { }); it('discoverWorkspace returns null when no candidate exists', () => { - assert.equal(discoverWorkspace(projectPath), null); + assert.equal(discoverWorkspace(projectPath, [tmpRoot]), null); + }); + + it('discoverWorkspace skips candidates outside allowedPaths without touching the filesystem', () => { + // Create a sibling workspace that DOES exist on disk but lies outside the allowed root. + // We force this by creating a parallel tmp tree and only allowing tmpRoot. + // The sibling pattern resolves to /workspace-; the parent of + // projectPath is tmpRoot, so the sibling itself is inside the allowed set — + // which is what we want for the happy case. To exercise the policy skip we use + // a separate "outside" project path whose sibling candidate lives outside. + const outsideRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'org-describe-outside-'))); + try { + const outsideProject = path.join(outsideRoot, 'OtherProject'); + fs.mkdirSync(outsideProject, { recursive: true }); + const outsideSibling = path.join(outsideRoot, 'workspace-OtherProject'); + fs.mkdirSync(outsideSibling, { recursive: true }); + + // With only tmpRoot allowed, discoverWorkspace MUST NOT return the outside sibling + // even though it exists on disk. + assert.equal(discoverWorkspace(outsideProject, [tmpRoot]), null); + } finally { + fs.rmSync(outsideRoot, { recursive: true, force: true }); + } }); }); @@ -403,3 +450,145 @@ describe('provar_org_describe — objects filter', () => { assert.equal(ghost.exists, false, 'object not in cache → exists=false'); }); }); + +// ── (h) Legacy cache formats — .xml / .object ──────────────────────────────── + +describe('provar_org_describe — legacy cache formats', () => { + it('(h.1) parses .xml CustomObject metadata and classifies required vs nillable correctly', () => { + // Regression guard for the required-flag bug: fast-xml-parser's default + // parseTagValue=true coerces "true" to the boolean true. + // The reader must treat boolean and string forms identically. + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeXmlCache( + connectionDir, + 'Account', + [ + { name: 'Name', type: 'string', required: true }, + { name: 'Phone', type: 'phone', required: false }, + ], + '.xml' + ); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + objects: ['Account'], + field_filter: 'all', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + required_fields: Array<{ name: string; nillable: boolean }>; + field_count: number; + }>; + assert.equal(objects.length, 1); + assert.equal(objects[0].exists, true); + assert.equal(objects[0].field_count, 2); + const byName = new Map(objects[0].required_fields.map((f) => [f.name, f.nillable])); + assert.equal( + byName.get('Name'), + false, + 'required field should have nillable=false (NOT misclassified as nillable)' + ); + assert.equal(byName.get('Phone'), true, 'non-required field should have nillable=true'); + }); + + it('(h.2) parses .object CustomObject metadata (legacy Eclipse layout)', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + writeXmlCache( + connectionDir, + 'Contact', + [ + { name: 'LastName', type: 'string', required: true }, + { name: 'Email', type: 'email', required: false }, + ], + '.object' + ); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + objects: ['Contact'], + field_filter: 'required', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + required_fields: Array<{ name: string }>; + field_count: number; + }>; + assert.equal(objects[0].exists, true); + assert.equal(objects[0].field_count, 2, 'field_count counts all parsed fields, regardless of filter'); + // field_filter='required' → only nillable=false survives + const names = objects[0].required_fields.map((f) => f.name); + assert.deepEqual(names, ['LastName'], 'only the required field should pass the filter'); + }); +}); + +// ── (i) Parse-error reporting ───────────────────────────────────────────────── + +describe('provar_org_describe — parse errors', () => { + it('(i) returns exists=true with error_message when a cache file is corrupt', () => { + // A cache file that EXISTS but does not parse must NOT be reported as exists=false + // (that would conflate "not cached" with "corrupt") — the contract is exists=true, + // field_count=0, error_message set. + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + fs.mkdirSync(connectionDir, { recursive: true }); + fs.writeFileSync(path.join(connectionDir, 'Account.json'), '{ not valid json', 'utf-8'); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'MyOrg', + objects: ['Account'], + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + field_count: number; + error_message?: string; + }>; + assert.equal(objects.length, 1); + assert.equal(objects[0].exists, true, 'corrupt cache file is "present" — not missing'); + assert.equal(objects[0].field_count, 0); + const errMsg = objects[0].error_message; + assert.ok(errMsg, 'error_message should describe the parse failure'); + assert.ok(errMsg.includes('Account.json'), `error_message should reference the file name; got: ${errMsg}`); + }); +}); + +// ── (j) Connection-name traversal — bare `..` ───────────────────────────────── + +describe('provar_org_describe — connection_name validation', () => { + it('(j) rejects bare ".." connection_name with PATH_TRAVERSAL and a clear message', () => { + // Regression guard for the broadened error message: the validator rejects `..` + // even when no separator is present. The message must mention BOTH conditions. + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + fs.mkdirSync(path.join(siblingWorkspace, '.metadata'), { recursive: true }); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: '..', + }); + + assert.equal(isError(result), true); + const body = parseText(result); + assert.equal(body['error_code'], 'PATH_TRAVERSAL'); + const msg = body['message'] as string; + assert.ok( + /path separators/i.test(msg) && msg.includes('..'), + `error message should mention both path separators and '..'; got: ${msg}` + ); + }); +}); From dbe4f37bb82a63eb1298ff9c09727b43309e5d04 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 08:32:38 -0500 Subject: [PATCH 15/17] =?UTF-8?q?PDX-489:=20fix(test)=20=E2=80=94=20realpa?= =?UTF-8?q?th=20tmp=20root=20in=20plan-mode=20test=20to=20match=20symlink?= =?UTF-8?q?=20canonicalisation?= 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(() => { From ad853472fc142b68ee816c0d6a1b5ac5b8958310 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 09:41:35 -0500 Subject: [PATCH 16/17] =?UTF-8?q?PDX-0:=20chore(release)=20=E2=80=94=20bum?= =?UTF-8?q?p=20version=20to=201.5.2-beta.1=20on=20develop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Develop accumulated 7 PRs from Sessions 6/7 backlog (PDX-485 thread-prep, PDX-486 VALIDATE-TYPO A+B, PDX-489 G3 DATA-001, PDX-490 G5 error_category, PDX-492 H2a provar_org_describe, PDX-493 H3 valueClass decimal alignment) since 1.5.1 was tagged. Per CLAUDE.md the develop branch tracks ..-beta., so the next publishable version is 1.5.2-beta.1. Without this bump a publish from develop would clash with 1.5.1 on the registry. Fix: Bump package.json.version, server.json.version (top-level), and server.json.packages[0].version from 1.5.1 to 1.5.2-beta.1, keeping all three identical per the version-sync rule in CLAUDE.md. No source or test changes. yarn compile clean, yarn lint clean, full mocha 1245 passing / 0 failing. --- package.json | 2 +- server.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 2a0d1730..c376bec1 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.1", + "version": "1.5.2-beta.1", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 01e3d72e..2cbea4c8 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.5.1", + "version": "1.5.2-beta.1", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.5.1", + "version": "1.5.2-beta.1", "transport": { "type": "stdio" }, From 94e1c8a66db0a14526a40c563375866885cf093b Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 20 May 2026 10:24:37 -0500 Subject: [PATCH 17/17] =?UTF-8?q?PDX-0:=20chore(release)=20=E2=80=94=20bum?= =?UTF-8?q?p=20version=20to=201.5.2=20for=20release?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Develop currently sits at 1.5.2-beta.1 after PR #189. The 1.5.2 release rolls up 7 PRs from the Sessions 6/7 backlog (PDX-485 warningCodes shared enum, PDX-486 VALIDATE-TYPO A+B, PDX-489 G3 DATA-001 plus path-safety hardening, PDX-490 G5 error_category + retryable, PDX-492 H2a provar_org_describe tool, PDX-493 H3 date/datetime/decimal valueClass). Per CLAUDE.md, the release branch drops the -beta.N suffix before the develop -> main merge so the release commit publishes 1.5.2 cleanly without a prerelease tag chain. Fix: Bump package.json.version, server.json.version, and server.json.packages[0].version from 1.5.2-beta.1 to 1.5.2 in lock-step per the version-sync rule. No source changes; release surface is exactly what landed on develop. --- package.json | 2 +- server.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index c376bec1..d7040ea1 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.2-beta.1", + "version": "1.5.2", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 2cbea4c8..6d392a13 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.5.2-beta.1", + "version": "1.5.2", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.5.2-beta.1", + "version": "1.5.2", "transport": { "type": "stdio" },