From debca7856a370639bd10e4824b8ebafb8bd2ac64 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 11:24:26 -0500 Subject: [PATCH 1/7] PDX-0: fix(mcp): address session 3 feedback D1-D4 + D7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1: add provar.testplan.create tool — creates plans/{name}/ directory and root .planitem; previously agents had to create plan files manually. D2: emit class="uiTarget" uri="..." for the 'target' argument and class="uiLocator" uri="..." for 'locator' argument in XML generation; previously both were written as valueClass="string" causing a ClassCastException in the Provar runtime. D3: SetValues / AssertValues steps now emit ... for the 'values' argument; previously a pipe-delimited string was written, causing an immediate ClassCastException. D4: attribute values matching {VarName} or {Obj.Field} are emitted as class="variable" with children; previously emitted as plain string literals which the runtime rejects. D7: provar.testcase.generate now includes a cleanup warning when ApexDeleteObject steps are present, explaining that cleanup steps after a failure point are skipped when stopOnError=false. Also updates StepSchema.attributes description and TOOL_DESCRIPTION to document all four special value conventions for agent grounding. Tests: 19 new assertions across testCaseGenerate.test.ts and testPlanTools.test.ts covering happy path, dry_run, and error cases. Smoke test: +1 entry for provar.testplan.create (TOTAL_EXPECTED 50->51). Co-Authored-By: Claude Sonnet 4.6 --- scripts/mcp-smoke.cjs | 15 +- src/mcp/tools/testCaseGenerate.ts | 99 +++++++++- src/mcp/tools/testPlanTools.ts | 115 ++++++++++++ test/unit/mcp/testCaseGenerate.test.ts | 243 +++++++++++++++++++++++++ test/unit/mcp/testPlanTools.test.ts | 141 +++++++++++++- 5 files changed, 598 insertions(+), 15 deletions(-) diff --git a/scripts/mcp-smoke.cjs b/scripts/mcp-smoke.cjs index 06b709f..68e52c5 100644 --- a/scripts/mcp-smoke.cjs +++ b/scripts/mcp-smoke.cjs @@ -248,7 +248,14 @@ async function runTests() { // ── 30. provar.testrun.rca ─────────────────────────────────────────────── await callTool('provar.testrun.rca', { project_path: TMP }); - // ── 31. provar.testplan.add-instance ───────────────────────────────────── + // ── 31. provar.testplan.create ──────────────────────────────────────────── + // TMP is not a Provar project → NOT_A_PROJECT result + await callTool('provar.testplan.create', { + project_path: TMP, + plan_name: 'SmokePlan', + }); + + // ── 33. provar.testplan.add-instance ───────────────────────────────────── // TMP is not a Provar project → NOT_A_PROJECT result await callTool('provar.testplan.add-instance', { project_path: TMP, @@ -256,14 +263,14 @@ async function runTests() { plan_name: 'SmokePlan', }); - // ── 32. provar.testplan.create-suite ───────────────────────────────────── + // ── 34. provar.testplan.create-suite ───────────────────────────────────── await callTool('provar.testplan.create-suite', { project_path: TMP, plan_name: 'SmokePlan', suite_name: 'SmokeSuite', }); - // ── 33. provar.testplan.remove-instance ────────────────────────────────── + // ── 35. provar.testplan.remove-instance ────────────────────────────────── await callTool('provar.testplan.remove-instance', { project_path: TMP, instance_path: 'plans/SmokePlan/SmokeSuite/smoke.testinstance', @@ -377,7 +384,7 @@ async function runTests() { server.on('close', () => { clearTimeout(overallTimer); // initialize + tools/list + 39 tools + prompts/list + 8 prompts/get (setup excluded from default count) - const TOTAL_EXPECTED = 50 + (INCLUDE_SETUP ? 1 : 0); + const TOTAL_EXPECTED = 51 + (INCLUDE_SETUP ? 1 : 0); let passed = 0; let failed = 0; diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index e053cf9..3badee2 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -69,6 +69,17 @@ function buildStepWarnings(steps: Array<{ api_id: string }>): string[] { ); } + // D7: Cleanup steps placed after a potential failure point are skipped when stopOnError=false. + if (resolvedIds.includes(SHORTHAND_TO_FQID['ApexDeleteObject'] ?? '')) { + warnings.push( + 'ApexDeleteObject detected (likely cleanup): with stopOnError=false Provar skips all steps after ' + + 'the first failure, so cleanup steps placed at the end of the test will NOT run when an earlier ' + + 'step fails — leaving orphaned records in the org. ' + + 'Wrap cleanup in a Provar TearDown callable, or place create/delete inside the same UiWithScreen ' + + 'clause so both run as a unit regardless of failure.' + ); + } + return warnings; } @@ -89,10 +100,18 @@ const StepSchema = z.object({ .record(z.string()) .default({}) .describe( - 'Step argument values as key/value pairs. Written as ' + - 'inside the element — the format Provar runtime requires. ' + + 'Step argument values as key/value pairs. Written as . ' + 'Do NOT rely on XML attributes on ; the runtime silently ignores them. ' + - 'Example: { "connectionName": "MyOrg", "objectApiName": "Opportunity" }' + 'Special value conventions (applied automatically by the generator): ' + + '(1) Variable references: wrap the name in braces, e.g. "{MyVar}" → emitted as class="variable" . ' + + ' Dotted paths are also supported: "{Obj.Field}" → two elements. ' + + '(2) SetValues / AssertValues: pass each variable name and its value as a flat key/value pair; ' + + ' the generator wraps them in ... automatically. ' + + ' Example for SetValues: { "testCaseName": "TC_New", "testType": "Acceptance testing" } ' + + '(3) target argument (UiWithScreen / UiWithRow): pass the sf:ui:target or ui:pageobject:target URI; ' + + ' emitted as class="uiTarget" uri="...". ' + + '(4) locator argument (UiDoAction / UiAssert): pass the locator URI; emitted as class="uiLocator" uri="...". ' + + 'All other string values use class="value" valueClass="string".' ), }); @@ -111,6 +130,12 @@ const TOOL_DESCRIPTION = [ 'ApexReadObject requires field names in attributes; omitting them produces MALFORMED_QUERY. Prefer ApexSoqlQuery.', 'AssertValues on SOQL results: index paths like "ResultList[0].Field" are not supported.', 'Use ForEach to iterate the result list, or SetValues to extract a field into a variable first.', + 'SetValues / AssertValues: pass named variable values as flat key/value pairs in attributes; ' + + 'the generator wraps them in ... automatically.', + '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="...".', + 'Cleanup warning: ApexDeleteObject steps near end of test will be skipped if an earlier step fails (stopOnError=false). Use a TearDown callable.', 'Validation: when validate_after_edit=true (default) the response includes a validation field and returns TESTCASE_INVALID if the generated XML fails structural checks.', 'Grounding: call provar.qualityhub.examples.retrieve before generating to get corpus examples for the scenario — correct XML structure for the step types you need.', ].join(' '); @@ -239,20 +264,75 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // ── XML builder ─────────────────────────────────────────────────────────────── +// APIs whose 'values' argument must use class="valueList"/ (D3). +const SET_VALUES_APIS = new Set([ + SHORTHAND_TO_FQID['SetValues'], // com.provar.plugins.bundled.apis.control.SetValues + SHORTHAND_TO_FQID['AssertValues'], // com.provar.plugins.bundled.apis.AssertValues +]); + +// Build the element for a single argument (D2/D4 aware). +function buildArgumentValue(key: string, val: string, indent: string): string { + // D4: {VarName} or {Obj.Field} → class="variable" with elements. + const varMatch = /^\{([\w.]+)\}$/.exec(val); + if (varMatch) { + const pathElements = varMatch[1] + .split('.') + .map((p) => `${indent} `) + .join('\n'); + return `${indent}\n${pathElements}\n${indent}`; + } + // D2: 'target' argument (UiWithScreen / UiWithRow) → class="uiTarget". + if (key === 'target') { + return `${indent}`; + } + // D2: 'locator' argument (UiDoAction / UiAssert) → class="uiLocator". + if (key === 'locator') { + return `${indent}`; + } + return `${indent}${escapeXmlContent(val)}`; +} + function buildArgumentsXml(attributes: Record, baseIndent = ' '): string { const entries = Object.entries(attributes); if (entries.length === 0) return ''; const argLines = entries - .map( - ([k, v]) => + .map(([k, v]) => { + const valueXml = buildArgumentValue(k, v, `${baseIndent} `); + return ( `${baseIndent}\n` + - `${baseIndent} ${escapeXmlContent(v)}\n` + + valueXml + '\n' + `${baseIndent}` - ) + ); + }) .join('\n'); return `\n${baseIndent}\n${argLines}\n${baseIndent}\n${baseIndent.slice(0, -2)}`; } +// D3: SetValues / AssertValues — all attributes become under a single 'values' argument. +function buildSetValuesXml(attributes: Record, baseIndent: string): string { + const entries = Object.entries(attributes); + if (entries.length === 0) return ''; + const i = (n: number): string => baseIndent + ' '.repeat(n); + const namedValueLines = entries + .map(([name, val]) => { + const valueXml = buildArgumentValue(name, val, `${i(3)} `); + return `${i(3)}\n${valueXml}\n${i(3)}`; + }) + .join('\n'); + return ( + `\n${i(0)}\n` + + `${i(0)}\n` + + `${i(1)}\n` + + `${i(2)}\n` + + namedValueLines + '\n' + + `${i(2)}\n` + + `${i(1)}\n` + + `${i(0)}\n` + + `${baseIndent.slice(0, -2)}\n` + + `${baseIndent.slice(0, -2)}` + ); +} + function buildFlatStepXml( step: { api_id: string; name: string; attributes: Record }, testItemId: number, @@ -260,7 +340,10 @@ function buildFlatStepXml( ): string { const guid = randomUUID(); const resolvedApiId = resolveApiId(step.api_id); - const argumentsXml = buildArgumentsXml(step.attributes, indent + ' '); + const baseIndent = indent + ' '; + const argumentsXml = SET_VALUES_APIS.has(resolvedApiId) + ? buildSetValuesXml(step.attributes, baseIndent) + : buildArgumentsXml(step.attributes, baseIndent); if (argumentsXml) { return ( `${indent} { + const requestId = makeRequestId(); + log('info', 'provar.testplan.create', { requestId, project_path, plan_name }); + + try { + assertPathAllowed(project_path, config.allowedPaths); + + const projectRoot = path.resolve(project_path); + + const testProjectFiles = fs.existsSync(projectRoot) + ? fs.readdirSync(projectRoot).filter((f) => f.endsWith('.testproject')) + : []; + if (testProjectFiles.length === 0) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify(makeError('NOT_A_PROJECT', `No .testproject file found in ${projectRoot}`, requestId)), + }, + ], + }; + } + + const planDir = path.join(projectRoot, 'plans', plan_name); + const planItemPath = path.join(planDir, '.planitem'); + + if (!overwrite && fs.existsSync(planItemPath)) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify( + makeError( + 'PLAN_EXISTS', + `Plan already exists: ${planItemPath}. Set overwrite: true to replace the .planitem file.`, + requestId + ) + ), + }, + ], + }; + } + + const guid = randomUUID(); + const xmlContent = buildPlanItemXml(guid); + + if (!dry_run) { + fs.mkdirSync(planDir, { recursive: true }); + fs.writeFileSync(planItemPath, xmlContent, 'utf-8'); + } + + const response = { + requestId, + plan_dir: planDir, + planitem_path: planItemPath, + guid, + dry_run: dry_run ?? false, + created: !dry_run, + next_steps: dry_run + ? 'Review the plan structure, then call provar.testplan.create with dry_run=false to write to disk.' + : `Plan created at ${planDir}. Use provar.testplan.create-suite to add suites, then provar.testplan.add-instance to wire test cases into the plan.`, + }; + 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 ?? 'CREATE_PLAN_ERROR'), + error.message, + requestId + ) + ), + }, + ], + }; + } + } + ); +} + // ── provar.testplan.add-instance ────────────────────────────────────────────── export function registerTestPlanAddInstance(server: McpServer, config: ServerConfig): void { @@ -358,6 +472,7 @@ export function registerTestPlanRemoveInstance(server: McpServer, config: Server // ── Convenience re-export ───────────────────────────────────────────────────── export function registerAllTestPlanTools(server: McpServer, config: ServerConfig): void { + registerTestPlanCreate(server, config); registerTestPlanAddInstance(server, config); registerTestPlanCreateSuite(server, config); registerTestPlanRemoveInstance(server, config); diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index bbdcf1b..da31040 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -460,6 +460,249 @@ describe('provar.testcase.generate', () => { }); }); + describe('D2 — uiTarget / uiLocator argument types', () => { + it('emits class="uiTarget" uri="..." for the target argument', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'UI Target Test', + steps: [ + { + api_id: 'UiWithScreen', + name: 'With page', + attributes: { target: 'sf:ui:target?pageObject=pageobjects.Account&flexiPage=Account_flexiPage' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="uiTarget"'), 'Expected class="uiTarget"'); + assert.ok(xml.includes('uri="sf:ui:target?'), 'Expected uri attribute with sf:ui:target value'); + assert.ok(!xml.includes('valueClass="string">sf:ui:target'), 'Must NOT emit uiTarget URI as a plain string value'); + }); + + it('emits class="uiLocator" uri="..." for the locator argument', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'UI Locator Test', + steps: [ + { + api_id: 'UiDoAction', + name: 'Click button', + attributes: { locator: 'sf:ui:locator:button?label=Save' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="uiLocator"'), 'Expected class="uiLocator"'); + assert.ok(xml.includes('uri="sf:ui:locator:'), 'Expected uri attribute with locator value'); + assert.ok(!xml.includes('valueClass="string">sf:ui:locator'), 'Must NOT emit locator URI as a plain string value'); + }); + + it('uiTarget also applies inside UiWithScreen wrapper when target_uri is non-SF', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'Non-SF With Target', + steps: [], + target_uri: 'ui:pageobject:target?pageId=pageobjects.LoginPage', + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="uiTarget"'), 'Wrapper UiWithScreen target should use uiTarget class'); + assert.ok(xml.includes('uri="ui:pageobject:target?pageId=pageobjects.LoginPage"'), 'URI should appear as attribute'); + }); + }); + + describe('D3 — SetValues / AssertValues use valueList/namedValues structure', () => { + it('SetValues emits with ', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'SetValues Test', + steps: [ + { + api_id: 'SetValues', + name: 'Set test vars', + attributes: { testCaseName: 'TC_New', testType: 'Acceptance testing' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="valueList"'), 'Expected class="valueList"'); + assert.ok(xml.includes('mutable="Mutable"'), 'Expected mutable="Mutable"'); + assert.ok(xml.includes(''), 'Expected element'); + assert.ok(xml.includes(''), 'Expected namedValue for testCaseName'); + assert.ok(xml.includes(''), 'Expected namedValue for testType'); + assert.ok(xml.includes(''), 'Expected argument id="values"'); + assert.ok( + !xml.includes('testCaseName|TC_New'), + 'Must NOT emit pipe-delimited string for SetValues' + ); + }); + + it('AssertValues also emits valueList/namedValues structure', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'AssertValues Test', + steps: [ + { + api_id: 'AssertValues', + name: 'Assert vars', + attributes: { opportunityName: 'My Opp' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="valueList"'), 'Expected valueList for AssertValues'); + assert.ok(xml.includes(''), 'Expected namedValue for opportunityName'); + }); + + it('non-SetValues steps still use flat argument structure', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'Flat Args Test', + steps: [ + { api_id: 'ApexCreateObject', name: 'Create record', attributes: { objectApiName: 'Opportunity' } }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes(''), 'Expected flat argument id'); + assert.ok(!xml.includes('valueList'), 'Must NOT emit valueList for non-SetValues steps'); + }); + }); + + describe('D4 — Variable references use class="variable" with elements', () => { + it('{VarName} emits class="variable" ', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'Variable Ref Test', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create record', + attributes: { provar__Test_Project__c: '{TestProjectId}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="variable"'), 'Expected class="variable"'); + assert.ok(xml.includes(''), 'Expected '); + assert.ok(!xml.includes('valueClass="string">{TestProjectId}'), 'Must NOT emit {VarName} as a string literal'); + }); + + it('{Obj.Field} dotted path emits two elements', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'Dotted Variable Test', + steps: [ + { + api_id: 'ApexCreateObject', + name: 'Create with nested var', + attributes: { Name: '{Opportunity.Name}' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes(''), 'Expected first path element'); + assert.ok(xml.includes(''), 'Expected second path element'); + }); + + it('variable reference also works inside SetValues namedValues', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'SetValues With Var', + steps: [ + { + api_id: 'SetValues', + name: 'Set with variable', + attributes: { projectId: '{TestProjectId}', label: 'Static Label' }, + }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('class="variable"'), 'Expected variable reference inside namedValues'); + assert.ok(xml.includes('')); + assert.ok(xml.includes('valueClass="string">Static Label'), 'Static value should still be a plain string'); + }); + + it('plain string values without braces are not treated as variable references', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'No Var Test', + steps: [{ api_id: 'ApexCreateObject', name: 'Create', attributes: { Name: 'Literal Name' } }], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + const xml = parseText(result)['xml_content'] as string; + assert.ok(xml.includes('valueClass="string">Literal Name'), 'Plain string should use valueClass="string"'); + assert.ok(!xml.includes('class="variable"'), 'No variable element expected'); + }); + }); + + describe('D7 — Cleanup warning for ApexDeleteObject', () => { + it('includes cleanup warning when ApexDeleteObject is in the step list', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'Create and Delete', + steps: [ + { api_id: 'ApexCreateObject', name: 'Create record', attributes: { objectApiName: 'Account' } }, + { api_id: 'ApexDeleteObject', name: 'Delete record', attributes: { objectApiName: 'Account' } }, + ], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const warnings = body['warnings'] as string[] | undefined; + assert.ok(Array.isArray(warnings) && warnings.length > 0, 'Expected at least one warning'); + assert.ok( + warnings.some((w) => w.includes('ApexDeleteObject') && w.includes('cleanup')), + 'Expected cleanup warning mentioning ApexDeleteObject' + ); + }); + + it('does NOT warn when no ApexDeleteObject steps are present', () => { + const result = server.call('provar.testcase.generate', { + test_case_name: 'No Cleanup', + steps: [{ api_id: 'ApexCreateObject', name: 'Create', attributes: {} }], + dry_run: true, + overwrite: false, + validate_after_edit: false, + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const warnings = body['warnings'] as string[] | undefined; + const hasCleanupWarning = warnings?.some((w) => w.includes('ApexDeleteObject')); + assert.ok(!hasCleanupWarning, 'No cleanup warning expected without ApexDeleteObject'); + }); + }); + describe('validate_after_edit', () => { it('includes validation field when validate_after_edit=true (default)', () => { const result = server.call('provar.testcase.generate', { diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index e3d7184..28f299f 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -96,6 +96,135 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); +// ── provar.testplan.create ──────────────────────────────────────────────────── + +describe('provar.testplan.create', () => { + describe('happy path', () => { + it('creates plan directory and .planitem, returns expected fields', () => { + makeProject(projectDir); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'MyNewPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['created'], true); + assert.equal(body['dry_run'], false); + assert.ok(typeof body['guid'] === 'string' && body['guid'].length > 0, 'Expected guid'); + + const planDir = path.join(projectDir, 'plans', 'MyNewPlan'); + assert.ok(fs.existsSync(planDir), 'Plan directory should be created'); + assert.ok(fs.existsSync(path.join(planDir, '.planitem')), '.planitem should be written'); + + const xml = fs.readFileSync(path.join(planDir, '.planitem'), 'utf-8'); + assert.ok(xml.includes(' { + makeProject(projectDir); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'GuidedPlan', + overwrite: false, + dry_run: false, + }); + + const body = parseText(result); + assert.ok(typeof body['next_steps'] === 'string' && body['next_steps'].length > 0, 'Expected next_steps'); + }); + }); + + describe('dry_run', () => { + it('returns created=false and does not write to disk', () => { + makeProject(projectDir); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'DryPlan', + overwrite: false, + dry_run: true, + }); + + assert.equal(isError(result), false); + const body = parseText(result); + assert.equal(body['created'], false); + assert.equal(body['dry_run'], true); + assert.ok(typeof body['guid'] === 'string', 'Expected guid even in dry_run'); + + const planDir = path.join(projectDir, 'plans', 'DryPlan'); + assert.equal(fs.existsSync(planDir), false, 'Directory must not be created in dry_run mode'); + }); + }); + + describe('error cases', () => { + it('returns NOT_A_PROJECT when .testproject is missing', () => { + fs.mkdirSync(projectDir, { recursive: true }); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'MyPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + assert.equal(errorCode(result), 'NOT_A_PROJECT'); + }); + + it('returns PLAN_EXISTS when .planitem already exists and overwrite=false', () => { + makeProject(projectDir); + makePlan(projectDir, 'ExistingPlan'); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'ExistingPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + assert.equal(errorCode(result), 'PLAN_EXISTS'); + }); + + it('overwrites .planitem when overwrite=true and plan already exists', () => { + makeProject(projectDir); + makePlan(projectDir, 'ExistingPlan'); + + const result = server.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'ExistingPlan', + overwrite: true, + dry_run: false, + }); + + assert.equal(isError(result), false); + assert.equal(parseText(result)['created'], true); + }); + + it('returns PATH_NOT_ALLOWED when project_path is outside allowedPaths', () => { + const strictServer = new MockMcpServer(); + registerAllTestPlanTools(strictServer as never, { allowedPaths: [tmpDir] }); + + const result = strictServer.call('provar.testplan.create', { + project_path: path.join(os.tmpdir(), 'outside-project'), + plan_name: 'MyPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + const code = errorCode(result); + assert.ok(code === 'PATH_NOT_ALLOWED' || code === 'PATH_TRAVERSAL', `Unexpected error code: ${code}`); + }); + }); +}); + // ── provar.testplan.add-instance ─────────────────────────────────────────────── describe('provar.testplan.add-instance', () => { @@ -585,14 +714,20 @@ describe('provar.testplan.remove-instance', () => { // ── registerAllTestPlanTools ─────────────────────────────────────────────────── describe('registerAllTestPlanTools', () => { - it('registers all three tools', () => { + it('registers all four tools', () => { const freshServer = new MockMcpServer(); registerAllTestPlanTools(freshServer as never, config); - // Each tool should be callable without throwing "Tool not registered" makeProject(projectDir); - // Verify each tool is registered by checking it doesn't throw + assert.doesNotThrow(() => { + freshServer.call('provar.testplan.create', { + project_path: projectDir, + plan_name: 'P', + overwrite: false, + dry_run: true, + }); + }); assert.doesNotThrow(() => { freshServer.call('provar.testplan.add-instance', { project_path: projectDir, From d6d4da60a90840d94e95d83f6df6695d755e4e3c Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 12:43:02 -0500 Subject: [PATCH 2/7] PDX-0: feat(validate): add UI-TARGET-001, UI-LOCATOR-001, SETVALUES-STRUCTURE-001, VAR-REF-001 rules Addresses Session 3 feedback D2 (uiTarget class), D3 (SetValues valueList structure), and D4 (variable reference detection). Mirrors quality-hub-agents backend rule IDs where they exist; VAR-REF-001 fills a gap present in both local and backend validation. --- src/mcp/tools/testCaseValidate.ts | 128 ++++++++-- test/unit/mcp/testCaseValidate.test.ts | 341 +++++++++++++++++++++++++ 2 files changed, 451 insertions(+), 18 deletions(-) diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index 607c9d3..f872b0a 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -312,9 +312,33 @@ export function validateTestCase(xmlContent: string, testName?: string): TestCas validateApiCall(call, issues); } + // VAR-REF-001 (gap in both local and quality-hub-agents backend): + // Detect {VarName} or {Obj.Field} literals stored as plain string values. + // Provar will pass these as-is to the API rather than resolving them as variable references. + const varLiteralRe = /]+valueClass="string"[^>]*>\{([\w.]+)\}<\/value>/g; + let varMatch: RegExpExecArray | null; + while ((varMatch = varLiteralRe.exec(xmlContent)) !== null) { + issues.push({ + rule_id: 'VAR-REF-001', + severity: 'WARNING', + message: `Argument value "{${varMatch[1]}}" looks like a variable reference but is stored as a plain string — Provar will not resolve it at runtime.`, + applies_to: 'argument', + suggestion: `Replace with . In provar.testcase.generate, use the {VarName} syntax in the attributes object — the generator converts it automatically.`, + }); + } + return finalize(issues, tcId, tcName, apiCalls.length, xmlContent, testName); } +/** Normalise fast-xml-parser's single-or-array representation of children. */ +function getArgList(call: Record): Array> { + const rawArgs = call['arguments'] as Record | undefined; + if (!rawArgs) return []; + const argRaw = rawArgs['argument']; + if (!argRaw) return []; + return (Array.isArray(argRaw) ? argRaw : [argRaw]) as Array>; +} + function validateApiCall(call: Record, issues: ValidationIssue[]): void { const callGuid = call['@_guid'] as string | undefined; const apiId = call['@_apiId'] as string | undefined; @@ -376,31 +400,99 @@ function validateApiCall(call: Record, issues: ValidationIssue[ }); } - // ASSERT-001: AssertValues using UI namedValues format instead of variable format - if (apiId?.includes('AssertValues')) { - const rawArgs = call['arguments'] as Record | undefined; - if (rawArgs) { - const argRaw = rawArgs['argument']; - const argList: Array> = !argRaw - ? [] - : Array.isArray(argRaw) - ? (argRaw as Array>) - : [argRaw as Record]; - const hasValuesArg = argList.some((a) => (a['@_id'] as string | undefined) === 'values'); - if (hasValuesArg) { + if (apiId) validateApiCallArgs(call, apiId, name, issues); +} + +function checkUiTarget(call: Record, apiId: string, stepName: string, issues: ValidationIssue[]): void { + const targetArg = getArgList(call).find((a) => (a['@_id'] as string | undefined) === 'target'); + if (!targetArg) return; + const valClass = (targetArg['value'] as Record | undefined)?.['@_class'] as string | undefined; + if (valClass && valClass !== 'uiTarget') { + const apiLabel = apiId.includes('UiWithRow') ? 'UiWithRow' : 'UiWithScreen'; + issues.push({ + rule_id: 'UI-TARGET-001', + severity: 'ERROR', + message: `${apiLabel} step "${stepName}" target argument uses class="${valClass}" — must be class="uiTarget".`, + applies_to: 'apiCall', + suggestion: + 'Emit the target as: or uri="ui:pageobject:target?pageId=...". ' + + 'In provar.testcase.generate the "target" attribute is converted automatically.', + }); + } +} + +function validateApiCallArgs( + call: Record, + apiId: string, + name: string | undefined, + issues: ValidationIssue[] +): void { + const stepName = name ?? '(unnamed)'; + + // UI-TARGET-001 (mirrors quality-hub-agents UI-SCREEN-TARGET-001): + // UiWithScreen / UiWithRow target argument must use class="uiTarget", not a plain string. + // A plain string causes: "Can not set IUiTargetValue field ... to java.lang.String" + if (apiId.includes('UiWithScreen') || apiId.includes('UiWithRow')) { + checkUiTarget(call, apiId, stepName, issues); + } + + // UI-LOCATOR-001 (local rule, no direct backend equivalent): + // UiDoAction / UiAssert / UiScrollToElement locator argument must use class="uiLocator". + if (apiId.includes('UiDoAction') || apiId.includes('UiAssert') || apiId.includes('UiScrollToElement')) { + const locatorArg = getArgList(call).find((a) => (a['@_id'] as string | undefined) === 'locator'); + if (locatorArg) { + const valClass = (locatorArg['value'] as Record | undefined)?.['@_class'] as string | undefined; + if (valClass && valClass !== 'uiLocator') { + issues.push({ + rule_id: 'UI-LOCATOR-001', + severity: 'ERROR', + message: `"${stepName}" locator argument uses class="${valClass}" — must be class="uiLocator".`, + applies_to: 'apiCall', + suggestion: + 'Emit the locator as: . ' + + 'In provar.testcase.generate the "locator" attribute is converted automatically.', + }); + } + } + } + + // SETVALUES-STRUCTURE-001 (mirrors quality-hub-agents SETVALUES-STRUCTURE-001): + // SetValues values argument must use class="valueList" with children. + // A plain string value causes an immediate ClassCastException at runtime. + if (apiId.includes('SetValues') && !apiId.includes('AssertValues')) { + const valuesArg = getArgList(call).find((a) => (a['@_id'] as string | undefined) === 'values'); + if (valuesArg) { + const valClass = (valuesArg['value'] as Record | undefined)?.['@_class'] as string | undefined; + if (valClass && valClass !== 'valueList') { issues.push({ - rule_id: 'ASSERT-001', - severity: 'WARNING', - message: `AssertValues step "${ - name ?? '(unnamed)' - }" uses namedValues format (argument id="values") — designed for UI element attribute assertions. For Apex/SOQL result or variable comparisons this silently passes as null=null.`, + rule_id: 'SETVALUES-STRUCTURE-001', + severity: 'ERROR', + message: `SetValues step "${stepName}" values argument uses class="${valClass}" — must use class="valueList" with children.`, applies_to: 'apiCall', suggestion: - 'Use separate expectedValue, actualValue, and comparisonType arguments for variable or Apex result comparisons.', + 'Wrap variable assignments in: ' + + 'value' + + '. In provar.testcase.generate pass each variable as a flat key/value pair ' + + 'in attributes — the generator builds the valueList structure automatically.', }); } } } + + // ASSERT-001: AssertValues using UI namedValues format instead of variable format + if (apiId.includes('AssertValues')) { + const hasValuesArg = getArgList(call).some((a) => (a['@_id'] as string | undefined) === 'values'); + if (hasValuesArg) { + issues.push({ + rule_id: 'ASSERT-001', + severity: 'WARNING', + message: `AssertValues step "${stepName}" uses namedValues format (argument id="values") — designed for UI element attribute assertions. For Apex/SOQL result or variable comparisons this silently passes as null=null.`, + applies_to: 'apiCall', + suggestion: + 'Use separate expectedValue, actualValue, and comparisonType arguments for variable or Apex result comparisons.', + }); + } + } } function finalize( diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index 628ce59..456fc09 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -293,6 +293,347 @@ describe('validateTestCase', () => { assert.ok(!r.issues.some((i) => i.rule_id === 'ASSERT-001'), 'ASSERT-001 should not fire for SetValues'); }); }); + + describe('UI-TARGET-001', () => { + it('errors when UiWithScreen target argument uses class="value" (plain string)', () => { + const r = validateTestCase( + ` + + + + + + sf:ui:target?object=Account + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'UI-TARGET-001'), + 'Expected UI-TARGET-001' + ); + const issue = r.issues.find((i) => i.rule_id === 'UI-TARGET-001')!; + assert.equal(issue.severity, 'ERROR'); + assert.ok(issue.message.includes('uiTarget'), `Message should mention uiTarget: ${issue.message}`); + }); + + it('does not fire when UiWithScreen target uses class="uiTarget"', () => { + const r = validateTestCase( + ` + + + + + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'UI-TARGET-001'), + 'UI-TARGET-001 should not fire for correct uiTarget class' + ); + }); + + it('does not fire when UiWithScreen has no target argument', () => { + const r = validateTestCase( + ` + + + + + 1280 + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'UI-TARGET-001'), + 'UI-TARGET-001 should not fire when no target argument present' + ); + }); + + it('also fires for UiWithRow steps with wrong target class', () => { + const r = validateTestCase( + ` + + + + + + sf:ui:target?object=Account + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'UI-TARGET-001'), + 'Expected UI-TARGET-001 for UiWithRow' + ); + }); + }); + + describe('UI-LOCATOR-001', () => { + it('errors when UiDoAction locator argument uses class="value" (plain string)', () => { + const r = validateTestCase( + ` + + + + + + sf:ui:locator:label?label=Save + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'UI-LOCATOR-001'), + 'Expected UI-LOCATOR-001' + ); + const issue = r.issues.find((i) => i.rule_id === 'UI-LOCATOR-001')!; + assert.equal(issue.severity, 'ERROR'); + assert.ok(issue.message.includes('uiLocator'), `Message should mention uiLocator: ${issue.message}`); + }); + + it('does not fire when UiDoAction locator uses class="uiLocator"', () => { + const r = validateTestCase( + ` + + + + + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'UI-LOCATOR-001'), + 'UI-LOCATOR-001 should not fire for correct uiLocator class' + ); + }); + + it('also fires for UiAssert steps with wrong locator class', () => { + const r = validateTestCase( + ` + + + + + + sf:ui:locator:label?label=Name + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'UI-LOCATOR-001'), + 'Expected UI-LOCATOR-001 for UiAssert' + ); + }); + }); + + describe('SETVALUES-STRUCTURE-001', () => { + it('errors when SetValues values argument uses class="value" (plain string)', () => { + const r = validateTestCase( + ` + + + + + + myVar=hello + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'SETVALUES-STRUCTURE-001'), + 'Expected SETVALUES-STRUCTURE-001' + ); + const issue = r.issues.find((i) => i.rule_id === 'SETVALUES-STRUCTURE-001')!; + assert.equal(issue.severity, 'ERROR'); + assert.ok(issue.message.includes('valueList'), `Message should mention valueList: ${issue.message}`); + }); + + it('does not fire when SetValues values argument uses class="valueList"', () => { + const r = validateTestCase( + ` + + + + + + + + + hello + + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'SETVALUES-STRUCTURE-001'), + 'SETVALUES-STRUCTURE-001 should not fire for correct valueList structure' + ); + }); + + it('does not fire when SetValues has no values argument (self-closing)', () => { + const r = validateTestCase( + ` + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'SETVALUES-STRUCTURE-001'), + 'SETVALUES-STRUCTURE-001 should not fire when no values argument present' + ); + }); + + it('does not fire for AssertValues (only targets SetValues)', () => { + const r = validateTestCase( + ` + + + + + + myVar=hello + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'SETVALUES-STRUCTURE-001'), + 'SETVALUES-STRUCTURE-001 should not fire for AssertValues' + ); + }); + }); + + describe('VAR-REF-001', () => { + it('warns when a plain string value contains a {VarName} pattern', () => { + const r = validateTestCase( + ` + + + + + + {AccountId} + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'Expected VAR-REF-001' + ); + const issue = r.issues.find((i) => i.rule_id === 'VAR-REF-001')!; + assert.equal(issue.severity, 'WARNING'); + assert.ok(issue.message.includes('{AccountId}'), `Message should include variable name: ${issue.message}`); + }); + + it('warns for dotted path {Obj.Field} stored as plain string', () => { + const r = validateTestCase( + ` + + + + + + {Record.Name} + + + + +` + ); + assert.ok( + r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'Expected VAR-REF-001 for dotted path' + ); + assert.ok( + r.issues.find((i) => i.rule_id === 'VAR-REF-001')!.message.includes('{Record.Name}'), + 'Message should include dotted variable name' + ); + }); + + it('does not fire when value uses class="variable" (correct structure)', () => { + const r = validateTestCase( + ` + + + + + + + + + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'VAR-REF-001 should not fire for correct class="variable" structure' + ); + }); + + it('does not fire for plain string content without curly braces', () => { + const r = validateTestCase( + ` + + + + + + John Smith + + + + +` + ); + assert.ok( + !r.issues.some((i) => i.rule_id === 'VAR-REF-001'), + 'VAR-REF-001 should not fire for plain string without curly braces' + ); + }); + }); }); // ── Handler-level tests (registerTestCaseValidate) ──────────────────────────── From 849c6b979508d63582a38923d985989a879c588b Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 14:10:57 -0500 Subject: [PATCH 3/7] =?UTF-8?q?PDX-0:=20fix:=20pre-landing=20review=20fixe?= =?UTF-8?q?s=20(D1=E2=80=93D3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1: add assertPathAllowed(planDir) in testPlanTools create-plan path D2: skip uiTarget/uiLocator dispatch inside SetValues namedValues context D3: remove AssertValues from SET_VALUES_APIS; update test to expect flat args Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/tools/testCaseGenerate.ts | 28 ++++++++++++++------------ src/mcp/tools/testPlanTools.ts | 1 + test/unit/mcp/testCaseGenerate.test.ts | 7 ++++--- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index 3badee2..e4fc75c 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -266,12 +266,12 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // APIs whose 'values' argument must use class="valueList"/ (D3). const SET_VALUES_APIS = new Set([ - SHORTHAND_TO_FQID['SetValues'], // com.provar.plugins.bundled.apis.control.SetValues - SHORTHAND_TO_FQID['AssertValues'], // com.provar.plugins.bundled.apis.AssertValues + SHORTHAND_TO_FQID['SetValues'] ?? '', // com.provar.plugins.bundled.apis.control.SetValues ]); // Build the element for a single argument (D2/D4 aware). -function buildArgumentValue(key: string, val: string, indent: string): string { +// inNamedValues: when true (inside SetValues namedValues), skip uiTarget/uiLocator dispatch. +function buildArgumentValue(key: string, val: string, indent: string, inNamedValues = false): string { // D4: {VarName} or {Obj.Field} → class="variable" with elements. const varMatch = /^\{([\w.]+)\}$/.exec(val); if (varMatch) { @@ -281,13 +281,15 @@ function buildArgumentValue(key: string, val: string, indent: string): string { .join('\n'); return `${indent}\n${pathElements}\n${indent}`; } - // D2: 'target' argument (UiWithScreen / UiWithRow) → class="uiTarget". - if (key === 'target') { - return `${indent}`; - } - // D2: 'locator' argument (UiDoAction / UiAssert) → class="uiLocator". - if (key === 'locator') { - return `${indent}`; + if (!inNamedValues) { + // D2: 'target' argument (UiWithScreen / UiWithRow) → class="uiTarget". + if (key === 'target') { + return `${indent}`; + } + // D2: 'locator' argument (UiDoAction / UiAssert) → class="uiLocator". + if (key === 'locator') { + return `${indent}`; + } } return `${indent}${escapeXmlContent(val)}`; } @@ -308,14 +310,14 @@ function buildArgumentsXml(attributes: Record, baseIndent = ' return `\n${baseIndent}\n${argLines}\n${baseIndent}\n${baseIndent.slice(0, -2)}`; } -// D3: SetValues / AssertValues — all attributes become under a single 'values' argument. +// D3: SetValues — all attributes become under a single 'values' argument. function buildSetValuesXml(attributes: Record, baseIndent: string): string { const entries = Object.entries(attributes); if (entries.length === 0) return ''; const i = (n: number): string => baseIndent + ' '.repeat(n); const namedValueLines = entries .map(([name, val]) => { - const valueXml = buildArgumentValue(name, val, `${i(3)} `); + const valueXml = buildArgumentValue(name, val, `${i(3)} `, true); return `${i(3)}\n${valueXml}\n${i(3)}`; }) .join('\n'); @@ -328,7 +330,7 @@ function buildSetValuesXml(attributes: Record, baseIndent: strin `${i(2)}\n` + `${i(1)}\n` + `${i(0)}\n` + - `${baseIndent.slice(0, -2)}\n` + + `${i(0)}\n` + `${baseIndent.slice(0, -2)}` ); } diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index ac16cd6..d1a6519 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -106,6 +106,7 @@ export function registerTestPlanCreate(server: McpServer, config: ServerConfig): } const planDir = path.join(projectRoot, 'plans', plan_name); + assertPathAllowed(planDir, config.allowedPaths); const planItemPath = path.join(planDir, '.planitem'); if (!overwrite && fs.existsSync(planItemPath)) { diff --git a/test/unit/mcp/testCaseGenerate.test.ts b/test/unit/mcp/testCaseGenerate.test.ts index da31040..e3729be 100644 --- a/test/unit/mcp/testCaseGenerate.test.ts +++ b/test/unit/mcp/testCaseGenerate.test.ts @@ -548,7 +548,7 @@ describe('provar.testcase.generate', () => { ); }); - it('AssertValues also emits valueList/namedValues structure', () => { + it('AssertValues uses flat argument structure (not valueList)', () => { const result = server.call('provar.testcase.generate', { test_case_name: 'AssertValues Test', steps: [ @@ -564,8 +564,9 @@ describe('provar.testcase.generate', () => { }); const xml = parseText(result)['xml_content'] as string; - assert.ok(xml.includes('class="valueList"'), 'Expected valueList for AssertValues'); - assert.ok(xml.includes(''), 'Expected namedValue for opportunityName'); + assert.ok(xml.includes(''), 'Expected flat argument id for AssertValues'); + assert.ok(!xml.includes('class="valueList"'), 'AssertValues must NOT emit valueList structure'); + assert.ok(!xml.includes(' { From c6ba663b0aa9a813449ec2796c2428f65ae20b7d Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 14:12:07 -0500 Subject: [PATCH 4/7] PDX-0: chore: bump to 1.5.0-beta.12; update docs for Wave 3 validator rules Add docs for UI-TARGET-001, UI-LOCATOR-001, SETVALUES-STRUCTURE-001, VAR-REF-001 Document generator XML argument conventions (uiTarget/uiLocator/variable/valueList) Co-Authored-By: Claude Sonnet 4.6 --- docs/mcp.md | 16 ++++++++++++++++ package.json | 2 +- server.json | 4 ++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index fa3dce8..3366c61 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -637,6 +637,18 @@ Generates an XML test case skeleton with UUID v4 guids and sequential `testItemI - Omit or use a `sf:` URI → flat Salesforce step structure (existing behaviour) - `ui:pageobject:target?pageId=pageobjects.Page` → wraps all steps in a `UiWithScreen` element (testItemId=1); substeps clause at testItemId=2; inner steps start at testItemId=3 +**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 | + +AssertValues uses **flat** argument structure (`expectedValue`, `actualValue`, `comparisonType`) — not the `valueList`/namedValues format. + **Input** | Parameter | Type | Required | Description | @@ -700,6 +712,10 @@ Validates an XML test case for schema correctness (validity score) and best prac - **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. - **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. +- **SETVALUES-STRUCTURE-001** (ERROR) — A `SetValues` step's `values` argument uses `class="value"` (plain string) instead of `class="valueList"` with `` children. This causes an immediate `ClassCastException` at runtime. +- **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `` elements. --- diff --git a/package.json b/package.json index 8c47f57..8f45989 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@provartesting/provardx-cli", "description": "A plugin for the Salesforce CLI to orchestrate testing activities and report quality metrics to Provar Quality Hub", - "version": "1.5.0-beta.11", + "version": "1.5.0-beta.12", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 781cc2a..c23758f 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.5.0-beta.11", + "version": "1.5.0-beta.12", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.5.0-beta.11", + "version": "1.5.0-beta.12", "transport": { "type": "stdio" }, From 7f7ea1cf035d5c4e87fc525c56ceb0e025f1be38 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 14:12:52 -0500 Subject: [PATCH 5/7] PDX-0: chore: fix duplicate comment numbers in mcp-smoke.cjs Co-Authored-By: Claude Sonnet 4.6 --- scripts/mcp-smoke.cjs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/mcp-smoke.cjs b/scripts/mcp-smoke.cjs index 68e52c5..39bdf46 100644 --- a/scripts/mcp-smoke.cjs +++ b/scripts/mcp-smoke.cjs @@ -255,7 +255,7 @@ async function runTests() { plan_name: 'SmokePlan', }); - // ── 33. provar.testplan.add-instance ───────────────────────────────────── + // ── 32. provar.testplan.add-instance ───────────────────────────────────── // TMP is not a Provar project → NOT_A_PROJECT result await callTool('provar.testplan.add-instance', { project_path: TMP, @@ -263,24 +263,24 @@ async function runTests() { plan_name: 'SmokePlan', }); - // ── 34. provar.testplan.create-suite ───────────────────────────────────── + // ── 33. provar.testplan.create-suite ───────────────────────────────────── await callTool('provar.testplan.create-suite', { project_path: TMP, plan_name: 'SmokePlan', suite_name: 'SmokeSuite', }); - // ── 35. provar.testplan.remove-instance ────────────────────────────────── + // ── 34. provar.testplan.remove-instance ────────────────────────────────── await callTool('provar.testplan.remove-instance', { project_path: TMP, instance_path: 'plans/SmokePlan/SmokeSuite/smoke.testinstance', }); - // ── 34. provar.nitrox.discover ──────────────────────────────────────────── + // ── 35. provar.nitrox.discover ──────────────────────────────────────────── // TMP has no .testproject → empty projects list, no crash await callTool('provar.nitrox.discover', { search_roots: [TMP] }); - // ── 35. provar.nitrox.validate ──────────────────────────────────────────── + // ── 36. provar.nitrox.validate ──────────────────────────────────────────── // Minimal valid root component → score 100 await callTool('provar.nitrox.validate', { content: JSON.stringify({ @@ -383,7 +383,7 @@ async function runTests() { // ---------------------------------------------------------------------------- server.on('close', () => { clearTimeout(overallTimer); - // initialize + tools/list + 39 tools + prompts/list + 8 prompts/get (setup excluded from default count) + // initialize + tools/list + 40 tools + prompts/list + 8 prompts/get (setup excluded from default count) const TOTAL_EXPECTED = 51 + (INCLUDE_SETUP ? 1 : 0); let passed = 0; let failed = 0; From c9fa3fe72b3f490024123882150a741b81b0fc28 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 14:57:56 -0500 Subject: [PATCH 6/7] PDX-0: fix: address Copilot PR review comments - Separate SetValues/AssertValues in StepSchema and TOOL_DESCRIPTION - Replace SET_VALUES_APIS Set with resolvedApiId.includes('SetValues') (mirrors validator) - Make target/locator dispatch API-aware (pass resolvedApiId through buildArgumentsXml) - Fix buildUiWithScreenXml to pass wrapperApiId so target emits uiTarget class - Validate plan_name has no path separators to prevent path.join injection - Add UiScrollToElement to UI-LOCATOR-001 docs and argument convention table Co-Authored-By: Claude Sonnet 4.6 --- docs/mcp.md | 4 +-- src/mcp/tools/testCaseGenerate.ts | 45 ++++++++++++++++--------------- src/mcp/tools/testPlanTools.ts | 11 ++++++++ 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 3366c61..5056eeb 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -642,7 +642,7 @@ Generates an XML test case skeleton with UUID v4 guids and sequential `testItemI | Argument key / value pattern | Emitted XML class | API context | | ------------------------------------ | ----------------------------- | ----------------------------------- | | `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | -| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert | +| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert, UiScrollToElement | | 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 | @@ -713,7 +713,7 @@ Validates an XML test case for schema correctness (validity score) and best prac - **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. - **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. +- **UI-LOCATOR-001** — A UiDoAction, UiAssert, or UiScrollToElement `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element. - **SETVALUES-STRUCTURE-001** (ERROR) — A `SetValues` step's `values` argument uses `class="value"` (plain string) instead of `class="valueList"` with `` children. This causes an immediate `ClassCastException` at runtime. - **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `` elements. diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index e4fc75c..dd9c256 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -105,12 +105,13 @@ const StepSchema = z.object({ 'Special value conventions (applied automatically by the generator): ' + '(1) Variable references: wrap the name in braces, e.g. "{MyVar}" → emitted as class="variable" . ' + ' Dotted paths are also supported: "{Obj.Field}" → two elements. ' + - '(2) SetValues / AssertValues: pass each variable name and its value as a flat key/value pair; ' + + '(2) SetValues: pass each variable name and its value as a flat key/value pair; ' + ' the generator wraps them in ... automatically. ' + - ' Example for SetValues: { "testCaseName": "TC_New", "testType": "Acceptance testing" } ' + - '(3) target argument (UiWithScreen / UiWithRow): pass the sf:ui:target or ui:pageobject:target URI; ' + + ' Example: { "testCaseName": "TC_New", "testType": "Acceptance testing" } ' + + '(3) AssertValues: pass assertion arguments as flat key/value pairs; emitted as flat elements, NOT wrapped in valueList/namedValues. ' + + '(4) target argument (UiWithScreen / UiWithRow): pass the sf:ui:target or ui:pageobject:target URI; ' + ' emitted as class="uiTarget" uri="...". ' + - '(4) locator argument (UiDoAction / UiAssert): pass the locator URI; emitted as class="uiLocator" uri="...". ' + + '(5) locator argument (UiDoAction / UiAssert / UiScrollToElement): pass the locator URI; emitted as class="uiLocator" uri="...". ' + 'All other string values use class="value" valueClass="string".' ), }); @@ -130,11 +131,13 @@ const TOOL_DESCRIPTION = [ 'ApexReadObject requires field names in attributes; omitting them produces MALFORMED_QUERY. Prefer ApexSoqlQuery.', 'AssertValues on SOQL results: index paths like "ResultList[0].Field" are not supported.', 'Use ForEach to iterate the result list, or SetValues to extract a field into a variable first.', - 'SetValues / AssertValues: pass named variable values as flat key/value pairs in attributes; ' + + 'SetValues: pass named variable values as flat key/value pairs in attributes; ' + 'the generator wraps them in ... automatically.', + 'AssertValues: pass assertion values as flat key/value argument pairs; emitted as flat arguments, NOT wrapped in namedValues. ' + + 'If AssertValues uses namedValues-shaped content, validation reports warning ASSERT-001.', '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="...".', + 'locator argument (UiDoAction/UiAssert/UiScrollToElement): pass the URI value; emitted as class="uiLocator" uri="...".', 'Cleanup warning: ApexDeleteObject steps near end of test will be skipped if an earlier step fails (stopOnError=false). Use a TearDown callable.', 'Validation: when validate_after_edit=true (default) the response includes a validation field and returns TESTCASE_INVALID if the generated XML fails structural checks.', 'Grounding: call provar.qualityhub.examples.retrieve before generating to get corpus examples for the scenario — correct XML structure for the step types you need.', @@ -264,14 +267,10 @@ export function registerTestCaseGenerate(server: McpServer, config: ServerConfig // ── XML builder ─────────────────────────────────────────────────────────────── -// APIs whose 'values' argument must use class="valueList"/ (D3). -const SET_VALUES_APIS = new Set([ - SHORTHAND_TO_FQID['SetValues'] ?? '', // com.provar.plugins.bundled.apis.control.SetValues -]); - // Build the element for a single argument (D2/D4 aware). // inNamedValues: when true (inside SetValues namedValues), skip uiTarget/uiLocator dispatch. -function buildArgumentValue(key: string, val: string, indent: string, inNamedValues = false): string { +// apiId: resolved API ID used to restrict key-name dispatch to the correct UI APIs. +function buildArgumentValue(key: string, val: string, indent: string, inNamedValues = false, apiId = ''): string { // D4: {VarName} or {Obj.Field} → class="variable" with elements. const varMatch = /^\{([\w.]+)\}$/.exec(val); if (varMatch) { @@ -282,24 +281,27 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal return `${indent}\n${pathElements}\n${indent}`; } if (!inNamedValues) { - // D2: 'target' argument (UiWithScreen / UiWithRow) → class="uiTarget". - if (key === 'target') { + // D2: 'target' argument → class="uiTarget" (only for UiWithScreen / UiWithRow). + if (key === 'target' && (apiId.includes('UiWithScreen') || apiId.includes('UiWithRow'))) { return `${indent}`; } - // D2: 'locator' argument (UiDoAction / UiAssert) → class="uiLocator". - if (key === 'locator') { + // D2: 'locator' argument → class="uiLocator" (only for UiDoAction / UiAssert / UiScrollToElement). + if ( + key === 'locator' && + (apiId.includes('UiDoAction') || apiId.includes('UiAssert') || apiId.includes('UiScrollToElement')) + ) { return `${indent}`; } } return `${indent}${escapeXmlContent(val)}`; } -function buildArgumentsXml(attributes: Record, baseIndent = ' '): string { +function buildArgumentsXml(attributes: Record, baseIndent = ' ', apiId = ''): string { const entries = Object.entries(attributes); if (entries.length === 0) return ''; const argLines = entries .map(([k, v]) => { - const valueXml = buildArgumentValue(k, v, `${baseIndent} `); + const valueXml = buildArgumentValue(k, v, `${baseIndent} `, false, apiId); return ( `${baseIndent}\n` + valueXml + '\n' + @@ -343,9 +345,10 @@ function buildFlatStepXml( const guid = randomUUID(); const resolvedApiId = resolveApiId(step.api_id); const baseIndent = indent + ' '; - const argumentsXml = SET_VALUES_APIS.has(resolvedApiId) + // Use SetValues structure for any SetValues API (string-match mirrors the validator). + const argumentsXml = resolvedApiId.includes('SetValues') ? buildSetValuesXml(step.attributes, baseIndent) - : buildArgumentsXml(step.attributes, baseIndent); + : buildArgumentsXml(step.attributes, baseIndent, resolvedApiId); if (argumentsXml) { return ( `${indent}\n '; return ( ` ${buildArgumentsXml({ target: targetUri }).trimEnd()}${clausesXml}` + ` name="With page" testItemId="1">${buildArgumentsXml({ target: targetUri }, ' ', wrapperApiId).trimEnd()}${clausesXml}` ); } diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index d1a6519..716c402 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -105,6 +105,17 @@ export function registerTestPlanCreate(server: McpServer, config: ServerConfig): }; } + if (plan_name.includes('/') || plan_name.includes('\\') || path.isAbsolute(plan_name)) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify(makeError('INVALID_PLAN_NAME', `plan_name must be a simple directory name without path separators: "${plan_name}"`, requestId)), + }, + ], + }; + } const planDir = path.join(projectRoot, 'plans', plan_name); assertPathAllowed(planDir, config.allowedPaths); const planItemPath = path.join(planDir, '.planitem'); From 7d148f3c504e400a006937d790ba19b7e13d8867 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Fri, 1 May 2026 15:32:59 -0500 Subject: [PATCH 7/7] PDX-0: fix: remove UiScrollToElement from UI-LOCATOR-001 (no corpus evidence) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UiScrollToElement has zero usage in the internal test corpus and no documented locator argument. The inclusion was speculative — reverting to UiDoAction/UiAssert only. Co-Authored-By: Claude Sonnet 4.6 --- docs/mcp.md | 4 ++-- src/mcp/tools/testCaseGenerate.ts | 11 ++++------- src/mcp/tools/testCaseValidate.ts | 4 ++-- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 5056eeb..3366c61 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -642,7 +642,7 @@ Generates an XML test case skeleton with UUID v4 guids and sequential `testItemI | Argument key / value pattern | Emitted XML class | API context | | ------------------------------------ | ----------------------------- | ----------------------------------- | | `target` key | `class="uiTarget"` | UiWithScreen, UiWithRow | -| `locator` key | `class="uiLocator"` | UiDoAction, UiAssert, UiScrollToElement | +| `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 | @@ -713,7 +713,7 @@ Validates an XML test case for schema correctness (validity score) and best prac - **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. - **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, UiAssert, or UiScrollToElement `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element. +- **UI-LOCATOR-001** — A UiDoAction or UiAssert `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element. - **SETVALUES-STRUCTURE-001** (ERROR) — A `SetValues` step's `values` argument uses `class="value"` (plain string) instead of `class="valueList"` with `` children. This causes an immediate `ClassCastException` at runtime. - **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `` elements. diff --git a/src/mcp/tools/testCaseGenerate.ts b/src/mcp/tools/testCaseGenerate.ts index dd9c256..d7dff52 100644 --- a/src/mcp/tools/testCaseGenerate.ts +++ b/src/mcp/tools/testCaseGenerate.ts @@ -111,7 +111,7 @@ const StepSchema = z.object({ '(3) AssertValues: pass assertion arguments as flat key/value pairs; emitted as flat elements, NOT wrapped in valueList/namedValues. ' + '(4) target argument (UiWithScreen / UiWithRow): pass the sf:ui:target or ui:pageobject:target URI; ' + ' emitted as class="uiTarget" uri="...". ' + - '(5) locator argument (UiDoAction / UiAssert / UiScrollToElement): pass the locator URI; emitted as class="uiLocator" uri="...". ' + + '(5) locator argument (UiDoAction / UiAssert): pass the locator URI; emitted as class="uiLocator" uri="...". ' + 'All other string values use class="value" valueClass="string".' ), }); @@ -137,7 +137,7 @@ const TOOL_DESCRIPTION = [ 'If AssertValues uses namedValues-shaped content, validation reports warning ASSERT-001.', '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/UiScrollToElement): pass the URI value; emitted as class="uiLocator" uri="...".', + 'locator argument (UiDoAction/UiAssert): pass the URI value; emitted as class="uiLocator" uri="...".', 'Cleanup warning: ApexDeleteObject steps near end of test will be skipped if an earlier step fails (stopOnError=false). Use a TearDown callable.', 'Validation: when validate_after_edit=true (default) the response includes a validation field and returns TESTCASE_INVALID if the generated XML fails structural checks.', 'Grounding: call provar.qualityhub.examples.retrieve before generating to get corpus examples for the scenario — correct XML structure for the step types you need.', @@ -285,11 +285,8 @@ function buildArgumentValue(key: string, val: string, indent: string, inNamedVal if (key === 'target' && (apiId.includes('UiWithScreen') || apiId.includes('UiWithRow'))) { return `${indent}`; } - // D2: 'locator' argument → class="uiLocator" (only for UiDoAction / UiAssert / UiScrollToElement). - if ( - key === 'locator' && - (apiId.includes('UiDoAction') || apiId.includes('UiAssert') || apiId.includes('UiScrollToElement')) - ) { + // D2: 'locator' argument → class="uiLocator" (only for UiDoAction / UiAssert). + if (key === 'locator' && (apiId.includes('UiDoAction') || apiId.includes('UiAssert'))) { return `${indent}`; } } diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index f872b0a..8c36431 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -437,8 +437,8 @@ function validateApiCallArgs( } // UI-LOCATOR-001 (local rule, no direct backend equivalent): - // UiDoAction / UiAssert / UiScrollToElement locator argument must use class="uiLocator". - if (apiId.includes('UiDoAction') || apiId.includes('UiAssert') || apiId.includes('UiScrollToElement')) { + // UiDoAction / UiAssert locator argument must use class="uiLocator". + if (apiId.includes('UiDoAction') || apiId.includes('UiAssert')) { const locatorArg = getArgList(call).find((a) => (a['@_id'] as string | undefined) === 'locator'); if (locatorArg) { const valClass = (locatorArg['value'] as Record | undefined)?.['@_class'] as string | undefined;