From 41803aebded8186e1ae901f771a7b8da26bb2241 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 4 Jun 2026 09:58:32 -0500 Subject: [PATCH 1/2] PDX-508: feat(mcp): port 7 back-end-only best-practice rules to local validator RCA: Seven rules existed only in the Quality Hub back-end rule set, so the local validator never evaluated them and its score could diverge from the API for tests that hit these runtime anti-patterns (variable refs stored as string literals, SetValues funcCall/zero-index string templates, DbConnect/dbConnectionName mismatch, and wrong-cased SF flow-button / record-type locators). Fix: Add the 7 rule definitions to the local rules JSON and implement backend-faithful validators (varStringLiteral as one-violation-per-value to match the back-end's list semantics; the other six as single-violation first-offender, with log-scaled count only on the two UI-locator checks). comparisonTypeEnum is intentionally excluded (ComparisonType is step-scoped). Adds 22 parity tests and docs. Preserves weighted-deduction score parity with the Lambda. --- docs/mcp.md | 8 + .../rules/provar_best_practices_rules.json | 111 +++++++ src/mcp/tools/bestPracticesEngine.ts | 280 ++++++++++++++++ test/unit/mcp/bestPracticesEngine.test.ts | 304 ++++++++++++++++++ 4 files changed, 703 insertions(+) diff --git a/docs/mcp.md b/docs/mcp.md index bfa7506..55cd297 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1084,6 +1084,14 @@ Validates an XML test case for schema correctness (validity score) and best prac - **`APEX-CONNECT-ARGS-001`** (`apexConnectValidArguments`, critical) — an `ApexConnect` step using an argument id outside the 20-id whitelist (overridable per rule via `check.validArgumentIds`). - **`APEX-CONNECT-CONNID-001`** (`apexConnectConnectionIdValueClass`, critical) — an `ApexConnect` `connectionId` value that uses `valueClass="string"` (or any non-`id` class) instead of `valueClass="id"`; leave it empty if there is no GUID. - **`APEX-REUSE-CONN-001`** (`apexConnectReuseConnection`, major) — an `ApexConnect` `reuseConnectionName` argument that carries a value; it must be left blank. +- **Runtime anti-pattern rules** — Major (`weight 5`) checks for XML that loads but silently misbehaves at runtime — typically variable references or expressions written as plain string literals. Like the rules above they run offline / in `local_fallback` and keep score parity with the Quality Hub back-end. Currently enforced: + - **`VAR-STRING-LITERAL-001`** (`varStringLiteral`) — an argument value is a whole-token `{VarName}` / `{Obj.Field}` stored as `class="value" valueClass="string"`; Provar passes the literal text instead of resolving the variable. Use ``. (The interpolation-tolerant target args `sfUiTargetObjectId` / `sfUiTargetResultName` are exempt.) Emits one violation per offending value. + - **`ASSERT-STR-VAR-001`** (`assertValuesStringExpr`) — an `AssertValues` `expectedValue`/`actualValue` is a whole `{…}` string literal, so the assertion compares the literal text instead of the variable's value. Use `class="variable"`. + - **`SETVALUES-FUNC-STR-001`** (`setValuesFuncCallString`) — a `SetValues` assigned value embeds a `{Func(args)}` call as a string literal; Provar will not evaluate it. Use a `class="funcCall"` value. + - **`SETVALUES-ZERO-IDX-001`** (`setValuesZeroIndexString`) — a `SetValues` string-template expression uses a `[0]` index; Provar string templates are 1-indexed, so this is out-of-bounds at runtime. Use `[1]` for the first item (or the XML variable-path structure). + - **`CONN-DB-002`** (`dbConnectResultNameMismatch`) — a DB operation (`SqlQuery`/`DbRead`/`DbInsert`/`DbUpdate`/`DbDelete`) sets a `dbConnectionName` that does not match any `DbConnect` `resultName` in the test, so the open connection can't be found. Defers to `CONN-DB-001` when there is no `DbConnect` at all. + - **`UI-LOCATOR-BUTTON-CASING-001`** (`uiLocatorButtonCasing`) — a `UiDoAction` locator uses a wrong-cased standard-button name: `name=Cancel` (use lowercase `name=cancel`) or `name=Continue`/`name=continue` on the record-type screen (use `name=save&path=selectRecordType`). + - **`UI-LOCATOR-RECORDTYPE-001`** (`uiLocatorRecordTypeField`) — a `UiDoAction` Record Type picker locator uses `name=recordTypeId`/`name=recordType` instead of `name=RecordType` with `field=RecordTypeId` in the binding. **Error codes** diff --git a/src/mcp/rules/provar_best_practices_rules.json b/src/mcp/rules/provar_best_practices_rules.json index 5c0c511..09001d9 100644 --- a/src/mcp/rules/provar_best_practices_rules.json +++ b/src/mcp/rules/provar_best_practices_rules.json @@ -2958,6 +2958,117 @@ }, "notes": "Mirrors QH UiActionNestingStructureValidator (lambda/src/validator/best_practices_engine.py). Emits one violation per offending step so that (rule_id, test_item_id) de-dups against the API. UiWithRow plays a dual role: itself a UI action that must be nested, and a container whose substeps clause satisfies the rule for its descendants.", "source": "Field observation: flat-emitted UI actions (sibling of UiWithScreen) fail to render in Provar IDE despite scoring 100/94 in earlier local validation" + }, + { + "id": "VAR-STRING-LITERAL-001", + "category": "TestCaseDesign", + "name": "Variable reference stored as plain string", + "description": "An argument value contains a {VarName} or {Obj.Field} pattern stored as class=\"value\" valueClass=\"string\". Provar passes the literal string to the API instead of resolving the variable — the step will silently receive the wrong input.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Replace with . Use dotted paths for nested references: . If using provar.testcase.generate, the {VarName} syntax in attributes is converted automatically.", + "check": { + "type": "varStringLiteral" + }, + "source": "Provar Test Step Schema: variable references must use class=\"variable\", not string literals" + }, + { + "id": "CONN-DB-002", + "category": "ConnectionsAndEnvironments", + "name": "DbConnect resultName must match dbConnectionName in DB operations", + "description": "The resultName set on a DbConnect step is the connection handle variable that all subsequent DB operations (SqlQuery, DbRead, DbInsert, DbUpdate, DbDelete) must reference via their dbConnectionName argument. A mismatch between DbConnect resultName and dbConnectionName causes a runtime failure because the operation cannot find the open connection. This is the leading cause of silent Database step failures in Provar test suites.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Ensure every DB operation step's dbConnectionName argument exactly matches the resultName set on the preceding DbConnect step. For example, if DbConnect sets resultName='SQLServer', every SqlQuery/DbRead/DbInsert/DbUpdate/DbDelete in the same test must set dbConnectionName='SQLServer'.", + "check": { + "type": "dbConnectResultNameMismatch", + "target": "testCase" + }, + "source": "Provar DB Connection Best Practices - DbConnect resultName / dbConnectionName alignment" + }, + { + "id": "SETVALUES-FUNC-STR-001", + "category": "StructureAndGrouping", + "name": "SetValues must not use string interpolation for function calls", + "description": "SetValues steps must use the correct XML structure for function calls () rather than embedding Provar's curly-brace template syntax as a plain string literal (e.g. {Count(AccountList)}). At runtime Provar reads the string literally and does not evaluate it, so the target variable is set to the text \"{Count(AccountList)}\" instead of the computed count. This pattern is generated by AI models that reproduce the Provar UI tooltip syntax verbatim.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Replace the string literal with the proper funcCall XML: . Supported functions include Count, Sum, Avg, Min, Max, Concat, StringLength, etc.", + "check": { + "type": "setValuesFuncCallString", + "target": "step" + }, + "notes": "AI models output {Count(var)} in a valueClass=string element because they mirror Provar's UI tooltip. The correct form is a funcCall value element. This causes silent test failures where the variable receives the literal string rather than the computed value.", + "source": "Field observation: SetValues function-call string interpolation anti-pattern" + }, + { + "id": "SETVALUES-ZERO-IDX-001", + "category": "StructureAndGrouping", + "name": "SetValues string expression must not use [0] index", + "description": "Provar's string-template engine is 1-indexed: {List[1]} accesses the first element. Using {List[0].Field} in a valueClass=string expression causes an out-of-bounds exception at runtime because index 0 is invalid in the template system. Note: the XML variable-path structure (0) correctly uses 0-based indexing via a different code path — this rule targets only the broken string-template form.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Either switch to the proper XML variable path structure (0) or, if you must use string interpolation, change [0] to [1] to access the first element.", + "check": { + "type": "setValuesZeroIndexString", + "target": "step" + }, + "notes": "AI models that generate string-template expressions derive index numbers from the literal test intent (e.g. 'first item = index 0') but Provar string templates are 1-indexed. This causes runtime out-of-bounds errors.", + "source": "Field observation: SetValues zero-index string interpolation anti-pattern" + }, + { + "id": "ASSERT-STR-VAR-001", + "category": "StructureAndGrouping", + "name": "AssertValues must not use string literal to reference a variable", + "description": "AssertValues steps must use a proper XML variable reference () when comparing against a stored variable. Using a string literal that wraps a variable name — e.g. {RowCount} — causes Provar to compare the literal text \"{RowCount}\" against the actual value, which always produces a false failure. The same applies to function-call expressions such as {Count(Results)}.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Replace the string literal with a proper variable reference: . For function calls, use a preceding SetValues step to compute the value into a variable, then reference that variable in AssertValues.", + "check": { + "type": "assertValuesStringExpr", + "target": "step" + }, + "notes": "AI models generate {VarName} in a valueClass=string element on the expectedValue side because they reproduce the Provar UI display format. The correct form is a class=variable element. This causes every assertion to fail with a string-vs-value mismatch.", + "source": "Field observation: AssertValues string-variable reference anti-pattern" + }, + { + "id": "UI-LOCATOR-BUTTON-CASING-001", + "category": "TestCaseDesign", + "name": "Standard Salesforce flow buttons must use correct locator pattern", + "description": "Standard Salesforce flow buttons must use the correct locator name. Cancel must use camelCase 'name=cancel'. The Continue button on the Record Type selection screen uses a special path-based locator 'name=save&path=selectRecordType' — using 'name=continue' or 'name=Continue' will not resolve and causes ProVar to show 'Not Available' at runtime. AI models commonly generate these incorrectly.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Cancel button: use name=cancel (lowercase). Continue button on record type selection: use name=save&path=selectRecordType (NOT name=continue or name=Continue). Corpus-validated patterns from 2000+ real test cases.", + "check": { + "type": "uiLocatorButtonCasing", + "target": "step", + "apiId": "com.provar.plugins.forcedotcom.core.ui.UiDoAction" + }, + "notes": "Corpus analysis: Cancel uses name=cancel (10 occurrences) vs name=Cancel (4). Continue on record type selection consistently uses name=save&path=selectRecordType across all corpus examples.", + "source": "Corpus analysis: AllPOCProjects + InternalProjects (2108 test cases)" + }, + { + "id": "UI-LOCATOR-RECORDTYPE-001", + "category": "TestCaseDesign", + "name": "Record Type field locator must use name=RecordType not name=recordTypeId", + "description": "On the Record Type selection screen (action=recordTypeNew), the Record Type picker field must use 'name=RecordType' in the locator URI with 'field=RecordTypeId' in the binding. AI models commonly generate 'name=recordTypeId' (camelCase API name), which causes ProVar to show 'Not Available' and the step fails at runtime.", + "appliesTo": ["Step"], + "severity": "major", + "weight": 5, + "recommendation": "Use name=RecordType (not name=recordTypeId or name=recordType) with field=RecordTypeId in the binding. Correct example: ui:locator?name=RecordType&binding=sf%3Aui%3Abinding%3Aobject%3Fobject%3D%7BtargetUrl%3Aobject%7D%26field%3DRecordTypeId", + "check": { + "type": "uiLocatorRecordTypeField", + "target": "step", + "apiId": "com.provar.plugins.forcedotcom.core.ui.UiDoAction" + }, + "notes": "Corpus analysis: 258 occurrences of name=RecordType+field=RecordTypeId vs 0 occurrences of name=recordTypeId. The {targetUrl:object} binding resolves dynamically to the current screen's object.", + "source": "Corpus analysis: AllPOCProjects + InternalProjects (2108 test cases)" } ] } diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 5c3d87d..0829f5b 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -1606,6 +1606,277 @@ function validateSetValuesInvalidElements(tc: XmlNode, rule: BPRule): BPViolatio return makeViolation(rule, message, count); } +// ── Back-end-only rules (Tier 4) ───────────────────────────────────────────── +// Ports of seven Quality Hub validators that existed only in the back-end rule +// set. All seven rules are severity=major / weight=5. Score parity: six emit a +// single violation (the back-end returns the first offender; `count` is set only +// for the two UI-locator checks, and only when >1 offender); `varStringLiteral` +// emits ONE violation per offending value (the back-end returns a list, scored +// linearly) — do not collapse it. + +const ASSERT_VALUES_API_ID = 'com.provar.plugins.bundled.apis.AssertValues'; +const DB_CONNECT_API_ID = 'com.provar.plugins.bundled.apis.db.DbConnect'; +const UI_DO_ACTION_API_ID = 'com.provar.plugins.forcedotcom.core.ui.UiDoAction'; + +// DB operation steps whose dbConnectionName must reference a DbConnect resultName +// (both the modern `db.*` and legacy `data.*` namespaces, mirroring the back-end). +const DB_OPERATION_API_IDS: ReadonlySet = new Set([ + 'com.provar.plugins.bundled.apis.db.SqlQuery', + 'com.provar.plugins.bundled.apis.db.DbRead', + 'com.provar.plugins.bundled.apis.db.DbInsert', + 'com.provar.plugins.bundled.apis.db.DbUpdate', + 'com.provar.plugins.bundled.apis.db.DbDelete', + 'com.provar.plugins.bundled.apis.data.SqlQuery', + 'com.provar.plugins.bundled.apis.data.DbRead', + 'com.provar.plugins.bundled.apis.data.DbInsert', + 'com.provar.plugins.bundled.apis.data.DbUpdate', + 'com.provar.plugins.bundled.apis.data.DbDelete', +]); + +/** Escape a literal for safe embedding in a RegExp (mirrors Python's re.escape). */ +function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Resolve an argument's text value, mirroring the back-end `get_argument_value`: + * `variable` → first `` element name, `compound` → concatenated `` + * text, otherwise the element text. Wrapper-aware (handles both the `` + * wrapper and direct `` children) via {@link findArgumentById}. + */ +function resolvedArgText(call: XmlNode, argId: string): string { + const arg = findArgumentById(call, argId); + if (!arg) return ''; + const raw = arg['value']; + const ve = Array.isArray(raw) ? (raw as unknown[])[0] : raw; + if (ve == null) return ''; + if (typeof ve === 'string') return ve.trim(); + if (typeof ve !== 'object') return String(ve).trim(); + const v = ve as XmlNode; + const cls = v['@_class'] as string | undefined; + if (cls === 'variable') { + const firstPath = toArr(v['path'] as XmlNode | XmlNode[])[0] as XmlNode | undefined; + return ((firstPath?.['@_element'] as string | undefined) ?? '').trim(); + } + if (cls === 'compound') { + const partsNode = (v['parts'] as XmlNode | undefined)?.['value']; + return toArr(partsNode as XmlNode | XmlNode[]) + .filter((p) => p && typeof p === 'object') + .map((p) => nodeText(p)) + .join(''); + } + return nodeText(v); +} + +const VAR_LITERAL_PATTERN = /^\{[\w.]+\}$/; +const VAR_LITERAL_TOLERATED_ARGS: ReadonlySet = new Set(['sfUiTargetObjectId', 'sfUiTargetResultName']); + +/** + * VAR-STRING-LITERAL-001 — a `{Var}`/`{Obj.Field}` token stored as + * `class="value" valueClass="string"` instead of `class="variable"`. Emits ONE + * violation per offending value (the back-end returns a list), so score parity + * is preserved; the two interpolation-tolerant target args are exempt. + */ +function validateVarStringLiteral(tc: XmlNode, rule: BPRule): BPViolation[] { + const violations: BPViolation[] = []; + for (const call of getAllApiCalls(tc)) { + const ctx = stepContext(call); + for (const { value, argId } of getStepValueElements(call)) { + if (value['@_class'] !== 'value' || value['@_valueClass'] !== 'string') continue; + const text = nodeText(value); + if (!VAR_LITERAL_PATTERN.test(text) || VAR_LITERAL_TOLERATED_ARGS.has(argId)) continue; + violations.push( + makeViolation( + rule, + `Argument value "${text}" looks like a variable reference but is stored as a plain string — Provar ` + + 'will not resolve it at runtime. Use instead ' + + `(step '${ctx.title}', testItemId=${ctx.tid})` + ) + ); + } + } + return violations; +} + +/** CONN-DB-002 — every DB operation's dbConnectionName must match a DbConnect resultName in the test. */ +function validateDbConnectResultNameMismatch(tc: XmlNode, rule: BPRule): BPViolation | null { + const calls = getAllApiCalls(tc); + const resultNames = new Set(); + for (const call of calls) { + if (call['@_apiId'] !== DB_CONNECT_API_ID) continue; + const rn = resolvedArgText(call, 'resultName'); + if (rn) resultNames.add(rn); + } + if (!resultNames.size) return null; // no DbConnect resultName — CONN-DB-001 covers a missing DbConnect + + const offenders: string[] = []; + for (const call of calls) { + if (!DB_OPERATION_API_IDS.has(call['@_apiId'] as string)) continue; + const ref = resolvedArgText(call, 'dbConnectionName'); + if (!ref || resultNames.has(ref)) continue; + offenders.push(`'${stepContext(call).title.slice(0, 40)}' uses dbConnectionName='${ref}'`); + } + if (!offenders.length) return null; + const names = [...resultNames].sort().join(', '); + return makeViolation( + rule, + `DbConnect resultName does not match dbConnectionName in ${offenders.length} DB operation(s): ` + + `${offenders[0]} but DbConnect resultName(s) are: ${names}` + ); +} + +/** The `` children of a SetValues `` (the assigned-value slot). */ +function getSetValuesValueElements(call: XmlNode): XmlNode[] { + const out: XmlNode[] = []; + for (const nv of collectElementsByTag(call, 'namedValue')) { + if (!nv || typeof nv !== 'object' || (nv as XmlNode)['@_name'] !== 'value') continue; + for (const v of toArr((nv as XmlNode)['value'] as XmlNode | XmlNode[])) { + if (v && typeof v === 'object') out.push(v); + } + } + return out; +} + +const SETVALUES_FUNC_EXPR = /\{[A-Za-z][A-Za-z0-9]*\s*\([^)]*\)\s*\}/; +const SETVALUES_ZERO_IDX = /\{[^}]*\[0\][^}]*\}/; + +/** First SetValues `valueClass="string"` value whose text matches `re` (string-template anti-patterns). */ +function firstSetValuesStringMatch( + tc: XmlNode, + re: RegExp +): { ctx: ReturnType; text: string } | null { + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== SETVALUES_API_ID) continue; + for (const ve of getSetValuesValueElements(call)) { + if (ve['@_class'] !== 'value' || ve['@_valueClass'] !== 'string') continue; + const text = nodeText(ve); + if (re.test(text)) return { ctx: stepContext(call), text }; + } + } + return null; +} + +/** SETVALUES-FUNC-STR-001 — SetValues uses `{Func(args)}` string interpolation instead of a `funcCall` value. */ +function validateSetValuesFuncCallString(tc: XmlNode, rule: BPRule): BPViolation | null { + const hit = firstSetValuesStringMatch(tc, SETVALUES_FUNC_EXPR); + if (!hit) return null; + return makeViolation( + rule, + 'SetValues uses string interpolation for a function call — the value will not be evaluated: ' + + `'${hit.ctx.title.slice(0, 50)}' value='${hit.text.slice(0, 60)}' (testItemId=${hit.ctx.tid})` + ); +} + +/** SETVALUES-ZERO-IDX-001 — SetValues string template uses a 0 index (templates are 1-indexed). */ +function validateSetValuesZeroIndexString(tc: XmlNode, rule: BPRule): BPViolation | null { + const hit = firstSetValuesStringMatch(tc, SETVALUES_ZERO_IDX); + if (!hit) return null; + return makeViolation( + rule, + 'SetValues string expression uses a [0] index — Provar string templates are 1-indexed, causing an ' + + `out-of-bounds error at runtime: '${hit.ctx.title.slice(0, 50)}' value='${hit.text.slice(0, 60)}' — use ` + + `[1] for the first item (testItemId=${hit.ctx.tid})` + ); +} + +const ASSERT_WHOLE_EXPR = /^\s*\{[^{}]+\}\s*$/; + +/** The direct `` element child of an argument (wrapper-aware), or undefined. */ +function directArgValueElement(call: XmlNode, argId: string): XmlNode | undefined { + const arg = findArgumentById(call, argId); + if (!arg) return undefined; + const raw = arg['value']; + const ve = Array.isArray(raw) ? (raw as unknown[])[0] : raw; + return ve && typeof ve === 'object' ? (ve as XmlNode) : undefined; +} + +/** ASSERT-STR-VAR-001 — AssertValues references a variable via a `{Var}` string literal instead of `class="variable"`. */ +function validateAssertValuesStringExpr(tc: XmlNode, rule: BPRule): BPViolation | null { + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== ASSERT_VALUES_API_ID) continue; + for (const argId of ['expectedValue', 'actualValue']) { + const v = directArgValueElement(call, argId); + if (!v || v['@_class'] !== 'value' || v['@_valueClass'] !== 'string') continue; + const text = nodeText(v); + if (!ASSERT_WHOLE_EXPR.test(text)) continue; + const ctx = stepContext(call); + return makeViolation( + rule, + 'AssertValues uses a string literal to reference a variable — the assertion compares the literal text, ' + + `not the variable value: '${ctx.title.slice(0, 50)}' ${argId}='${text.slice(0, 60)}' should use ` + + ` (testItemId=${ctx.tid})` + ); + } + } + return null; +} + +/** The `uri` of a UiDoAction's `locator` argument value (`class="uiLocator"`), or '' if absent. */ +function getUiDoActionLocatorUri(call: XmlNode): string { + const arg = getCallArguments(call).find((a) => a['@_id'] === 'locator'); + if (!arg) return ''; + for (const v of toArr(arg['value'] as XmlNode | XmlNode[])) { + if (v && typeof v === 'object' && v['@_class'] === 'uiLocator') { + return (v['@_uri'] as string | undefined) ?? ''; + } + } + return ''; +} + +// Standard SF flow buttons whose locator name must use the corpus-validated casing/path. +const UI_WRONG_BUTTONS: ReadonlyArray = [ + ['Cancel', "use 'name=cancel' (lowercase)"], + ['continue', "the Continue button on record type selection screens uses 'name=save&path=selectRecordType'"], + ['Continue', "the Continue button on record type selection screens uses 'name=save&path=selectRecordType'"], +]; + +/** UI-LOCATOR-BUTTON-CASING-001 — Cancel/Continue flow buttons must use the correct locator name. */ +function validateUiLocatorButtonCasing(tc: XmlNode, rule: BPRule): BPViolation | null { + const offenders: Array<{ ctx: ReturnType; wrong: string; explanation: string; uri: string }> = []; + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== UI_DO_ACTION_API_ID) continue; + const uri = getUiDoActionLocatorUri(call); + if (!uri) continue; + for (const [wrong, explanation] of UI_WRONG_BUTTONS) { + if (new RegExp(`name=${escapeRegExp(wrong)}(&|$)`).test(uri)) { + offenders.push({ ctx: stepContext(call), wrong, explanation, uri }); + break; // only report the first match per step (mirrors the back-end) + } + } + } + if (!offenders.length) return null; + const f = offenders[0]; + return makeViolation( + rule, + `Step '${f.ctx.title}' uses incorrect button name 'name=${f.wrong}': ${f.explanation}. Incorrect button ` + + `names cause Provar to show 'Not Available' and fail at runtime. Current URI: ${f.uri.slice(0, 120)} ` + + `(testItemId=${f.ctx.tid})`, + offenders.length + ); +} + +const UI_RECORDTYPE_WRONG = /name=recordType(Id)?(&|$)/; + +/** UI-LOCATOR-RECORDTYPE-001 — the Record Type picker locator must use `name=RecordType`, not `name=recordType(Id)`. */ +function validateUiLocatorRecordTypeField(tc: XmlNode, rule: BPRule): BPViolation | null { + const offenders: Array<{ ctx: ReturnType; uri: string }> = []; + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== UI_DO_ACTION_API_ID) continue; + const uri = getUiDoActionLocatorUri(call); + if (!uri || !UI_RECORDTYPE_WRONG.test(uri)) continue; + offenders.push({ ctx: stepContext(call), uri }); + } + if (!offenders.length) return null; + const f = offenders[0]; + return makeViolation( + rule, + `Step '${f.ctx.title}' uses an incorrect Record Type field locator. The Record Type picker must use ` + + "'name=RecordType' (not 'name=recordTypeId' or 'name=recordType') with 'field=RecordTypeId' in the binding. " + + `Current URI: ${f.uri.slice(0, 150)} (testItemId=${f.ctx.tid})`, + offenders.length + ); +} + // ── Validator dispatch map ──────────────────────────────────────────────────── type ValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation | null; @@ -1632,6 +1903,13 @@ const VALIDATOR_REGISTRY: Record = { apexConnectValidArguments: validateApexConnectValidArguments, apexConnectConnectionIdValueClass: validateApexConnectConnectionIdValueClass, setValuesInvalidElements: validateSetValuesInvalidElements, + // Tier 4 — back-end-only rules (single-violation ports) + dbConnectResultNameMismatch: validateDbConnectResultNameMismatch, + setValuesFuncCallString: validateSetValuesFuncCallString, + setValuesZeroIndexString: validateSetValuesZeroIndexString, + assertValuesStringExpr: validateAssertValuesStringExpr, + uiLocatorButtonCasing: validateUiLocatorButtonCasing, + uiLocatorRecordTypeField: validateUiLocatorRecordTypeField, // 'regex' is dispatched separately (needs metadata) // 'uiActionNestingStructure' is dispatched separately (emits one violation per offending step) }; @@ -1643,6 +1921,8 @@ const MULTI_VALIDATOR_REGISTRY: Record = { uiActionNestingStructure: validateUiActionNestingStructure, nitroxConnectInvalidArgs: validateNitroxConnectInvalidArgs, nitroxVariantArgRequired: validateNitroxVariantArgRequired, + // Tier 4 — emits one violation per offending value (back-end returns a list) + varStringLiteral: validateVarStringLiteral, }; // ── XML parser (shared settings) ───────────────────────────────────────────── diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index da8c1f9..15eb708 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -1006,3 +1006,307 @@ ${stepsXml} }); }); }); + +describe('back-end-only rules (Tier 4)', () => { + const GUID_T4_TC = '550e8400-e29b-41d4-a716-4466554407c0'; + const GUID_T4_S1 = '550e8400-e29b-41d4-a716-4466554407c1'; + const APEX_CREATE = 'com.provar.plugins.forcedotcom.core.testapis.ApexCreateObject'; + const ASSERT_VALUES = 'com.provar.plugins.bundled.apis.AssertValues'; + const SET_VALUES = 'com.provar.plugins.bundled.apis.control.SetValues'; + const DB_CONNECT = 'com.provar.plugins.bundled.apis.db.DbConnect'; + const SQL_QUERY = 'com.provar.plugins.bundled.apis.db.SqlQuery'; + const UI_DO_ACTION = 'com.provar.plugins.forcedotcom.core.ui.UiDoAction'; + + function buildTc(stepsXml: string): string { + return ` + + +${stepsXml} + +`; + } + + function find(violations: BPViolation[], ruleId: string): BPViolation | undefined { + return violations.find((v) => v.rule_id === ruleId); + } + + function countOf(violations: BPViolation[], ruleId: string): number { + return violations.filter((v) => v.rule_id === ruleId).length; + } + + // Wrap one around a raw on an ApexCreateObject step (avoids triggering other Tier 4 rules). + function buildArgStep(valueXml: string): string { + return buildTc(` + + ${valueXml} + + `); + } + + // ── varStringLiteral (VAR-STRING-LITERAL-001) — multi-violation ───────────── + describe('varStringLiteral — VAR-STRING-LITERAL-001', () => { + it('fires for a {Var} stored as class="value" valueClass="string"', () => { + const v = find( + runBestPractices(buildArgStep('{AccountId}')).violations, + 'VAR-STRING-LITERAL-001' + ); + assert.ok(v, 'Expected VAR-STRING-LITERAL-001 to fire for {AccountId}'); + assert.equal(v?.severity, 'major'); + }); + + it('emits one violation per offending value (back-end returns a list)', () => { + const xml = buildTc(` + + {AccountId} + {Obj.Field} + + `); + assert.equal( + countOf(runBestPractices(xml).violations, 'VAR-STRING-LITERAL-001'), + 2, + 'each offending value is its own violation' + ); + }); + + it('passes when the value is a proper class="variable" reference', () => { + const v = find( + runBestPractices(buildArgStep('')).violations, + 'VAR-STRING-LITERAL-001' + ); + assert.ok(!v, `A class="variable" reference should pass, got: ${v?.message}`); + }); + + it('does not fire for the interpolation-tolerant args (sfUiTargetObjectId)', () => { + const xml = buildTc(` + + {AccountId} + + `); + const v = find(runBestPractices(xml).violations, 'VAR-STRING-LITERAL-001'); + assert.ok(!v, 'sfUiTargetObjectId tolerates {Var} interpolation'); + }); + + it('does not fire when the surrounding text is more than a bare token', () => { + const v = find( + runBestPractices(buildArgStep('Hello {Name}')).violations, + 'VAR-STRING-LITERAL-001' + ); + assert.ok(!v, 'only a whole-string {Token} is flagged, not embedded references'); + }); + }); + + // ── dbConnectResultNameMismatch (CONN-DB-002) ────────────────────────────── + describe('dbConnectResultNameMismatch — CONN-DB-002', () => { + function dbStep(apiId: string, argId: string, value: string, tid: string): string { + return ` + + ${value} + + `; + } + + it('fires when a DB operation references a dbConnectionName that no DbConnect produced', () => { + const xml = buildTc( + `${dbStep(DB_CONNECT, 'resultName', 'SQLServer', '1')}\n${dbStep( + SQL_QUERY, + 'dbConnectionName', + 'WrongName', + '2' + )}` + ); + const v = find(runBestPractices(xml).violations, 'CONN-DB-002'); + assert.ok(v, 'Expected CONN-DB-002 to fire for a mismatched dbConnectionName'); + assert.equal(v?.severity, 'major'); + assert.ok(v?.message.includes('SQLServer'), `Message should name the valid resultName(s): ${v?.message}`); + }); + + it('passes when the dbConnectionName matches the DbConnect resultName', () => { + const xml = buildTc( + `${dbStep(DB_CONNECT, 'resultName', 'SQLServer', '1')}\n${dbStep( + SQL_QUERY, + 'dbConnectionName', + 'SQLServer', + '2' + )}` + ); + const v = find(runBestPractices(xml).violations, 'CONN-DB-002'); + assert.ok(!v, `A matching dbConnectionName should pass, got: ${v?.message}`); + }); + + it('does not fire when there is no DbConnect (delegated to CONN-DB-001)', () => { + const xml = buildTc(dbStep(SQL_QUERY, 'dbConnectionName', 'SQLServer', '1')); + const v = find(runBestPractices(xml).violations, 'CONN-DB-002'); + assert.ok(!v, 'with no DbConnect resultName the mismatch rule defers to CONN-DB-001'); + }); + }); + + // ── setValuesFuncCallString (SETVALUES-FUNC-STR-001) ─────────────────────── + describe('setValuesFuncCallString — SETVALUES-FUNC-STR-001', () => { + function setValuesStep(valueXml: string): string { + return buildTc(` + + + + + ${valueXml} + + + + + `); + } + + it('fires for a {Func(args)} string interpolation', () => { + const v = find( + runBestPractices(setValuesStep('{Count(AccountList)}')) + .violations, + 'SETVALUES-FUNC-STR-001' + ); + assert.ok(v, 'Expected SETVALUES-FUNC-STR-001 to fire for {Count(...)}'); + assert.equal(v?.severity, 'major'); + }); + + it('passes when the value is a proper funcCall element', () => { + const v = find( + runBestPractices( + setValuesStep( + '' + ) + ).violations, + 'SETVALUES-FUNC-STR-001' + ); + assert.ok(!v, `A funcCall value should pass, got: ${v?.message}`); + }); + }); + + // ── setValuesZeroIndexString (SETVALUES-ZERO-IDX-001) ────────────────────── + describe('setValuesZeroIndexString — SETVALUES-ZERO-IDX-001', () => { + function setValuesStep(text: string): string { + return buildTc(` + + + + + ${text} + + + + + `); + } + + it('fires for a [0] index in a string template', () => { + const v = find(runBestPractices(setValuesStep('{AccountList[0].Name}')).violations, 'SETVALUES-ZERO-IDX-001'); + assert.ok(v, 'Expected SETVALUES-ZERO-IDX-001 to fire for [0]'); + }); + + it('passes for a 1-indexed string template', () => { + const v = find(runBestPractices(setValuesStep('{AccountList[1].Name}')).violations, 'SETVALUES-ZERO-IDX-001'); + assert.ok(!v, `A [1] index should pass, got: ${v?.message}`); + }); + }); + + // ── assertValuesStringExpr (ASSERT-STR-VAR-001) ──────────────────────────── + describe('assertValuesStringExpr — ASSERT-STR-VAR-001', () => { + function assertStep(argId: string, valueXml: string): string { + return buildTc(` + + ${valueXml} + + `); + } + + it('fires when expectedValue is a {Var} string literal', () => { + const v = find( + runBestPractices(assertStep('expectedValue', '{RowCount}')) + .violations, + 'ASSERT-STR-VAR-001' + ); + assert.ok(v, 'Expected ASSERT-STR-VAR-001 to fire for a {RowCount} literal'); + assert.equal(v?.severity, 'major'); + }); + + it('passes when the value is a proper class="variable" reference', () => { + const v = find( + runBestPractices(assertStep('expectedValue', '')) + .violations, + 'ASSERT-STR-VAR-001' + ); + assert.ok(!v, `A class="variable" reference should pass, got: ${v?.message}`); + }); + + it('does not fire for a plain literal that is not a brace expression', () => { + const v = find( + runBestPractices(assertStep('expectedValue', 'Acme')) + .violations, + 'ASSERT-STR-VAR-001' + ); + assert.ok(!v, 'a literal value that is not a {…} expression is fine'); + }); + }); + + // ── uiLocatorButtonCasing (UI-LOCATOR-BUTTON-CASING-001) ─────────────────── + describe('uiLocatorButtonCasing — UI-LOCATOR-BUTTON-CASING-001', () => { + function uiStep(uri: string): string { + return buildTc(` + + + + `); + } + + it('fires for the wrong-cased Cancel button (name=Cancel)', () => { + const v = find(runBestPractices(uiStep('ui:locator?name=Cancel')).violations, 'UI-LOCATOR-BUTTON-CASING-001'); + assert.ok(v, 'Expected UI-LOCATOR-BUTTON-CASING-001 to fire for name=Cancel'); + assert.equal(v?.severity, 'major'); + }); + + it('fires for name=Continue (record-type Continue needs name=save&path=selectRecordType)', () => { + const v = find(runBestPractices(uiStep('ui:locator?name=Continue')).violations, 'UI-LOCATOR-BUTTON-CASING-001'); + assert.ok(v, 'Expected UI-LOCATOR-BUTTON-CASING-001 to fire for name=Continue'); + }); + + it('passes for the correct lowercase name=cancel', () => { + const v = find(runBestPractices(uiStep('ui:locator?name=cancel')).violations, 'UI-LOCATOR-BUTTON-CASING-001'); + assert.ok(!v, `name=cancel is correct and should pass, got: ${v?.message}`); + }); + + it('does not match a prefix (name=Cancelled must not fire)', () => { + const v = find(runBestPractices(uiStep('ui:locator?name=Cancelled')).violations, 'UI-LOCATOR-BUTTON-CASING-001'); + assert.ok(!v, 'the name= boundary must not match a longer name'); + }); + }); + + // ── uiLocatorRecordTypeField (UI-LOCATOR-RECORDTYPE-001) ─────────────────── + describe('uiLocatorRecordTypeField — UI-LOCATOR-RECORDTYPE-001', () => { + function uiStep(uri: string): string { + return buildTc(` + + + + `); + } + + it('fires for name=recordTypeId', () => { + const v = find( + runBestPractices(uiStep('ui:locator?name=recordTypeId&field=RecordTypeId')).violations, + 'UI-LOCATOR-RECORDTYPE-001' + ); + assert.ok(v, 'Expected UI-LOCATOR-RECORDTYPE-001 to fire for name=recordTypeId'); + assert.equal(v?.severity, 'major'); + }); + + it('fires for the lowercase name=recordType', () => { + const v = find(runBestPractices(uiStep('ui:locator?name=recordType')).violations, 'UI-LOCATOR-RECORDTYPE-001'); + assert.ok(v, 'Expected UI-LOCATOR-RECORDTYPE-001 to fire for name=recordType'); + }); + + it('passes for the correct name=RecordType', () => { + const v = find( + runBestPractices(uiStep('ui:locator?name=RecordType&field=RecordTypeId')).violations, + 'UI-LOCATOR-RECORDTYPE-001' + ); + assert.ok(!v, `name=RecordType is correct and should pass, got: ${v?.message}`); + }); + }); +}); From 46001bad11af024646280db0c45305dbf4a31568 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 4 Jun 2026 10:09:44 -0500 Subject: [PATCH 2/2] PDX-508: docs(mcp): fix user-facing copy in two locator rule descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review flagged two user-facing copy errors carried over verbatim from the back-end rule text: UI-LOCATOR-BUTTON-CASING-001 called 'name=cancel' "camelCase" when it is lowercase, and both new UI-locator rules spelled the product "ProVar" instead of "Provar". Fix: Correct "camelCase" to "lowercase" for the Cancel locator and normalise "ProVar" to "Provar" in both rule descriptions. Description text only — no validator logic or score change. --- src/mcp/rules/provar_best_practices_rules.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mcp/rules/provar_best_practices_rules.json b/src/mcp/rules/provar_best_practices_rules.json index 09001d9..c8b7989 100644 --- a/src/mcp/rules/provar_best_practices_rules.json +++ b/src/mcp/rules/provar_best_practices_rules.json @@ -3040,7 +3040,7 @@ "id": "UI-LOCATOR-BUTTON-CASING-001", "category": "TestCaseDesign", "name": "Standard Salesforce flow buttons must use correct locator pattern", - "description": "Standard Salesforce flow buttons must use the correct locator name. Cancel must use camelCase 'name=cancel'. The Continue button on the Record Type selection screen uses a special path-based locator 'name=save&path=selectRecordType' — using 'name=continue' or 'name=Continue' will not resolve and causes ProVar to show 'Not Available' at runtime. AI models commonly generate these incorrectly.", + "description": "Standard Salesforce flow buttons must use the correct locator name. Cancel must use lowercase 'name=cancel'. The Continue button on the Record Type selection screen uses a special path-based locator 'name=save&path=selectRecordType' — using 'name=continue' or 'name=Continue' will not resolve and causes Provar to show 'Not Available' at runtime. AI models commonly generate these incorrectly.", "appliesTo": ["Step"], "severity": "major", "weight": 5, @@ -3057,7 +3057,7 @@ "id": "UI-LOCATOR-RECORDTYPE-001", "category": "TestCaseDesign", "name": "Record Type field locator must use name=RecordType not name=recordTypeId", - "description": "On the Record Type selection screen (action=recordTypeNew), the Record Type picker field must use 'name=RecordType' in the locator URI with 'field=RecordTypeId' in the binding. AI models commonly generate 'name=recordTypeId' (camelCase API name), which causes ProVar to show 'Not Available' and the step fails at runtime.", + "description": "On the Record Type selection screen (action=recordTypeNew), the Record Type picker field must use 'name=RecordType' in the locator URI with 'field=RecordTypeId' in the binding. AI models commonly generate 'name=recordTypeId' (the camelCase API name), which causes Provar to show 'Not Available' and the step fails at runtime.", "appliesTo": ["Step"], "severity": "major", "weight": 5,